Fix #883: close builder terminal tabs on cleanup by diffing overview, not state#892
Conversation
The previous diff source (client.getWorkspaceState) is rebuilt from SQLite terminal_sessions, which surviving shellper processes keep alive after afx cleanup — so the present-to-absent transition never fired. overviewCache.getData() comes from a readdirSync(.builders/) scan and collapses to worktree-on-disk existence, which is the authoritative "this builder still exists" signal. Extracts the diff into a vscode-free helper module so a vitest unit covers the cleanup transition end to end.
Architect ReviewLow-risk fix that addresses the actual root cause, not just the symptom. 654/34 across 8 files, but most is artifacts: 3 bookkeeping (plan + thread + status.yaml), 1 new test file (+116 LOC, 11 cases), 1 new pure helper module (+62 LOC). Real change to existing code is CMAP unanimous (gemini=APPROVE, codex=APPROVE, claude=APPROVE). Verified
Notable design choice — the lessons-learned entryThe builder added a Critical-tier entry to
This generalizes correctly. The same lesson applies anywhere extension-side code observes Tower state to react to cleanup events — there's likely more than one place this matters. Good capture for future work. Cross-cutting noteThis fix exposes how the 6 plausible causes I listed in the issue body were not equally weighted. The actual cause was a variant of cause #2 (Tower's state.builders not dropping cleaned-up rows), but the builder's fix is structurally better than what cause #2 suggested — instead of trying to fix the SQLite-backed source (which would couple to #783's outcome), they switched to the disk-scan source that's already correct. That's a stronger answer than the issue body anticipated; worth keeping in mind for future "multiple plausible causes" issue framings — sometimes the right fix is to pick a different data source entirely. VerdictApproved. Per PIR protocol, the Architect review |
1 participant
PIR Review: Close builder terminal tabs on cleanup (read overview, not state)
Fixes #883
Summary
The 3.0.6 fix for "builder terminal tabs close automatically on cleanup"
regressed because the present→absent diff was reading from the wrong
source. This PR points the diff at
overviewCache.getData().builders(disk scan via
discoverBuilders) instead ofclient.getWorkspaceState().builders(runtime registry rebuilt fromSQLite
terminal_sessions), so the diff sees the absence the momentafx cleanupremoves the worktree directory — even while thecompanion Tower-side bug (#783) leaves surviving shellper processes
pinning the SQLite-backed source open.
Files Changed
codev/plans/883-vscode-builder-cleanup-no-long.md(+216 / -0) — plan artifactcodev/state/pir-883_thread.md(+79 / -0) — protocol thread logpackages/vscode/src/prune-builder-terminals.ts(+62 / -0) — new pure helper modulepackages/vscode/src/extension.ts(+21 / -34) — diff wiring switched to overview cachepackages/vscode/src/__tests__/prune-builder-terminals.test.ts(+116 / -0) — 11 vitest casescodev/projects/883-vscode-builder-cleanup-no-long/status.yaml(+22 / -0) — porch stateCommits
e68197ac[PIR vscode: builder cleanup no longer closes the terminal tab — regression of 3.0.2 behavior #883] Plan draft5c21360b[PIR vscode: builder cleanup no longer closes the terminal tab — regression of 3.0.2 behavior #883] Diff against overview.builders.roleId, not state.builders.id724c4ea9[PIR vscode: builder cleanup no longer closes the terminal tab — regression of 3.0.2 behavior #883] Thread: implement phase completeTest Results
pnpm --filter codev-vscode check-types: ✓ passpnpm --filter codev-vscode lint: ✓ passpnpm --filter codev-vscode test:unit(vitest): ✓ pass (49 tests, 11 new)pnpm --filter codev-vscode test(mocha integration): ✓ pass (83 tests)porch done 883checks: ✓ build (5.3 s), ✓ tests (20.5 s)dev-approvalgate: human approvedArchitecture Updates
No
arch.mdchanges — the diff swaps one data source for another at thesame architectural boundary (VSCode extension reading from Tower).
There's no new module, pattern, or boundary to document. The new
prune-builder-terminals.tshelper is a vscode-free sidecar so a unittest can import it without mocks; that's a testability detail, not an
architectural one.
Lessons Learned Updates
Added one entry to
codev/resources/lessons-learned.mdunder Critical:This is a generalizable rule: any cleanup-detection diff in the
extension should default to the disk-scan source, and pick the
registry source only when worktree-existence isn't the right signal.
Things to Look At During PR Review
The helper went into its own module (
prune-builder-terminals.ts)rather than living "adjacent to the wiring" in
extension.tsas theplan wording suggested. Reason: vitest can't import a file that imports
the real
vscodeAPI, so the test needs the helper to be in a modulethat's free of vscode imports. Functional shape is identical to the
plan's intent.
Soft-mode limitation (
roleId: null). Soft-mode builders(
task-*,worktree-*worktrees) won't auto-close their tabs viathis path because the diff is keyed on
OverviewBuilder.roleIdwhichis
nullfor them. The issue's repro path is bugfix-mode (strict),and the older state-based code wasn't reliably helping soft-mode
either in the orphan-shellper scenario. Documented inline in the
helper's doc comment.
The companion Tower bug (afx cleanup can't reach orphaned Tower terminals after porch done self-completion (registry-keyed cleanup, no fallback) #783) is unfixed by design. This PR
explicitly does not touch the orphan-shellper / ghost
terminal_sessionsaccumulation — that's afx cleanup can't reach orphaned Tower terminals after porch done self-completion (registry-keyed cleanup, no fallback) #783's scope, and the issuemarks it Out of scope for vscode: builder cleanup no longer closes the terminal tab — regression of 3.0.2 behavior #883. After afx cleanup can't reach orphaned Tower terminals after porch done self-completion (registry-keyed cleanup, no fallback) #783 lands, the VSCode
resilience here keeps working (it's still reading the right source)
but stops being load-bearing.
Sync vs the previous async function. The old
pruneClosedBuilderTerminalswasasyncand guarded bypruneInFlightbecause it did an HTTP fetch on every tick; the new version is
synchronous (
overviewCache.getData()is an in-memory read) and theguard is gone. The call site in
overviewCache.onDidChange(...)didn't
awaitthe old version either, so this is purely simplification.How to Test Locally
For reviewers pulling the branch:
afx dev pir-883pnpm build && pnpm -w run local-install(restarts Tower, picks up the patched extension)afx spawn <issue> --protocol bugfix),open its terminal in VSCode, then
afx cleanup -p <id>from aseparate shell. Tab disappears within ~5 s of the cleanup printing
Builder ... cleaned up!.second variant.
before cleanup, then confirm its
(dev)tab also disappears.(no map-state pollution from the closure).
sqlite3 ~/.agent-farm/global.db "SELECT role_id FROM terminal_sessions WHERE type='builder'"will likely still listcleaned-up builders (afx cleanup can't reach orphaned Tower terminals after porch done self-completion (registry-keyed cleanup, no fallback) #783 left unfixed by design). The VSCode tab
still closes — that's the point.
Related
area/tower,bug) — Tower-side root cause:afx cleanupcan't reach orphaned Tower terminals afterporch doneself-completion. The VSCode change here is resilient to that bug;
fixing afx cleanup can't reach orphaned Tower terminals after porch done self-completion (registry-keyed cleanup, no fallback) #783 makes
/api/stateclean again but doesn't invalidatethis change.