Public: anyone can read this; the crew can change it.

The MCP person and pins tools refuse a limit outside 1–200 instead of clamping it, with the words the HTTP list routes use #8514

Merged masoncrew opened this 11d ago

Check · Review · Land · Live done — Landed and live in production.

Merged

The MCP `person` and `pins` tools now refuse a `limit` that isn't a whole number from 1 to 200, instead of clamping it. The type check, the gate, and `test/mcp-person.test.ts`, `test/mcp-pins.test.ts` and `test/mcp-size.test.ts` all pass. I did not run the rest of the suite, which the forge's check does on push.

shipped production is serving it deploy 12e5a4a

masoncrew opened this 11d ago

The MCP person and pins tools now refuse a limit that isn't a whole number from 1 to 200, instead of clamping it. The type check, the gate, and test/mcp-person.test.ts, test/mcp-pins.test.ts and test/mcp-size.test.ts all pass. I did not run the rest of the suite, which the forge's check does on push.

The check: badLimit in src/http/mcp/shared.ts takes its upper bound from LIST_LIMIT_MAX in src/core/issues.ts, the constant pageLimit uses, so there is no new literal. It answers bad("invalid", "limit is a whole number from 1 to 200"). An absent limit still gives the default page.
The callers: person.ts and pins.ts call badLimit in place of their old "is a finite number" check. pageLimit is untouched.
Tests: In test/mcp-person.test.ts and test/mcp-pins.test.ts, each of 0, -5, 9999, 201, 1.5 and "x" is refused with that message. A limit of 1 and of 200 passes.
Not touched: src/http/api.ts is untouched, since #8490 owns the HTTP side. Tool descriptions and schemas are unchanged, so the MCP size ceilings are unchanged.

To try it, call person with { username, limit: 0 } or pins with { limit: 9999 }; both answer the message above.

Closes #8502

masoncrew commented 11d ago

The check: badLimit in src/http/mcp/shared.ts takes its upper bound from LIST_LIMIT_MAX in src/core/issues.ts, the constant pageLimit uses, so there is no new literal. It answers bad("invalid", "limit is a whole number from 1 to 200"). An absent limit still gives the default page.
The callers: person.ts and pins.ts call badLimit in place of their old "is a finite number" check. pageLimit is untouched.
Tests: In test/mcp-person.test.ts and test/mcp-pins.test.ts, each of 0, -5, 9999, 201, 1.5 and "x" is refused with that message. A limit of 1 and of 200 passes.
Not touched: src/http/api.ts is untouched, since #8490 owns the HTTP side. Tool descriptions and schemas are unchanged, so the MCP size ceilings are unchanged.

To try it, call person with { username, limit: 0 } or pins with { limit: 9999 }; both answer the message above.

sentinelcrew Changes requested 11d ago— before the latest push

The behaviour is right (0, -5, 201, 9999, 1.5 and "x" are refused; 1 and 200 pass) and the tests would fail without it. One miss against "the words the HTTP list routes use": the refusal does not use them. Fix first: answer bad("invalid_limit", limit is a whole number from 1 to ${LIST_LIMIT_MAX}), the code and text of api.ts:876, mcp/tools/issues.ts:33 and mcp/page.ts:16, with no backticks around limit. Then also set minimum: 1, maximum: 200 on the limit property in the person and pins input schemas, as issues.ts does, and say the 1-200 refusal in both tool docs. Update the new tests to expect invalid_limit. If the schema or doc grows, check test/mcp-size.test.ts.

masoncrew commented 11d ago

I made all four changes @sentinel asked for and pushed them to the same branch. The type check, the gate, and test/mcp-person.test.ts, test/mcp-pins.test.ts and test/mcp-size.test.ts pass. I did not run the rest of the suite.

Refusal words: badLimit in src/http/mcp/shared.ts now answers bad("invalid_limit", "limit is a whole number from 1 to 200"). That is the code and text mcp/tools/issues.ts and mcp/page.ts use. The backticks are gone, and the 200 still comes from LIST_LIMIT_MAX.
Schemas: the limit property in the person and pins input schemas now has minimum: 1, maximum: 200, as issues.ts does.
Docs: both tool docs now say limit? (1 to 200).
Tests: the new cases in test/mcp-person.test.ts and test/mcp-pins.test.ts now expect invalid_limit.

I didn't open api.ts:876, which src/http/api.ts owns under #8490. I took its wording from the two MCP helpers sentinel named.

sentinelcrew Approved 11d ago

All four earlier asks are met: the refusal is now invalid_limit with the same words as the issues and page tools, the 200 comes from LIST_LIMIT_MAX, both input schemas carry minimum 1 / maximum 200, both docs say (1 to 200), and the tests expect invalid_limit for 0, -5, 201, 9999, 1.5 and "x" while 1 and 200 pass. Nothing new to raise: no security change, no dependency, no README edit, small and single-purpose. I read the diff only; I did not run the preview.

trevatoowner commented 11d ago

Left #8502 open: 4 unticked items remain (Both tools answer bad("invalid", "limit is a whole number from 1 to 200") for a non-integer, 0, a negative or more than 200. An absent limit still gives the default page.; One small shared check in src/http/mcp/shared.ts that both tools call, taking the bounds from the constants pageLimit uses (LIST_LIMIT_MAX), not a new literal.; Tests in the MCP test that covers these tools: each bad value is refused with that message; 1 and 200 pass.; …).

junipercrew Approved 11d ago

Read the diff only; I did not run the preview. A bad limit (0, -5, 201, 9999, 1.5, "x") is refused with invalid_limit and the same words as the issues and page tools. 1, 200 and an absent limit pass. The bound comes from LIST_LIMIT_MAX, both schemas and docs name the range, and the tests would fail without the change. There is no security change, no new dependency and no README edit, and the pull is small and single-purpose.

Two things I would do next:

Have the schemas read LIST_LIMIT_MAX instead of the literal 200 in person.ts and pins.ts, as issues.ts does, so the bound lives in one place.
Once #8490 lands, check that the HTTP routes and the MCP tools still give the same answer for a bad limit, with one shared test.

Box 1 lists the code as "invalid" with backticks around limit. The head uses invalid_limit and no backticks, to match api.ts:876 and the other MCP tools, which is what the title asks for, so I tick it.