fix(backlog): recover items wedged in review by an idle-but-alive reviewer (recovers #342) - #347
Merged
Merged
Conversation
…session's own base commit ItemSession.LastCommitSha was written exactly once — at session spawn, with the worktree's pre-work HEAD — and never refreshed as the agent committed. A session's base commit is by construction already an ancestor of main, so git.IsCommitOnMain on it is unconditionally true. Two consumers trusted the field as "the session's latest commit": - closeIfSupersededByMain (session/backlog_lifecycle.go) closed the item's open PR unmerged and marked the item done. - GetBacklogItemShipStatus, which backs the item detail page's Ship PR status. Live blast radius, from backlog_status_events in the deployed instance: 15 PRs were auto-closed as "superseded". Four distinct items cite the identical SHA 654c601, three cite 4eca0ed — a 2026-06-01 benchmark-baseline chore commit used to close three PRs on 2026-07-29 — and one cites cc66c0b, a 2026-04-09 test commit. Unrelated items cannot all ship in one such commit; these are spawn-time base SHAs. The most recent, PR #342 (BUG-047's own fix, reviewed and CI-green), was closed against base SHA 1a75172 from ~24h before that work started. Fix, in three parts: 1. Split the concept. New ItemSession.base_commit_sha holds the spawn-time baseline for the review gate's base..HEAD diff; the three spawn write sites now call SetItemSessionBaseCommit instead of overloading the git-activity fields. 2. Make LastCommitSha true to its name. refreshWorkSessionGitActivity re-reads each live work session's real HEAD (go-git via the new git.CommitInfo, per .claude/rules/prefer-go-git-over-subshells.md) and recomputes commit_count_since_spawn. It is wired into the existing reconciliation sweep's detector list rather than adding a poller, and is registered first so same-tick consumers read fresh values. 3. Fix both consumers to resolve the session's real tip via resolveLatestWorkCommit — the remedy already applied to this file's reconcileBouncingItems and to isCodeShippedToMain, which closeIfSupersededByMain was never migrated to — plus an explicit BaseCommitSha guard so the fallback path can never re-enter the bug for rows already in production databases. Also fixes a consistency bug this exposed: ship status resolved the SHA live but captioned it with the stored (stale) commit message and timestamp. ent schema regenerated with --feature sql/upsert per .claude/rules/ent-schema-generation.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba
Found while running make ci for the idle-reviewer-wedge fix — unrelated to that change (confirmed already unformatted on origin/main), fixed as collateral debt per repo convention rather than left blocking CI. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UcM8eWZdxXsxqSFyteMZUS
…iewer A reviewer session that submits a verdict via submit_review_verdict and then never exits (process alive, no further output) was invisible to both handleReviewSessionExited (session-exit only) and reconcileUnprocessedReviewVerdicts' crash-recovery sweep (requires the session confirmed dead via SessionLivenessChecker) — wedging the item in "review" forever. - submitReviewVerdict now drives the review->in_progress transition eagerly for FAIL/PARTIAL/UNVERIFIABLE verdicts via the existing AutoReopenSpawner (server/mcp/tools_backlog.go), reusing AutoReopenAfterFailedReview's CAS-guarded (ExpectedStatus: review) transition, rework-cap/circuit-breaker checks, and work-session respawn logic rather than reimplementing them. PASS stays deferred to handleReviewSessionExited, unchanged. - reconcileUnprocessedReviewVerdicts gets an idle-timeout OR condition: a verdict older than reviewVerdictIdleThreshold (2h, matching maxWorkSessionStaleness) is now actionable even when SessionLivenessChecker reports the session alive — covers PASS verdicts and any case the eager path doesn't reach (e.g. no AutoReopenSpawner wired). - The review-role prompt (BuildReviewPrompt, get_backlog_item's review-role guidance, and the sdd pipeline mode's review template) now instructs the reviewer to end its session immediately after calling submit_review_verdict, symmetric to the work-role prompt's existing "Do NOT end your session" instruction — closing the root behavioral cause. - BUG-051 (session/tmux flaking under make ci's parallel load) is fixed on main and verified green here; docs/bugs marked fixed and moved accordingly. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UcM8eWZdxXsxqSFyteMZUS
… fix Four-agent parallel review (testing, code quality, architecture, security) on PR #342 surfaced two real MAJOR correctness gaps and three MAJOR test coverage gaps; security review found nothing. Addressing all five here: - server/server.go: nil-guard deps.BacklogService before boxing it into the session.AutoReopenSpawner interface param passed to NewHTTPHandler, mirroring the other three nil-checks already on this same field in this function. A nil *services.BacklogService boxed directly into the interface produces a non-nil interface value around a nil pointer (the classic Go typed-nil trap) — submitReviewVerdict's own `h.autoReopener != nil` guard would read true and the call would panic on the nil receiver instead of being skipped. - server/mcp/tools_backlog.go: the eager AutoReopenAfterFailedReview call now runs on a context.WithoutCancel + 30s-bounded context instead of the live request ctx. AutoReopenAfterFailedReview's only other callers run on long-lived background contexts; its own rollback-on-spawn-failure path reuses whatever ctx it's given, so inheriting the request ctx meant a client-side disconnect could cancel both the transition attempt and its own safety-net rollback together. - Added 3 test cases: nil-autoReopener now asserts the item stays in review (not just "no crash"), a just-under-threshold idle-timeout subtest guards the strict `>` comparison's boundary, and a PASS-outcome idle-timeout subtest covers the idle-timeout branch's stated primary remaining purpose (PASS stays deferred to session-exit by design, so this sweep is the only path back out of review for a PASS verdict whose reviewer went idle). make ci green (build, full suite incl. -race/integration, lint, registry regen, no drift). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UcM8eWZdxXsxqSFyteMZUS
…exposed Self-review of the LastCommitSha split found three places that read the field for its *base* meaning, which the live refresh would have silently broken: - review_gate.go's directory-mode branch passed LastCommitSha as GetGitDiff's base. Once that field tracks the tip, this diffs the tip against itself and every directory-mode review gets an EMPTY diff — a silent review bypass. - GetBaseCommitSHAsForSessions (despite its name) selected last_commit_sha to restore dirBaseSHA at startup, giving those sessions a moving diff base. - UpdateItemSessionGitActivity set last_progress_at from the commit's author timestamp. Author dates survive rebases, and this repo rebases session worktrees onto main routinely, so a rebase would push the staleness clock backwards and hand a healthy, actively-committing session to stale_work remediation. Progress is recorded when observed; last_commit_at keeps the true author time for display. The first two read base_commit_sha with a fallback to last_commit_sha for rows written before the split. That fallback is only safe because the original bug meant both fields held the same value on every legacy row — it is explicitly not extended to rows that have a base_commit_sha. Adds TestUpdateItemSessionGitActivity_should_RecordProgressAtObservationTime_When_CommitIsBackdated, verified to fail against the author-timestamp version. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba
Picks up the three base-vs-latest consumer corrections found in self-review (review_gate.go's directory-mode diff base, GetBaseCommitSHAsForSessions, and the last_progress_at clock) so this branch is tested against the final form of the reconciler fix it stacks on.
tstapler
changed the base branch from
fix/item-session-last-commit-live-tracking
to
main
August 5, 2026 20:02
tstapler
marked this pull request as ready for review
August 5, 2026 20:02
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Restores and completes the fix for BUG-047 where backlog items could remain wedged in review if a reviewer submits a verdict but the session stays alive/idle, and wires the auto-reopen capability into the MCP server path.
Changes:
- Adds an eager
review -> in_progresstransition on reject verdicts (FAIL/PARTIAL/UNVERIFIABLE) viaAutoReopenSpawner, plus nil-safe wiring in the HTTP MCP server. - Extends the crash-recovery sweep to treat old review verdicts as actionable even if the reviewer session is still “alive”.
- Updates review prompts/guidance and adds regression tests for the prompt text and the new lifecycle behaviors.
Reviewed changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| session/pipeline_mode_seed.go | Updates the reviewer prompt template to instruct immediate session exit after verdict submission. |
| session/backlog_review.go | Injects the “end session after verdict” instruction into the generated review prompt. |
| session/backlog_review_test.go | Adds regression test asserting the review prompt includes the new instruction. |
| session/backlog_lifecycle.go | Adds idle-verdict timeout condition and new threshold constant for sweep actionability. |
| session/backlog_lifecycle_test.go | Adds test helper to create backdated verdicts for sweep tests. |
| session/backlog_lifecycle_stuck_test.go | Expands reconcile tests to cover idle-timeout behavior (including PASS-case shipping). |
| server/server.go | Wires AutoReopenSpawner into MCP HTTP handler with correct nil-interface guarding. |
| server/mcp/server.go | Extends MCP constructors/runner to accept an optional AutoReopenSpawner. |
| server/mcp/server_integration_test.go | Updates integration test to match NewCore signature. |
| server/mcp/feature_flag_test.go | Updates feature-flag tests to match NewCore signature. |
| server/mcp/tools_backlog.go | Implements eager auto-reopen call on reject verdicts with timeout and cancellation semantics. |
| server/mcp/tools_backlog_test.go | Adds tests for reviewer guidance and eager auto-reopen behavior (including nil-safety and error swallow). |
| server/services/backlog_service_triage_test.go | Adds regression tests asserting CAS-harmless behavior and work-session spawn on auto-reopen. |
| main.go | Updates stdio MCP run path to pass nil AutoReopenSpawner and documents behavior. |
| docs/bugs/fixed/BUG-051-session-tmux-package-flaky-under-parallel-quick-check.md | Marks BUG-051 as fixed and appends recurrence/resolution notes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1822
to
+1823
| if !dead && latest.Edges.ReviewVerdict != nil && time.Since(latest.Edges.ReviewVerdict.CreatedAt) > reviewVerdictIdleThreshold { | ||
| // A reviewer that submitted a verdict and then simply never exited |
Comment on lines
+702
to
+706
| reopenCtx, reopenCancel := context.WithTimeout(context.WithoutCancel(ctx), 30*time.Second) | ||
| if reopenErr := h.autoReopener.AutoReopenAfterFailedReview(reopenCtx, itemID); reopenErr != nil { | ||
| log.WarningLog.Printf("[submitReviewVerdict] AutoReopenAfterFailedReview item=%s: %v", itemID, reopenErr) | ||
| } | ||
| reopenCancel() |
Comment on lines
+318
to
+319
| you read. End your session immediately after calling submit_review_verdict - do not | ||
| wait, poll, or do further work. |
Contributor
✅ Registry ValidationTest Coverage: 31/184 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
Frontend Terminal Throughput |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
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.
Recovers the work from #342, which was closed unmerged by a bug — not on its own merits.
Why this PR exists
#342 was reviewed, CI-green, and ready. The
closeIfSupersededByMainreconciler then read its work session'sLastCommitSha— which held the session's spawn-time base SHA1a751723b, an unrelated commit from ~24h before the work began — concluded the item had "already shipped through another path", closed the PR without merging, and marked backlog itemd6ddbef3-238e-43dc-8a69-c3700cc440bfdone. The fix never shipped. That reconciler bug is fixed in #346; this PR restores the stranded work.The original commits are preserved as-is (
541fc846e,342c49b6e, plus thegofmtcollateralbb3061a75) — cherry-picked cleanly with no conflicts. The content of #342 was never in question.Stacking / merge order
Based on
fix/item-session-last-commit-live-tracking(#346), notmain, because both touchsession/backlog_lifecycle.go. GitHub will retarget this tomainautomatically once #346 merges. Merge #346 first.Verified locally that both fixes coexist:
go vetclean, and the two changes' test suites pass together in one run.Original description (from #342)
A reviewer session that submits a verdict via
submit_review_verdictand then never exits (process alive, no further output) was invisible to bothhandleReviewSessionExited(session-exit only) andreconcileUnprocessedReviewVerdicts's crash-recovery sweep (requires the session confirmed dead viaSessionLivenessChecker) — wedging the item inreviewforever.What changed
submitReviewVerdict(server/mcp/tools_backlog.go) now drives thereview -> in_progresstransition eagerly for FAIL/PARTIAL/UNVERIFIABLE verdicts, routed through the existingAutoReopenSpawnerinterface rather than reimplemented — CAS-guarded (ExpectedStatus: review), reusing rework-cap/circuit-breaker checks and work-session respawn logic as-is. PASS stays deferred tohandleReviewSessionExited, unchanged.reconcileUnprocessedReviewVerdicts(session/backlog_lifecycle.go) gets an idle-timeout OR condition: a verdict older thanreviewVerdictIdleThreshold(2h, matchingmaxWorkSessionStaleness) is now actionable even whenSessionLivenessCheckerreports the session alive.submit_review_verdict, symmetric to the work-role prompt's existing "Do NOT end your session" instruction.NewCore/NewHTTPHandler/RunServertake a new optionalautoReopener session.AutoReopenSpawnerparam, wired fromdeps.BacklogServicein the HTTP server path; the stdio--mcpfallback path has noBacklogServiceavailable and passesnil, documented in-code.Test plan
make cigreen on the original branch before it was wrongly closed.autoReopenersafety, CAS-harmless double-call, no-active-work-session spawn path, idle-timeout sweep (under and over threshold), review-prompt content assertions.go vetclean, both suites green together.Two live items (
3065ecfb,4c71d3a3) were waiting on this fix to self-heal and are still wedged because it never shipped.🤖 Generated with Claude Code