feat(webapp): undo a whole agent run from the run card - #156
Open
ssowonny wants to merge 2 commits into
Open
Conversation
The run card was grouped for this and stopped one button short: every row inside it carried an action, the header carried none, so reverting a bad run meant clicking file by file and hoping you got them all. POST /api/p/<id>/undo-run works out, for every path the run touched, the op that puts it back — a put at the pre-run blob, or a delete for a file the run created — and writes them all in ONE journal append. That is the atomicity argument, not an optimization: one Put of one object either lands or it does not, so there is no half-undone run to report. appendOps is the batch write every path in the package now goes through; appendOp is its single-op call. Selection is by the journal an op was READ FROM, never op.Device — that field is arbitrary JSON any member with write access can put in their own journal, and the card attributes rows the same way. The note form additionally requires an empty Session, because runs.ts can never file a session-carrying op under a note-keyed card. Append-only throughout: the run's own ops are never edited or removed, so one-writer-per-journal and deterministic replay both survive. The undo's ops carry a note naming the run, so the undo is itself a run card you can undo. The confirm asks the server for the file list rather than deriving it from the loaded feed (paged and filterable, so a client-computed list is wrong exactly when the run is old), lists every path with its action, and names the one thing that can burn someone: a file a teammate changed after the run is reverted too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
planUndo already refuses a path the hub's own upload door would refuse — a peer can push one under .bdrive/ or with a control character in it — but the dialog listed only what the undo WOULD do, which reads as "all of it". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
TL;DR
appendOpsbatch is a prerequisite the verb can't ship without). Details below.Closes BEA-82.
What it does
Reverting a bad agent run meant opening the card, clicking file by file, and hoping you got them all. The grouping was built expecting the run-wide verb —
runs.tssays so in its own comment ("which is what a later run-wide restore needs") — and BEA-6 shipped everything except it.For every distinct path the run touched, the server works out the op that puts it back:
delete— the run created it, so un-create itXputat blobX, with its size and modeA move the agent made needs no special handling — it's a delete of the old path plus a put of the new, both inside the run, so undoing both paths is already right. No
buildMoveIndexhere.The three things that can't break
One
Putis the entire atomicity argument.appendOpdid a full read-modify-write of the hub journal per op, so a 50-file undo would have been 50 whole-journal round trips — and, worse, 50 chances to stop halfway.appendOpstakesupmuonce, does oneloadOps, assignsSeq/Lamportacross the batch, and does oneExists+Get+Put. One object either lands or it doesn't, so there is no half-rollback to report.appendOpis now justappendOpsof one, so the package has exactly one write path.If a later change ever loops
appendOpinside the handler, the feature silently becomes partially-applicable with no error anywhere.TestUndoRunOneJournalWriteasserts the backendPutcount, not the response — a loop answers 200 too. Please keep that assertion.Selection is by
sourcedOp.From, neverop.Device. An op'sDevicefield is arbitrary JSON any member with write access can put in their own journal; the journal key is the part/store'sownJournalgates, which is why History already attributes rows that way. Getting this wrong is invisible in every normal test, because on an honest journal the two are equal — soTestUndoRunSelectsByJournalKeywrites a dishonest one: a second device forgesdev1's device id and session inside its own journal, and the plan still reverts onlydev1's real op.The note form additionally requires
Session == "".runs.tskeys a session-carrying op ass\0…and can never file it under a note-keyed group, so a note-keyed undo that ignored this would revert ops the card never showed.Op.Noteis user-settable viabdrive sync --note;Op.Sessionisn't. The endpoint takes the session wherever the run has one.What you're accepting
A file a teammate changed after the run gets reverted too. This is the one genuine product judgment here — no code settles it. Per-row restore already overwrites current content without asking whose it is, and last-writer-wins is the model. But it's also the one place this feature can burn someone, so the confirm counts those paths, marks each one inline, and says it in words. At zero, the whole warning block is absent. It's one branch and one line of copy to flip if you'd rather it refused.
Anyone with write can undo anyone's run.
restoreandremoveare bothproj(PermWrite, …)and neither checks authorship; an author-scoped undo would be the only write in the hub that does. A read-only member sees no button and gets a 403 from the route, preview included.The undo is itself a run card, and two undos of the same run merge into one card. They're the same run; the spec accepts it.
Preview → write is not transactional. Another op can land between the two calls. The write recomputes the plan server-side and never trusts the client's, so the cost is a confirm that was one op stale, never a wrong write.
Deviations from the reviewed plan
Two, both stated rather than silent:
appendOpsand its two tests stand alone, and every existing caller goes through the single-op path unchanged.refusedpaths. The plan addedRefusedto the planner ("not in the spec but not optional") but described a dialog listing only what the undo would do. A dialog that silently drops a category reads as "all of it", so it gets a line. Second commit on the branch.Everything else is as planned, including both of the plan's corrections to the spec (select by the journal, not
op.Device; get the file list from the server, not the loaded feed — that window is paged and filterable, so a client-computed list is wrong exactly when the run is old).What it looks like
The header's note used to have all the room it wanted. Adding the button made the header shrink everything proportionally, which truncated
claude-code session 8f21e4down toclaude-code session …— so the note now holds its size (still capped at 46%) and the meta, which every row inside the card repeats anyway, absorbs the shrink. On mobile the button wraps onto its own line andflex-shrinkcomes back, since a wrapping header that refuses to shrink pushes a long note past the card edge.Mobile (375px)
Architecture changes
architecture/webapp-server.md:RemoteSourcegainsappendOps(and the note at the bottom of the write path now says N ops in one read-modify-write, withappendOpas its single-op call). A newundoRunDoorjoinsrestore/removeas the third write door, depending onsourcedOpfor selection and onRemoteSource.appendOpsfor the write. Nothing was removed.flowchart TB RemoteSource["<div style='text-align:left'><b>RemoteSource</b><br/>+Backend remote.Backend<br/>+Device Identity<br/>+Remove(ctx, path, who, note)<br/>-loadSourcedOps(ctx) []sourcedOp<br/>-appendOp(ctx, op)<br/><span style='background:#22c55e55;padding:0 4px;border-radius:3px'>✅ -appendOps(ctx, ops) ONE read-modify-write</span></div>"] sourcedOp["<div style='text-align:left'><b>sourcedOp</b><br/>+Op journal.Op<br/>+From journal key's device</div>"] undoRunDoor["<div style='text-align:left'><b>undoRunDoor</b><br/>POST /api/p/id/undo-run<br/>planUndo(sourced, undoSel) undoPlan<br/>undoSel: Device From-journal, Session xor Note<br/>undoPlan: Ops, Actions, Skipped, After, Refused<br/>preview: plan only, no write, no quota</div>"] RemoteSource -. "attribution comes from the journal key" .-> sourcedOp undoRunDoor -. "<span style='background:#22c55e55;padding:0 5px;border-radius:3px'>✅ selects a run by the journal it was read from</span>" .-> sourcedOp undoRunDoor -- "<span style='background:#22c55e55;padding:0 5px;border-radius:3px'>✅ appendOps - the whole run in one Put</span>" --> RemoteSource classDef added fill:#22c55e22,stroke:#22c55e,stroke-width:2px class undoRunDoor added linkStyle 1 stroke:#22c55e,stroke-width:2px linkStyle 2 stroke:#22c55e,stroke-width:2pxarchitecture/webapp-frontend.mdgains a note only — no types or relationships changed there — recording thatRunGroup's header carries the verb, that it asks the server for the file list rather than deriving it from the loaded feed, and thatmodal.tsx'sConfirm.messagewidened fromstringtoReactNodefor it (the prompt's one-field API is untouched, and every existing caller passes a string).What was run
go build ./...,go vet ./...,go test ./...— all green.TestUndoRunNoteKeyedIgnoresSessionOps,TestUndoRunSelectsByJournalKey, reserved paths, the 400 shapes and the 404, 403 for a non-member and a read-only member (preview too), the quota block viarecQuota,TestUndoRunOneJournalWritecounting backendPuts, andTestAppendOpsOrderingforSeq/Lamportincrease and MaxInt64 saturation (at saturation lamports stop increasing andjournal.Lessorders the rest through(time, device, seq)— so the assertion is non-decreasing-with-saturation, not strictly increasing).TestUndoRunConvergesininternal/syncer— real devices through a real hub over afile://store, per the repo's "a sync feature without a multi-device test is untested where it matters". A writes two files with a session and cycles, B syncs, the test POSTsundo-runat the hub, and both devices materialize the pre-run content. It also readsdeva.jsonlback to prove the run's own ops are still there, session id and all.npm run e2e— 172 passed, 1 pre-existing skip. New coverage insession-run.spec.ts: the confirm's file list and warning, Cancel means nothing happened, the undo, and then undoing the undo card (which is both the acceptance criterion and how the spec leaves the shared fixture as it found it). Plus a read-only member seeing no button.frontend/check-dist.sh—internal/webapp/static is fresh.Verdict
The endpoints, the run key, the grouping and the per-row verbs were all already where the spec said they were, so the risk here is concentrated in two places: the one-
Putbatch and the by-journal selection. Both have tests written to fail loudly if someone regresses them.The one open loop, unchanged from the spec: undoing a run that a teammate's device produced is allowed, because nothing else in the hub is author-scoped. If you want it scoped, it's one branch in the planner.
Build session
(Only works on the machine that ran the build.)