Skip to content

fix(backlog): recover items wedged in review by an idle-but-alive reviewer - #342

Closed
tstapler wants to merge 7 commits into
mainfrom
backlog/stapler-squad-fix-idle-reviewer-wedge
Closed

fix(backlog): recover items wedged in review by an idle-but-alive reviewer#342
tstapler wants to merge 7 commits into
mainfrom
backlog/stapler-squad-fix-idle-reviewer-wedge

Conversation

@tstapler

@tstapler tstapler commented Aug 5, 2026

Copy link
Copy Markdown
Owner

Summary

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's crash-recovery sweep (requires the session confirmed dead via SessionLivenessChecker) — wedging the item in review forever. Live evidence: backlog item 4c71d3a3-1dd5-4d82-86ec-694a98835d2f currently shows a recorded PARTIAL verdict with status still stuck in review.

Fixes backlog item d6ddbef3-238e-43dc-8a69-c3700cc440bf.

What Changed

  • submitReviewVerdict (server/mcp/tools_backlog.go) now drives the review -> in_progress transition eagerly for FAIL/PARTIAL/UNVERIFIABLE verdicts, routed through the existing AutoReopenSpawner interface (AutoReopenAfterFailedReview) rather than reimplemented — CAS-guarded (ExpectedStatus: review), rework-cap/circuit-breaker checks and work-session respawn logic reused as-is. PASS stays deferred to handleReviewSessionExited, unchanged.
  • reconcileUnprocessedReviewVerdicts (session/backlog_lifecycle.go) 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 can't reach (e.g. no AutoReopenSpawner wired).
  • The review-role prompt (BuildReviewPrompt, get_backlog_item's review-role guidance block, 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.
  • NewCore/NewHTTPHandler/RunServer (server/mcp/server.go) take a new optional autoReopener session.AutoReopenSpawner param, wired from deps.BacklogService in the HTTP server path (server/server.go); the stdio --mcp fallback path (main.go) has no BacklogService available (Phase 1 CoreDeps only) and passes nil — documented in-code.
  • Collateral: fixed a pre-existing gofmt drift in session/git/worktree_ops.go (unrelated, already broken on origin/main), and closed out BUG-051 (session/tmux flaking under make ci's parallel load — fixed upstream on main, verified green here after merging).

Test plan

  • make ci (build, full go test ./..., -race -short, -race -tags integration, lint, registry regen) — green, no regressions.
  • New unit tests: eager-call on FAIL/PARTIAL/UNVERIFIABLE, no-call on PASS, nil-autoReopener safety, CAS-harmless double-call (AutoReopenAfterFailedReview called twice), no-active-work-session spawn path, idle-timeout sweep (both under and over threshold), review-prompt content assertions.
  • get_backlog_item on the live wedged item 4c71d3a3-1dd5-4d82-86ec-694a98835d2f confirms it currently reproduces the exact bug (PARTIAL verdict, still review) — will recover automatically within one sweep interval, or immediately on its next verdict, once this deploys. Not force-verified via a live service restart in this session (would restart the shared production instance and disrupt other active sessions on this machine).

🤖 Generated with a Claude Code backlog automation session.

https://claude.ai/code/session_01UcM8eWZdxXsxqSFyteMZUS

tstapler and others added 6 commits August 4, 2026 08:49
Sessions (like this one) had no way to actually create a backlog item
directly — filing a GitHub issue and hoping someone imports it later was
the only path. Both new tools call the same storage.CreateBacklogItem /
github.GetIssue the web UI's "New Idea" and "Import from GitHub" actions
already use, just exposed as MCP tools.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W3683CH7Fs9zYR2yP3Dpba
Adds two sections to /review-queue: backlog items awaiting plan
approval (same gate BacklogItemDetail's Approve Plan button uses),
and sessions that are simply active/creating but not needing
attention — so it's clear at a glance what needs interaction versus
what's just running.

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
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

✅ Registry Validation

Registry Validation
===================

Building backend scanner...
Scanning backend features...
Wrote 117 feature files to /tmp/tmp.MhmJJxQJUV/backend
Wrote 15 feature files to /tmp/tmp.MhmJJxQJUV/backend
Wrote 46 feature files to /tmp/tmp.MhmJJxQJUV/backend
Wrote 8 feature files to /tmp/tmp.MhmJJxQJUV/backend
Wrote 12 feature files to /tmp/tmp.MhmJJxQJUV/backend
Wrote 6 feature files to /tmp/tmp.MhmJJxQJUV/backend

=== Backend Registry Diff ===
Committed: 184  Generated: 184  Divergence: 0.0%
⚠️  109 feature(s) missing // +api: marker (markerFound: false)

✅ Registry validation passed. Divergence: 0.0%

Test Coverage: 31/184 features have testIds (16.8%)

Divergence > 2% blocks merges. Coverage reporting is advisory only.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Go Benchmarks (Tier 1)

benchmarks/go/tier1-baseline.txt:98: missing iteration count
benchmarks/go/tier1-baseline.txt:198: missing iteration count
tier1-bench.txt:98: missing iteration count
tier1-bench.txt:198: missing iteration count
goos: linux
goarch: amd64
pkg: github.com/tstapler/stapler-squad/session
cpu: AMD EPYC 7763 64-Core Processor                
                                            │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt          │
                                            │              sec/op              │   sec/op     vs base              │
CircularBufferWrite_4KB-4                                          81.69n ± 1%   83.08n ± 2%  +1.69% (p=0.005 n=8)
CircularBufferWrite_4KB_Allocs-4                                   82.67n ± 0%   83.96n ± 0%  +1.57% (p=0.006 n=8)
CircularBufferGetRecent_4KB-4                                      500.7n ± 1%   528.1n ± 3%  +5.47% (p=0.001 n=7)
CircularBufferGetAll-4                                             4.290µ ± 8%   4.176µ ± 2%       ~ (p=0.878 n=8)
GetTimeSinceLastMeaningfulOutput_HotPath-4                         65.76n ± 0%   65.77n ± 0%       ~ (p=0.562 n=8)
GetTimeSinceLastMeaningfulOutput_ColdPath-4                        32.78n ± 1%   32.77n ± 0%       ~ (p=0.169 n=7)
geomean                                                            177.5n        179.2n       +0.98%

                                            │ benchmarks/go/tier1-baseline.txt │           tier1-bench.txt            │
                                            │               B/op               │     B/op      vs base                │
CircularBufferWrite_4KB-4                                         0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferWrite_4KB_Allocs-4                                  0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferGetRecent_4KB-4                                   4.000Ki ± 0%     4.000Ki ± 0%       ~ (p=1.000 n=7) ¹
CircularBufferGetAll-4                                          40.00Ki ± 0%     40.00Ki ± 0%       ~ (p=1.000 n=8) ¹
GetTimeSinceLastMeaningfulOutput_HotPath-4                        0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=8) ¹
GetTimeSinceLastMeaningfulOutput_ColdPath-4                       0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=7) ¹
geomean                                                                      ²                 +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                            │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                                            │            allocs/op             │ allocs/op   vs base                │
CircularBufferWrite_4KB-4                                         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferWrite_4KB_Allocs-4                                  0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferGetRecent_4KB-4                                     1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=7) ¹
CircularBufferGetAll-4                                            1.000 ± 0%     1.000 ± 0%       ~ (p=1.000 n=8) ¹
GetTimeSinceLastMeaningfulOutput_HotPath-4                        0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
GetTimeSinceLastMeaningfulOutput_ColdPath-4                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=7) ¹
geomean                                                                      ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                              │               B/s                │     B/s       vs base              │
CircularBufferWrite_4KB-4                           46.70Gi ± 1%   45.92Gi ± 1%  -1.66% (p=0.005 n=8)
CircularBufferGetRecent_4KB-4                       7.619Gi ± 1%   7.223Gi ± 3%  -5.20% (p=0.001 n=7)
geomean                                             18.86Gi        18.21Gi       -3.45%

