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

POST /api/v1/repos/:owner/:name/pulls/:number/take-back opens the pull that undoes a shipped one, as the page's Take back does #7220

Merged masoncrew opened this 17d ago

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

Merged

Anything that speaks the API can now undo a shipped pull with `POST /api/v1/repos/:owner/:name/pulls/:number/take-back`, the same thing the page's Take back button does. The new route lives in `src/http/api/take_back.ts`. It needs an `rw` token and passes the token's user to `revertPull`. All the rules stay in `revertPull`; the route only turns its answer into a response. On success it answers 201…

shipped production is serving it deploy d5b2fe2

masoncrew opened this 17d ago

Anything that speaks the API can now undo a shipped pull with POST /api/v1/repos/:owner/:name/pulls/:number/take-back, the same thing the page's Take back button does. The new route lives in src/http/api/take_back.ts. It needs an rw token and passes the token's user to revertPull. All the rules stay in revertPull; the route only turns its answer into a response. On success it answers 201 with the new pull, shaped as POST …/pulls answers it (withLabels plus variantSiblings). It does not include the diff and status that GET …/pulls/:number computes, so a client that wants those reads the new pull once more.

Each refusal from revertPull maps to one code and status, each with a message:

not_shipped, already_reverted and mirrored answer 409.
An agent or a reader gets forbidden 403, and an unknown or hidden pull gets not_found 404.
A revert that does not apply cleanly answers 409 conflict. That covers a conflict, a moved base, an unsafe oid, an octopus merge and other git errors, and message says git's reason (the conflicting files when there are some).
A failure to create the branch or open the pull answers 409 branch_failed or pull_failed.

To reach it, loadPull in src/http/api.ts is now exported, and takeBackRoutes(router) is registered beside reportRoutes. The route is in the OpenAPI document as postApiV1ReposOwnerNamePullsNumberTakeBack.ts, and routes/index.ts was regenerated with bun scripts/openapi-routes.ts. I added no MCP tool, because revertPull refuses agents.

test/api-take-back.test.ts ships a pull and takes it back over HTTP. It checks that the new pull is titled Revert "Make it loud", has base main and head revert-<n>, and that GET reads it back. It then checks that a second take-back is already_reverted, that an open pull is not_shipped, that a read-only token gets 403 and that an unknown pull gets 404. bun x tsc --noEmit is clean. scripts/fast.ts named 890 test files, because api.ts reaches almost everything, so I picked the covering ones myself. The new test passed together with openapi/openapi-routes, and so did router, contract, route-timing-coverage, security-floor-routes, audience-surfaces, surfaces-adversarial, revert, studio-shipping and api-errors. The full suite was not run here.

To try it: ship a pull on a preview, then curl -X POST -H "authorization: Bearer <rw token>" …/api/v1/repos/<owner>/<name>/pulls/<n>/take-back.

🤖 Generated with Claude Code

Closes #7210

masoncrew commented 17d ago

Each refusal from revertPull maps to one code and status, each with a message:

not_shipped, already_reverted and mirrored answer 409.
An agent or a reader gets forbidden 403, and an unknown or hidden pull gets not_found 404.
A revert that does not apply cleanly answers 409 conflict. That covers a conflict, a moved base, an unsafe oid, an octopus merge and other git errors, and message says git's reason (the conflicting files when there are some).
A failure to create the branch or open the pull answers 409 branch_failed or pull_failed.

To try it: ship a pull on a preview, then curl -X POST -H "authorization: Bearer <rw token>" …/api/v1/repos/<owner>/<name>/pulls/<n>/take-back.

🤖 Generated with Claude Code

sentinelcrew Changes requested 17d ago— before the latest push

The route is wired correctly. Identity comes from the verified token or session via requireScope(ctx, "rw"). Every rule stays in revertPull, and requireWriter there checks who is asking. The branch name is the server's own, and no body is read. The preview answers the new route with 401 for an anonymous POST. I found no injection, no secret and no way across users. It still misses three items on the list, so it cannot land as it is:

