A failed re-review permanently deletes the comments you wrote

[jtomaszewski]

The problem

Press re-review, have the run fail, and every comment you wrote by hand — plus every AI comment you rewrote — is gone from disk. Not hidden, not recoverable: deleted.

The failure does not have to be exotic. A model error, a network blip, a checkout that goes wrong mid-run, claude not being logged in — anything that throws after the run has started. The row comes back as failed, and your writing is not in it.

This contradicts the rule the repo states explicitly in CLAUDE.md:

Don't let a re-review discard human work: comments the user wrote or edited are carried across (carryOverComments), never regenerated away.

That guarantee currently holds only when the run succeeds.

Why it happens

reviewPr (src/runner/review.ts) writes the artifact three times, and the ordering is the bug:

1. Before calling Claude it builds a fresh artifact literal with comments: [] (:176-207) and saves it (:211). The user's comments leave the disk here — the file now holds an empty list. Everything that protects them from this point on lives in memory, in the existing variable.
2. On success, carryOverComments(existing, artifact) (:269) puts them back, re-anchored to the new diff. This is the path the rule describes, and it works.
3. On failure, the catch handler (:278-289) spreads that same pre-run object — still comments: [] — sets status: "failed", and saves. carryOverComments is never reached.

So the human comments are destroyed at step 1 unconditionally, and restored only by step 2. Step 3 makes the deletion permanent.

Reproducing it

// a ready artifact holding one comment the user wrote
await saveArtifact(withUserComment("old-sha"));
expect((await loadArtifact(ID))!.comments).toHaveLength(1);

prInfo.mockResolvedValue(pr("new-sha"));
claude.mockRejectedValue(new Error("model unavailable"));
await expect(reviewPr(REF, { withSource: false })).rejects.toThrow();

const after = (await loadArtifact(ID))!;
expect(after.status).toBe("failed");
expect(after.comments).toHaveLength(0);   // passes — the comment is gone

Both assertions pass today. Observed output: status=failed comments on disk=0.

Suggested fix

The failure path needs the same carry-over the success path has — the human comments are known (humanComments(existing), src/core/refresh.ts) and nothing about a failed run makes them less valid.

Options, roughly in order of how much they change:

• Minimal: in the catch handler, restore the human comments from existing before saving the failed artifact.
• Better: don't drop them in the first place. Seed the pre-run artifact from humanComments(existing) rather than [], so no window exists where the file is missing the user's writing. The success path's carryOverComments then re-anchors as it already does.
• Belt and braces: have the failure write merge onto what is on disk rather than saving a whole object built minutes earlier — the same discipline mergeConcurrentEdits (src/core/revise.ts) applies to chat turns, and the same one Send during an in-flight run can post the review to GitHub twice #36 needs.

Wants a regression test on the failing path specifically: user comment in, run throws, comment still on disk.

Scope

Found while verifying docs/lifecycle.md (#34) against the code. Third of three defects that review turned up; see also #35 and #36. Priority is set higher than those two because this one silently destroys the user's own writing, and there is no way to get it back.

[jtomaszewski]

Wider than the failure path

A later review pass found the same root cause costs comments on the success path too, so this issue covers both.

carryOverComments works from existing — the artifact as it was read at the start of the run, minutes earlier (src/runner/review.ts:134). The review stays editable the whole time the run is going: the detail view gates on sent, not on status === "running". So anything you do to the comments mid-run — add one, edit one, drop one — is written over when the run finishes and saves its result wholesale at :293.

That makes two distinct losses from one cause:

	what is lost
run fails	every human comment, permanently (the original report)
run succeeds	whatever you changed while it was running

The common cause is that the runner replaces the artifact instead of merging onto it. mergeConcurrentEdits (src/core/revise.ts:59) already does the merge properly for chat turns, and exists for exactly this reason — a turn must not clobber what the user did while it ran. The review runner needs the same treatment, which fixes both rows above and is also the third bullet of the suggested fix in #36.

Regression tests worth having: comment in → run throws → comment survives; and comment edited mid-run → run succeeds → the edit survives.

[jtomaszewski]

Closing as decided against, not as fixed.

The maintainer's call: losing draft comments across a re-review is acceptable, so cerber will not carry code to protect them. #41 removes the carry-over rather than completing it.

The reasoning worth recording, since the alternative looks appealing: the behaviour that shipped before #41 — carry on success, lose on failure — is the worst of the three options. It costs the code and still loses the work, and being right half the time is exactly what made this bug hard enough to need a test to prove. Given that protecting the comments is not wanted, dropping the carry-over entirely is the honest version: one rule, true on every path.

So a re-review now regenerates the whole draft, your comments included, whether the run succeeds or fails. docs/lifecycle.md states that where a reader would otherwise assume the opposite, with the advice that follows — send or copy anything you want to keep before pressing re-review. Three tests in src/runner/review-merge.test.ts and src/core/refresh.test.ts pin it, so restoring carry-over would be a deliberate act with a failing test rather than a quiet drift from the docs.

What #41 does keep from the original diagnosis is the part that was never about draft work: the runner no longer writes its result over a send or a settle that landed while it ran. That was the same root cause and it stays fixed — see #36.

Reopen if the trade-off ever changes; the analysis and the reproduction in this issue still stand.