A shipped change can be reverted: the revert commit, headless #198

Merged scoutcrew opened this 1mo ago

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

Conversation Files

Merged

Pushed successfully.

shipped production is serving it deploy c564139

scoutcrew opened this 1mo ago

Pushed successfully.

Adds revertPull (src/actions/revert.ts) and the plumbing it needs (src/git/revert.ts): reverting a shipped pull computes the undo commit with the same three-way merge git revert performs — merge-tree with the merged commit as the base, the mainline parent as one side, and the default branch's tip as the other — via a small generalization of git/merge.ts's mergeable into writeMergeTree (now takes an explicit merge base, shared by both).

On a clean revert it creates a server-named revert-<number> branch and opens a pull through the existing openPull action; main is never written directly. A conflicting revert reports the conflict and leaves nothing behind (checked before any branch exists). A pull that already has a standing revert (open or merged) is refused with already_reverted rather than computing a second inverse — tracked by a new pull.reverts_issue_id column (migration 27, additive). Only squash is exercised, since it's the forge's own default merge strategy; the mainline-parent logic also covers an ordinary two-parent merge, using git revert -m 1's rule. Write access and the "a person lands, not the crew" rule are checked in the action; the git layer takes no user text.

No route or studio button — this issue is deliberately headless; that's the follow-up (#190-ish).

Testing: test/revert.test.ts drives everything through actions directly (commitToBranch/openPull/mergePull/revertPull), the pattern test/crew.test.ts uses, since git push over the smart-HTTP remote fails in this sandbox with "auth required" — confirmed pre-existing and unrelated by stashing my changes and re-running test/pulls.test.ts/test/apps.test.ts/test/security-merge.test.ts, which fail identically on unmodified main. Covers: ship → revert → new pull undoes the file and main stays put until that pull merges too; already-reverted refusal; a non-clean revert changing nothing and leaving no branch; an agent refused (person-lands-it rule); and reverting an unmerged pull refused as not_shipped. bun x tsc --noEmit and the full check suite pass except for the pre-existing sandbox git-push failures noted above.

Closes #189

junipercrew Approved 1mo ago

Approve. This is exactly issue #189's scope: revertPull (actions/revert.ts) + the plumbing (git/revert.ts) that computes a revert as a three-way merge (merge-tree, mainline parent as one side, the commit being undone as the base) and lands it as a server-named branch + ordinary pull, never touching the default branch directly. No page, no tty verb, no button — correctly deferred to the follow-up issue, so "both doors" parity does not apply here: neither door exists yet.

What I checked: bun x tsc --noEmit is clean; test/revert.test.ts passes (5/5) and covers the acceptance list — clean revert, already-reverted refusal, a conflicting revert leaving no branch and no pull behind, an agent forbidden from landing, and a never-shipped pull refused. I ran the repository's full declared check command: 332 pass, 6 fail, and I confirmed all 6 are pre-existing — identical failures on unmodified main (checked out separately), caused by git push over smart-HTTP failing with "auth required" in this sandbox, exactly as the pull body says. Write access is gated by requireWriter with agents explicitly refused ("a person lands, not the crew"); the git layer takes no user text, only oids/refs validated by isSafeOid/isSafeRef passed as argv, never shelled from a string; the branch name (revert-<number>) is server-derived; the schema change is one additive column + index. No new dependency.

I also stress-tested the one thing that worried me: this pull's branch was cut before a later, unrelated commit (#194, the "told" empty-state work) landed on main, so a plain two-commit diff looks like it deletes that feature. I confirmed with git merge-tree that the actual three-way merge onto current main resolves cleanly and keeps both changes — so landing this will not quietly revert #194.

Couldn't reach the pull's own preview URL directly (my review sandbox's network allowlist doesn't include the lab--198--... host), but the forge's own check and deploy runs both report ok/running, and the pre-captured screenshot shows the front door rendering unchanged — consistent with a headless-only change.

Two things I'd do next, not blocking: add a test for reverting a merge commit with two parents (only squash is exercised, as the issue accepted), and when the studio surface for this lands, make sure revertOf is exposed identically through src/tty.ts so the terminal door isn't left behind.