Repository navigation
fix(ci): read MCP tools from the registry in the agent trust guard - #459
Conversation
…st guard
The guard only reads direct server.registerTool('<literal>', ...) calls.
Since #447 the MCP server registers every tool through one generic
registrar dispatched from the tool registry, so these cases fail until
the guard reads that form.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
#447 moved MCP tool registration from literal server.registerTool calls to TOOL_REGISTRY plus one generic registrar, so check:agent-trust failed on main with 'MCP registrations require supported literal tool names'. The guard now reads literal tool names from TOOL_REGISTRY and accepts a non-literal registerTool name only inside a const registrar = (spec, handler) => (definition) => ... factory. That registrar may only be called as a literal-keyed value of one const handler map, and the map may only be dispatched as map[definition.name](definition). Registered names therefore equal the handler map keys, which must equal the registry names, and each handler must call exactly one ./tools/ implementation listed in the feature map. The feature map gains the seven trade tools, in registry order. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4faab6fabe
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (name && ts.isStringLiteralLike(name)) { | ||
| addTool(name.text, handlerImplementations(node.arguments[2])); |
There was a problem hiding this comment.
Reject direct registrations when using the registry
When the registry-based registrar exists, this branch still accepts a literal server.registerTool('extra', ...) and adds it only to registeredTools. If the feature map also lists extra, both checks pass because the registry comparison uses only handlerMapTools, even though the tool bypasses TOOL_REGISTRY, toolset filtering, and read-only selection. This is especially risky for an environment-conditional registration that tests do not exercise. Reject literal registrations in registry mode, or include them in the registry-to-handler comparison.
AGENTS.md reference: AGENTS.md:L79-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 72e7755 (failing tests first in b4b0bc4, where 15 new cases fail against the previous guard). Once server.ts has a registry registrar, any literal registerTool('<name>', ...) now fails the guard with MCP tool <name> at <file:line> bypasses TOOL_REGISTRY; register every tool through the registry handler map, even when the feature map lists it. Tests cover the literal call placed both before and after the registrar.
| if (ts.isCallExpression(entry)) { | ||
| const [name] = entry.arguments; | ||
| if (name && ts.isStringLiteralLike(name)) return name.text; | ||
| } | ||
| if (ts.isObjectLiteralExpression(entry)) { | ||
| const names = entry.properties.filter( | ||
| (property) => | ||
| !ts.isPropertyAssignment(property) || | ||
| literalPropertyName(property.name) === 'name', |
There was a problem hiding this comment.
collectRegistryToolNames accepts any call with a literal first argument, even when it is not readTool. It also ignores computed properties that could overwrite an object's name. A future edit can therefore pass this check while the server looks up a handler under a different name. Check the helper being called, reject computed properties that could replace name, and add negative tests for both forms.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/check-agent-trust.mjs
Line: 259-267
Comment:
**Wrong registry names can pass**
`collectRegistryToolNames` accepts any call with a literal first argument, even when it is not `readTool`. It also ignores computed properties that could overwrite an object's `name`. A future edit can therefore pass this check while the server looks up a handler under a different name. Check the helper being called, reject computed properties that could replace `name`, and add negative tests for both forms.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 72e7755 (failing tests first in b4b0bc4, where 15 new cases fail against the previous guard). A call entry in TOOL_REGISTRY is accepted only when its callee resolves to the readTool function declared in registry.ts, and that function is verified first: one declaration whose body is a single return of an object literal with only literal keys (no spread or computed key) and exactly one name property bound to its first parameter. Object entries must use only literal-keyed property assignments, so a spread, shorthand, or computed key such as ['na' + 'me'] is rejected. Tests cover otherTool('tool_a', ...), a computed key in an entry, and a readTool that returns a fixed name, spreads extra properties, adds a computed key, or is missing.
| ); | ||
| const name = literalPropertyName(property.name); | ||
| handlerMapTools.push(name); | ||
| addTool(name, handlerImplementations(call.arguments[1])); |
There was a problem hiding this comment.
The implementation check reads the handler map's second argument without checking that the registrar uses it. A registrar that ignores that argument and registers () => executeB() still passes when the map supplies () => executeA() and the feature map lists its file. This leaves future changes to the registered callback unchecked. Verify that the registrar forwards the supplied handler, and add a test where it replaces that handler.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/check-agent-trust.mjs
Line: 491
Comment:
**Wrong handlers can pass**
The implementation check reads the handler map's second argument without checking that the registrar uses it. A registrar that ignores that argument and registers `() => executeB()` still passes when the map supplies `() => executeA()` and the feature map lists its file. This leaves future changes to the registered callback unchecked. Verify that the registrar forwards the supplied handler, and add a test where it replaces that handler.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 72e7755 (failing tests first in b4b0bc4, where 15 new cases fail against the previous guard). The registrar factory's second parameter must now reach registerTool's callback: every use of it has to sit inside the third argument as a call or a call argument, and the factory may call nothing imported from ./tools/ except the context helper (packages/mcp/src/tools/context.ts, an explicit allowlist in the guard). Tests cover a registrar that replaces the handler with () => executeB(), wraps it with an extra implementation call, uses it only conditionally, references it outside the callback, or registers no callback.
| references( | ||
| checker.getSymbolAtLocation(mapDeclaration.name), | ||
| mapDeclaration.name, | ||
| (reference) => { |
There was a problem hiding this comment.
Missing dispatch goes unnoticed
Removing the dispatch loop leaves no map references to reject, but the guard has already counted every map key as a registered tool. The check therefore stays green even when this path registers nothing. Require at least one supported dispatch and add a test with the dispatch removed.
Prompt To Fix With AI
This is a comment left during a code review.
Path: bin/check-agent-trust.mjs
Line: 494-497
Comment:
**Missing dispatch goes unnoticed**
Removing the dispatch loop leaves no map references to reject, but the guard has already counted every map key as a registered tool. The check therefore stays green even when this path registers nothing. Require at least one supported dispatch and add a test with the dispatch removed.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.There was a problem hiding this comment.
Fixed in 72e7755 (failing tests first in b4b0bc4, where 15 new cases fail against the previous guard). Map keys still count as registered tools, but the guard now requires the handler map to be dispatched at least once, and every dispatch must be tools[definition.name](definition) as the body of a for (const definition of ...) loop over that same definition. Tests cover removing the dispatch entirely and dispatching once outside a loop.
Add cases the registry path must reject: a literal registerTool next to the registrar, a registry entry built by a helper other than a verified readTool or with a computed key, a registrar that does not forward its handler, and a handler map that is never dispatched from a for...of loop over the definition. These fail against the current guard. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The guard now fails closed when server.ts registers any literal tool alongside the registry registrar, when a registry call is not to a readTool whose body returns its first parameter as the only name, when an object entry has a spread, shorthand, or computed key, when the registrar does not pass its handler (and no tool implementation) to registerTool's callback, and when the handler map is not dispatched as for (const definition of ...) tools[definition.name](definition). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 72e775577a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const [loopVariable] = ts.isForOfStatement(loop) | ||
| ? (loop.initializer.declarations ?? []) | ||
| : []; |
There was a problem hiding this comment.
Validate the dispatch loop's iterable
In the added registry fixture, replacing selected with [] still makes the acceptance test pass because this validation checks the loop variable but never inspects loop.expression. A future change could therefore iterate TOOL_REGISTRY and bypass toolset/read-only selection, or iterate an empty/unrelated collection and register nothing, while the registry, handler-map, and feature-map comparisons remain green. Require the iterable to resolve to the result of the supported selectTools path.
AGENTS.md reference: AGENTS.md:L80-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in acd102c. Before the change, a dispatch loop over [] passed the guard. The loop must now iterate either selectTools(...) imported from ./tools/registry.js, or a const bound to that call, which is the shape server.ts uses (const selectedTools = selectTools({...})). selectTools itself must be export function selectTools returning TOOL_REGISTRY.filter(...). New fixtures reject loops over TOOL_REGISTRY, [], an unrelated const, a let binding, and a selectTools that returns its own list. All of them failed before the fix.
| const [literal] = literalRegistrations; | ||
| if (literal) | ||
| throw new Error( | ||
| `MCP tool ${literal.text} at ${at(literal)} bypasses TOOL_REGISTRY; register every tool through the registry handler map`, | ||
| ); |
There was a problem hiding this comment.
Reject literal registrations when the registry is empty
The mixed-registration case from the prior review is fixed, but fresh fixture evidence shows that an empty TOOL_REGISTRY plus exclusively literal registerTool calls still passes: registrar remains undefined, this rejection is skipped, the registry-to-handler comparison sees two empty lists, and the feature map can match the direct calls. That permits a wholesale move away from registry filtering without CI noticing, so reject literalRegistrations whenever the registry source exists rather than only inside the if (registrar) branch.
AGENTS.md reference: AGENTS.md:L80-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in acd102c. Before the change, an empty TOOL_REGISTRY with only literal registerTool calls passed. The guard now rejects an empty TOOL_REGISTRY, and it rejects any literal registerTool call whenever packages/mcp/src/tools/registry.ts exists, not only when a registrar is found. Fixtures cover the empty-registry case and a literal registration beside a non-empty registry with no registrar. Both failed before the fix. npm run check:agent-trust still passes on the real server.ts and registry.ts.
The registry guard checked the dispatch loop's variable but not what it iterated, so a loop over TOOL_REGISTRY or [] passed. It also rejected literal registerTool calls only when a registrar existed, so an empty TOOL_REGISTRY plus literal registrations passed. The loop must now iterate selectTools(...) imported from ./tools/registry.js, or a const bound to that call. selectTools must return TOOL_REGISTRY.filter(...), TOOL_REGISTRY must be non-empty, and any literal registration fails whenever the registry module exists. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: acd102ca53
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| filtered.name.text !== 'filter' || | ||
| !ts.isIdentifier(filtered.expression) || | ||
| checker.getSymbolAtLocation(filtered.expression)?.valueDeclaration !== | ||
| registryDeclaration |
There was a problem hiding this comment.
Enforce the
selectTools predicate
This validation only confirms that selectTools returns TOOL_REGISTRY.filter(...); it accepts return TOOL_REGISTRY.filter(() => true). In that case servers configured for a restricted toolset or read-only operation register every tool while the agent-trust guard remains green. Validate the filtering semantics or exercise both toolset and read-only selections behaviorally.
AGENTS.md reference: AGENTS.md:L80-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not extending the static guard for this one. The guard's job is to keep the set of registered tool names and their implementation files statically reviewable, and it now pins the dispatch to selectTools(...) from the registry. What the filter selects is behavior, and behavior tests own it: packages/mcp/src/tools/registry.test.ts asserts that a documents-only selection returns exactly the documents tools and that read-only mode drops every non-read-only tool (track_container included). A TOOL_REGISTRY.filter(() => true) rewrite fails both tests. Proving filter semantics from the AST would turn the guard into a partial interpreter, which each review round shows is a moving target.
| return ( | ||
| !!declaration && | ||
| ts.isVariableDeclaration(declaration) && | ||
| ts.isVariableDeclarationList(declaration.parent) && | ||
| (declaration.parent.flags & ts.NodeFlags.Const) !== 0 && | ||
| isSelectToolsCall(declaration.initializer) |
There was a problem hiding this comment.
Tie dispatch selection to the configured options
The follow-up validation now accepts any imported selectTools(...) call without inspecting its arguments. Changing the initializer or loop to selectTools({ toolsets: DEFAULT_TOOLSETS, readOnly: false }) therefore passes the guard while ignoring options.toolsets and options.readOnly, potentially exposing write or disallowed tools. Require the dispatch iterable to originate from the intended options-based selection rather than merely checking the callee name.
AGENTS.md reference: AGENTS.md:L80-L81
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Same reasoning as the sibling thread: this is selection behavior, which tests cover rather than the AST guard. If server.ts passed fixed arguments to selectTools, the request's toolset selection would stop taking effect. packages/mcp/src/tools/registry.test.ts builds the real server with toolsets: [] and asserts tools/list is empty ("answers tools/list with an empty list when no toolset has tools"), so fixed arguments fail it; tests/api-handler.test.ts separately asserts the gateway passes the narrowed toolsets and read-only flag into the server. Keeping the guard structural and the semantics in tests.
Bring in the tool registry and toolsets (#447) and the registry-aware agent trust guard (#459). set_party_role and set_custom_field now register through TOOL_REGISTRY in the core toolset as tier A writes, after list_parties, keeping their destructive and idempotent annotations. Read-only mode drops them. The write deadline moves from a positional argument to the writeDeadlineAt server option, and the gateway passes it there. The tier invariant test now requires destructiveHint only for tier C, so a reversible tier A write can still be marked destructive. The server instructions keep the three-writes paragraph for the default toolsets and no longer claim that no tool deletes or overwrites data. The tool list and instruction snapshots, the feature map and the read-only header docs name the two tools. The Postman collection is main's, since docs/openapi.json matches main. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
Problem.
npm run check:agent-trustfails on main, so CI is red for every PR:Cause. The guard from #384 found MCP tools by reading literal
server.registerTool('<name>', ..., handler)calls inpackages/mcp/src/server.ts. #447 replaced those calls withTOOL_REGISTRYinpackages/mcp/src/tools/registry.tsand one genericserver.registerTool(definition.name, ...)inside thetoolregistrar. The guard cannot read a non-literal name, so it fails. The feature map also still listed 14 tools, and main registers 21.Fix. The guard now reads the registry form, and it stays syntax-aware and fail-closed:
TOOL_REGISTRYentries. Each entry must bereadTool('<literal>', ...)or an object literal with a literalnameand no spreads.registerToolname is accepted only as<definition>.nameinside oneconst <registrar> = (spec, handler) => (definition) => ...factory. Any other non-literal name still fails with the original message, now with a source line.consthandler map. The map may only be dispatched asmap[definition.name](definition), so the registered name is always a map key../tools/implementation, and that file must match the feature map.registerToolcalls are still counted, so the existing receiver and alias tests are unchanged.skills/agent-trust/feature-map.jsongains the seven trade tools (all inpackages/mcp/src/tools/trade-intel.ts), in registry order. The paved-path guide now describes the registry plus handler-map steps for adding a tool.How the guard still fails closed. The 14 new tests in
bin/check-agent-trust.test.mjsuse a fixture with main's registry and handler-map shape. They reject an unmapped new tool, a handler bound to the wrong implementation, a registry entry with no handler, non-literal registry entries, computed map keys, a registrar used outside the map, a fabricated or mismatched dispatch, and a dynamic name outside a registrar. The test commit comes before the fix. Against the old guard, those 14 fail and the 53 existing tests pass.I also ran the guard on the real source with an unlisted tool. Adding
probe_unlistedto the registry and the handler map gaveMCP feature map drifted, withprobe_unlistedin the registered list. With the tool in the registry only, the guard gaveMCP tool registry against the server handler map drifted. I reverted both edits.Verification.
npm run check:agent-trust:PASS agent trust guards,PASS OpenAPI SDK and MCP generation.npm run test:agent-trust: 72 pass, 0 fail.npx vp run check: exit 0.npx vp run test: exit 0. That covers agent-trust 72, risk-policy 49, and the package suites (153, 418 and 550 tests).Not in this PR. The "Advisory policy (never authorizes or merges)" job fails for every PR with
Resource not accessible by integration. That failure started before this change and is not caused by it.🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmith-botwith what you need.The PR appears safe to merge, with three non-blocking improvements to keep the guard from approving unsupported registration changes.
Fix with agent prompt
Summary
Updates the agent-trust guard to read
TOOL_REGISTRYand the server's handler map.Acknowledged exclusions:
Resource not accessible by integrationfailure as pre-existing and outside this PR.Reviews (1) · Last reviewed commit: "fix(ci): read MCP tools from the registr..." · Reviewed by Greptile