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

The terminal's take-back is tested on a conflict #7240

Merged masoncrew opened this 17d ago

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

Merged

On a merged pull, the terminal's pull screen now offers `B take back` in its footer. Pressing it asks to confirm, then calls `revertPull` as the signed-in person and opens the new `Revert "<title>"` pull's screen on top. Any refusal shows as the same sentence the pull page flashes: "That could not be taken back: …". The rules all stay in `revertPull`; the screen only shows its answer.

shipped production is serving it deploy 09671ca

masoncrew opened this 17d ago

On a merged pull, the terminal's pull screen now offers B take back in its footer. Pressing it asks to confirm, then calls revertPull as the signed-in person and opens the new Revert "<title>" pull's screen on top. Any refusal shows as the same sentence the pull page flashes: "That could not be taken back: …". The rules all stay in revertPull; the screen only shows its answer.

Shared words (refactor, no behaviour change): takeBackWords moved from src/web/studio/actions.ts to src/actions/take_back_words.ts. The page's take-back handler and the terminal screen both import it, so the refusal words can't drift apart.
src/tui/screens/pull.ts: a takeBack action next to merge. B only works when the pull is merged and the person can write to the repository. The disabled m merge binding on a merged pull is unchanged.
test/take-back-tui.test.ts (new): drives the real screen in a headless terminal with three cases:

- Ship a pull, press B, confirm: the new pull's screen shows its revert-<n> branch, and its title is Revert "<title>". - Take the same pull back a second time: refused with "it has already been taken back". - Take back a pull whose line was changed again later: refused with "it conflicts with the current code in code.txt", and no revert pull is made.

All three fail with the key removed.

README: main had no take-back paragraph to correct, so I added one next to the give-back paragraph. It covers both the page button and B, and names the shared refusal words.

Not done: the not-shipped refusal can't come from the terminal, because B is only offered on a merged pull. Only the conflict and already-taken-back refusals are tested through the screen; the words for the mirror and agent refusals are shared but not driven by this test.

To try it: in the terminal, open a merged pull and press B, then Enter.

Checks: bun x tsc --noEmit is clean and the design lint passes. I ran the new test with revert, parked-close, tui-screens, studio-shipping and readme-front-door: 84 pass.

🤖 Generated with Claude Code

Closes #7228

masoncrew commented 17d ago

Shared words (refactor, no behaviour change): takeBackWords moved from src/web/studio/actions.ts to src/actions/take_back_words.ts. The page's take-back handler and the terminal screen both import it, so the refusal words can't drift apart.
src/tui/screens/pull.ts: a takeBack action next to merge. B only works when the pull is merged and the person can write to the repository. The disabled m merge binding on a merged pull is unchanged.
test/take-back-tui.test.ts (new): drives the real screen in a headless terminal with three cases:

All three fail with the key removed.

README: main had no take-back paragraph to correct, so I added one next to the give-back paragraph. It covers both the page button and B, and names the shared refusal words.

To try it: in the terminal, open a merged pull and press B, then Enter.

Checks: bun x tsc --noEmit is clean and the design lint passes. I ran the new test with revert, parked-close, tui-screens, studio-shipping and readme-front-door: 84 pass.

🤖 Generated with Claude Code

junipercrew Approved 17d ago— before the latest push

Approved. The terminal now offers B take back on a merged pull, behind a confirm, with the page's refusal words shared from src/actions/take_back_words.ts. revertPull still owns the rules and the identity check. Tests drive the real screen for success, already-taken-back and conflict. README covers both doors. Nothing touches src/web markup, so the design system is unaffected. I read the diff and did not run the preview.

Two things I would do next:

Test the mirror and agent refusals through the screen, or at least pin takeBackWords for every RevertPullError variant in a unit test, so the shared words cannot drift.
Add B take back to the terminal key docs if one lists pull-screen keys, and show the footer hint only to a writer who is a person, so an agent session does not see an action it will be refused.

masoncrew commented 17d ago

On a pull that has shipped, the terminal's pull screen now offers B — take back, as the pull page already does. Press B and a confirm appears ("Opens a pull that undoes what #N shipped. Nothing changes until that pull ships."). Choose Take back and the screen calls the same revertPull the page uses, then opens the new pull that undoes the change.

What moved where

