LearnNewsExamplesServices
Frontmatter
titlefix(ai): declare x-pass-as-object on the file-system MCP tools
authornovice-22
stateMerged
createdAtJul 31, 2026, 8:09 PM
updatedAtJul 31, 2026, 8:45 PM
closedAtJul 31, 2026, 8:45 PM
mergedAtJul 31, 2026, 8:45 PM
branchesdev ← fix/file-system-pass-as-object
urlhttps://github.com/neomjs/neo/pull/16235
contentTrust
projected
quarantined0
signals[]
Merged
novice-22
novice-22 commented on Jul 31, 2026, 8:09 PM

Resolves #16231

Five lines of YAML on the five file-system operations whose handler destructures a single object, plus the dispatch-level spec that was missing. ToolService.callTool() branches on x-pass-as-object: declared, the validated object reaches the handler whole; absent, the arguments are spread positionally. Every argument-taking FileSystemService handler destructures, the contract declared the annotation on nothing, and so absolutePath arrived as undefined from the day the server was added. get_mcp_tool_handbook is deliberately left unannotated — its handler takes the toolId positionally, so annotating the file wholesale would have broken it.

Evidence: L3 achieved (the real toolService.callTool() invoked against the real server surface, both dispatch branches, before and after the change; real child processes for check_syntax) → L3 required. The close-target's ACs are entirely observable inside CI, so the sandbox/achievable gap the ladder exists to track does not open here. Residual: none. The stdio-transport reading in #16231 was taken against the published 13.0.0 / 13.1.0 packages with a real MCP client; on dev I verified in-process only, which is why the stdio session is listed under Post-Merge Validation rather than claimed here.

Authored by novice-22, an external contributor. Claude Opus assisted with the diagnosis and the spec, per CONTRIBUTING §5; the analysis and the code are mine and my responsibility.

Deltas from ticket

Three, all additive:

  1. The ticket proposed the annotation on five operations. Building the cross-server case confirmed get_mcp_tool_handbook must stay unannotated in all six servers, so the spec pins that explicitly rather than leaving it implied.

  2. The cross-server assertion is not a new file. OpenApiValidatorCompliance.spec.mjs already carries the neural-link x-handler recurrence guard from #14542, which is the same assertion narrowed to one server, so this generalizes that test in place rather than starting a parallel one somewhere else. gitlab-workflow is included even though the module-level servers list in that file omits it.

  3. The same cross-server case surfaced two operations that are unannotated, do not crash, and still do not fully work. They are recorded with a reason instead of changed, since they are outside this ticket:

    • knowledge-base.get_ingestion_progress — getIngestionProgress({staleAfterMs = 60000} = {}). The = {} default keeps a positional call from throwing: destructuring a bare number yields undefined, the fallback 60000 wins, and the caller's staleAfterMs never lands.
    • github-workflow.get_pull_request_diff — getPullRequestDiff(options) documents {Object|number} and re-wraps a bare number, so pr_number still resolves. The other three declared parameters (file, sha, files_only) are dropped by arity, so a caller asking for one file's diff gets the whole diff.

    github-workflow.get_local_issue_by_id is genuinely positional (getIssueById(issueNumber)) and is listed as correct.

    Happy to file those two as their own ticket, or to fold them in here if you would rather they land together.

Test Evidence

  • test/playwright/unit/ai/mcp/server/file-system/toolServiceDispatch.spec.mjs — new, 4 tests, dispatch-level
  • test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs — one test added beside the #14542 guard, cross-server

Existing coverage is green on both sides of the defect and blind to it: FileSystemPolicy.spec.mjs substitutes callTool with a mock, and FileSystemService.spec.mjs calls the handlers directly with an object. The seam between them is the only place the defect is observable, so the new spec calls the real callTool exported by the real toolService.mjs.

npx playwright test -c test/playwright/playwright.config.unit.mjs --project=unit-brain --no-deps \
    test/playwright/unit/ai/mcp/server/file-system/ test/playwright/unit/ai/mcp/validation/
→ 62 passed

(--no-deps because neither directory needs the Chroma setup the project declares.)

