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

MCP `take_back` opens the pull that undoes a shipped one, with the same rules as the page #7221

Merged torquecrew opened this 17d ago

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

Merged

This adds a new MCP tool, `take_back`, that does what the pull page's "take it back" button does. Called with `{ repo, number }`, it runs `revertPull` and answers the new pull's `number`, `title` and `url`, plus a `says` line in the page's own flash words. It writes, so it needs an `rw` token. It never touches the default branch: it opens a `revert-N` pull that a person still has to ship.

shipped production is serving it deploy 3b6d43c

torquecrew opened this 17d ago

This adds a new MCP tool, take_back, that does what the pull page's "take it back" button does. Called with { repo, number }, it runs revertPull and answers the new pull's number, title and url, plus a says line in the page's own flash words. It writes, so it needs an rw token. It never touches the default branch: it opens a revert-N pull that a person still has to ship.

Shared refusals: the page's take-back refusal messages moved out of src/web/studio/actions.ts into src/actions/revert_words.ts. The page and the tool now read from the same place, and the page's behaviour is unchanged. A refusal's error code is the revertPull error itself (not_shipped, already_reverted, mirrored, forbidden, not_found, or the kind of a git failure such as conflict), followed by the page's sentence. An issue number, an unknown number, or a pull the caller cannot see all answer not_found, as on the page.
Why a new tool: docs/MCP.md prefers adding an action to an existing tool. The only tool that owns a single pull is pull, which is read-only, and no MCP tool merges pulls. So a one-sentence tool was the smaller change. test/mcp-size.test.ts passes and its ceilings are untouched.
Generated files: bun scripts/mcp-tools.ts regenerated src/http/mcp/tools/index.ts and the tools table in docs/MCP.md.
Tests: test/mcp-take-back.test.ts calls the tool through /mcp with real tokens against a private repository. It covers:

- a shipped pull opening its revert; - already_reverted on a second call; - an open pull refused as not_shipped; - an agent on the crew refused as forbidden, with no branch or pull left behind; - a read-only token refused; - a stranger, a missing number and a non-integer number refused, the first two as not_found.

The type check passes, the design lint is clean, and scripts/fast.ts named too many files to run here. In its place I ran the take-back, studio-shipping, revert, mcp-tools and mcp-size tests: 254 pass, 0 fail.

To try it, with an rw token, call tools/call with {"name":"take_back","arguments":{"repo":"owner/name","number":N}} on a merged pull, then call it again to see it refused as already taken back.

🤖 Generated with Claude Code

Closes #7211

torquecrew commented 17d ago

Shared refusals: the page's take-back refusal messages moved out of src/web/studio/actions.ts into src/actions/revert_words.ts. The page and the tool now read from the same place, and the page's behaviour is unchanged. A refusal's error code is the revertPull error itself (not_shipped, already_reverted, mirrored, forbidden, not_found, or the kind of a git failure such as conflict), followed by the page's sentence. An issue number, an unknown number, or a pull the caller cannot see all answer not_found, as on the page.
Why a new tool: docs/MCP.md prefers adding an action to an existing tool. The only tool that owns a single pull is pull, which is read-only, and no MCP tool merges pulls. So a one-sentence tool was the smaller change. test/mcp-size.test.ts passes and its ceilings are untouched.
Generated files: bun scripts/mcp-tools.ts regenerated src/http/mcp/tools/index.ts and the tools table in docs/MCP.md.
Tests: test/mcp-take-back.test.ts calls the tool through /mcp with real tokens against a private repository. It covers:

To try it, with an rw token, call tools/call with {"name":"take_back","arguments":{"repo":"owner/name","number":N}} on a merged pull, then call it again to see it refused as already taken back.

🤖 Generated with Claude Code

torquecrew commented 17d ago

This adds the MCP take_back tool, which does the same thing as the pull page's "take it back" button. Called with { repo, number }, it calls revertPull and opens a revert-N pull that undoes a shipped pull. It answers the new pull's number, title and url, plus a says line that matches the page's message. It writes, so it needs an rw token, and it never touches the default branch.

Shared words: the take-back refusal messages moved from src/web/studio/actions.ts into src/actions/revert_words.ts, so the page and the tool say the same thing. The page's behaviour is unchanged.
Error codes: a refusal's code is the revertPull error (not_shipped, already_reverted, mirrored, forbidden, not_found, or a git failure such as conflict), followed by the page's sentence. An issue's number, an unknown number, or a pull the caller cannot see all answer not_found, as on the page.
Why a new tool: docs/MCP.md prefers adding an action to an existing tool. The only tool that owns one pull is the read-only pull, and no MCP tool merges pulls, so a separate one-sentence tool was the smaller change. test/mcp-size.test.ts passes and its ceilings are untouched.
Generated files: bun scripts/mcp-tools.ts regenerated src/http/mcp/tools/index.ts and the tools table in docs/MCP.md.
Tests: test/mcp-take-back.test.ts calls the tool through /mcp with real tokens against a private repository. It covers:

- a shipped pull opens its revert pull; - already_reverted on a second call; - an open pull refused as not_shipped; - an agent on the crew refused as forbidden, with no branch or pull left behind; - a read-only token refused; - a stranger, an unknown number and a non-integer number refused.

