fix(mcp): gate the loop tools on the agent permission edge, and make the gate a mechanism (abilityai/trinity-enterprise#628)

[webmixgamer]

Summary

• run_agent_loop never ran the agent permission edge (checkAgentAccess), and the backend behind it resolves an agent-scoped key to its owner (Invariant security: implement safe tar extraction with symlink/hardlink validation #8) — so an agent key could start a loop on any same-owner sibling with no agent_permissions edge. Reproduced live: the loop ran on the sibling.
• The gate was a per-tool discipline with ten spellings across nine modules; the tool added last called none of them. This PR makes it a mechanism: one implementation of the agent-scope edge (src/mcp-server/src/access.ts::checkAgentEdge), a TOOL_ACCESS_POLICY row for every registered tool, a registration-time assertion (server.ts refuses a tool without a row, an enforce on a parameter the tool does not declare, or a none on a tool whose parameters name an agent), and an enforce-wrapper applied before withAudit.
• The loop-id tools (get_loop_status, stop_loop) resolve the loop's agent, gate, then act; a denial withholds the payload behind Loop '<id>' not found or not accessible (the id was the caller's only input — naming the agent would disclose whose loop it is); a failed resolve sends no stop and names the escape hatch.
• chat.ts's agent/system branch now delegates to the shared gate; unknown scopes are denied before its user branch — an allowlist (feat: machine identities for admin/ops APIs — service credentials that survive enforced 2FA #2323) that closes a fall-through which promoted an unnamed agent key to the same-owner rule. Zero behaviour change for live scopes (OPERATOR_SCOPES = user/agent/system; verified every caller).
• This is a tool-surface gate, not a capability boundary. The REST routes behind the tools are still owner-equivalent for an agent key; that ruling is abilityai/trinity-enterprise#629, and the 53 baselined rows in the table are its work list (the destructive subset — delete_agent, start_agent, stop_agent, set_agent_github_pat, export_agent_data, call_user, get_agent_auth — is named there).

Changes

• src/mcp-server/src/access.ts (new) — checkAgentEdge, resolveClient (moved from chat.ts), TOOL_ACCESS_POLICY (130 rows: 1 enforce, 31 in-tool, 53 baselined, 45 none), policyFor, withAgentAccess
• src/mcp-server/src/server.ts — addToolWithAudit runs policyFor; enforce rows wrapped; registerDynamicTool takes a policy
• src/mcp-server/src/tools/loops.ts — verified header; resolve → gate → act for the loop-id pair
• src/mcp-server/src/tools/chat.ts, tools/dynamic-agents.ts, types.ts, client.ts
• .github/workflows/mcp-server-test.yml — offline ESM boot smoke of dist/server.js (bundler resolution accepts an extensionless import that Node ESM rejects at runtime)
• Tests: src/mcp-server/src/access.test.ts (14), src/mcp-server/src/tools/loops.test.ts (16), src/mcp-server/src/access-wiring.test.ts (3 — review C1: the registered tool over a real transport with an agent key and a counted backend), tests/journeys/test_j10_agent_calls_agent_journey.py (strict xfail removed; +1 loop-id denial test)
• Docs: run-agent-loop.md, agent-permissions.md, agent-to-agent-collaboration.md, mcp-agent-exposure.md, mcp-orchestration.md, architecture/mcp-server.md, requirements/scheduling.md §38.1, orchestration-invariant-catalog.md P-02, learnings.md, docs/security-reports/cso-diff-2026-09-15-628-loop-permission-gate.*

Test Plan

• MCP package: npm run build clean; npm test 409/409 (375 → +34)
• Wiring executed (review C1): access-wiring.test.ts boots createServer in key mode, presents an agent-scoped key over the streamable-HTTP transport, and the stub backend counts POST /api/agents/<target>/loops — no edge ⇒ denial and zero posts; edge ⇒ the loop starts; self ⇒ starts with no permission read
• Mutation battery (scratch copies, restored byte-identical): wrapper bypass, dropped gate on stop_loop, a tool losing its row, a loop_id-only tool losing its row, gate result ignored, gate after the side effect, a stale row, unknown-scope fallthrough, unnamed agent key, payload returned on denial — 10/10 red
• Boot smoke: node --input-type=module -e "await import('./dist/server.js')" resolves
• Live on a local stack (rebuilt image): the original reproduction answered run_agent_loop with success:false and the byte-identical chat_with_agent reason; J10 full file 9 passed / 2 xfailed (bug: agent-to-agent chat chains have no depth guard — a runaway A→B→A bounce is bounded only by capacity parks and hop timeouts #2806, bug(mcp): a denied chat_with_agent / fan_out call is audited as a successful tool call #2807) / 2 keyed-skips — the flipped test and the new loop-id test green
• tests/unit guards that scan src/mcp-server: 864 passed
• /review 0 critical · /cso --diff 0 introduced (report committed)

Journey Impact: extends: J10

Mutation: src/mcp-server/src/access-wiring.test.ts — with server.ts::addToolWithAudit composing the wrapper and then handing tool.execute (not execute) to withAudit (the review's mutation), the no-edge case goes red: the stub backend receives POST /api/agents/bravo/loops and the caller reads success: true; restored byte-identical, 409/409. The ten-mutation battery on the gate, the policy table and the loop-id pair is listed under Test Plan.

Fixes abilityai/trinity-enterprise#628

(Cross-repo: the keyword links but does not auto-close the private issue — closed at release. AC-02's audit is posted on the issue; the 53 baselined rows are the same list in code.)

🤖 Generated with Claude Code

[dolho]

/review + /cso --diff — head 0dd58bb2 vs merge-base 0bddbc95 — REQUEST CHANGES, one finding

Files: 22 (+1328/−174) · Scope: CLEAN — the gate, its table, the registration assertion, the loop-id pair, the docs, and the J10 flip; nothing outside ent#628. Lane: C (auth boundary in src/mcp-server).

The design is the right one and I want to say so before the finding: the class is exactly Invariant #8's admin-gate story one layer up (five patched call sites ⇒ the gate was wrong), and making the row a registration-time contract that the code can contradict — a tool with no row cannot boot, an enforce on a phantom parameter cannot boot, a none on a tool naming an agent cannot boot — is the mechanism, not another discipline. I re-ran the totality mutation myself: delete get_agent_a2a_card's row → createServer refuses, 4 red. checkAgentEdge is an allowlist with the unknown-scope arm on the deny side (#2323), connector.ts/auth.ts never route through chat.ts's gate so the "zero live behaviour change" claim holds by grep, list_channel_groups' baselined note names a real Telegram gap for ent#629 rather than hiding it, and the loop-id denial is the #186 shape. 406/406, boot smoke resolves, all checks green.

❌ C1 — the one line that makes the mechanism real has no executing test (Confidence 9/10)

src/mcp-server/src/server.ts:576-586:

const execute = policy.kind === "enforce" ? withAgentAccess(tool.name, tool.execute, policy, …) : tool.execute;
const wrapped: any = { ...tool, execute: withAudit(tool.name, execute, auditTargetId) };

Mutation, restored from a scratch copy: change the second line to withAudit(tool.name, tool.execute, auditTargetId) — i.e. the wrapper is built and thrown away. npm test → 406 passed, 0 failed. run_agent_loop is ungated again and the suite is green.

Why: loops.test.ts:66-79 builds the composition itself — "run_agent_loop exactly as server.ts registers it: the real row, the real wrapper" — and access.test.ts:114 calls withAgentAccess directly. Both prove the wrapper works; neither proves addToolWithAudit applies it. The totality test (access.test.ts:51) boots createServer but reads builtinToolNames only. So the property the PR body leads with — an agent key with no edge cannot start a loop on a sibling — is delivered by unexecuted wiring, and your own mutation table's "wrapper bypass → red" must have been a different bypass than the one above. This is the #2811 class from yesterday's train (the reaction extracted and tested; the wiring that calls it not), and it is the load-bearing line here.

Fix: drive it through createServer. inline-auth-transport.test.ts already boots the server and calls tools over HTTP with a real client; add one case with an authenticate returning {scope: "agent", agentName: "a", mcpApiKey: …}, isAgentPermitted (or GET /api/agents/a/permissions) stubbed to [], call run_agent_loop with agent_name: "b", and assert the Access denied envelope — then the mutation above is red. One test; the harness exists.

Informational

• I1 withAgentAccess passes an absent target through to the tool. Correct for run_agent_loop (an agent key defaults to itself, a user key is told the parameter is required), and stated in the docstring — noting it because a future enforce row on a tool whose absent-parameter default is not self would gate nothing. policyFor cannot see defaults; the row's reviewer has to.
• I2 A returned denial is audited as a successful call — the PR names #2807 as the owner, and that is the right place for it, not here.
• I3 checkAgentEdge with no auth context is allowed — dev-mode installs with no authenticate; pre-existing behaviour moved, not widened.

Clean

• Auth: allowlist not denylist; system bypass unchanged; user passes through because the backend scopes it by role/grant this layer cannot see (#2824 named). No secret in any denial string; the loop-id denial withholds the agent name.
• Order of operations: stop_loop resolves → gates → stops; a failed resolve sends no stop and names the escape hatch (loops.test.ts case 3, executed).
• Docs: six flow docs + architecture/mcp-server.md + requirements §38.1 + catalog P-02 + learnings, all in-diff; /cso --diff report committed, 0 introduced.
• J10: the ent#628 xfail is gone and the file's remaining two are #2806/#2807 as stated.

Setting status-needs-fix per #2815; it clears on your push.

[vybe]

merge-train: not on this train — changes requested, fix not yet pushed.

The open CHANGES_REQUESTED review (C1) stands: src/mcp-server/src/server.ts:576-586 composes the withAgentAccess wrapper, and no test executes that composition through createServer — the mutation that builds the wrapper and discards it stays green at 406/406. A new test that drives run_agent_loop over the inline-auth transport with an agent key and an empty edge is what the finding calls for, which needs the author's intent rather than a mechanical fix.

Rides the next train once the test is pushed and the review is re-requested.

[github-actions]

⚠️ Nightly unit-suite check skipped — merge conflict against dev.

Resolve by running git merge dev locally and pushing the result. The next nightly run will re-test once the conflict is gone.

[webmixgamer]

C1 addressed — the composition site now has an executing test (6591704e6)

You were right, and it was the gap my own /review had listed as "I3: J10 live covers it" — the journey lane is not the unit suite, and your mutation proved it: build the wrapper in addToolWithAudit and hand tool.execute to withAudit, 406 green, run_agent_loop ungated.

Fix: src/mcp-server/src/access-wiring.test.ts, on the inline-auth-transport.test.ts harness as you suggested — a real createServer in key mode, a real MCP client presenting an agent-scoped key over the streamable-HTTP transport (the stub's /api/mcp/validate answers scope: "agent", agent_name: "alpha"), and a stub backend that answers GET /api/agents/alpha/permissions and counts every POST /api/agents/<target>/loops. Three cases:

1. no edge → the caller reads {success:false, error:"Access denied", reason:"Permission denied: Agent 'alpha' is not permitted to communicate with 'bravo'…"} and the backend received zero loop posts;
2. edge → success:true, loop_id, one post to /api/agents/bravo/loops;
3. self (no agent_name) → starts, and no permission-edge read was paid.

Your mutation, re-run against it: withAudit(tool.name, tool.execute, auditTargetId) at server.ts:586 → case 1 is the one red (loopPosts = ["bravo"], success: true); server.ts restored byte-identical (cmp), npm test 409/409. Recorded on the PR's Mutation: line per the new evidence bar (#2833).

I1 (an absent target passes through) — agreed and left as is: run_agent_loop's absent default is self, and policyFor cannot see a tool's defaults; the row's reviewer has to. Case 3 above pins today's behaviour so a row added on a tool whose absent default is not self would at least have a precedent to read. I2 — #2807's. I3 — pre-existing, moved not widened.

Also in this push: dev moved again under the branch (10 commits, incl. the J10 model-gate fix #2837), so the append-only learnings.md conflicted a second time — merged in a scratch worktree, dev's entries first (58c0e4a26, ledger 237 = dev's 236 + this PR's one); J10 auto-merged cleanly.

[dolho]

Re-review — head 6591704e6 vs dev — APPROVE

What moved on the PR's side since 0dd58bb24: one file, src/mcp-server/src/access-wiring.test.ts; the gated sources (server.ts, access.ts, loops.ts, chat.ts) are byte-unchanged from the head I reviewed, and everything else in the range is dev arriving through the merge (58c0e4a26; learnings.md at 237 = dev's 236 + this PR's one, no markers).

C1 — closed, and I verified it the way it was found. The new test boots a real createServer in key mode, presents an agent-scoped key over the streamable-HTTP transport (the stub's /api/mcp/validate answers scope: "agent"), answers GET /api/agents/alpha/permissions, and counts every POST /api/agents/<target>/loops — three cases (no edge → refusal + zero posts; edge → one post; self → no permission read). On the PR head: npm test 409/409. Then my original mutation, replayed from a scratch copy — server.ts:586 handing tool.execute to withAudit, the wrapper built and discarded — now fails exactly one test, with the observable the gate exists for:

not ok 1 - an agent key with no edge is refused at the REGISTERED tool, and no loop starts
    expected a refusal, got: {"success":true,"loop_id":"loop_1","agent_name":"bravo",…}

server.ts restored byte-identical (cmp), 409/409 again. The load-bearing line has an executing test that reads the backend's own receipt, not the tool's return value — the #2829 bar, met.

I1–I3 stand as informational (I1 pinned by case 3, as the author notes). CI on the head: 25 pass, 4 skip, 0 fail; MERGEABLE.

Design unchanged from my first pass: the row is a registration-time contract, checkAgentEdge is an allowlist, the loop-id denial is the #186 shape. Ready for the next train.