auto-fix batch 2026-04-28-002530

.claude/skills/resolving-issues/SKILL.md

@@ -51,9 +51,10 @@ All sub-fixes land on one master branch per session. Master PR is opened **only
 4. No in-scope issues? Noop. Skip the rest. No master branch created, no PR opened.
 5. Create fresh master branch for this session (push, no PR yet — see ### Master branch setup).
 6. Per issue, sequential, max 10 per run:
+   - **Pre-dispatch sync:** before spawning each implementer, `git fetch origin <master-batch>` + `git reset --hard origin/<master-batch>` in the coordinator's checkout. Prior implementers' merges + your own session-open commit must be the worktree base; stale local state poisons the next worktree.
    - Spawn fresh implementer agent.
    - Implementer: worktree off master branch → research subagents if needed → fix → tests → sub-PR into master branch → merge gate → squash-merge.
-   - Track `Fixes #N` for final PR body assembly.
+   - Track `Fixes #N` for final PR body assembly. **If implementer reports the issue was already fixed upstream and closed it directly with a caveman comment**, do NOT include in `Fixes` list — issue is already closed, `Fixes` keyword would be a no-op or worse a stale link. Note in `## Already-Fixed` master-PR section instead.
    - Tear down worktree.
    - Next issue.
 7. Implementer finds related rot? File follow-up issue.
@@ -81,6 +82,7 @@ Fresh agent per issue, scoped to one issue + master branch ref. Steps:
 9. **Merge gate:** if sub-PR CI runs (rare — only when workflow `branches: [main]` filter matches), wait for green. If CI doesn't run (sub-PR base ≠ main is the common case), local `just check` green from step 7 IS the gate. Merge with `mcp__github__merge_pull_request` `merge_method: squash`.
 10. CI red after one fix attempt OR local `just check` red OR mid-fix block → **file a follow-up GH issue** (caveman body, link the original issue + cite the blocker), then **close the sub-PR** (don't leave it as a draft for someone to resume). The next scheduled run will see the follow-up issue in the queue and pick it up. Return control to coordinator.
 11. Tear down worktree on merge OR on close-after-blocker.
+12. **Already-fixed-upstream path:** if pre-flight investigation (e.g. `cargo audit`, file-state grep, `cargo tree`) shows the issue was resolved by a recently-merged upstream PR, do NOT open a dead sub-PR. Leave a caveman comment on the original issue naming the upstream PR + the fix location, close the issue as `completed`, tear down the worktree, report back. Coordinator records this under `## Already-Fixed` in the master PR — NOT under `Fixes`.
 
 ## Lessons Learned
 
@@ -98,6 +100,7 @@ Never defer skill edits to a follow-up — they ship with the run that surfaced
 - Read, dispatch, monitor. Implementers touch files.
 - One worktree per issue. Sequential between issues. Tear down after merge or draft-park.
 - **Exception:** the master branch's own session-open commit + Lessons Learned skill edits (see ## Lessons Learned). Coordinator commits these directly to the master branch.
+- **Webhook subscriptions are informational.** When `<github-webhook-activity>` arrives for a sub-PR opened by an implementer, the implementer owns its merge gate. Coordinator does NOT investigate CI / review state — that's the implementer's job, and the implementer is still running. Acknowledge briefly + keep waiting. Only act on the webhook if no implementer is running for that PR (i.e. the implementer already finished and the webhook arrived later as a stale event).
 
 ### Sequential between issues
 - One issue at a time. No parallel implementers.

crates/agent/src/scopes.rs

@@ -5,19 +5,32 @@
 
 use std::collections::HashSet;
 
+/// URI of the join-links resource. The `link_id` values surfaced through this
+/// resource are single-step bearer credentials for `WireMessage::JoinRequest`,
+/// so any scope below `Full`/`Admin` MUST be denied access. See issue #436
+/// (audit finding AUD-2).
+const JOIN_LINKS_URI: &str = "willow://server/join-links";
+
 /// Scope of a bearer token, controlling tool and resource access.
+///
+/// Resource access is **not** uniform across all scopes: `willow://server/join-links`
+/// exposes bearer credentials usable by `WireMessage::JoinRequest`, so it is
+/// restricted to the privileged scopes (`Full`, `Admin`) and to `Custom` token
+/// allowlists that explicitly include the URI.
 #[derive(Debug, Clone, Default)]
 pub enum TokenScope {
     /// All tools, all resources.
     #[default]
     Full,
-    /// No tools, all resources.
+    /// No tools, all resources except `willow://server/join-links`.
     ReadOnly,
-    /// Messaging tools only, all resources.
+    /// Messaging tools only, all resources except `willow://server/join-links`.
     Messaging,
     /// All tools, all resources (semantically distinct from Full for audit).
     Admin,
-    /// Explicit allowlist of tool names.
+    /// Explicit allowlist of tool *and* resource names. A name in the set
+    /// authorises either a tool call or a resource read with that exact
+    /// identifier.
     Custom(HashSet<String>),
 }
 
@@ -45,9 +58,16 @@ impl TokenScope {
     }
 
     /// Returns true if the given resource URI is allowed.
-    pub fn allows_resource(&self, _uri: &str) -> bool {
-        // All scopes allow all resources.
-        true
+    ///
+    /// `willow://server/join-links` exposes single-step bearer credentials and
+    /// is therefore gated to `Full`/`Admin` (or a `Custom` set that lists it).
+    /// All other URIs are allowed under `ReadOnly` and `Messaging`.
+    pub fn allows_resource(&self, uri: &str) -> bool {
+        match self {
+            Self::Full | Self::Admin => true,
+            Self::ReadOnly | Self::Messaging => !uri.starts_with(JOIN_LINKS_URI),
+            Self::Custom(set) => set.contains(uri),
+        }
     }
 }
 
@@ -73,6 +93,45 @@ mod tests {
         assert!(scope.allows_resource("willow://server/channels"));
     }
 
+    #[test]
+    fn readonly_scope_denies_join_links() {
+        let scope = TokenScope::ReadOnly;
+        assert!(
+            !scope.allows_resource("willow://server/join-links"),
+            "ReadOnly scope must NOT expose join-links: link_id is a bearer credential (issue #436)"
+        );
+    }
+
+    #[test]
+    fn messaging_scope_denies_join_links() {
+        let scope = TokenScope::Messaging;
+        assert!(
+            !scope.allows_resource("willow://server/join-links"),
+            "Messaging scope must NOT expose join-links: link_id is a bearer credential (issue #436)"
+        );
+    }
+
+    #[test]
+    fn full_and_admin_allow_join_links() {
+        assert!(TokenScope::Full.allows_resource("willow://server/join-links"));
+        assert!(TokenScope::Admin.allows_resource("willow://server/join-links"));
+    }
+
+    #[test]
+    fn custom_resource_allowlist_gates_join_links() {
+        // Custom set without the URI denies it.
+        let mut set = HashSet::new();
+        set.insert("send_message".to_string());
+        let scope = TokenScope::Custom(set);
+        assert!(!scope.allows_resource("willow://server/join-links"));
+
+        // Custom set including the URI grants it.
+        let mut set = HashSet::new();
+        set.insert("willow://server/join-links".to_string());
+        let scope = TokenScope::Custom(set);
+        assert!(scope.allows_resource("willow://server/join-links"));
+    }
+
     #[test]
     fn messaging_allows_only_messaging_tools() {
         let scope = TokenScope::Messaging;
@@ -88,19 +147,25 @@ mod tests {
         assert!(!scope.allows_tool("kick_member"));
         assert!(!scope.allows_tool("create_server"));
         assert!(scope.allows_resource("willow://identity"));
+        assert!(scope.allows_resource("willow://server/channels"));
     }
 
     #[test]
     fn custom_allows_only_listed_tools() {
         let mut set = HashSet::new();
         set.insert("send_message".to_string());
         set.insert("react".to_string());
+        // Custom is also an explicit resource allowlist now (issue #436),
+        // so include the identity URI to keep its access in this test.
+        set.insert("willow://identity".to_string());
         let scope = TokenScope::Custom(set);
         assert!(scope.allows_tool("send_message"));
         assert!(scope.allows_tool("react"));
         assert!(!scope.allows_tool("create_channel"));
         assert!(!scope.allows_tool("kick_member"));
         assert!(scope.allows_resource("willow://identity"));
+        // URI not in the set is denied.
+        assert!(!scope.allows_resource("willow://server/channels"));
     }
 
     #[test]

crates/agent/tests/e2e.rs

@@ -573,17 +573,72 @@ async fn readonly_token_hides_tools() {
         visible.iter().map(|t| &t.name).collect::<Vec<_>>()
     );
 
-    // Resources should all be visible
+    // All resources except `willow://server/join-links` are visible. The
+    // join-links resource exposes single-step bearer credentials usable by
+    // `WireMessage::JoinRequest`, so it is restricted to `Full`/`Admin`
+    // (audit finding AUD-2, issue #436).
     let resources = willow_agent::resources::list_resources();
     for r in &resources {
-        assert!(
-            server.scope.allows_resource(&r.raw.uri),
-            "ReadOnly should allow resource: {}",
-            r.raw.uri
-        );
+        if r.raw.uri == "willow://server/join-links" {
+            assert!(
+                !server.scope.allows_resource(&r.raw.uri),
+                "ReadOnly must NOT expose {}: link_id is a bearer credential",
+                r.raw.uri
+            );
+        } else {
+            assert!(
+                server.scope.allows_resource(&r.raw.uri),
+                "ReadOnly should allow resource: {}",
+                r.raw.uri
+            );
+        }
     }
 }
 
+/// Integration-tier check: under `ReadOnly`, the same filter pipeline used by
+/// `WillowMcpServer::list_resources` must omit the join-links entry.
+///
+/// `ServerHandler::list_resources` cannot be invoked directly from outside
+/// rmcp (the `RequestContext<RoleServer>` is not externally constructible),
+/// so we replicate the exact filter pipeline that the handler runs:
+/// `resources::list_resources().filter(|r| scope.allows_resource(&r.raw.uri))`.
+#[tokio::test]
+async fn readonly_list_resources_omits_join_links() {
+    use willow_agent::scopes::TokenScope;
+    use willow_agent::server::WillowMcpServer;
+
+    let (client, _broker) = test_client();
+    let server = WillowMcpServer::with_scope(client.clone(), TokenScope::ReadOnly);
+
+    let visible: Vec<String> = willow_agent::resources::list_resources()
+        .into_iter()
+        .filter(|r| server.scope.allows_resource(&r.raw.uri))
+        .map(|r| r.raw.uri)
+        .collect();
+
+    assert!(
+        !visible.iter().any(|u| u == "willow://server/join-links"),
+        "ReadOnly list_resources must omit join-links, got: {visible:?}"
+    );
+    // Sanity: other resources are still listed.
+    assert!(visible.iter().any(|u| u == "willow://identity"));
+    assert!(visible.iter().any(|u| u == "willow://server/channels"));
+
+    // `Full` scope must still see join-links (positive control).
+    let full = WillowMcpServer::with_scope(client, TokenScope::Full);
+    let visible_full: Vec<String> = willow_agent::resources::list_resources()
+        .into_iter()
+        .filter(|r| full.scope.allows_resource(&r.raw.uri))
+        .map(|r| r.raw.uri)
+        .collect();
+    assert!(
+        visible_full
+            .iter()
+            .any(|u| u == "willow://server/join-links"),
+        "Full scope should still list join-links"
+    );
+}
+
 #[tokio::test]
 async fn messaging_scope_restricts_tools() {
     use willow_agent::scopes::TokenScope;
@@ -733,31 +788,33 @@ async fn read_resource_allowed_uri_returns_resource() {
     );
 }
 
-/// Test that `WillowMcpServer::read_resource` would reject a URI that the scope
-/// denies, with the same error code (`INVALID_REQUEST`) that `call_tool` uses.
+/// Test that `WillowMcpServer::read_resource` rejects `willow://server/join-links`
+/// for a `ReadOnly`-scoped token with `INVALID_REQUEST`, matching the error
+/// code `call_tool` uses when scope blocks a tool.
 ///
-/// `read_resource` on `WillowMcpServer` mirrors `call_tool`'s gate:
+/// `read_resource` on `WillowMcpServer` has two layers:
 /// 1. Scope check — returns `Err(ErrorData)` immediately if the URI is blocked
 /// 2. Dispatch to `resources::read_resource` — only reached if scope allows
 ///
-/// Today no built-in `TokenScope` variant denies any resource, so we drive the
-/// negative path with a stub closure that mimics a future scope returning false
-/// for one URI. This pins the contract: blocked reads return `INVALID_REQUEST`,
-/// not silently dispatch. Pattern mirrors `readonly_scope_rejects_send_message`.
+/// Layer 1 is what this test pins: under `ReadOnly`, join-links is now denied
+/// (audit finding AUD-2, issue #436). The `RequestContext<RoleServer>` needed
+/// by the `ServerHandler` trait method cannot be constructed from outside the
+/// rmcp crate, so we replicate the exact gate that `read_resource` runs.
 #[tokio::test]
-async fn denied_uri_rejects_with_invalid_request() {
+async fn readonly_scope_rejects_join_links_read() {
     use rmcp::model::ErrorCode;
+    use willow_agent::scopes::TokenScope;
+    use willow_agent::server::WillowMcpServer;
+
+    let (client, _broker) = test_client();
+    let server = WillowMcpServer::with_scope(client, TokenScope::ReadOnly);
 
-    // Stub scope that denies one specific URI — mimics what a future tightened
-    // scope (e.g. hiding `willow://server/join-links` from messaging tokens)
-    // would do once `TokenScope::allows_resource` returns something other
-    // than `true`.
     let denied_uri = "willow://server/join-links";
-    let scope_allows_resource = |uri: &str| uri != denied_uri;
 
+    // The real `ReadOnly` scope must deny the join-links URI now.
     assert!(
-        !scope_allows_resource(denied_uri),
-        "stub scope must deny {denied_uri}"
+        !server.scope.allows_resource(denied_uri),
+        "ReadOnly must deny {denied_uri} (link_id is a bearer credential)"
     );
 
     // Replicate exactly what WillowMcpServer::read_resource does before
@@ -783,10 +840,10 @@ async fn denied_uri_rejects_with_invalid_request() {
         "error message should mention the blocked URI"
     );
 
-    // Sanity: a URI the stub allows must pass the gate.
+    // Sanity: non-credential URIs still pass under ReadOnly.
     assert!(
-        scope_allows_resource("willow://identity"),
-        "stub scope must allow non-denied URIs"
+        server.scope.allows_resource("willow://identity"),
+        "ReadOnly must still allow non-credential URIs"
     );
 }