Reverting only the YAML to its dev state and rerunning the same command:

→ 4 failed, 58 passed
    toolServiceDispatch.spec.mjs — every argument-taking tool survives the real dispatch
    toolServiceDispatch.spec.mjs — run_playwright_test reaches its OWN guard
    toolServiceDispatch.spec.mjs — every tool this server LISTS is annotated
    OpenApiValidatorCompliance.spec.mjs — every server: an argument-taking operation carries x-pass-as-object

A standalone probe driving the same toolService.callTool() outside Playwright, before the change on dev:

tools/list -> healthcheck, get_mcp_tool_handbook, read_file, write_file,
              list_directory, check_syntax, run_playwright_test

OK    healthcheck            {"status":"healthy"}
OK    get_mcp_tool_handbook  {"toolId":"read_file","found":true,...}
FAIL  read_file              403 Forbidden: Canonical containment could not be established (ERR_INVALID_ARG_TYPE)
FAIL  write_file             (same)
FAIL  list_directory         (same)
FAIL  check_syntax           (same)
FAIL  run_playwright_test    (same)

After:

OK    read_file              {"content":"export const ok = 1;\n"}
OK    write_file             "success"
OK    list_directory         [{"name":"probe.mjs","isDirectory":false,"isFile":true},...]
OK    check_syntax           "Syntax OK"
OK    run_playwright_test    403 Forbidden: Can only execute Playwright specs within the test/playwright/ directory.

The last line is the intended outcome, not a failure: the probe file sits outside test/playwright/, so the tool's own directory guard answers. Reaching that guard requires a real path string, which is what makes it a regression assertion — under the broken dispatch the rejection comes from the sandbox guard instead, with a different message.

Worth one note for triage: on current dev the broken dispatch no longer surfaces as a raw TypeError. The fail-closed canonicalization guard classifies ERR_INVALID_ARG_TYPE as a 403 containment refusal, so the symptom now reads as a path-permission problem rather than an argument-shape one.

Whole MCP tree, my branch against a pristine dev checkout in the same environment:

test/playwright/unit/ai/mcp/   (project unit-brain)

dev        493 passed, 11 failed
this PR    498 passed, 11 failed

The +5 are the tests this PR adds. The 11 failures are the same count, in the same two spec files, on both sides: McpServersHealth.spec.mjs and DestructiveOperationGuard.spec.mjs, neither of which this PR touches. I did not diagnose why they fail in my checkout, only that this branch does not change them.

Lints that trigger on the changed paths, all run locally:

  • ai/scripts/lint/lint-identity-vocabulary.mjs (ai/mcp/server/*/openapi.yaml) → 6 surfaces, 0 violations
  • buildScripts/util/check-aiconfig-test-mutation.mjs (test/**/*.mjs) → 1046 files, 0 new violations
  • buildScripts/util/check-ticket-archaeology.mjs (test/playwright/**/*.mjs) → 0 violations
  • npm run ai:lint-mcp-test-locations → OK

Post-Merge Validation

  • The five tools answer over a real stdio MCP client session, not just in-process dispatch
  • An agent harness that mounts the file-system server can complete a read → check_syntax → write loop
  • Decide whether the two argument-dropping operations named under Deltas become their own ticket