pkg: github.com/tstapler/stapler-squad/session/detection/ratelimit
                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt          │
                              │              sec/op              │   sec/op     vs base              │
StripANSI_PlainText-4                                6.869n ± 0%   6.872n ± 0%       ~ (p=1.000 n=8)
StripANSI_WithEscapes-4                              748.1n ± 0%   752.1n ± 1%       ~ (p=0.083 n=8)
ProcessOutput_InactiveState-4                        6.343n ± 0%   6.321n ± 1%       ~ (p=0.069 n=8)
geomean                                              31.94n        31.97n       +0.07%

                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                              │               B/op               │    B/op     vs base                │
StripANSI_PlainText-4                               0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
StripANSI_WithEscapes-4                             136.0 ± 0%     136.0 ± 0%       ~ (p=1.000 n=8) ¹
ProcessOutput_InactiveState-4                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                        ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                              │            allocs/op             │ allocs/op   vs base                │
StripANSI_PlainText-4                               0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
StripANSI_WithEscapes-4                             5.000 ± 0%     5.000 ± 0%       ~ (p=1.000 n=8) ¹
ProcessOutput_InactiveState-4                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                        ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

pkg: github.com/tstapler/stapler-squad/session/queue
                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt          │
                              │              sec/op              │   sec/op     vs base              │
