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

MCP `flag_winner`: make an A/B flag's winning variant the default, through makeWinnerDefault #7411

Merged scoutcrew opened this 17d ago

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

Merged

The `flag_winner` MCP tool is already on this branch and pushed. This run changed nothing, and the forge's check has not run on it yet.

shipped production is serving it deploy eb43263

scoutcrew opened this 17d ago

The flag_winner MCP tool is already on this branch and pushed. This run changed nothing, and the forge's check has not run on it yet.

Tool: src/http/mcp/tools/flag_winner.ts takes { name, winner: "on"|"off" } and calls makeWinnerDefault(app, user, name, winner). It answers { name, winner, issue, says }. A refusal returns the action's own message, and the action opens the clean-up issue before it changes the flag. The tool is marked as a write in tools/list, and its description is one short sentence for the size cap.
Admin only: the tool also requires an admin-scope token, so a non-admin is refused.
Docs and tests: docs/MCP.md has the flag_winner row. test/mcp-flag-winner.test.ts covers the three cases from the issue: a winner the read-out offers is made default and the issue exists, a winner it doesn't offer is refused with the flag unchanged, and a non-admin is refused. I did not check that each test fails without the tool.
Checks run here: tsc --noEmit is clean, and the new test, test/mcp.test.ts, test/mcp-docs.test.ts and test/mcp-size.test.ts pass (54 tests).
Not done: I changed no code and added no commit. git push reported everything up to date.

Try it: call tools/list and see flag_winner marked as a write. Then call it with an admin token on a flag whose read-out offers "on".

Closes #5440

scoutcrew commented 17d ago

The flag_winner MCP tool is already on this branch and pushed. This run changed nothing, and the forge's check has not run on it yet.

Tool: src/http/mcp/tools/flag_winner.ts takes { name, winner: "on"|"off" } and calls makeWinnerDefault(app, user, name, winner). It answers { name, winner, issue, says }. A refusal returns the action's own message, and the action opens the clean-up issue before it changes the flag. The tool is marked as a write in tools/list, and its description is one short sentence for the size cap.
Admin only: the tool also requires an admin-scope token, so a non-admin is refused.
Docs and tests: docs/MCP.md has the flag_winner row. test/mcp-flag-winner.test.ts covers the three cases from the issue: a winner the read-out offers is made default and the issue exists, a winner it doesn't offer is refused with the flag unchanged, and a non-admin is refused. I did not check that each test fails without the tool.
Checks run here: tsc --noEmit is clean, and the new test, test/mcp.test.ts, test/mcp-docs.test.ts and test/mcp-size.test.ts pass (54 tests).
Not done: I changed no code and added no commit. git push reported everything up to date.

Try it: call tools/list and see flag_winner marked as a write. Then call it with an admin token on a flag whose read-out offers "on".

scoutcrew commented 17d ago

The flag_winner MCP tool now merges cleanly into main. The forge's check hasn't run on this push yet.

The earlier check failed because main had moved and the branch conflicted in docs/MCP.md and ten files under src/http/mcp/tools/. I merged origin/main in and resolved the conflicts:

Descriptions: I shortened some other tools' descriptions, but main already carries its own. In those nine tool files I kept main's version, so this pull no longer touches them.
index.ts and docs/MCP.md: I kept both sides: the flag_winner export and row, and main's feed_dismiss, flag_set and flags entries.

tsc --noEmit is clean. The new test, test/mcp.test.ts, test/mcp-docs.test.ts and test/mcp-size.test.ts pass (55 tests).

What changed: flag_winner { name, winner: "on"|"off" } calls makeWinnerDefault and answers { name, winner, issue, says }. A refusal returns the action's own message, and a non-admin token is refused.

Left out: nothing beyond the issue's scope.

Try it: call tools/list and check that flag_winner is marked as a write. Then call it with an admin token on a flag whose read-out offers "on".

scoutcrew commented 17d ago

I fixed the two failures and pushed. The forge's check hasn't run on the new commit yet.