Commits

  • fix(ai): declare x-pass-as-object on the file-system MCP tools (#16231) — annotation, dispatch spec, and the #14542 guard generalized to every server
neo-opus-grace
neo-opus-grace APPROVED reviewed on Jul 31, 2026, 8:42 PM

PR Review Summary

Status: Approved

🪜 Strategic-Fit Decision

Per §9 Strategic-Fit Step-Back:

  • Decision: Approve
  • Rationale: This is the fix that ends a four-occurrence recurrence rather than the fourth instance of the patch. The annotation is the minimum correct change; the generalized guard converts a per-incident fix pattern into an invariant that widens with the tool surface. No debt is created and nothing is deferred into a follow-up ticket, so Approve is the honest shape rather than Approve+Follow-Up.

Peer-Review Opening: @novice-22 — this is your first contribution here and it is stronger than the bar we hold maintainers to. You diagnosed the defect from published npm packages with a real MCP client, traced it to the dispatch branch, proved the fix locally before proposing it, and then wrote the test that explains why nobody caught it. Approved, and the notes below are appreciation and one non-blocking suggestion, not gating.

🧭 Patch-Blind Premise Snapshot

  • Inputs Read Before Patch: #16231 body + discussion; the three cited prior recurrences #8332 / #9545 / #14542 (all CLOSED, same defect class); current dev ai/mcp/ToolService.mjs dispatch and initializeToolMapping(); FileSystemService.mjs handler signatures; the pre-existing #14542 recurrence guard in OpenApiValidatorCompliance.spec.mjs; the six servers' openapi.yaml annotation counts; changed-file list; exact-head CI.
  • Expected Solution Shape: Declare the annotation on exactly the argument-taking operations whose handlers destructure an object — per operation, never per file, because at least one file-system handler is genuinely positional. Plus a test at the contract/handler seam, since both sides were independently green while the join was broken. The guard should widen with the tool surface rather than name tools.
  • Patch Verdict: Matches and improves on it. The five annotations are exactly the argument-taking operations. The new dispatch spec asserts real return values rather than absence-of-throw, and the cross-server guard is generalized from the existing #14542 one rather than added beside it. dev confirms the prior guard was narrowed twice over — to neural-link and to x-handler operations only — so widening it is the structurally correct response to a fourth occurrence.
  • Premise Coherence: Coheres with verify-before-assert to an unusual degree. The author validated the fix on one operation before proposing it, then wrote assertions chosen to discriminate: the broken dispatch does not throw, it yields a 403 from the sandbox guard, so "did not throw" would have passed against the bug. Asserting file content and distinguishing which 403 returns is the difference between a test and a receipt. Also coheres with friction→gold: the "why it went unnoticed" analysis converts one defect into a standing invariant.

🕸️ Context & Graph Linking

  • Target Epic / Issue ID: Resolves #16231
  • Related Graph Nodes: #8332, #9545, #14542 (same defect class, all closed), PR #10873 (flagged the undocumented annotation)
  • Origin Session ID: 59fd6dbb-fc01-482b-a778-bed01f16e699

🔬 Depth Floor

Challenge: The two silent-degradation operations recorded as exceptions — github-workflow:get_pull_request_diff and knowledge-base:get_ingestion_progress — carry honest reasons but no pointer to where they get fixed. The author was explicit that they are "listed to keep this guard green, NOT because the dispatch is right", which is the right instinct and refuses to launder a defect into a sanctioned exception. The residual risk is temporal: once the follow-up ticket exists, an entry with a reason but no tracking anchor reads to a future maintainer as an approved permanent exception rather than known debt. Non-blocking — the entries are correct today and the ticket does not exist yet.

Worth stating plainly because it is under-sold in the PR: get_pull_request_diff silently returns the whole diff when a caller asks for one file. Agents on this project use that tool during review, and receiving more than requested reads as thoroughness rather than as a bug. That finding is arguably more valuable than the one this PR fixes, because the reported defect fails loudly and that one does not.

Rhetorical-Drift Audit (per guide §7.4):

  • PR description: framing matches what the diff substantiates. The claim is scoped to the file-system server plus a generalized guard; both are delivered.
  • Anchor & Echo summaries: the spec JSDoc describes the seam mechanically — contract branch, handler shape, why both existing suites were green — with no metaphor or overshoot.
  • [RETROSPECTIVE] tag: N/A, none claimed.
  • Linked anchors: #8332 / #9545 / #14542 verified as the same defect class on live GitHub; #10873's undocumented-annotation point is accurately characterized.

Findings: Pass.

🧠 Graph Ingestion Notes

  • [KB_GAP]: x-pass-as-object is required for correctness, silent when absent, and still undocumented in the docs surface — the exact gap PR #10873's review raised. Four recurrences suggest the annotation's discoverability is the root cause and the guard is the mitigation.
  • [TOOLING_GAP]: GitHub auto-holds workflows for first-time contributors, so CI needed an operator nudge. Nothing wrong on the author's side; worth knowing for the next external PR.
  • [RETROSPECTIVE]: The durable lesson is the test-shape one, and it generalizes past this annotation: when one suite mocks the dispatcher and another calls handlers directly, both contracts pass while the join between them is broken. Coverage of both sides is not coverage of the seam. The second lesson is the author's own: a guard whose exception list requires a written reason per entry cannot silently absorb the next defect.

🎯 Close-Target Audit

  • Close-targets identified: #16231
  • #16231 confirmed not epic-labeled (bug, testing)

Findings: Pass. Scope matches the close target exactly — the two adjacent silent-degradation defects were deliberately excluded and recorded rather than folded in.

📡 MCP-Tool-Description Budget Audit

  • Single-line preferred — no description: blocks added or modified; the diff adds only x-pass-as-object: true keys
  • No internal cross-refs introduced into any description payload
  • No architectural narrative added to descriptions
  • External standard URLs — N/A
  • 1024-char cap — unaffected

Findings: Pass. The PR touches openapi.yaml but adds no description surface.

N/A Audits — 📑 🪜 🔗

N/A across listed dimensions: a five-key annotation plus tests introduces no new consumed surface needing a Contract Ledger, no runtime effect beyond CI reach needing evidence-ladder declaration, and no skill/convention/AGENTS surface.

🧪 Test-Evidence & Location Audit

  • Execution evidence: exact-head required CI green at 1d75c262b9 (10/10).
  • Reviewer falsifier: ran the author's suites on a local checkout of the PR head — npm run test-unit -- test/playwright/unit/ai/mcp/server/file-system/toolServiceDispatch.spec.mjs test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs → 52 passed. Two named concerns checked and dropped: repoRoot/tmp is gitignored (.gitignore:81) with existing precedent in two specs, so the fixture leaves no litter; and the ticket references in test titles match established house style rather than tripping the archaeology lint, which passed.
  • Test location: both files sit with their owning suites — the dispatch spec under the file-system server's unit path, the cross-server guard beside the recurrence guard it generalizes.

Findings: Pass. The tools/list-driven assertion deserves specific credit: it reads the served surface rather than naming tools, so a seventh file-system tool added later is covered without editing this spec. That is the difference between a test that catches this bug and a guard that prevents its class.

📋 Required Actions

No required actions — eligible for human merge.

📊 Evaluation Metrics

Verdict weights: 30% premise / right thing, 30% architecture + placement, 30% diff correctness, 10% AC/audit sanity.

  • [ARCH_ALIGNMENT]: 96 - Per-operation rather than per-file, correctly identifying that get_mcp_tool_handbook must stay positional; generalizes the existing guard instead of adding a parallel one, keeping one invariant in one home.
  • [CONTENT_COMPLETENESS]: 95 - Fix, seam coverage, cross-server guard, and reasoned exception list. The two adjacent defects are recorded with reasons rather than silently absorbed or scope-crept.
  • [EXECUTION_QUALITY]: 97 - Assertions discriminate rather than merely pass: real return values, the specific 403 distinguished, negative cases proving per-operation scope, and the author's own helper made to fail loudly on an unresolved $ref because a quietly empty argument list is the shape of the defect being fixed.
  • [PRODUCTIVITY]: 95 - Restores six broken tools and closes a four-occurrence recurrence class in 223 additions, zero deletions.
  • [IMPACT]: 88 - The file-system server is not on the critical path for our current harnesses, which bounds the immediate impact; the standing invariant across all six servers is the larger and more durable part.
  • [COMPLEXITY]: 40 - Small diff, but it required understanding the dispatch branch, the argName derivation, and why two existing suites were both green.
  • [EFFORT_PROFILE]: Quick Win - Five lines of contract plus the guard that stops the class recurring.

The instinct I would most like to keep from this PR is the one in the $ref branch: you made your own helper fail loudly rather than skip silently, on the grounds that a quietly empty argument list is exactly the defect under repair. Applying a PR's lesson to its own test is not a common habit. Thank you — this is a genuinely good first contribution, and the PRs welcome badge means what it says.