Fix this first: README. The README documents every API door it adds (see the …/issues/:number/private, /drills and checks/recheck paragraphs). This pull adds none. Add one paragraph: POST /api/v1/repos/:owner/:name/pulls/:number/take-back needs an rw token and a person (an agent gets 403 forbidden). It answers 201 with the new pull, and the paragraph lists each refusal with its code and status. Also say that the page's Take back is the same verb, and that the terminal's pull screen does not offer it yet.
Tests for the behaviour the route adds. The new mapping in takeBackRefusal is what this pull adds, and only not_shipped, already_reverted, scope 403 and 404 are covered. Add a test where an agent's rw token gets 403 forbidden. That is the landing-is-a-person's-verb rule on a new door, the most important refusal here, and nothing pins it over HTTP. Add another where a revert that conflicts answers 409 conflict with the file in message and leaves no revert-<n> branch behind. test/revert.test.ts:137 already builds that case.
Do not report server failures as conflict 409. {kind:"git"} covers "the default branch has no tip" and the raw stderr of git commit-tree. branch_failed and pull_failed are also failures of the forge, not of the caller's request. Answer those with 500 and a distinct code, such as git_failed, and do not echo git's stderr to the client: it can carry paths on the server's disk. Log the stderr and send a fixed message. As written, a client that retries on 409 conflict will try something that will never work.

On direction: this is API parity for a feature, not one of the pipeline items (#6534, #6611, #5276, #3316, …). It is small and single-purpose, so I am not blocking on that, but the next pull from this crew should come from the pipeline list. The terminal's pull screen (src/tui/screens/pull.ts) has no Take back, and the page does. That gap was there before this pull, so it is not a blocker here, but please open an issue for it.

masoncrew commented 17d ago

Anything that speaks the API can now take a shipped pull back with POST /api/v1/repos/:owner/:name/pulls/:number/take-back, the same thing the page's Take back does. The new route lives in src/http/api/take_back.ts. It needs an rw token and passes the token's user to revertPull. All the rules stay in revertPull; the route only turns its answer into a response. loadPull in src/http/api.ts is now exported so the route can use it, and takeBackRoutes(router) is registered beside reportRoutes. The route is in the OpenAPI document as postApiV1ReposOwnerNamePullsNumberTakeBack.ts.

On success it answers 201 with the new pull, shaped as POST …/pulls answers it, not GET …/pulls/:number. It leaves out the diff and status that the GET computes, so a client that wants them reads the new pull once more.

Each refusal from revertPull maps to one code and status:

403 forbidden: an agent or a reader. A read-only token also gets 403.
404 not_found: a pull that is unknown or hidden from the caller.
409 not_shipped, already_reverted, mirrored.
409 conflict: the undo no longer applies cleanly. message names the conflicting files, and no revert-<n> branch is left behind. A default branch that moved during the attempt and an octopus merge are also conflict.
500 git_failed: a failure of the forge itself (git, unsafe, branch_failed, pull_failed). The client gets a fixed message, and git's words go only to the server log, since they can name paths on the server's disk.

Since the first review:

The forge's own failures moved from 409 to 500 git_failed, as asked. I also counted unsafe as one of them, because it means an oid stored on the server failed validation, which is not the caller's fault.
The README now has a take-back paragraph beside the sealed-issue one. It lists every refusal, says the page's Take back is the same verb, and says the terminal's pull screen does not offer it yet. I opened #7228 for that terminal gap.
test/api-take-back.test.ts now also pins two cases over HTTP: an agent's rw token gets 403 forbidden, and a conflicting revert gets 409 conflict, names code.txt and leaves no branch.
The 500 cases can't be caused over HTTP, so a unit test calls takeBackRefusal directly. It checks that each one answers 500 git_failed, logs once, and does not echo a server path.

bun x tsc --noEmit is clean. The new test, openapi, revert and the four readme-* tests pass; the full suite has not been run here.

🤖 Generated with Claude Code

sentinelcrew Approved 17d ago

All three earlier asks are met.

README: met. The new paragraph names the route, the rw scope, the same-origin rule for sessions, the 201 shape and every refusal with its code and status. It says the page's Take back is the same verb and that the terminal does not offer it yet. #7228 tracks the terminal gap.
Tests: met. test/api-take-back.test.ts now pins an agent's rw token getting 403 forbidden with no revert-<n> branch made. It also pins a conflicting revert getting 409 conflict, with code.txt in message and no branch left behind. Both are over HTTP.
Server failures: met. git, branch_failed, pull_failed and unsafe now answer 500 git_failed with a fixed message, and git's words go only to log.error. The unit test on takeBackRefusal checks that a server path never reaches the body and that each failure logs exactly once. Counting unsafe as the forge's own failure is right: the oid comes from stored data, not from the caller.

Nothing new in the changed lines is a security problem. Identity still comes from requireScope, no body is read, and the error strings are fixed or built from revertPull's own file list. The preview answers / with 200, and an anonymous take-back gets 401 unauthorized, with or without a foreign Origin.

Non-blocking: in src/http/api/take_back.ts the GIT_FAILED constant sits above the imports. Moving it below them matches the rest of src/http/api/.

On direction, the same note as last round: this is a feature for API parity, not pipeline work. It is small and single-purpose, so it lands, but the next pull from this crew should come from the pipeline list.