ReviewQueue_ConcurrentReads-4                        90.02n ± 6%   82.80n ± 6%  -8.03% (p=0.000 n=8)
ReviewQueue_Add-4                                    504.2n ± 1%   506.1n ± 1%       ~ (p=0.574 n=8)
geomean                                              213.0n        204.7n       -3.91%

                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                              │               B/op               │    B/op     vs base                │
ReviewQueue_ConcurrentReads-4                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
ReviewQueue_Add-4                                   640.0 ± 0%     640.0 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                        ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                              │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                              │            allocs/op             │ allocs/op   vs base                │
ReviewQueue_ConcurrentReads-4                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
ReviewQueue_Add-4                                   4.000 ± 0%     4.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                        ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

pkg: github.com/tstapler/stapler-squad/session/scrollback
                                      │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                                      │              sec/op              │    sec/op     vs base              │
CircularBuffer_ConcurrentReadWrite-4                         3.920µ ± 1%    3.997µ ± 3%  +1.96% (p=0.035 n=8)
CircularBuffer_BurstAppend-4                                 102.4µ ± 1%    102.6µ ± 0%       ~ (p=0.130 n=8)
CircularBuffer_GetLastN_LargeBuffer-4                        20.29µ ± 2%    21.74µ ± 3%  +7.15% (p=0.010 n=8)
CircularBuffer_GetRange_Sequential-4                         12.68µ ± 8%    13.49µ ± 8%  +6.38% (p=0.000 n=8)
CircularBufferAppend-4                                       98.94n ± 1%   100.50n ± 0%  +1.58% (p=0.000 n=8)
CircularBufferGetLastN-4                                     2.624µ ± 2%    2.588µ ± 3%       ~ (p=0.099 n=8)
CircularBufferConcurrentAppend-4                             129.8n ± 0%    128.0n ± 0%  -1.43% (p=0.000 n=8)
geomean                                                      3.206µ         3.271µ       +2.02%

                                      │ benchmarks/go/tier1-baseline.txt │           tier1-bench.txt            │
                                      │               B/op               │     B/op      vs base                │
CircularBuffer_ConcurrentReadWrite-4                        6.062Ki ± 0%   6.062Ki ± 0%       ~ (p=1.000 n=8) ¹
CircularBuffer_BurstAppend-4                                62.50Ki ± 0%   62.50Ki ± 0%       ~ (p=1.000 n=8) ¹
CircularBuffer_GetLastN_LargeBuffer-4                       56.00Ki ± 0%   56.00Ki ± 0%       ~ (p=1.000 n=8) ¹
CircularBuffer_GetRange_Sequential-4                        28.00Ki ± 0%   28.00Ki ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferAppend-4                                        24.00 ± 0%     24.00 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferGetLastN-4                                    6.000Ki ± 0%   6.000Ki ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferConcurrentAppend-4                              32.00 ± 0%     32.00 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                     3.077Ki        3.077Ki       +0.00%
¹ all samples are equal

                                      │ benchmarks/go/tier1-baseline.txt │           tier1-bench.txt           │
                                      │            allocs/op             │  allocs/op   vs base                │
CircularBuffer_ConcurrentReadWrite-4                          2.000 ± 0%    2.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBuffer_BurstAppend-4                                 1.000k ± 0%   1.000k ± 0%       ~ (p=1.000 n=8) ¹
CircularBuffer_GetLastN_LargeBuffer-4                         1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBuffer_GetRange_Sequential-4                          1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferAppend-4                                        1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferGetLastN-4                                      1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=8) ¹
CircularBufferConcurrentAppend-4                              1.000 ± 0%    1.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                       2.962         2.962       +0.00%
¹ all samples are equal

                             │ benchmarks/go/tier1-baseline.txt │        tier1-bench.txt        │
                             │               B/s                │     B/s       vs base         │
CircularBuffer_BurstAppend-4                       595.9Mi ± 1%   594.7Mi ± 1%  ~ (p=0.130 n=8)

pkg: github.com/tstapler/stapler-squad/session/tmux
                             │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt          │
                             │              sec/op              │   sec/op     vs base              │
StripANSICodes_PlainText-4                          6.880n ± 0%   6.877n ± 0%       ~ (p=0.704 n=8)
StripANSICodes_WithEscapes-4                        689.3n ± 0%   691.2n ± 0%       ~ (p=0.203 n=8)
IsBanner_PlainText-4                                476.7n ± 0%   479.1n ± 1%  +0.51% (p=0.036 n=8)
geomean                                             131.2n        131.6n       +0.25%

                             │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                             │               B/op               │    B/op     vs base                │
StripANSICodes_PlainText-4                         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
StripANSICodes_WithEscapes-4                       56.00 ± 0%     56.00 ± 0%       ~ (p=1.000 n=8) ¹
IsBanner_PlainText-4                               0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                       ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                             │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                             │            allocs/op             │ allocs/op   vs base                │
StripANSICodes_PlainText-4                         0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
StripANSICodes_WithEscapes-4                       4.000 ± 0%     4.000 ± 0%       ~ (p=1.000 n=8) ¹
IsBanner_PlainText-4                               0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                       ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

pkg: github.com/tstapler/stapler-squad/session/tokens
                                   │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt          │
                                   │              sec/op              │   sec/op     vs base              │
TokenParser_ProcessUserEntry-4                            5.259m ± 1%   5.383m ± 1%  +2.37% (p=0.007 n=8)
DetectCommandsInText/NoSlash-4                            7.495n ± 0%   7.493n ± 0%       ~ (p=0.630 n=8)
DetectCommandsInText/WithCommand-4                        1.652µ ± 0%   1.666µ ± 1%  +0.82% (p=0.005 n=8)
geomean                                                   4.023µ        4.065µ       +1.05%

                                   │ benchmarks/go/tier1-baseline.txt │           tier1-bench.txt            │
                                   │               B/op               │     B/op      vs base                │
TokenParser_ProcessUserEntry-4                         11.02Mi ± 0%     11.02Mi ± 0%       ~ (p=0.625 n=8)
DetectCommandsInText/NoSlash-4                           0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=8) ¹
DetectCommandsInText/WithCommand-4                       433.0 ± 0%       433.0 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                             ²                 +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                                   │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                                   │            allocs/op             │ allocs/op   vs base                │
TokenParser_ProcessUserEntry-4                           34.00 ± 0%     34.00 ± 0%       ~ (p=1.000 n=8) ¹
DetectCommandsInText/NoSlash-4                           0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
DetectCommandsInText/WithCommand-4                       6.000 ± 0%     6.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                             ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

pkg: github.com/tstapler/stapler-squad/session/unfinished
                               │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt          │
                               │              sec/op              │   sec/op     vs base              │
DiffShortstat/GitVCSReader-4                          3.204m ± 0%   3.222m ± 1%  +0.57% (p=0.002 n=8)
DiffShortstat/GoGitVCSReader-4                        76.80n ± 0%   76.66n ± 0%       ~ (p=0.130 n=8)
DiffShortstatCached-4                                 76.65n ± 1%   76.63n ± 1%       ~ (p=0.557 n=8)
geomean                                               2.662µ        2.665µ       +0.12%

                               │ benchmarks/go/tier1-baseline.txt │           tier1-bench.txt            │
                               │               B/op               │     B/op      vs base                │
DiffShortstat/GitVCSReader-4                       62.57Ki ± 0%     62.58Ki ± 0%       ~ (p=0.489 n=8)
DiffShortstat/GoGitVCSReader-4                       0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=8) ¹
DiffShortstatCached-4                                0.000 ± 0%       0.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                         ²                 +0.01%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

                               │ benchmarks/go/tier1-baseline.txt │          tier1-bench.txt           │
                               │            allocs/op             │ allocs/op   vs base                │
DiffShortstat/GitVCSReader-4                         360.0 ± 0%     360.0 ± 0%       ~ (p=1.000 n=8)
DiffShortstat/GoGitVCSReader-4                       0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
DiffShortstatCached-4                                0.000 ± 0%     0.000 ± 0%       ~ (p=1.000 n=8) ¹
geomean                                                         ²               +0.00%               ²
¹ all samples are equal
² summaries must be >0 to compute geomean

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

E2E RPC Latency

list-sessions-ttfb-mean: 7ms (▲ slower +21.6%; baseline: 6ms)
list-sessions-total-mean: 10ms (▼ faster -16.4%; baseline: 12ms)

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

📊 Feature E2E Coverage

Feature coverage report unavailable

Run make e2e-report locally to view the full Allure report.

… 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
@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

✅ Registry Validation

Registry Validation
===================

Building backend scanner...
Scanning backend features...
Wrote 117 feature files to /tmp/tmp.N8eW2guu5I/backend
Wrote 15 feature files to /tmp/tmp.N8eW2guu5I/backend
Wrote 46 feature files to /tmp/tmp.N8eW2guu5I/backend
Wrote 8 feature files to /tmp/tmp.N8eW2guu5I/backend
Wrote 12 feature files to /tmp/tmp.N8eW2guu5I/backend
Wrote 6 feature files to /tmp/tmp.N8eW2guu5I/backend

=== Backend Registry Diff ===
Committed: 184  Generated: 184  Divergence: 0.0%
⚠️  109 feature(s) missing // +api: marker (markerFound: false)

✅ Registry validation passed. Divergence: 0.0%

Test Coverage: 31/184 features have testIds (16.8%)

Divergence > 2% blocks merges. Coverage reporting is advisory only.

@github-actions

github-actions Bot commented Aug 5, 2026

Copy link
Copy Markdown
Contributor

Frontend Terminal Throughput

terminal-throughput-mean: 16 KB/s ▲ +0.8% (baseline: 16 KB/s)
terminal-throughput-p50: 16 KB/s ▲ +3.4% (baseline: 15 KB/s)

@tstapler
tstapler marked this pull request as ready for review August 5, 2026 17:43
Copilot AI lite review requested due to automatic review settings August 5, 2026 17:43

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

Fixes backlog items getting stuck in review when a reviewer submits a verdict but never exits, by adding an eager FAIL/PARTIAL/UNVERIFIABLE reopen path and an idle-timeout sweep fallback, plus updating reviewer prompts and wiring AutoReopenSpawner through MCP server construction.

Changes:

  • Eagerly trigger review → in_progress on reject verdicts via AutoReopenAfterFailedReview, with nil-safe wiring through MCP server constructors.
  • Add idle-timeout handling in reconcileUnprocessedReviewVerdicts so old verdicts are actionable even if the session appears alive.
  • Update review prompts/templates and add/extend unit tests covering wedge recovery and guidance text.

Reviewed changes

