Repository navigation
fix(mcp-bridge): harden argument handling for routed tools - #349
Merged
Merged
Conversation
The browser / cloud / comms / ml tools share a cartridge and are told apart by a routing key derived from the tool name. Route them from one table (ROUTED_TOOLS), write the routing key after the caller's arguments, and refuse arguments their inputSchema does not declare. Non-routed tools keep their existing argument handling. Adds mcp-bridge/tests/routing_args_test.js (node, deno and bun) and runs it in e2e.yml and `npm test`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 40 minutes. View limit detailsLimit details: You’ve used the included review currently available. Review configuration: ⚙️ Run configuration
📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🏁 path-claims benchCommit NumbersHost-dependent — compare deltas across commits, not absolute values. |
🔍 Hypatia Security ScanFindings: 113 issues detected
View findings[
{
"reason": "Job `sonarqube` in build.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/build.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "sonarqube"
},
{
"reason": "Job `triage` in label-triage.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/label-triage.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "triage"
},
{
"reason": "Job `sync` in labels.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/labels.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "sync"
},
{
"reason": "Job `deploy` in pages-deploy.yml has no `timeout-minutes:` declaration. Default is 6 hours — a stuck codeload fetch or runner hang can burn budget. Add `timeout-minutes: 10` (or proportional).",
"type": "missing_timeout_minutes",
"file": ".github/workflows/pages-deploy.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"recipe_id": "recipe-add-workflow-timeout-minutes",
"job": "deploy"
},
{
"reason": "Step uses `peter-evans/repository-dispatch` with `token: ${{ secrets.FARM_DISPATCH_TOKEN }}` but has no `if: secrets.FARM_DISPATCH_TOKEN != ''` gate. On repos where the secret hasn't been propagated the action fails on every push, red-maining the repo. Add the step-level gate (or env+if pattern) so the missing-secret path is a clean skip instead of a red.",
"type": "secret_action_without_presence_gate",
"file": ".github/workflows/instant-sync.yml",
"action": "peter-evans/repository-dispatch",
"rule_module": "workflow_audit",
"severity": "high",
"fix_recipe": "add_secret_presence_gate"
},
{
"reason": "codeql.yml does not list `language: actions` in its matrix, but the repo has workflow files. CodeQL's `actions` language scans workflow YAML for injection and other CI/CD-specific weaknesses — every repo with workflows benefits. Add an entry to `matrix.include` with `language: actions` + `build-mode: none`.",
"type": "codeql_missing_actions_language",
"file": ".github/workflows/codeql.yml",
"action": "flag",
"rule_module": "workflow_audit",
"severity": "medium",
"fix_recipe": "add_codeql_actions_language"
},
{
"line": 39,
"reason": "job in .github/workflows/labels.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/labels.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
},
{
"line": 46,
"reason": "job in .github/workflows/push-email-notify.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/push-email-notify.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
},
{
"line": 32,
"reason": "job in .github/workflows/build.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/build.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
},
{
"line": 44,
"reason": "job in .github/workflows/container-publish.yml references `secrets.*` but does not install `step-security/harden-runner` — review outbound-egress monitoring",
"type": "RE001",
"file": ".github/workflows/container-publish.yml",
"action": "report",
"rule_module": "research_extensions",
"severity": "medium"
}
]Powered by Hypatia Neurosymbolic CI/CD Intelligence |
7 of 14 tasks
hyperpolymath
added a commit
that referenced
this pull request
Oct 8, 2026
## Summary Every MCP tool's `inputSchema` declares `additionalProperties: false`, but the bridge enforced that only for the 13 routed tools (browser/cloud/comms/ml, since #349). This PR enforces it for every tool in the full list, so a call carrying an argument its schema does not declare is refused with JSON-RPC `-32602` before dispatch. ## 📌 New pins Head SHA: **`f3b48f2b1d7c18f9f8f85a6db3312fd59b761d78`**. No action, lockfile or container pins added or changed. ## Changes - `mcp-bridge/lib/dispatcher.js`: `validateRoutedArgs` becomes `validateDeclaredArgs` and applies to every tool. The routing-key refusal for routed tools is kept. The deprecated `coord_promote_to_supervisor` alias is checked against `coord_promote_to_master`'s schema, and a tool missing from the full list is refused as `-32601 Unknown tool` in the gate. - `mcp-bridge/lib/tools.js`: declares arguments the handlers already read. - `sender_role` (optional enum) on `coord_send` and `coord_send_gated`. - `role` and `capabilities` on `coord_register`, copied from the local-coord-mcp `cartridge.json`, which already accepts them. - `mcp-bridge/tests/declared_args_test.js` (new): - one case per tool plus the alias, checking that an undeclared argument is refused; - one case per tool checking that every declared argument is accepted; - three end-to-end `tools/call` cases (non-routed, coord, unknown tool). - `mcp-bridge/tests/routing_args_test.js`: the old test asserted that `coord_send` accepts an undeclared `sender_role`. It now asserts that `coord_send` accepts the declared argument. - `package.json` `test` script runs the new file. `CHANGELOG.adoc` has a line under Unreleased. **Scope:** this checks top-level argument names only. Types, `required` and enums are unchanged. ## RSR Quality Checklist ### Required - [x] Tests pass: `bun test mcp-bridge/tests/` gives 210 pass, 0 fail, and `npm run test` (the repo's `node --test` script) gives 173 pass, 0 fail. - [ ] Code is formatted: the repo has no JS formatter configured for `mcp-bridge/`. I matched the surrounding style. - [ ] Linter is clean: no JS linter is configured for `mcp-bridge/`. Not run. - [x] No banned language patterns: plain `.js` only, no new TypeScript or Python. - [ ] No `unsafe` blocks without `// SAFETY:` comments: not applicable, no Rust or Zig touched. - [x] No banned functions. - [x] SPDX license headers present: the new test file carries the MPL-2.0 header used by its siblings. - [x] No secrets, credentials, or `.env` files included. ### As Applicable - [ ] `.machine_readable/*.a2ml`: not applicable; A2ML is retired (D308). - [x] Documentation updated for user-facing changes: `CHANGELOG.adoc`. - [ ] `TOPOLOGY.md`: not applicable, architecture unchanged. - [x] `CHANGELOG` updated. - [ ] New dependencies reviewed: not applicable, there are none. - [ ] ABI/FFI changes validated: not applicable, no `src/abi/` or `ffi/zig/` change. ## Pre-existing red checks (deferred) Both checks below are also red on `main` at `7dd5897d`. This PR does not touch the code either one covers. - `governance / UUID v7 conformance` is deferred to #347. - `SonarQube` is deferred to #338. ## Testing - **Suite:** - `bun test mcp-bridge/tests/`: 210 pass, 0 fail. - `npm run test`: 173 pass, 0 fail. - **Planted positive (mutant run):** I put the pre-change behaviour back temporarily (`if (!routingKey) return null;`, which checks routed tools only). With that in place, `declared_args_test.js` fails exactly 58 cases: 55 non-routed tools, the alias, and the two end-to-end refusals. 84 cases still pass. Then I restored the fix. - **Schema coverage:** - Every `args.<field>` read in `api-clients.js` (60 reads) is declared. I confirmed the check catches a missing field by planting one undeclared read. - The coord tool schemas match the local-coord-mcp manifest, apart from the two `coord_register` fields now added. - The per-tool table test calls `validateDeclaredArgs` directly, so the 60/min rate limiter cannot change which error a refusal returns. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Harden argument handling in the MCP bridge for the routed tools: browser, cloud, comms and ml. These tools share a cartridge and are told apart by a routing key derived from the tool name. After this change the tool name alone decides the route, and arguments that a routed tool's
inputSchemadoes not declare are refused with-32602.📌 New pins
Head SHA: f01e55a. This PR adds or changes no action, lockfile or container pins.
Changes
mcp-bridge/lib/dispatcher.js:ROUTED_TOOLStable (tool name → cartridge plus routing key). It replaces fourswitcharms. The routing key is written after the caller's arguments.validateRoutedArgsanddeclaredArgs. The hardening gate refuses arguments that a routed tool'sinputSchemadoes not declare.arguments.dispatchTool,hardeningGateand the new helpers.coord_sendreadssender_role, which no schema declares, so applying the check to every tool would break it.mcp-bridge/tests/routing_args_test.js(new, 16 tests). Each routed tool still reaches its cartridge with the right key. An argument cannot change the routing key. Undeclared arguments are refused. Non-routed handling is unchanged.fetchis stubbed for this file only and restored afterwards, because bun shares one process across files..github/workflows/e2e.yml(node, deno and bun unit lines) andpackage.jsontestnow include the new file.RSR Quality Checklist
Required
--test: 68/68 across the four bridge test files.bun test: 68/68.deno test: 68/68.mcp-bridge/JS. The code follows the surrounding style by hand.mcp-bridge/locally. CI scanners will report on this PR.npm testscript line already existed and only gained a file name.unsafeblocks: there is no Rust or Zig in this change.MPL-2.0. The modified files keep theirs..envfiles.As Applicable
.machine_readable/*not updated: project state and integrations are unchanged.TOPOLOGY.mdnot updated: architecture is unchanged.Testing
node --test mcp-bridge/tests/{routing_args,dispatch,http_transport,path_claims}_test.js: 68 pass, 0 fail.bun test(same files): 68 pass, 0 fail.deno test --allow-read --allow-env --allow-run --allow-net(same files): 68 passed.routing_args_test.jsagainstorigin/main'sdispatcher.jsgives 8 pass and 8 fail. Every routing and undeclared-argument test fails there; the well-formed-call and non-routed tests pass.🤖 Generated with Claude Code
https://claude.ai/code/session_019j8She9eTFx54r6aL6sCHP