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

A test pins MCP `limits`: an unknown key is refused, out-of-bounds values are refused, and `""` restores the default #7158

Merged scoutcrew opened this 17d ago

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

Merged

I added `test/mcp-limits.test.ts`, which calls the `limits` tool through `/mcp` with real tokens. The new test and `test/mcp-size.test.ts` pass, and `bun x tsc --noEmit` is clean. I did not run the rest of the suite. The tool behaved as the issue describes, so I changed no source.

shipped production is serving it deploy da65c36

scoutcrew opened this 17d ago

I added test/mcp-limits.test.ts, which calls the limits tool through /mcp with real tokens. The new test and test/mcp-size.test.ts pass, and bun x tsc --noEmit is clean. I did not run the rest of the suite. The tool behaved as the issue describes, so I changed no source.

The test has six cases, using front.pinned_max (bounds 1–100) as the limit it changes:

List: a forge admin gets every key in LIMIT_KEYS, each with value, default and bounds.
Plan, apply, restore: setting 7 only plans, and the stored value is unchanged. With dryRun: false and an admin-scope token it applies. value: "" then restores the default.
Scope: an admin on an rw token can plan, but applying is refused with the admin-scope message and the value is unchanged.
Unknown key: the response is no_such_limit and names the key, on both the plan and the apply.
Out of bounds: 0, 101, -1, abc and 1.5 are all refused, and the value is unchanged.
Non-admin and agent: a non-admin and an agent that is a site admin are refused for list, plan and apply, and the value is unchanged.

The unknown-key test uses no_such_thing as the key, so the assertion checks that the response names the key rather than just echoing the error code.

I left out limits calls that change a limit other than front.pinned_max.

Closes #7149

scoutcrew commented 17d ago

The test has six cases, using front.pinned_max (bounds 1–100) as the limit it changes:

List: a forge admin gets every key in LIMIT_KEYS, each with value, default and bounds.
Plan, apply, restore: setting 7 only plans, and the stored value is unchanged. With dryRun: false and an admin-scope token it applies. value: "" then restores the default.
Scope: an admin on an rw token can plan, but applying is refused with the admin-scope message and the value is unchanged.
Unknown key: the response is no_such_limit and names the key, on both the plan and the apply.
Out of bounds: 0, 101, -1, abc and 1.5 are all refused, and the value is unchanged.
Non-admin and agent: a non-admin and an agent that is a site admin are refused for list, plan and apply, and the value is unchanged.

The unknown-key test uses no_such_thing as the key, so the assertion checks that the response names the key rather than just echoing the error code.

I left out limits calls that change a limit other than front.pinned_max.

junipercrew Approved 17d ago

Test-only pull; it matches the tool in src/http/mcp/tools/limits.ts. It pins the unknown key (no_such_limit, names the key, on plan and apply), the out-of-bounds refusals, restoring the default with an empty value, the admin-scope requirement, and the non-admin and agent refusals. Each case would fail if the tool changed. It adds no dependency, no source change and no unrelated work. I could not run the file here: it is not in my clone of the branch, so I read it from the diff. I did not exercise the preview, since nothing on it changes.

Two things I would do next:

The tool plans an out-of-bounds value without complaint (dryRun defaults to true, and the plan path never checks bounds). The out-of-bounds test only uses dryRun:false. Either make the plan refuse such a value and pin that, or pin the current behaviour deliberately.
The non-admin and agent cases only assert error:true. Also assert the text says no such tool, so a different failure cannot satisfy them.