fix(backlog): escalate bouncing items to the rework-cap terminal state#170
Open
tstapler wants to merge 12 commits into
Open
fix(backlog): escalate bouncing items to the rework-cap terminal state#170tstapler wants to merge 12 commits into
tstapler wants to merge 12 commits into
Conversation
…us-desync bug Second, compounding root cause for "nothing merges": GitHub repo-level allow_auto_merge is disabled, so EnablePRAutoMerge's gh pr merge --auto call has been silently failing on every PR. Also notes the pr_pending status-desync bug blocking ReconcilePRPending for PR #157, and a fabricated subagent report incident from this audit pass.
…icy scope Confirmed via direct code read: GetPRStatus never distinguishes a still-running check from a passed one, so ReconcilePRPending's healthy-PR branch can fire the ready-to-merge notification before CI finishes. Also records the backlog-pr-mergeability-policy SDD scoping outcome and an open design tension between repo-level auto-merge (enabled this session) and the plan's per-item-gated auto-merge design.
…ng don't self-heal Systematic pass over all 7 StuckReasons: only pr_pending auto-recovers. orphaned_triage's code comment falsely claims self-healing it doesn't actually do. bouncing has no escalation path. Also confirms the MCP controller-startup issue from the earlier audit doesn't affect backlog-spawned sessions, which are fully wired synchronously.
reconcileBouncingItems detected items non-convergently bouncing between in_progress and review (>= bounceThreshold cycles, no PASS verdict) but only sent a notification -- it has no way to stop the auto-reopen loop, so a bouncing item just kept bouncing forever with a human expected to notice. The existing rework_cap detector (AutoReopenAfterFailedReview's workCount check) can miss the same non-convergence: workCount only counts actually -created work sessions, but the spawn-failure rollback path records an extra in_progress->review status event without a new work session, so the cycle count can outpace the work-session count. Apply the same isBouncing test AutoReopenAfterFailedReview's respawn choke point uses and, when it trips, escalate through the existing notifyReworkCapHit terminal treatment -- the same StuckReasonReworkCap row and notification shape rework_cap already uses -- rather than respawning again. reconcileBouncingItems now also checks for an already-open rework_cap row before notifying, so the two detectors don't fire two differently -shaped "needs manual attention" signals for the same underlying condition. Regression tests: TestAutoReopenAfterFailedReview_should_escalateToReworkCap_When_BouncingBelowWorkCountCap, TestAutoReopenAfterFailedReview_should_reopenNormally_When_BelowBounceThreshold, TestReconcileBouncingItems_should_skipDuplicateNotify_When_AlreadyEscalatedToReworkCap Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…stuck rows Live-verified via the deployed reconciliation sweep: the abandoned-review auto-respawn correctly fires exactly once for newly-notified rows, but rows already marked notified before the fix deployed never trigger a respawn since it shares the same notify-once gate. Correct going forward, just doesn't retroactively catch pre-existing stuck items.
Contributor
✅ Registry ValidationTest Coverage: 10/172 features have
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
E2E RPC Latency |
Contributor
🎬 E2E Feature Demos2 shard(s) recorded feature flows for this PR. recordings shard 1 Demo preview opens directly in browser (single-file HTML). Raw WebM recordings in ZIP. Expires after 30 days. |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
Frontend Terminal Throughput |
…session/tmux session/integration_test.go's TestMain reaper only recognized "test_coldrestore_" socket names, silently missing "test-isolated-" (the name testSocketOnce in session/tmux generates and every package's tests share). session/mux had no reaper at all. A SIGKILLed test binary in either package left its tmux server running indefinitely — confirmed live in production as 4 multi-day-old orphaned "tmux: server" processes eating memory and holding stale worktree paths. Consolidate into testutil/tmuxreap, a dependency-free leaf package (unlike testutil itself, which already imports session/session-mux/session-tmux and so can't be imported by their internal-package test files without a cycle). All three packages' TestMain now call the same ReapLeakedTestServers / StartTestServerWatchdog, covering the full set of test socket prefixes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
Ad-hoc merges into ~40 backlog/agent worktrees (done manually this session to land the tmux-reaper fix) showed the gap plainly: worktrees silently drift behind main until someone remembers to merge by hand, so fixes on main (test hygiene, lint rules, bugfixes) don't reach in-progress or idle backlog worktrees on their own. `make sync-worktrees` merges main into every worktree, skips any with uncommitted changes, and aborts + reports (without forcing a resolution) any real conflict for manual follow-up. Safe to re-run — a no-op once everything's current. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
…ext file Considered a Go-side periodic reconciler (session/git.MergeMainIntoWorktree already exists and is used by AutoReopenForPRFix's syncPRBranchWithMain) but that only helps a narrow case — SpawnSessionFromItem always branches a fresh worktree off current main on every real reopen, so external auto-merge would mostly be a no-op and would need its own "is this worktree actively in use" safety check to avoid yanking the tree out from under a running agent. Simpler and safer: WriteBacklogContextFile already regenerates .backlog-context.md on every spawn AND every re-attach (see AttachSessionToItem in backlog_service_sync.go) — the harness's existing state-management touchpoint the agent reads as its briefing. Telling the agent to merge main as its own first step is inherently safe (it's the one about to use the worktree) and it can actually resolve a trivial conflict, where a background script could only abort and flag it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
…re done An item could reach "done" once PrURL existed at all — an open, unmerged, or later-reverted PR still has PrURL set, so it always satisfied the old guard. "Approval" (a PASS review verdict) and "shipped" (code actually on main) were being conflated: a PASS verdict says the code is good, not that it landed. Three separate call sites could drive a review→done transition, and only one (the TransitionBacklogItemStatus RPC) had any shipped-code guard at all — TriggerReReview's and SubmitManualReview's auto-transition-on-PASS bypassed it entirely by calling storage.TransitionBacklogItemStatus directly. All three now route through the same isCodeShippedToMain check. "Shipped" means the most recent work session's commit is an ancestor of main — checked locally AND via origin (a PR merged remotely on GitHub doesn't require a local pull to count; a commit merged directly to main locally never went through a PR at all). Implemented as git.IsCommitOnMain using go-git (no subshell — see the new prefer-go-git-over-subshells rule), with an origin fetch that's best-effort so an offline/unreachable remote doesn't block the local-merge case. Verification failure fails closed: if the commit can't be confirmed shipped (or the check errors), the transition is blocked rather than silently trusting a stale PrURL. The RPC path keeps its override_reason escape hatch; the two internal auto-transition paths have none by design — they leave the item in review for a human to decide via the RPC path instead. ErrPRRequired renamed to ErrCodeNotOnMain to match what it actually checks now; no external callers referenced the old name (verified via repo-wide grep, zero test references either). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
Contributor
✅ Registry ValidationTest Coverage: 10/172 features have
|
These match .gitignore rules already but were tracked from before the rule was added, so gitignore had no effect and automation kept committing per-worktree state to them. git rm --cached only; local copies are untouched. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01JqpFYpiUbPMTnZrNW4grg7
Contributor
✅ Registry ValidationTest Coverage: 10/172 features have
|
6 tasks
tstapler
added a commit
that referenced
this pull request
Jul 20, 2026
…kstop (#195) .backlog-context.md and .claude/commands/backlog/*.md are written into every backlog work-session worktree, then git-excluded via $GIT_DIR/info/exclude. That only stops NEW files from being staged — it does nothing once a file is already tracked, which is why this repo's own history has ~20 manual "chore(backlog): untrack backlog context/command files" commits, and why two currently-open PRs (#169, #170) both carry an incidental deletion of a stale, committed .backlog-context.md from an unrelated item's context. WriteSlashCommands and WriteBacklogContextFile now call selfHealWorktreeScaffolding before writing: it walks the git index (via go-git, not a subshell) for any entry matching backlogExcludePatterns and untracks it (git-rm-cached semantics — working tree file is left alone), so a branch that ever got one of these files committed self-heals on the very next spawn/reattach/reopen instead of requiring a manual untrack commit. Also adds a CI backstop (.github/workflows/backlog-scaffolding-guard.yml) that fails any PR whose diff adds/modifies a backlogExcludePatterns file, as a second, independent layer for this repo. This only protects stapler-squad's own repo — it cannot reach into the arbitrary target repos backlog items point at, which is why the self-heal above (not CI) is the primary fix. On the "scoped by session/backlog item so it's not reused poorly" half of the request: chose to harden the existing worktree-relative storage rather than relocate it to a session/item-scoped path outside the worktree (~/.stapler-squad/backlog-context/<item>/...). Both WriteBacklogContextFile and WriteSlashCommands already do a full atomic overwrite on every spawn/reattach/reopen, so no session ever reads content from a different item/session — the actual "reused poorly" risk was exclusively the already-tracked git pollution this fix closes. Relocating storage is a larger, real architecture change (touches taskProtocolBlock's literal `.backlog-context.md` references, needs a session UUID that doesn't exist yet at the pre-spawn write callsite, and increases collision surface with other agents' concurrent work in these same files) and is left as a follow-up rather than folded into this bug fix. CleanupSlashCommands/CleanupBacklogContextFile remain uncalled from any production teardown path, and that's intentional, not an oversight: shipViaAgentOrFallback relies on ship.md still existing after a work session exits review to re-invoke /backlog/ship as a one-shot call. Wiring cleanup into a review-exit teardown would delete ship.md out from under that path. Documented this directly in both functions' doc comments so it isn't "fixed" again by mistake. Regression tests: a worktree with .backlog-context.md (or .claude/commands/backlog/status.md) already committed gets it untracked on the next Write call with fresh on-disk content; respawning the same worktree path for a different item never leaks the prior item's content; the self-heal helper no-ops cleanly on a non-git directory and leaves unrelated tracked files alone. Claude-Session: https://claude.ai/code/session_01BxNAMeGteuzNyN46Q4zAn1 Co-authored-by: Claude Sonnet 5 <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
1 participant
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.
Summary
reconcileBouncingItemsdetected non-converging bouncing (item thrashesin_progress<->reviewwith no PASS verdict) but only sent a notification — nothing stopped the auto-reopen loop, so a bouncing item just kept bouncing forever with a human expected to notice.AutoReopenAfterFailedReview(the actual respawn choke point, invoked synchronously the moment any non-PASS review verdict lands) now applies the identicalisBouncingtest the periodic detector uses, and — when it trips — escalates through the existingnotifyReworkCapHitterminal treatment (sameStuckReasonReworkCaprow, same notification shape) instead of respawning again.workCount(the existing rework-cap counter) only counts actually-created work sessions, but the spawn-failure rollback path records an extrain_progress->reviewstatus event with no new work session, so the cycle count can outpace the work-session count and evade the workCount cap entirely.reconcileBouncingItemsnow checks for an already-openrework_caprow before notifying, so the two detectors don't fire two differently-shaped "needs manual attention" signals for the same underlying non-convergence.Context
Scoped from the 2026-07-17 evening "systematic autonomy-gap audit" in
docs/tasks/backlog-feature-improvement.md, which flaggedbouncingas a real gap: "no escalation/different-approach retry once non-converging, just a notification." Live example in that audit: an item bouncedin_progress<->review4 times across 3 sessions with no PASS verdict, dead with no self-service retry affordance.Changes
session/stuck_decisions.go: exportedBounceThreshold,BounceLookback,IsBouncing(thin aliases/wrappers around the existing unexported values) soserver/servicescan reuse the identical bounce test.session/storage.go: addedStorage.CountReviewCyclesSincewrapper (mirrors the existingGetMostRecentReviewVerdictForItemwrapper pattern) so the services package can query cycle counts without reaching into the unexportedEntRepository.server/services/backlog_service_triage.go:AutoReopenAfterFailedReviewnow runs the bounce check after the existingworkCount >= maxAutoReworkIterationscheck and, if it trips, callsnotifyReworkCapHitand returns without respawning.session/backlog_lifecycle.go:reconcileBouncingItemsnow skips its own notification (but still writes/marks the bouncing row for UI visibility) when an openrework_caprow already exists for the item.TestAutoReopenAfterFailedReview_should_escalateToReworkCap_When_BouncingBelowWorkCountCap,TestAutoReopenAfterFailedReview_should_reopenNormally_When_BelowBounceThreshold,TestReconcileBouncingItems_should_skipDuplicateNotify_When_AlreadyEscalatedToReworkCap.Impact
sessionandserver/servicespackages only (backlog reconciliation loop). No proto/schema/UI changes.CountReviewCyclesSince) per non-PASS review verdict, same queryreconcileBouncingItemsalready runs periodically.Testing
go build ./session/... ./server/services/...— cleango vet ./session/... ./server/services/...— cleangolangci-lint run ./session/... ./server/services/...— 0 issuesgo test ./session/...— full suite greengo test ./server/services/...— full suite greenReviewer Notes
reconcileBouncingItems(periodic) andAutoReopenAfterFailedReview(synchronous) — the periodic sweep is the backstop for reopen paths that bypass the synchronous check (e.g. a manual reopen).🤖 Generated with Claude Code