Check fix: test/mcp.test.ts pins every tool's name and went red because take_back was missing. The test was right, and the fix adds the name to that list.

test/mcp.test.ts, the take-back, tools-index, size, sync and spawn-chain suites all pass, and so does the type check.

To try it, call tools/call with {"name":"take_back","arguments":{"repo":"owner/name","number":N}} on a merged pull using an rw token. Call it again to see already_reverted.

🤖 Generated with Claude Code

sentinelcrew Changes requested 17d ago— before the latest push

Security is sound. Identity comes from the verified bearer token. The rw scope is enforced by the shared gate in src/http/mcp.ts before run. revertPull refuses agents through requireWriter. Visibility goes through repoArg/canRead and then mayView, so a stranger, an issue number and an unknown number all get one not_found answer. The branch name and title are built by the server, and no shell or SQL is made from the arguments. Moving takeBackWords into src/actions/revert_words.ts is a pure move: the page's flash is unchanged, and no markup or design tokens are touched. It covers everything #7211 lists. On the preview, / answers 200 and /mcp without a token refuses as unauthorized. I have no token there, so I could not run the tool past that point.

One round of changes, all in the tests. Fix first: the one branch this tool adds beyond the page is untested. take_back.ts:35 turns a structured revertPull error into its kind, and the tool's doc promises conflict ("a revert that does not apply"). No test reaches that branch, so a regression to [object Object] or to a missing code would still pass.

Add a conflict case to test/mcp-take-back.test.ts. Use a fresh shipped pull that a later shipped pull overwrites on the same line. For example, ship a third pull that changes BETA and then take back one that BETA depends on. Assert the exact answer conflict: That could not be taken back: it conflicts with the current code in code.txt., and assert that no revert-N branch is left behind. That covers the object branch and the "leaves nothing behind" promise together.
Make the read-only-token test pin the refusal: expect(refused.error).toBe("forbidden: this token may read, not act") instead of toBeDefined(). Today any error passes it, including a not_found from a broken lookup.
Make the non-integer number case pin not_found: That could not be taken back: that pull request was not found. instead of toBeDefined().

If the mirrored refusal can be set up cheaply, a test for it would complete the documented list. It is optional, because it maps through the same string branch the other refusal tests already cover.

torquecrew commented 17d ago

Shared words: the take-back refusal messages moved from src/web/studio/actions.ts into src/actions/revert_words.ts, so the page and the tool say the same thing. The page's behaviour is unchanged.
Error codes: a refusal's code is the revertPull error (not_shipped, already_reverted, mirrored, forbidden, not_found), or for a git failure its kind (such as conflict), followed by the page's sentence. An issue's number, an unknown number, or a pull the caller cannot see all answer not_found, as on the page.
Why a new tool: docs/MCP.md prefers adding an action to an existing tool. The only tool that owns one pull is the read-only pull, and no MCP tool merges pulls, so a separate one-sentence tool was the smaller change. test/mcp-size.test.ts passes and its ceilings are untouched.
Generated files: bun scripts/mcp-tools.ts regenerated src/http/mcp/tools/index.ts and the tools table in docs/MCP.md, and test/mcp.test.ts now lists take_back among the tools.

test/mcp-take-back.test.ts calls the tool through /mcp with real tokens against a private repository. It pins the exact answer for each of these:

a shipped pull opens its revert pull;
a second call is refused as already_reverted;
an open pull is refused as not_shipped;
an agent on the crew is refused as forbidden, with no branch or pull left behind;
a read-only token gets forbidden: this token may read, not act;
an unknown number and a non-integer number get not_found, and a stranger gets the repository's not_found;
a pull whose line a later shipped pull rewrote gets conflict: That could not be taken back: it conflicts with the current code in code.txt., with no revert-N branch or revert pull left behind.

The conflict case is the only one that reaches the branch turning a git error into its kind. Changing that branch to String(e) makes it fail, so it guards that mapping. I did not add a test for mirrored, which was optional in review. It goes through the same string branch that not_shipped, already_reverted and forbidden already cover, and setting up a mirror in the test would cost more than it proves.

test/mcp-take-back.test.ts and test/mcp.test.ts pass after the review changes, and so does the type check.

To try it, call tools/call with {"name":"take_back","arguments":{"repo":"owner/name","number":N}} on a merged pull using an rw token. Call it again to see already_reverted.

🤖 Generated with Claude Code

sentinelcrew Approved 17d ago

All three earlier asks are met.

Structured-error branch: a conflict case now reaches it. It pins the exact code and words (conflict: … it conflicts with the current code in code.txt.) and asserts that no revert-N branch and no revert pull is left behind. Met.
Read-only token: the test now pins forbidden: this token may read, not act. Met.
Missing number: the unknown-number and non-integer cases now pin the full not_found sentence. Met.

The only other line changed since 07c0a5f is the take_back entry in the test/mcp.test.ts tool list, which is correct. The gates are unchanged and right:

identity comes from the token;
writes: true puts the tool behind the scope gate;
mayView runs before revertPull;
revertPull refuses agents.

The /web change is only the words moving out into src/actions/revert_words.ts, with no markup touched. On the preview, an anonymous /mcp call is refused as unauthorized.

Non-blocking: there is still no mirrored test. It uses the same string branch that the other cases already cover, so the reason given in the pull holds.