Deleting a file does not remove it from the review queue#171
Merged
tstapler merged 14 commits intoJul 18, 2026
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.
…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.
…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
Documents the isCodeShippedToMain fix (PrURL alone no longer proves shipped; all 3 done-transition call sites now gated) and the resulting audit of all 11 current status=done items: 4 confirmed MERGED via gh, 7 confirmed on main via git ancestry, zero improperly closed. Also re-notes the live DB path trap (workspaces/d685c4b1a423cca3/sessions.db, not the top-level ~/.stapler-squad/sessions.db) that the existing "Skill Fix Needed" entry already flagged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
The Version Control section in BacklogItemDetail only ever showed live VCSStatus, fetched via a live in-memory session Instance (findInstanceFast). Once a done item's work session is stopped and its worktree cleaned up — the normal end state — there's no Instance left to query, so the widget silently rendered nothing. Not a bug exactly (there's no live "now" left to show for a deleted directory), but a real gap: no way to see how an item shipped after the fact. Adds GetBacklogItemShipStatus, a new read-only RPC answering "did this actually ship" from durable evidence instead: repo_path + the most recent work session's commit. Reuses this session's IsCommitOnMain (shipped locally or via a merged PR) and adds git.BranchAheadBehind (go-git, no subshell) to show the branch's position relative to main when it still exists, or "(deleted — already merged)" once it's been cleaned up post-ship. Frontend: useBacklogItemShipStatus + ShipStatusDisplay render as the fallback exactly when the live vcsStatus comes back empty, so active sessions keep seeing live git status and done items get the historical "shipped via PR #N" / "shipped directly to main" badge instead. Also fixes the backend feature-registry scanner: GetBacklogItemShipStatus wasn't in proto_scanner.go's methodToID table, so it fell back to the raw method name as its feature ID and never matched its own +api: marker. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
… view
Two real gaps in "what happened to this item's code," both closed with
durable-git-history evidence rather than a live session:
1. GetBacklogItemDiff fell back to diffing whatever's currently checked out
in item.RepoPath once a work session's worktree directory was gone (the
normal state for a done item) — silently wrong or empty, not what the
branch actually shipped. GetGitDiffRef already existed for exactly this
("worktrees share the same object store, so any ref reachable from any
worktree resolves correctly regardless of dir") but wasn't being used
here. Now always diffs from item.RepoPath with an explicit headRef, so it
works identically whether the worktree still exists or not.
2. Tyler: "are we able to identify which commits were shipped to main from
the branch so I can see that diff, kind of like we link to the PR" — adds
git.ListShippedCommits (base..head ancestry walk, same pattern as
BranchAheadBehind) and wires it into GetBacklogItemShipStatus as
ShipStatusDisplay's commit list (newest first, like a PR's Commits tab),
plus a "View Diff" button that opens the same ReviewChangesModal already
used by the review flow — now unconditionally, not gated to review status,
since the diff fix above makes it work for any item regardless of status.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01XXewUpVzfJEKJoKMg99JNN
…es clean Deleting the only uncommitted file in a worktree left a stale entry in the Unfinished Work scanner's resultStore since nothing ever cleared it once the worktree scanned clean again. Mirrors the existing CleanWorktree removal pattern from review_queue_poller.go/ review_queue_determiner.go instead of inventing a new mechanism. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NC245UX8ukDqzudPNZcQtu
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NC245UX8ukDqzudPNZcQtu
Contributor
✅ Registry ValidationTest Coverage: 11/173 features have
|
Contributor
UX Analysis
|
Contributor
Go Benchmarks (Tier 1) |
Contributor
📊 Feature E2E CoverageFeature coverage report unavailable
|
Contributor
Frontend Terminal Throughput |
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. |
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: 11/173 features have
|
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
Fixes a bug where deleting a file that was the sole uncommitted change in a worktree left a stale entry in the Unfinished Work review queue instead of clearing it.
Changes
scanRepo, mirroring the existingCleanWorktreeremoval pattern used by the review queue poller/determinerremoveStaleResultto delete the stored result and publishEventUnfinishedWorkRemovedso subscribers drop the entry immediately rather than waiting for it to age outTestScanRepo_RemovesStaleResultWhenWorktreeBecomesCleancovering the dirty→clean transition and the removal event.backlog-context.md,.claude/commands/backlog/*) to the current item IDTest plan
go test ./session/unfinished/...covering the newTestScanRepo_RemovesStaleResultWhenWorktreeBecomesCleantest