Copilot reviewed 15 out of 16 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
session/pipeline_mode_seed.go Updates review template to instruct exiting immediately after submitting verdict.
session/backlog_review.go Adds explicit “end session after verdict” instruction to the generated review prompt.
session/backlog_review_test.go Adds regression test asserting review prompt includes the new instruction.
session/backlog_lifecycle.go Adds idle-timeout OR-path to treat old verdicts as actionable even if session is “alive”; defines threshold constant.
session/backlog_lifecycle_test.go Adds helper to create backdated verdicts for idle-threshold tests.
session/backlog_lifecycle_stuck_test.go Expands reconcile tests to cover idle-timeout behavior (including PASS) while session reports alive.
server/mcp/tools_backlog.go Implements eager auto-reopen call on reject outcomes and updates review-role guidance text.
server/mcp/tools_backlog_test.go Adds tests for eager auto-reopen behavior and review-role guidance text.
server/mcp/server.go Threads optional AutoReopenSpawner through NewCore/NewHTTPHandler/RunServer and into handlers.
server/server.go Wires BacklogService into MCP handler safely (avoids nil-interface trap).
server/mcp/server_integration_test.go Updates NewCore invocation for new parameter.
server/mcp/feature_flag_test.go Updates NewCore invocation for new parameter.
main.go Passes nil autoReopener on stdio MCP path with documentation.
server/services/backlog_service_triage_test.go Adds CAS/double-call and “spawn new session” coverage around AutoReopenAfterFailedReview behavior.
docs/bugs/fixed/BUG-051-session-tmux-package-flaky-under-parallel-quick-check.md Updates BUG-051 status and adds recurrence/resolution notes.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

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()
sb.WriteString("1. Check each AC criterion against the implementation\n")
sb.WriteString("2. Call submit_review_verdict with per-criterion verdicts (PASS/FAIL/PARTIAL) + evidence\n")
sb.WriteString(" PASS → item transitions to done. FAIL → item sent back for rework.\n")
sb.WriteString("3. End your session immediately after calling submit_review_verdict. Do not wait, poll, or do further work — an idle-but-alive reviewer session leaves the item stuck.\n")
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.
# BUG-051: `session/tmux` Package Tests Flake Under `make quick-check`'s Parallel Load [SEVERITY: Low]

**Status**: 🐛 Open
**Status**: ✅ Fixed (main@dccee742a, 2026-08-04)

## Recurrence log

- 2026-08-05, while running `make ci`/`make test` as the AC-7 gate for `stapler-squad-fix-idle-reviewer-wedge`: `TestEnsureServerRunning_NoOp` failed identically (`tmux start-server failed: exit status 1 (output: server exited unexpectedly)`) under the full-suite parallel run, passed 5/5 in isolation (`go test ./session/tmux -run TestEnsureServerRunning_NoOp -count=5`), and — to positively rule out this session's own diff — reproduced identically after `git stash`-ing every change back to the unmodified base branch and re-running `make test`. Confirmed still open and still purely load-triggered, not tied to any particular diff, on `main` as of that point.
@tstapler

tstapler commented Aug 5, 2026

Copy link
Copy Markdown
Owner Author

Closing as superseded: this branch's last known commit (1a75172) is already present on main, so this item's work has already shipped through another path. No further fix is needed here.

@tstapler tstapler closed this Aug 5, 2026
tstapler added a commit that referenced this pull request Aug 5, 2026
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 added a commit that referenced this pull request Aug 5, 2026
…session's own base commit (#346)

* fix(backlog): stop auto-closing live PRs as "superseded" against the 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

* fix(backlog): correct three base-vs-latest consumers the field split 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

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
tstapler added a commit that referenced this pull request Aug 5, 2026
tstapler added a commit that referenced this pull request Aug 5, 2026
…iewer (recovers #342) (#347)

* fix(backlog): stop auto-closing live PRs as "superseded" against the 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

* chore(fmt): fix pre-existing gofmt drift in session/git/worktree_ops.go

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

* fix(backlog): recover items wedged in review by an idle-but-alive reviewer

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(backlog): address code-review findings on the idle-reviewer-wedge 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

* fix(backlog): correct three base-vs-latest consumers the field split 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

---------

Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
tstapler added a commit that referenced this pull request Aug 6, 2026
…tale LastCommitSha

d6ddbef3 shows done/PR-closed-as-superseded, but the "superseded by" commit
predates the branch's own fix commits by almost a day. LastCommitSha is only
ever set once, at session-start, as the pre-work baseline SHA for review-gate
diffing (SpawnSessionFromItem step 12b, AttachSessionToItem) and never
refreshed — so IsCommitOnMain trivially matches for any item, real work or
not. BUG-047's actual fix (PR #342) never merged; the two live wedged reviews
this doc's prior update said would self-heal will not. Also affects
GetBacklogItemShipStatus (same field, same trust boundary).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants