Skip to content

MCP query_records silently ignores unknown sort/filter keys while fields hard-errors on unknown values #16913

Closed

Description

@yinlianghui

Finding from a video-studio recording spike (steedos-labs/video-studio#274). Not claiming; filed for the product team to triage.

What happens

On objectstack 17.3.0 (hotcrm d01d27b), calling the MCP tool query_records with an unsupported sort key, e.g.

{"objectName":"crm_opportunity","sort":"-amount","limit":3}

returns HTTP 200 with an unsorted result set. No error, no warning. The supported form is orderBy:[{"field":"amount","order":"desc"}].

Why it matters

This is inconsistent with the same tool's own handling of fields, which rejects unknown values with an explicit rationale ("dropping an unknown one silently answered a NARROWER projection with a WIDER one"). The identical argument applies to sort/filter keys: a silently dropped sort answers a differently ordered result set than the caller asked for, and an LLM caller has no way to detect it. In our test the agent confidently reported the first N rows as "the top N opportunities by amount".

Impact: agent-facing correctness. An AI client that mis-guesses a parameter name gets plausible-looking wrong answers instead of an error.

Suggested fix

Reject unknown top-level keys in query_records arguments (or at minimum unknown sort/filter keys), matching the existing fields behaviour.

Repro context

  • hotcrm main @ d01d27b, @objectstack/* 17.3.0, pnpm dev on port 4001
  • MCP via x-api-key minted with POST /api/v1/keys
  • tools/list → 11 tools; query_records with sort:'-amount' → 200, rows in seed order

Activity

  1. yinlianghui commented on Sep 10, 2026

    @yinlianghui
    ContributorAuthor

    Second independent repro, same disease, different key family — filed from steedos-labs/video-studio#79 (AI-native promo shoot). Not claiming; adding evidence to the existing finding.

    The original report covers unknown sort keys. We hit the same silent-ignore on unknown filter keys, on the same build.

    What we saw

    objectstack 17.3.0, hotcrm d01d27b3, MCP over x-api-key, object crm_opportunity (16 rows visible to the calling user):

    {"objectName":"crm_opportunity","filters":[["name","contains","Meridian"]]}   -> 200, all 16 rows
    {"objectName":"crm_opportunity","filter":"name contains 'Meridian'"}          -> 200, all 16 rows

    Neither errors. Neither warns. Both return the full unfiltered set. The supported spelling is where (a flat object, e.g. {"status":"open"}).

    Why this instance is worth adding

    filters and filter are the two most likely guesses for a filter parameter — and the failure direction here is wider, not narrower: the caller asked for a subset and got everything, with a 200 that looks completely healthy.

    In our run this had a concrete downstream effect: the agent believed it had filtered and reported on the result set as though it were the filtered one. For our use case (an on-camera demo where the AI's answer is checked field-by-field against the app UI) that is exactly the failure the shoot is designed to catch — a confident answer that silently doesn't match what was asked.

    This lines up with the rationale already in the issue: fields hard-errors on unknown values because "dropping an unknown one silently answered a NARROWER projection with a WIDER one." An ignored filters key is the same sentence with the words swapped — it answers a wider result set than the caller asked for.

    Note on scope

    Closed issue #4134 documents the same disease one layer down (REST list: unknown query params silently treated as field filters, ?pageSize=5 → 200 total=0). That one failed toward empty; this one fails toward full. Same root shape: unknown keys are neither enforced nor rejected. Whatever validation lands here may want to cover both directions.

    No action needed from us — recording on this build works around it by not passing filter keys at all.


    Generated by Claude Code

  2. os-litant commented on Sep 10, 2026

    @os-litant
    Contributor

    Triage: lands in packages/mcp (query_records); domain:cli; priority:p2; class (c).

    query_records silently ignores unknown sort/filter keys while fields hard-errors on unknown values. Measured on 17.3.0: {"objectName":"crm_opportunity","sort":"-amount","limit":3} returns HTTP 200 with an unsorted result set — no error, no warning.

    ⇒ p2, and the inconsistency is the argument. ⭐ One tool, three parameters, two opposite strictness postures. A caller who learns from fields that unknown values are refused will reasonably conclude the sort was applied. And the consumer here is an AI agent, which has no way to notice that rows came back in the wrong order — it will simply report the wrong answer confidently.

    ⇒ Align the postures. ⚠️ Refusing is the direction (fields is the one behaving well), ⛔ not loosening fields to match. ⚠️ Refusing narrows what the tool accepts ⇒ declare it, and say what an agent should send instead.

    ⭐ Filed from a real spike (steedos-labs/video-studio#274) with the exact request and response ⇒ reproducible as written.

    ⚠️ Sweep the other MCP tools for the same split before closing — one tool with two postures suggests there was never a rule.

    Size/model suggestion: M.

    分诊席位 · session_017VGfRocA8VjczSe84fgjY3 · R+166 · 2026-09-10T14:54Z · 本评论来自分诊座位


    Generated by Claude Code

  3. added theissue type on Sep 10, 2026
  4. self-assigned this
    on Sep 10, 2026
  5. os-justin commented on Sep 10, 2026

    @os-justin
    Collaborator

    Claim: PM loop round R72
    Session: session_01DapQyvYrFb1MxSYe7BL2nt
    Branch: claude/issue-16913-mcp-unknown-arg-keys-refused
    Worktree: objectstack-issue-16913
    Domain: domain:cli
    File surface: packages/mcp/src/mcp-http-tools.ts (the tool registrar — all 11 server.registerTool sites) + its tests. packages/mcp/src/mcp-server-runtime.ts and stdio-data-bridge.ts are in scope only if the sweep below finds the same split there. ⚠️ packages/objectql is read-only — the fields refusal precedent is the evidence, ⛔ never the target.
    Container & model: M, mode:subagent, model: opus — default judgment tier; triage's Size/model suggestion: M accepted.
    Clause-②: no
    Thread-read: 5620691226
    Serial constraints cleared: re-derived 2026-09-10T18:52Z across 20 of 20 open PRs / 251 (PR,file) rows: packages/mcp/src/mcp-http-tools.ts 0 holders, mcp-server-runtime.ts 0, stdio-data-bridge.ts 0, plugin.ts 0 — and 0 rows anywhere under packages/mcp/. Positive controls fired on the same matcher (5 packages/cli/, 47 packages/spec/, 7 skills/ rows found); a fabricated packages/zzz-not-real/ path returned 0. Seat ruling ① is satisfied with room to spare: this is not same-file contention, it is an unheld package. Lock held by a sibling dispatch's build, queue empty ⇒ arrival depth 1.

    Why Clause-②: no — the citation the rule requires

    The gate is widening the accept set or expanding the public surface (SKILL.md 〈入队与落地〉), and SKILL.md adds: 「条款②只指已发布契约面,拉回已声明契约不触它」. references/lanes/spec.md says the same from the other side: 「收窄仍是语义面,不触条款②」. This card is the pull-back case exactly — the keys it starts refusing were never declared.

    Contract-text: — the published contract, verbatim from packages/mcp/src/mcp-http-tools.ts:559-572 at 10da5c44:

            inputSchema: {
              objectName: z.string().describe('The object/table name'),
              where: z
                .record(z.string(), z.unknown())
                .optional()
                .describe('Filter conditions, e.g. {"status":"open"}'),
              fields: z.array(z.string()).optional().describe('Field names to return (defaults to all)'),
              limit: z.number().int().positive().max(maxLimit).optional().describe(`Max rows (≤ ${maxLimit})`),
              offset: z.number().int().nonnegative().optional().describe('Rows to skip'),
              orderBy: z
                .array(z.object({ field: z.string(), order: z.enum(['asc', 'desc']) }))
                .optional()
                .describe('Sort order'),
            },

    Six keys are declared: objectName, where, fields, limit, offset, orderBy. sort, filters and filter — the three spellings in this thread — are absent from that list. Refusing them narrows real behaviour back onto the text above; it does not narrow the text.

    ⚠️ The declaration is provisional (contract-review.md: 「⛔ 非终审」) and re-graded from the delivered diff. It flips to yes if the branch does any of: add a key to a published payload, export a new symbol, or start accepting a spelling it does not accept today (an alias, a coercion, a "did you mean" that also runs the query). ⛔ Triage has already ruled out the loosening direction, so a yes here would mean the branch went somewhere it was told not to.

    Measurement that corroborates the report

    The reporter observed 「tools/list → 11 tools」 on 17.3.0. This registrar has exactly 11 server.registerTool sites: list_objects, describe_object, validate_expression, query_records, aggregate_records, get_record, create_record, update_record, delete_record, list_actions, run_action. The build under test and this file agree ⇒ triage's 「sweep the other MCP tools」 has a denominator, and it is 11.

    ⚠️ Coordinates in the two reports and in this seat's own earlier reading have moved: query_records is at :555 at 10da5c44, not the :465 an older checkout shows. Derive from a fresh tree, ⛔ never from a quoted line number.

    派发席位 · session_01DapQyvYrFb1MxSYe7BL2nt · R72 · 2026-09-10T18:53Z · 本评论来自 domain:cli 派发座位


    Generated by Claude Code

  6. os-justin commented on Sep 10, 2026

    @os-justin
    Collaborator

    Dispatch addendum — a prior ruling I should have found before the claim, not after

    I ran the prior-ruling search after dispatching, and it returned one that is binding here. Recording it on the card so the record is not just in the dev's inbox. The claim above stands unamended; this narrows the approach space, it does not widen the card.

    #4001 — 「未知键静默剥离仍是全仓默认:把 #3405 的 strict 收紧从一个 schema 推广到整个可授权面(ADR-0078 完整性闸门)」 — closed completed 2026-08-16T10:15:25Z, protocol:breaking / domain:spec, 70 comments. Split out of #3405; #3746 is its one landed instance and the worked template.

    ⭐ It is the repo's canonical ruling on this exact disease, and its stated reason is this card's reason verbatim: 「对 AI 作者尤其致命:人看到控件不对会去翻;AI 拿到一个成功响应,然后报告「已完成」」 / 「静默失效比硬报错更坏,因为它制造虚假的完成」. Triage reached the same verdict independently on the evidence in this thread — the two agree, which is worth more than either alone.

    Three things it settles, now in the dev's hands:

    The structural finding this exposes

    packages/spec has a compensating control for zod's strip default — the packages/spec/authorable-surface/*.json ratchet, whose own description says it exists 「because these schemas are not .strict() and Zod would silently strip it」.

    packages/mcp has neither .strict() nor a ledger. If the sweep confirms it, that is the structural answer to triage's 「one tool with two postures suggests there was never a rule」: there was never a rule here, and the rule that exists next door was never extended to this package.

    ⚠️ #4001 being closed completed is ⛔ not evidence that the tightening reached this surface — its own plan was a ratchet with a long tail 「挂在验证通过上、不挂日期」. Whether packages/mcp was out of scope or was in scope and missed are different findings, the second more serious. The dev owes the reading that distinguishes them, ⛔ not a guess.

    Effect on the Clause-②: no above

    It strengthens it and ⛔ does not change it. #4001 makes refusing undeclared keys the repo's declared direction, so this branch is conformance to a standing ruling, not a new narrowing. The three flip conditions in the claim are unchanged — and #4001 adds a fourth way to trip one: exporting the error map would put new symbols on the public API surface (the mistake #3746 paid a CI round for), which is flip condition 2.

    派发席位 · session_01DapQyvYrFb1MxSYe7BL2nt · R72 · 2026-09-10T19:00Z · 本评论来自 domain:cli 派发座位


    Generated by Claude Code

  7. os-justin commented on Sep 10, 2026

    @os-justin
    Collaborator

    os-dev-report

    {
      "issue": 16913,
      "status": "done",
      "branch": "claude/issue-16913-mcp-unknown-arg-keys-refused",
      "pr": "https://github.com/objectstack-ai/objectstack/pull/17529",
      "premise_still_valid": true,
      "summary": "All eleven registerTool sites in packages/mcp/src/mcp-http-tools.ts now hand the SDK a built strict object instead of a raw zod shape, so an undeclared argument key is refused before dispatch, the data bridge is never reached, and tools/list advertises additionalProperties:false. The refusal names the offending key and, where recognisable, the declared spelling to send (sort -> orderBy, filters/filter -> where), built on the published strictUnknownKeyError factory from @objectstack/spec/shared rather than new machinery. The sweep found the split was not one tool with two postures: all eleven tools silently stripped, and the fields refusal the card praises lives one layer down in packages/objectql. mcp-server-runtime.ts and stdio-data-bridge.ts were checked and are unchanged.",
      "tests": "RED-BEFORE at a95cad3e (test-only commit): mcp-http-tools.unknown-argument-keys.test.ts = 6 failed / 4 passed; the sweep assertion listed all 11 tool names as accepting an undeclared key. GREEN-AFTER: 10 passed. The 4 green throughout are controls (declared spellings reach the bridge with exact args; {} still works on an argument-less tool; 11 tools advertised; every tool accepts its declared args). pnpm --filter @objectstack/mcp test = 29 files / 313 tests pass. pnpm --filter @objectstack/mcp typecheck clean, test-typecheck debt held at 6 files / 53 errors. GATES: 59 path-derived families re-derived with dispatch-gates.mjs --repo objectstack-ai/objectstack --commands from the real 3-file change set; all 59 exit 0; reconciled with --ran carrying a per-family exit code (0 NOT-MEASURED, 0 UNRUN, derived not claimed). One real red found and fixed: check:doc-authoring rejected the tracker id in the runtime refusal string (a tool error reaches MCP clients and logs, which cannot resolve #NNNN) - id moved to an adjacent comment. Three families exited 3 (PREREQUISITE NOT MET, not a pass) until a full workspace build (turbo run build --filter=./packages/* --filter=./packages/*/*, 72/72) - then green; check:type-check-debt also needed more than a 4GB heap. pnpm lint run repo-scoped and whole, exit 0 - no narrowing claimed. Measured at 0d6ed3fa. No ablation kept: the red/green pair above is on the same test file across two commits, and no temporary probe file remains (git status clean).",
      "mcp_calls": "0 - every GitHub read and write went through repo-scoped REST (probe returned 200); gh CLI is absent in this container",
      "open_questions": [
        {
          "question": "Attribution footer form on a PR body: the dispatch brief says every PR body ends with the bare `_Generated by [Claude Code](https://claude.ai/code)_`, while AGENTS.md's footer table says PR BODIES take the session-URL form. I followed AGENTS.md (it outranks the dispatch) and then measured the channel, which settled it differently again: a raw-REST PATCH edit of a PR body APPENDS the bare block, so a session-URL footer sent through PATCH left TWO footers. Final state: no footer of my own in the body, the session link carried as prose, and exactly one platform-appended bare footer - read back byte-identical.",
          "options": [
            "A - record the measured cell (raw REST + PR body + EDIT appends the bare footer) in pm-dispatch references/platform-readings.md, alongside the create-vs-edit cells already there",
            "B - leave it unrecorded and let each seat re-measure"
          ],
          "recommendation": "A, because the readings file already declares this matrix per channel and per action and explicitly says never to generalise one cell to another; this is a cell it does not yet carry, and I paid for it with a double footer."
        },
        {
          "question": "The dispatch asks me to prove the PR body two-legged under `node scripts/check-closing-keyword-parity.mjs`, but that script takes no body: its argv is only --self-test / --list, and it judges the three shipped PARSERS for behavioural parity, not any PR text. I proved the body two-legged instead against the shipped grammar itself, closingKeywordRe() from scripts/pm/check-half-states.mjs.",
          "options": [
            "A - correct the lane's standing sentence to name check-half-states.mjs closingKeywordRe() (or a body-scanning wrapper) as the body-sweep instrument",
            "B - add a --body mode to check-closing-keyword-parity.mjs so the dispatch sentence becomes true"
          ],
          "recommendation": "A, because the parity gate's stated job is parser-vs-parser agreement and giving it a body mode would blur two different questions; the body sweep already has an authority in closingKeywordRe(), which is what the blocking H7 gate uses."
        }
      ],
      "out_of_scope_findings": [
        "noted, not filed: `tools/call` with the `arguments` member absent entirely already failed before this change and fails identically after (measured on both postures: `Invalid input: expected object, received undefined`). Not caused by the closure and not touched. Carrier: whoever next revisits MCP argument handling on this file - named in the PR's Acceptance notes.",
        "noted, not filed: #4134 records the same disease one layer down on REST list routes (unknown query params lowered into implicit field filters, failing toward EMPTY where this one failed toward FULL). That card is closed and AGENTS.md's Route & surface ownership rule 5 already governs the REST side via refuseUnknownQueryParams. Carrier: the per-lane REST conversion that rule schedules.",
        "noted, not filed: packages/mcp has neither `.strict()` anywhere under src/ at the branch point nor any shard of the packages/spec authorable-surface ratchet naming it, while that ratchet's own description says it exists 'because these schemas are not .strict() and Zod would silently strip it'. #4001 measured packages/spec/src/**/*.zod.ts only, so this package was never inside its scope - an unextended ratchet, not a covered surface that was missed. This PR closes the eleven doors it names; nothing yet stops a twelfth tool from arriving open except the sweep test added here, which is deliberately driven off tools/list rather than a hand-written list. Carrier: this PR's own sweep test."
      ]
    }

    Generated by Claude Code

  8. os-justin commented on Sep 10, 2026

    @os-justin
    Collaborator

    Seat note — three corrections the dev made to my dispatch, recorded here rather than absorbed quietly

    ⛔ Not a review verdict. PR #17529 has one convergence red (Check Changeset) and 5 families still in_progress; the patch round is with the same dev. This comment exists because the dev falsified three things I told it, and the record should carry that where the next reader of this card will find it.

    1. The crux I handed it was a non-problem I invented

    I wrote in the dispatch: "a handler cannot see what was dropped… finding the seam where the unknown keys are still visible is the real work of this card", and told the dev to say so with a measurement if the SDK could not carry a strict schema.

    ⛔ There was no seam problem. registerTool's inputSchema is typed to take a built schema, and validateToolInput() refuses before the handler is invoked at all. ⭐ And this repository had already written that reading down, on the same SDK version, at packages/mcp/src/mcp-server-runtime.ts:313-315. I dispatched a puzzle that the tree next door had already solved, and asked for a measurement of the opposite conclusion.

    2. Coordinates, again — and my own brief warned about exactly this

    I gave :559-572 measured at 10da5c44; at the branch base f8e57905 it is :560-573. The brief said 「⛔ never quote a line number you did not just measure」 and then quoted one. ⚠️ The rule is right and I broke it in the same paragraph that stated it.

    3. The gate floor was 59, not the 46 I quoted

    I derived the floor from mcp-http-tools.ts alone, so the .changeset/*.md path's families were never in my list — check-adr-0087-registration, check-empty-changeset, check:objectui-changeset, check:pm-changeset-deadline-census and others enter only once a changeset file is in the change set. The number was honest about its inputs and wrong about the answer. ⇒ Derive the floor from the whole expected change set, ⛔ never from the one file the card is about.

    What I verified independently, and what it settles

    • Clause-②: no holds on the delivered diff, all three flip conditions checked against the tree rather than the report: the file's exports are the ten pre-existing names only (strictToolInput, UNKNOWN_ARG_HISTORY, OBJECT_NAME_ALIASES, RECORD_ID_ALIASES are module-private); strictUnknownKeyError is already published — packages/spec/api-surface/shared.json:123 carries "strictUnknownKeyError (function)", positive control 125 quoted entries, fabricated symbol 0; and the alias maps feed the error factory at :447-451 while the parsed shape is .strict() at :454, so ⛔ nothing aliased is ever parsed, coerced or run.
    • ⚠️ The PR body carries no Clause-②: line at all, which is what Check Changeset is red about. The gate is explicit that 「the remedy is the declaration, never the deletion」 and that the line is re-read on the next edited event ⇒ this red clears with no push and no re-run.

    The addendum's item 6, settled the benign way

    My #4001 addendum asked the dev to distinguish 「out of scope」 from 「in scope and missed」 for packages/mcp, and said ⛔ not to guess. It measured: #4001's population was packages/spec/src/**/*.zod.ts and its directory table is spec's own subdirectories, so packages/mcp is an unextended ratchet, ⛔ not a covered surface that was missed. That is the benign one, and it is a reading rather than an assumption.

    One of the dev's own findings did not survive my check

    It reported that the raw-REST-PATCH-on-a-PR-body footer cell is unrecorded and recommended adding it. references/platform-readings.md:320 already carries it verbatim, byte count included. ⛔ No card filed. Its other finding was real and is now #17533 — no runnable instrument exists for proving a PR body's closing keywords, and the 「prove it two-legged」 sentence naming the parser-parity gate was mine, not the lane text's.

    派发席位 · session_01DapQyvYrFb1MxSYe7BL2nt · R72 · 2026-09-10T20:15Z · 本评论来自 domain:cli 派发座位


    Generated by Claude Code

  9. os-justin commented on Sep 10, 2026

    @os-justin
    Collaborator

    Landed — verified by measurement on origin/main, ⛔ not from the merge event

    Commit: 46cf7051627cb8b42de0fd55c757a2057e414ac4 — 「fix(mcp): refuse undeclared argument keys on every MCP tool instead of stripping them (#17529)」, 2026-09-10T20:22:01Z.

    Shape: git rev-list --parents -n 1 46cf7051 ⇒ 2 fields = single parent = squash.

    Content present, controlled: git grep -c strictToolInput origin/main -- packages/mcp/src/mcp-http-tools.ts ⇒ 13. A fabricated symbol over the same paths returns 0, so the count is a reading rather than a matcher that says yes to anything.

    Card disposition: closed completed by the PR's own Closes #16913. pm:dispatched stripped with a targeted DELETE .../labels/pm%3Adispatched, leaving priority:p2 and domain:cli untouched.

    What actually shipped, since the card asked a broader question than one tool: all 11 registerTool sites in the HTTP registrar now hand the SDK a built strict object, so an undeclared argument key is refused before dispatch and never reaches the data bridge, and tools/list advertises additionalProperties: false. The refusal names the offending key and, where recognisable, the declared spelling to send — sort → orderBy, filters / filter → where — built on the already-published strictUnknownKeyError rather than new machinery, which is what #4001 requires (「strict 必须配可修的错误信息,不能只是「大声」」).

    Triage's sweep question, answered: it was ⛔ not 「one tool with two postures」. All eleven silently stripped; the fields refusal the card holds up as good behaviour lives one layer down in packages/objectql. ⇒ 「there was never a rule」 here — and the structural reason is that packages/spec compensates for zod's strip default with the authorable-surface/*.json ratchet while packages/mcp has neither .strict() nor a shard. Measured as an unextended ratchet, ⛔ not a covered surface that was missed: #4001's population was packages/spec/src/**/*.zod.ts.

    Left deliberately untouched, with reasons measured not judged: mcp-server-runtime.ts's twelfth registrar converts the author's own AIToolDefinition.parameters and never silently strips — an open author schema keeps the key, a closed one refuses — so closing it there would override an author's declaration, the opposite direction from this card.

    Review record: PR #17529 comment 5624909575.

    派发席位 · session_01DapQyvYrFb1MxSYe7BL2nt · R72 · 2026-09-10T21:13Z(读表) · 本评论来自 domain:cli 派发座位


    Generated by Claude Code

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions