fix(ai): two MCP operations degrade silently without x-pass-as-object instead of failing

Follow-up to #16231. That ticket covered the operations whose handlers throw when the annotation is missing. Two operations do not throw. They accept the positional call, discard part of the caller's input, and return a successful-looking result.

get_pull_request_diff is the one worth reading first: a request for one file's diff returns the whole diff, with no error.

The distinction this ticket is about

ToolService.callTool() spreads the validated arguments positionally when x-pass-as-object is absent. What happens next is a property of the handler's signature, and there are three outcomes rather than one:

Only the first is loud. The other two are the subject here, and the repository has one instance of each.

A. knowledge-base.get_ingestion_progress — a default parameter absorbs the call

// ai/services/knowledge-base/IngestionService.mjs
getIngestionProgress({staleAfterMs = 60000} = {}) {

Observed on current dev, dispatching through the real ToolService with the real openapi.yaml:

caller sent      : {staleAfterMs: 5000}
handler received : [5000]
signature        : getIngestionProgress({staleAfterMs = 60000} = {})
-> object call   : 5000
-> this dispatch : 60000      <-- caller value discarded

Destructuring a bare Number yields undefined for every key, so the = 60000 default wins. The = {} on the parameter is what keeps it from throwing.

The consequence is specific to what this tool is for: staleAfterMs is the threshold that decides whether an active run is reported as stalled. An operator narrowing the window to catch a stall early gets the 60000 answer while believing they asked for 5000.

B. github-workflow.get_pull_request_diff — arity drops three declared arguments

// ai/services/github-workflow/PullRequestService.mjs
async getPullRequestDiff(options) {
    const { pr_number, file, sha, files_only } = typeof options === 'number' || typeof options === 'string'
        ? { pr_number: parseInt(options, 10) }
        : (options || {});

The contract for /pull-requests/{pr_number}/diff declares four parameters, and initializeToolMapping() derives argNames from them in declaration order: pr_number, file, sha, files_only.

Same dispatch, same conditions:

caller sent      : {pr_number: 42, file: 'src/Neo.mjs', files_only: true}
handler received : [42, "src/Neo.mjs", null, true]
signature        : getPullRequestDiff(options)
-> object call   : {pr_number: 42, file: "src/Neo.mjs", files_only: true}
-> this dispatch : {pr_number: 42}      <-- file / sha / files_only gone

Worth being precise about where this goes wrong: the dispatcher does produce all four values. The handler declares one parameter, so JavaScript binds the first and drops the rest. The {Object|number} tolerance then re-wraps the bare 42 into {pr_number: 42}, which is why the call succeeds and returns a real diff.

The failure mode is the awkward direction. file is a narrowing argument, so losing it returns more than was requested, and files_only: true asks for structured JSON but yields full diff text. Agents on this project call this tool during review; receiving more than requested reads as thoroughness rather than as a bug, which is why it survives being used.

These two are the whole set

Enumerating every operation across the six servers that takes arguments and lacks the annotation, on current dev:

Nine entries, seven of them genuinely positional handlers whose arity matches the contract exactly. The two above are the only ones where the contract and the signature disagree without saying so.

Fix

Declare the annotation on both. Verified on current dev against a patched copy of each contract, same dispatch:

[A] knowledge-base.get_ingestion_progress
    handler received : [{"staleAfterMs":5000}]
    -> resolves to   : 5000                    <-- caller value now lands

[B] github-workflow.get_pull_request_diff
    handler received : [{"pr_number":42,"file":"src/Neo.mjs","files_only":true}]
    -> resolves to   : {"pr_number":42,"file":"src/Neo.mjs","files_only":true}

  /ingestion/progress:
    get:
      summary: Get Ingestion Progress
      operationId: get_ingestion_progress
      x-neo-tool-tier: extended
      x-pass-as-object: true

  /pull-requests/{pr_number}/diff:
    get:
      summary: Get PR Diff
      operationId: get_pull_request_diff
      x-neo-tool-tier: read
      x-pass-as-object: true

getPullRequestDiff already documents @param {Object|number} options and handles the object form, so the annotation restores the shape the function was written for. Its number-tolerance branch becomes dead for MCP callers but is harmless to keep.

Guard

OpenApiValidatorCompliance.spec.mjs currently lists both in positionalHandlers with a reason. Those two entries should be removed by this change, leaving only handlers whose arity genuinely matches the contract.

That also settles the residual raised in the #16235 review: an exception carrying a reason but no tracking anchor reads to a future maintainer as an approved permanent exception rather than as known debt. After this ticket, every remaining entry is correct rather than tolerated, so there is nothing left to anchor.

One thing the guard still cannot see. It knows only "unannotated", and the three outcomes in the table above are indistinguishable from the contract alone: get_local_issue_by_id and get_ingestion_progress both declare one argument and neither is annotated, but one is right and one is not. The difference lives in the handler signature, which the contract never references. Deciding whether that is worth checking mechanically, and how, is a bigger question than this fix and I have not attempted it here.

Why the existing tests do not catch it

Same shape as #16231, and worth restating because the symptom is different.

Both behaviours are covered, by name, and both suites call the handlers directly with an object, which is the shape the annotation would produce:

// test/.../github-workflow/PullRequestService.spec.mjs:933
test('files_only parameter returns structured JSON without diff body', async () => {
    const result = await PullRequestService.getPullRequestDiff({pr_number: 10747, files_only: true});
    expect(Array.isArray(result.files)).toBe(true);

// test/.../github-workflow/PullRequestService.spec.mjs:943
test('file parameter filters the diff output', async () => {
    const result = await PullRequestService.getPullRequestDiff({pr_number: 10747, file: '...'});
    expect(result.result).toContain('Sub 4 Payload Audit Results');

// test/.../knowledge-base/IngestionService.spec.mjs:411
const snapshot = Service.getIngestionProgress({staleAfterMs: 1});
expect(snapshot).toMatchObject({stalled: true, ...});

These are green and they are not wrong: the functions do exactly that when called that way. But there is a test named "file parameter filters the diff output" passing in CI while, through the contract that ships, file never reaches the function and the diff is not filtered. Same for files_only, and for the stalled: true assertion that only holds because staleAfterMs: 1 arrived.

Nothing calls either handler through ToolService.callTool. The one place get_pull_request_diff appears in a tool-level spec is github-workflow/toolService.spec.mjs:408, where the name sits in an authorization classification list rather than being dispatched.

There is one extra requirement here that #16231 did not have. A dispatch-level test catches the throwing case the moment it runs, because the call fails. These two calls succeed, so the assertion has to be on the value the handler received, or on a result that could only follow from the right value. "Dispatched without error" passes against both defects.

Environment

• neo.mjs dev at aa721ca4
• Node.js 22
• Dispatch driven through the real ToolService with each server's real openapi.yaml; the service mapping was replaced with a recorder so the arguments could be observed without GitHub or Chroma round-trips