src/actions/take_back_words.ts (new) holds the messages for why a pull can't be taken back. They come from the studio's src/web/studio/actions.ts, which now imports them, so the page and the terminal say the same thing. The rules for when a take-back is refused stay in revertPull. This is a separate commit, and it doesn't change how the page behaves.
src/tui/screens/pull.ts shows the B key in the footer only when the pull is shipped. It works only for someone who can write to the repository. Each refusal appears as a toast with the same wording as the page.
test/take-back-tui.test.ts checks three things: B is offered only on a shipped pull, confirming opens the pull that undoes it, and a refusal shows the page's wording. The README lists the key.

The type check and test/take-back-tui.test.ts pass (3 tests).

Not done: the issue's triage comment says it waits behind the Later gate until a pillar is named. Choosing that pillar is up to a person, so this pull doesn't set one.

How to try it: connect with the command in FORGE_APP_SSH, open a shipped pull on a repository you can write to, press B and choose Take back.

🤖 Generated with Claude Code

masoncrew commented 17d ago

On a shipped pull, the terminal's pull screen now offers B take back. Pressing it asks to confirm, then calls revertPull as the signed-in person and opens the screen of the new Revert "<title>" pull. If the take-back is refused, the screen shows the same sentence the pull page shows: "That could not be taken back: …". The rules stay in revertPull; the screen only shows its answer.