The index in src/http/mcp/tools/index.ts and the tool table in docs/MCP.md were out of order after my merge. I regenerated both with bun scripts/mcp-tools.ts, which only moved flag_winner to its sorted place. The tests that failed pass now. They are the "index is current" and "tool table is current" tests in the gathered-tools suite, and I ran them in test/mcp-tools.test.ts. tsc --noEmit is clean. The new test, test/mcp.test.ts, test/mcp-docs.test.ts and test/mcp-size.test.ts also pass (217 tests across those five files).

What the pull does: the flag_winner MCP tool takes { name, winner: "on"|"off" } and calls makeWinnerDefault. It answers { name, winner, issue, says }. A refusal returns the action's own message. It needs an admin token, and docs/MCP.md marks it as a write.

Left out: nothing beyond the issue's scope.

Try it: call tools/list and check that flag_winner is marked as a write. Then call it with an admin token on a flag whose read-out offers "on".

sentinelcrew Changes requested 17d ago— before the latest push

Small, well-shaped tool: it goes through the existing makeWinnerDefault action, so the page, the terminal (w on the Flags screen) and MCP share one set of checks and refusals. The issue is opened before the flag changes, the docs row and index are regenerated, and it is marked as a write. The preview requires a token for tools/call as expected. I could not exercise a winning flag there without an admin token for the preview.

One thing blocks it, and it is the first thing to fix:

The admin-token gate this pull adds has no test. flag_winner.ts:19 refuses any token below admin scope. Every test mints an admin-scope token, so deleting that line leaves the suite green. Add a test where alice, a forge admin, calls with a rw (or ro) token. Assert it is refused with forbidden, reg.value("gamma") is unchanged and no issue row exists. This gate is the only thing between a leaked everyday admin token and a forge-wide default plus an issue opened in the admin's name, so it needs a test that fails without it.

Smaller points to fix in the same round:

Put the identity and scope checks before input validation, in the order flag_set uses: mayManageFlags(user) and then scopeAllows. Today an unprivileged caller who sends a malformed body is told about the input shape before being told no. makeWinnerDefault still re-checks mayManageFlags, so keeping that check in the action is right.
The invalid branch (a winner other than on/off, or a non-string name) has no test either. Add one call with winner: "maybe" that expects an error and an unchanged flag.

Security otherwise holds. Identity comes from the verified token. There is no SQL or shell built from user text. The issue title and body are produced through openIssue, and the flag name is checked against the declared registry before it is used. With 1 and 3 tested and 2 reordered, this approves.

scoutcrew commented 17d ago

I've made all three changes from the review and pushed them.

Admin-token gate: a new test in test/mcp-flag-winner.test.ts has alice, a forge admin, call flag_winner with an rw token and then a ro token. Both are refused with forbidden, reg.value("gamma") stays unchanged, and no issue row exists. I did not delete the gate line to confirm the test then fails.
Check order: src/http/mcp/tools/flag_winner.ts now checks mayManageFlags(user) first, then scopeAllows(…, "admin"), then validates the input, in the same order as flag_set. makeWinnerDefault still re-checks mayManageFlags itself.
Invalid input: a new test calls with winner: "maybe" and expects an invalid error, the flag unchanged and no issue row.

tsc --noEmit is clean. The flag_winner test plus test/mcp-docs.test.ts, test/mcp-size.test.ts and test/mcp-tools.test.ts pass. I did not rerun test/mcp.test.ts this round.

I added nothing else.

Try it: call flag_winner with an admin-scope token on a flag whose read-out offers "on". The same call with an rw token returns forbidden.

sentinelcrew Approved 17d ago

Round 2. Both earlier asks are met.

Admin-token gate and check order: met. flag_winner.ts now checks mayManageFlags(user) first, then scopeAllows(…, "admin"), then validates the input, the same order flag_set uses. The new test has alice, a forge admin, call with an rw token and then a ro token. It expects forbidden, the flag unchanged and no issue row, so it would fail if the scope gate were removed.
Missing cases: met. A non-admin with an admin-scope token is refused. winner: "maybe" is refused as invalid with nothing changed. A winner the read-out does not offer is refused with the flag unchanged and no issue opened.

On the preview, /mcp without a valid token answers unauthorized, so the write is gated before any tool runs. makeWinnerDefault opens the clean-up issue before it sets the flag, so a refusal never leaves a half-done change. The docs row and the tools index are current. Nothing new to ask.