fix(runner): stop a review going stale after you open it, and close three smaller holes

[jtomaszewski]

Closes #35
Closes #36
Closes #38
Closes #39
Closes #40

Also settles #37, closed as not planned — the decision is recorded there and explained under "The comment decision" below.

The problem

Open a drafted review, and cerber stops re-reviewing that PR. The author pushes, and the review you are looking at goes on describing code that has moved — for good. Nothing in the queue tells you it has gone stale, and no poll ever corrects it. This is the ordinary path, not a corner: you glance at a draft, decide it isn't urgent, and come back tomorrow to an opinion about yesterday's commits.

Three quieter faults sit alongside it. Sending a review while a run was in flight could post it to GitHub twice — a hole in the one rule the product is built on. The local API would record a review as sent to GitHub when nothing had been sent. And starting the server raced its own first poll, so two writers could land on the same leftover row after an unclean shutdown.

All four were found by verifying docs/lifecycle.md against the code it describes, in #34.

The fix

The stale-review bug came from a field meaning two different things. The freshness guard asked "is this draft current?" by comparing the head commit recorded on the artifact against the PR's head — but opening a review refreshes it, which moves that field forward so the comments stay anchored to current code. Nothing is re-read at that moment. The guard was comparing the new head against itself.

Runs now record run.reviewedSha — the commit the AI actually read — and the guard compares against that instead. Opening a review no longer counts as reviewing it.

The double-send had its own cause: the runner replaced the artifact wholesale when it finished, putting sent: null back over a send that had landed meanwhile, after which the "already sent" guard waved a second submission through. The runner now folds its result onto what is on disk, and both send paths refuse while a run is in flight.

Details

• src/core/refresh.ts — mergeRunResult folds a finished run onto the current artifact instead of overwriting it: the run owns the draft it regenerated, the user owns the decisions. userOwnsStatus is the shared predicate for that, applied on the success and failure paths, so a settle made mid-run is not reopened by a run that then errors. carryOverComments and humanComments are removed — nothing carries comments across any more.
• src/runner/review.ts — records reviewedSha on success; the guard falls back to the old comparison when it is absent.
• src/server/index.ts — Send answers 409 while a run is in flight, checking the persisted status and this process's in-memory claim, because neither alone sees both a run in another terminal and one started here moments ago. PATCH /api/reviews/:key accepts only reviewed and skipped.
• src/cli/index.ts — cerber send refuses on a running artifact, going on the status alone, which is all another terminal can see. serve reconciles leftover runs before starting anything that polls.
• src/core/state.ts — reconcileRunning takes an inUse predicate and skips runs this process owns. The reorder above fixes the race by wiring; this fixes it by construction, whatever order things start in. It reuses the pattern evictOldCheckouts already established.
• src/core/artifact.ts — run.reviewedSha, nullable with a default.

The comment decision

#37 reported that a re-review destroys comments you wrote. It does — and after review that is now intended behaviour rather than a bug: a re-review regenerates the whole draft, your comments included, on success and on failure alike.

The alternative was carrying them across, which is what shipped before this branch: carry on success, lose on failure. That is the worst of the three options — it costs the code and still loses the work, and being right half the time is what made the bug hard enough to need a test to prove. Given that protecting the comments was not wanted, dropping the carry-over entirely is the honest version: one rule, true on every path. docs/lifecycle.md now states it plainly, with the advice that follows — send or copy anything you want to keep before pressing re-review.

Three tests pin it, so restoring carry-over would be a deliberate act with a failing test rather than a quiet drift from the docs.

Verification

pnpm typecheck, pnpm test (451 passed / 31 files, up from 433 measured on this branch point) and pnpm build all clean; CI green.

Every new test was confirmed to fail without its fix, by reverting each fix and re-running rather than assuming: reverting the reviewedSha guard fails its test, and reverting the two server guards fails 4 of 7 tests in guards.test.ts.

• src/runner/review-merge.test.ts (new) drives a real run end to end with claude stubbed, acting on the artifact mid-run as a user would — a send is not undone, a settle is not reopened on either the success or failure path, the recorded head is the one read, and the draft is replaced whichever way the run ends.
• src/server/guards.test.ts (new) covers both send refusals, separating the cross-process case from the in-process one, and the status allowlist.
• src/runner/review.test.ts — a draft opened after a push is still re-reviewed; one whose recorded review is of this very head is not.
• src/core/state.test.ts — reconciliation leaves a run this process owns alone.

Not covered: src/cli/index.ts has no tests, so neither the CLI's send refusal nor the serve reconcile ordering is exercised. That matches the repository, which has no CLI test harness at all; adding one is out of scope here. The half of the ordering fix that carries the actual safety — reconcileRunning({ inUse }) — is covered.

Breaking changes

Nothing in the artifact contract: run.reviewedSha is nullable with a default, so artifacts written before it load unchanged and fall back to the previous comparison.

Two behaviour changes worth naming:

• PATCH /api/reviews/:key rejects any status but reviewed and skipped with a 400. The cockpit only ever sent those two.
• A re-review no longer carries your comments across even when it succeeds — previously it did. This is the decision above, and it is the one change here a user could experience as a loss.

Follow-ups

None outstanding from this PR. The review pass found four minor items and all were fixed in 0307042 — three lines in docs/lifecycle.md that this branch's own changes had outdated, plus an unactionable error message in the CLI — so nothing was deferred to an issue.

[jtomaszewski]

🤖 om-open-pr — 🏷️ label rationale

• 🔍 review — ready for code review; this PR starts in review like every PR this step opens.
• 🐛 bug — five defects, no new capability: comment loss on re-review, a suppressed re-review, a double send, an over-permissive status endpoint, and a startup race.
• 🧪 needs-qa — the fixes change behaviour a person can see and should exercise by hand: Send now refuses while a run is in flight, a draft you have opened is still re-reviewed after a push, and comments must survive a re-review that errors. The regression tests pin all of it, but none of them is the cockpit.
• 🔥 priority-extreme — A failed re-review permanently deletes the comments you wrote #37 is silent, permanent loss of the user's own writing, and it is happening in the released version today; that is the data-loss tier, and this PR is the fix for it.
• ⚠️ risk-high — deliberately not risk-medium: the diff touches the persisted artifact schema, the runner's save path, both send paths, the status endpoint and startup ordering. That is cross-cutting and it is the persistence contract, which is the combination this tier exists for. Worth reviewing the merge semantics in mergeRunResult closely.

qaGate is false in .ai/agentic.config.json, so needs-qa records that manual QA is wanted — it does not block the merge.

[jtomaszewski]

🤖 om-open-pr — run summary

Branch: jtomaszewski/pr-lifecycle-docs-v1 → main (one commit, 576c2e8)

What shipped

Six issues closed — five code defects (#35–#39) and the doc corrections (#40). All five were found by verifying docs/lifecycle.md against the code it describes, on #34.

Two of them turned out to share a single cause — the runner replacing the artifact instead of merging onto it — so the fix for #37 also removes half of #36. The other half (refusing to send while a run is in flight) is its own guard.

Verification

gate	result
pnpm typecheck	clean
pnpm test	455 passed / 32 files (was 433)
pnpm build	clean

Every one of the 22 new tests was confirmed to fail without its fix by reverting each fix and re-running, not by assuming. Counts are in the PR body.

Two judgement calls the reviewer should weigh

1. carryOverComments is deleted. Nothing called it once the runner stopped carrying only on success. Leaving it would have been worse than dead code: it is named in CLAUDE.md as the mechanism protecting human work, so a future reader would have taken it for the live guard. Its four behaviours are re-pinned on mergeRunResult.
2. serve starts the daemon before it reconciles leftover running artifacts #38 got more than its issue asked for. The issue proposed reordering startup. I did that and gave reconcileRunning an inUse predicate so it skips runs this process owns. Ordering alone is a promise about wiring and cannot be tested at the seam; the predicate holds whatever order things start in, and matches the pattern evictOldCheckouts already uses. Say if you would rather have only the reorder.

Not in scope

#33 and #18 are still open. Both predate this work and neither was part of what #34 turned up.

Worth knowing

I found a gap in my own fix while reading the diff back: the failure path would have overwritten a skipped you set mid-run with failed — the same class of bug as #37, one path over. Fixed with the shared userOwnsStatus predicate and its own test before commit.

[jtomaszewski]

🤖 om-open-pr — 🏷️ label rationale (updated — supersedes the rationale above)

Scope changed after the maintainer decided against fixing #37, so two labels move with it.

• 🔍 review — ready for code review.
• 🐛 bug — four defects, no new capability: a suppressed re-review, a double send, an over-permissive status endpoint, and a startup race.
• 🧪 needs-qa — the fixes change behaviour a person can see and should exercise by hand: Send now refuses while a run is in flight, and a draft you have opened is still re-reviewed after a push.
• 🔺 priority-high — was priority-extreme. That rating was for the data-loss fix; with A failed re-review permanently deletes the comments you wrote #37 closed as not planned, no data-loss fix is in this PR. What remains is release-blocking rather than an active incident — the double send is a hole in the product's one hard rule.
• 🟡 risk-medium — was risk-high. The comment-merge logic that touched the persistence contract most broadly is gone with the A failed re-review permanently deletes the comments you wrote #37 revert. What is left is an additive nullable schema field, two endpoint guards and a startup reorder: an ordinary multi-area change with tests.

qaGate is false, so needs-qa records that manual QA is wanted — it does not block the merge.

On the linkage: the PR body no longer says Closes #37, but the first commit's message still does, so GitHub may show #37 as closed by this PR. #37 is already closed as not planned with the reasoning recorded there — the outcome is right either way. History was left intact rather than force-pushed; say the word if you would rather the two commits were squashed into one.

[jtomaszewski]

🤖 om-pr-autopilot — claimed

Driving this PR: diagnose → classify → chain → report. Invoked with --allow-merge, so the chain may end in a merge if the state matrix reaches row 9 and the QA gate is satisfied.

The lock is released on every exit path, including failure.

[jtomaszewski]

🤖 om-auto-review-pr — re-entry under the existing lock

om-pr-autopilot already holds the claim for this chain and $CURRENT_USER owns it, so this is re-entry rather than a new claim. Running with --autofix (eligible: the PR is mine and the flag was passed by the fixing chain).

The lock is retained at the end of this step, not released — the chain continues.

[jtomaszewski]

🔍 Code Review

Verdict: approve — no blockers, no majors. Four minor/nit findings, all fixed in-loop in 0307042 because each was drift introduced by this branch itself rather than pre-existing work.

Submitted as a comment, not an approval: GitHub refuses APPROVE from a PR's own author, so the verdict below is the review's real conclusion and the review state is the closest thing the API allows. A second pair of eyes is still worth having on mergeRunResult.

✅ Validation gate

All three commands from .ai/agentic.config.json, in order:

command	result
pnpm typecheck	clean
pnpm test	451 passed / 32 files
pnpm build	clean

🧭 Scope and correctness

The diff does what the five linked issues describe, and the two that shared a root cause are fixed by one change rather than two patches. mergeRunResult is the centre of it and reads correctly: the run owns the draft, the user owns the decisions, and userOwnsStatus is applied on both the success and failure paths — a settle made mid-run is not reopened by a run that then errors, which is the kind of asymmetry this class of bug usually leaves behind.

Two design points I checked rather than assumed:

• refresh is reset to null by a successful run. Correct, not a leak: the artifact is now at the run's own diff, so a record of a previous re-anchor would describe a state that no longer exists.
• fresh.pr wins over current.pr. Safe, because POST /refresh returns early on a running artifact, so pr/diff/comments cannot desynchronise mid-run.

🐛 Findings

All four fixed in 0307042.

• minor — docs/lifecycle.md:138 — claimed cerber send refuses "with a 409". A CLI has no HTTP status; it prints the reason and exits non-zero. Only the cockpit answers 409.
• minor — docs/lifecycle.md §6.3 — listed what a successful run persists without run.reviewedSha, the field the entire freshness fix turns on.
• minor — docs/lifecycle.md §6.4 — said a failure writes failed; this branch made that conditional on userOwnsStatus.
• nit — src/cli/index.ts:183 — "wait for it to finish" is unactionable when the run died mid-flight. Now names re-running or restarting cerber serve as the way to clear a stale status.

These matter more than their severity suggests: this document's stated contract is that every rule names the file it lives in so that code changes which outdate it are findable. A reference doc that drifts on the same branch that changed the code is the failure mode it exists to prevent.

🧪 Test coverage

Strong where it counts. Every new test was verified to fail without its fix by reverting each fix and re-running — reverting the reviewedSha guard fails its test, reverting the two server guards fails 4 of 7 guard tests. That is the difference between tests that pin behaviour and tests that merely pass.

One honest gap, recorded rather than fixed: src/cli/index.ts has no tests — neither the new send-refusal nor the serve reconcile ordering is covered. That matches the repository, which has no CLI test file at all, so adding the harness is out of scope for this PR. The reconcileRunning({ inUse }) half of the ordering fix is covered in src/core/state.test.ts, which is the half that carries the actual safety.

⚠️ Worth knowing

inFlight blocks Send on a stale running status, which is a new way to be stuck if a detached run dies without its catch handler firing. The recovery path exists — re-review resets the status, as does restarting serve — and the CLI message now says so. Rare enough not to hold the PR.

🔁 Backward compatibility

No breaks. run.reviewedSha is nullable with a default, so older artifacts load unchanged and fall back to the previous comparison. The PATCH allowlist is a deliberate narrowing; the cockpit only ever sent the two statuses it still accepts.

[jtomaszewski]

🤖 om-auto-review-pr — completed: APPROVE (submitted as a comment review — GitHub refuses APPROVE from a PR's own author; the attempt was made and rejected with Can not approve your own pull request, not skipped).

Autofix ran and fixed four minor/nit findings in 0307042, all of them drift this branch introduced into its own reference doc plus one unactionable CLI message. No blockers, no majors, so nothing was deferred and no follow-up issue was filed.

Pipeline label moved review → merge-queue. needs-qa kept: user-facing behaviour changed and no QA evidence exists, though qaGate is false so it does not block.

Lock retained — chain continues.

[jtomaszewski]

🤖 om-pr-autopilot — run summary

Chain: diagnose → om-auto-fix-pr → om-auto-review-pr --autofix → om-approve-merge-pr. Invoked with --allow-merge.

What each step did

step	outcome
diagnose	Matrix rows 2, 5, 9 matched. Rows 1, 3, 4, 6, 7, 10 skipped — reasons below.
om-auto-fix-pr 41 --max-iterations 3	Base already current (merge-base == origin/main), so the mandatory base-merge was a no-op. One loop iteration; no CI stabilization needed.
om-auto-review-pr 41 --autofix	Verdict: approve. Four minor/nit findings, all fixed in-loop in 0307042. No blockers, no majors, so nothing was deferred and no follow-up issue was filed.
om-approve-merge-pr 41	Stopped at step 3 — see below.

What the review found

All four were drift this branch introduced into its own work, which is why they were fixed here rather than filed:

• docs/lifecycle.md claimed cerber send refuses "with a 409" — a CLI has no HTTP status; it prints and exits non-zero.
• §6 listed what a successful run persists without run.reviewedSha, the field the entire freshness fix turns on.
• §6 said a failure writes failed; this branch made that conditional on userOwnsStatus.
• The CLI's send refusal said "wait for it to finish", which is unactionable if the run died — it now names the recovery.

A reference doc drifting on the same branch that changed the code is precisely the failure it exists to prevent, so these mattered more than their severity suggests.

Gates

• CI: green on 0307042 — test passed, release skipped by design. No wait was needed, so the ci.maxWaitMinutes budget (20) is untouched. main is not branch-protected, so there are no required checks; every reported check was treated as required.
• QA: needs-qa is unmet and no evidence exists. qaGate is false, so it is advisory rather than blocking — recorded here rather than waved through. The diff touches no web/ file, so matrix row 7 (UI QA) never fired: there is no screen to capture.
• Hard blocks: none — no qa-failed, do-not-merge, blocked, or active qa.

Why the merge did not happen

om-approve-merge-pr step 3 could not submit the approving review: GitHub refuses APPROVE from a PR's own author (Can not approve your own pull request). The skill's documented behaviour there is to report and ask rather than proceed straight to merge, and this is one of the gated human-decision cases the autopilot stops on.

That gate is worth honouring here rather than routing around, because merging would mean:

1. landing this with no independent approval — the only review on it is mine, of my own code;
2. needs-qa unmet, with no QA evidence; and
3. an npm publish — .github/workflows runs semantic-release on every green merge to main, and these are fix: commits, so merging ships a patch release of @fullstackhouse/cerber to the public registry.

The PR is merge-ready on every mechanical signal (MERGEABLE / CLEAN). The decision left is a human one.

Labels are merge-queue, bug, needs-qa, priority-high, risk-medium; in-progress is released with this comment.

[github-actions]

🎉 This PR is included in version 0.24.1 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