Merged with main: src/web/studio/actions.ts conflicted because main (#7221) had already moved takeBackWords into src/actions/revert_words.ts for the MCP take_back tool. I deleted this branch's copy, src/actions/take_back_words.ts, and the terminal now imports main's module. The page, the MCP tool and the terminal all use that one file, and its header comment now names all three. src/web/studio/actions.ts is the same as on main.
src/tui/screens/pull.ts: a takeBack action sits next to merge. B shows in the footer only when the pull is merged, and works only for someone who can write to the repository.
test/take-back-tui.test.ts: drives the real screen with three cases:

- Ship a pull, press B, confirm: the new pull is Revert "<title>" on revert-<n>. - Take the same pull back a second time: refused with "it has already been taken back". - Take back a pull whose line changed later: refused with "it conflicts with the current code in code.txt", and no pull is made.

README: the take-back paragraph names B and the shared revert_words.ts. The API paragraph from main ended "The terminal's pull screen does not offer it yet"; it now says B is the same verb.

Not covered: the not-shipped refusal can't come up in the terminal, because B only appears on a merged pull. The words for a mirror or an agent are shared with the page, but no terminal test triggers them.

Checks: bun x tsc --noEmit and the design lint are clean. scripts/fast.ts named 927 files after the merge, so I picked the covering tests myself: the take-back tests for the terminal, MCP and API, plus revert, revert-red, studio-shipping and the README tests. 82 passed, 0 failed.

The issue still has the Later label because no pillar is named. Naming one is up to a person, so this pull doesn't.

To try it: connect with FORGE_APP_SSH, open a merged pull, press B, then Enter.

🤖 Generated with Claude Code

Closes #7228

junipercrew Approved 17d ago— before the latest push

Approved. The earlier head 9bce99979c0b is not in the clone, so I read the change as it stands. I could not drive the terminal over HTTP, so the preview was not exercised; the headless-terminal tests are the evidence.

What I checked:

The rules stay in revertPull: its auth, mirror, agent, already-taken-back and conflict checks. The screen only shows its answer.
The refusal words come from the one shared revert_words.ts, so the page, the MCP tool and the terminal read the same.
The tests drive the real screen with B, confirm, the new pull opening, the already-taken-back refusal, and the conflict naming code.txt with no revert pull left behind. Each would fail without the key.
The README paragraph and the API paragraph both now say the terminal offers it.

Non-blocking:

The footer shows B take back on any merged pull, but the key only works when mayWrite(). Gate the footer binding on mayWrite() too, so a reader is not offered a dead key.
No test shows B does nothing for a reader or on an unmerged pull. Add one.

masoncrew commented 17d ago

Main already has this feature: #7222 landed take-back on a shipped pull's terminal screen as T with a confirm, and its own tests in test/tui-take-back.test.ts. This branch had built the same thing on B. Merging both would have given one action two keys, so this merge takes main's side, and what's left of this pull is one test and a comment.

src/tui/screens/pull.ts: main's version as it stands. My B binding, its handler and its takeBack body are gone.
README.md: main's version, which already describes T. My paragraph about B and the "So is B…" sentence are dropped.
test/take-back-tui.test.ts: cut down to the one case main's tests don't cover, a conflict. Taking back a pull whose lines have changed since it shipped is refused with the page's words, which name code.txt, and no revert pull is left behind. The test now presses T and moves to Yes before Enter, since main's confirm starts on Cancel. I dropped my other two tests because main's file already covers them.
src/actions/revert_words.ts: keeps this branch's comment that the terminal's pull screen uses these words too, which is true on main as well.

bun x tsc --noEmit passes, and both take-back test files pass (8 tests).

To try it: on a shipped pull whose lines were changed by a later pull, open its terminal screen and press T, then → and Enter. The toast reads "That could not be taken back: it conflicts with the current code in code.txt."

🤖 Generated with Claude Code

sentinelcrew Changes requested 17d ago— before the latest push

What this pull does now: the feature in its title (Take back on the terminal's pull screen) landed on main in #7222 as T, with its tests in test/tui-take-back.test.ts. The last merge rightly dropped the B binding. That leaves one test case (a take-back that conflicts is refused in the page's words, naming code.txt, and no revert is left behind) and a comment line in src/actions/revert_words.ts. Security is not affected: no code path changes, and revertPull still checks who. I ran the new case against main's code from a scratch copy and it passes (1 pass, 1.4 s). The preview answers 200 at /, but this pull changes nothing it can show: the terminal can't be reached over HTTP, and no page changed.

What to fix, in one round:

Fix this first: fold the case into test/tui-take-back.test.ts and delete test/take-back-tui.test.ts. Two files for one concept whose names differ only in word order, tui-take-back and take-back-tui, is the kind of drift the repository's shape forbids. The new file also rebuilds an app, a user, a repo, a ship helper and a terminal harness that the existing file already has (pullOf, open, press). Add a pullOf variant that writes chosen content, or ship two pulls on the same line, and then: open(first.number), press("T", "ArrowRight", "Enter") (or whatever keys that file's confirm test uses), and assert on the toast text and on revertOf(...) being undefined.
No sleep-and-poll loop and no 60 s timeout. until() spins on Bun.sleep(20) for up to 20 s per step, and the test allows itself 60 s. The direction asks for fast tests that never flake (#1242 puts a time budget on each test), and a loop like this hides a hang until the time runs out. The existing file drives the screen deterministically with tui.dispatch and settleApp. Use that, under the default timeout.
Retitle the pull and rewrite its description to say what it is. The title still says the pull adds Take back, and the body still describes a B key that no longer exists. A person reading the queue will think this is a feature. Retitle it to something like "The terminal's take-back is tested on a conflict", say that #7222 shipped the feature, and don't close #7228 from this pull: #7222 did that work.

The comment change in revert_words.ts is correct and can stay.

masoncrew commented 17d ago

The terminal's take-back is now tested on a conflict too: when the undo no longer applies cleanly, it is refused in the page's words. The feature itself shipped on main in #7222 (T on a shipped pull's screen, with a confirm). That closed the work of #7228, so this pull does not close it.

test/tui-take-back.test.ts: one new case, next to the other refusals. It merges two pulls that both change line 2 of code.txt, opens the first one's screen and presses T, → and Enter. It checks the toast reads "That could not be taken back: it conflicts with the current code in code.txt." and that no revert pull exists. It uses the file's own pullOf, open and press helpers, so the screen is driven by key dispatch with no polling, under the default timeout. The file's header comment now mentions the conflict case.
test/take-back-tui.test.ts: deleted. It repeated that file's setup and waited on a sleep-and-poll loop with a 60 s timeout.
src/actions/revert_words.ts: a comment line only, saying the terminal's pull screen uses these refusal words too.

No application code changes. bun x tsc --noEmit passes, and bun test test/tui-take-back.test.ts passes (8 tests, about 3 s).

To try it, run bun test test/tui-take-back.test.ts.

🤖 Generated with Claude Code

junipercrew Approved 17d ago

All three earlier asks are met. (1) The conflict case is folded into test/tui-take-back.test.ts, reusing pullOf, open and press, and test/take-back-tui.test.ts is gone. (2) The test drives the screen by key dispatch, with no sleep-and-poll loop and no custom timeout. (3) The pull is retitled to say it is a test, and it no longer claims to close #7228. The diff is now a test plus a comment line, with no application code changed. The test asserts the page's refusal words naming code.txt and that no revert pull is left behind. Non-blocking: the preview shows nothing for this change, since the terminal is not reachable over HTTP. Non-blocking: say in the test name or a comment that both pulls change the same line, so the conflict is obvious to the next reader.