Skip to content

mcp(validate): make checkInputNarrowing compare property schemas, not just property names #10041

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

checkInputNarrowing (scripts/lib/validate-mcp/invariants.ts) is the build-failing invariant that a
server may only narrow the contract, added because "the override is typed as any z.ZodObject at all, and
no existing check could see a widening". Its implementation compares two things and nothing else:

const contractProperties = new Set(Object.keys((tool.inputSchema as { properties?: Record<string, unknown> }).properties ?? {}));
const contractRequired = new Set((tool.inputSchema as { required?: string[] }).required ?? []);
for (const property of Object.keys(advertised.inputSchema.properties ?? {})) {
  if (!contractProperties.has(property)) failures.push(`${tool.name} advertises input property ${property}, which its contract does not declare`);
}
for (const property of advertised.inputSchema.required ?? []) {
  if (!contractRequired.has(property)) failures.push(`${tool.name} requires input property ${property}, which its contract does not require`);
}

Top-level property names and the top-level required list. The property SCHEMAS are never compared,
at any depth. ListedTool.inputSchema (same file) does not even model them —
properties?: Record<string, unknown>.

That is precisely blind to the overrides the invariant shipped alongside. Of the five
registerStdioTool overrides in packages/loopover-mcp/bin/loopover-mcp.ts (lines 1604, 1618, 1630,
1780, 1893), the two that were hand-written for exactly this reason differ from their contract entry
only inside a property:

  • StdioComparePrVariantsInput (packages/loopover-contract/src/tools/local-branch.ts:393) —
    { variants: z.array(LocalScoreInput) }, where the contract's ComparePrVariantsInput (:387) is
    { variants: z.array(LocalScorePreviewInput) }.
  • StdioCompareLocalVariantsInput (:259) — { variants: z.array(CurrentBranchInput) }, where the
    contract's CompareLocalVariantsInput (:255) is { variants: z.array(LocalBranchAnalysisInput) }.

Both advertise the single property variants, and both require it — so both pass checkInputNarrowing
with the check having verified nothing at all about the difference that actually exists. Swap either
element type for a wider one and the check still passes, which is the exact failure mode the commit
message for this invariant describes: "a widened schema simply gets widened arguments and passes".

The same hole exists at the top level. An override declaring login: z.number() where the contract has
login: z.string() is a divergence, not a narrowing, and the check reports nothing: the name matches and
the required list matches.

The comment claiming this is unavoidable — "Narrowing is defined mechanically, which is all a schema
comparison can honestly do here" — is not accurate. Both sides are already JSON Schema by the time the
check runs (toJsonSchema, packages/loopover-contract/src/tool-definition.ts:104), so the advertised
subtree for a shared property can be compared against the contract's directly.

Requirements

  • checkInputNarrowing must, for every property present in BOTH the advertised and the contract input
    schema, compare the two property subtrees and report a failure when they are not identical and the
    advertised one is not a recognised narrowing of the contract's.
  • The recognised narrowings must be enumerated explicitly, not inferred: an advertised subtree is a
    narrowing of the contract's when it is deep-equal, or when it differs only by (a) a removed
    properties key, (b) an added or tightened minimum/maximum/minLength/maxLength/minItems/
    maxItems, (c) an enum that is a subset of the contract's, or (d) recursion through items /
    properties under those same rules. Anything else — a different type, an added property, a loosened
    bound, an enum member the contract does not list — is a failure.
  • ListedTool.inputSchema must model the property subtrees (Record<string, unknown> values are enough;
    the comparison walks them) so the check has something to read.
  • The check must keep returning a list of human-readable failures rather than throwing, in the same
    ${tool.name} advertises ... phrasing as the existing two messages, so one run reports every problem.
  • All three surfaces already run this check via validateSurface
    (test/contract/validate-mcp.test.ts:164). The five existing overrides and every non-overridden tool
    must still pass after the change — if one does not, that is a real finding to fix, not a reason to
    weaken the rule.
  • What must NOT change: diffToolSets, checkAdvertisedShape, checkAdvertisedMetadata,
    checkEveryToolCalled, checkVersionLock and checkWatchedPathsExist; the two existing
    checkInputNarrowing failure messages; and the registerStdioTool override seam itself.

⚠️ Required pattern: the check stays a pure function over the two JSON Schema documents, in
scripts/lib/validate-mcp/invariants.ts, returning string[] — the file header states why ("Split from
the driver so every branch is reachable from a unit test without booting a server"). What does NOT
satisfy this issue: (a) comparing the zod objects instead of the projected JSON Schemas, which would
couple the validator to zod internals and to the contract package's private shapes; (b) asserting strict
deep equality of the whole inputSchema, which would fail all five legitimate overrides and force them
to be deleted; (c) deleting the Stdio* overrides so the check has nothing to look at.

Deliverables

  • checkInputNarrowing in scripts/lib/validate-mcp/invariants.ts compares the property subtrees of
    every shared property, recursively, and reports a failure for a non-narrowing difference.
  • ListedTool.inputSchema models property subtrees.
  • Unit tests in the existing test/unit/validate-mcp-helpers.test.ts covering each rule with both
    arms: identical subtree (pass), removed nested property (pass), tightened bound (pass), subset enum
    (pass), changed type (fail), added nested property (fail), loosened bound (fail), enum member the
    contract does not list (fail), and a difference nested two levels deep under items.properties
    (fail).
  • A regression test named for this bug asserting that a synthetic listed tool whose variants.items
    declares a property the contract's element type does not is reported as a failure — the case the
    current check passes.
  • npm run validate:mcp (i.e. test/contract/validate-mcp.test.ts) is green against all three real
    servers with the strengthened check.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for
example comparing only top-level property types and not recursing into items/properties, or
adding the comparison without the nested-difference regression test — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's
coverage.include lists src/**/*.ts, packages/loopover-engine/src/**/*.ts,
packages/loopover-{contract,mcp,miner} paths and packages/discovery-index/src/**/*.ts — it does not
list scripts/**, so Codecov does not gate this path. The unit tests above are still mandatory: the
whole reason these invariants live in a pure module is that every branch is reachable without booting a
server, and an unenforced invariant with untested branches is the defect this issue is about. Every
comparison rule introduced needs both a passing and a failing case.

Expected Outcome

A registerStdioTool override that widens or diverges from its contract entry inside a property — the
shape both existing hand-written overrides take — fails validate:mcp instead of passing it, so the
"a server may only NARROW" rule is enforced where the overrides actually differ rather than only where
they happen not to.

Links & Resources

  • scripts/lib/validate-mcp/invariants.tscheckInputNarrowing and ListedTool
  • packages/loopover-contract/src/tools/local-branch.ts:259 / :393 — the two overrides the check cannot see into
  • packages/loopover-mcp/bin/loopover-mcp.ts:897registerStdioTool and its { input } override
  • packages/loopover-contract/src/tool-definition.ts:104toJsonSchema, which makes both sides comparable
  • test/contract/validate-mcp.test.ts:164 — where the check runs for all three surfaces
  • test/unit/validate-mcp-helpers.test.ts — the existing home for these unit tests

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions