[Juliusolsson05]

Fixes #863.

The bug

scrubGridRowMetadata ran only at the persistence boundary (sessionOwnership.ts), so it cleaned the copy autosave writes — not the state the app runs on. Memory and disk disagreed for the rest of the session, and a Grid Dispatch row could stay bound to a project that no longer existed:

1. the row's index filters to a project with no agents, so it is blank;
2. New Agent from one of its empty lanes resolves the dead tab, hits if (!tab) return null in createDetachedDispatchAgent, and fails with no feedback — the placement overlay stays open, because only a successful spawn closes it, and the user has to guess that Escape is the way out.

The fix

workspaceWithoutSessions is the one place a project leaves live state, so it is the one place that has to say so.

The scrub runs on every commit, not only when a project left

My first version guarded it on emptied.size > 0. A mutation caught that this silently kept the helper's second prune from ever running: expandedParents of sessions that just closed stayed expanded. There is nothing to pay for dropping the guard — scrubGridRowMetadata returns the same stage object when it changes nothing, so a close that touches no row still does not re-allocate the stage the user arranged (#681). That property is its own test.

The silent branch now speaks

The cause that made if (!tab) return null reachable is fixed above, so it should be unreachable. It stays because the next stale target must be visible rather than silent — being silent is what made this one hard to see at all.

The invariant, not three cases

pool.test.ts's header already claimed "no row ever names a project that does not exist" as its first invariant, and expectCoherent did not check it. It does now, so every existing and future case in that file carries the guarantee.

Mutation results

mutation	result
never scrub (the bug)	3 failed
scrub against the pre-close tab list	2 failed
scrub against the pre-close session list	1 failed
scrub before the lane clear instead of after	EQUIVALENT MUTANT — 0 failed. Corrected after review: the two helpers write disjoint stage fields (clearTiledLaneSessions → lanes, scrubGridRowMetadata → rows), so the order genuinely does not matter. The "8 failed" first published here was wrong, and it was the table's most impressive number.
always scrub (drop the guard)	became the code — it was the better answer

No survivors.

npx tsc -b clean on Node 24. --project unit --project renderer src/renderer/src/workspace/: 1076 passed.

🤖 Generated with Claude Code

---

Review round (reviewer requested changes; fixed in 3af2bc55)

A blocking regression was found: the scrub re-allocated every row whenever any row changed, and row object identity is load-bearing — the lane-selection race check in dispatch.ts compares rows across a wake, so an unrelated close during a cold wake (up to 30s) made a just-woken agent never get placed, silently. Each row now keeps its original object unless that row changed, pinned in laneSelectionWake.renderer.test.tsx with a control.

Also fixed: the legacy projectTabId is folded rather than deleted; the creation failure path closes the placement overlay instead of leaving it up and latched, with caller-appropriate copy; toEqual → toBe on the row-identity assertion; a bound base fixture so expectCoherent's loop is not iterating an empty list; and two WHY comments that claimed properties the code did not have.

Post-fix mutation results, whole renderer+unit workspace suites (1224 tests): re-allocate every row 2 failed; delete the legacy fold 1; delete the toast and close 2; keep the toast but not the close 2; one copy for both callers 1; never scrub 7; restore the emptied.size guard 1; drop the expandedParents prune 3.

Full reply: the review comment on this PR.

[Juliusolsson05]

Reviewer's report worked — finding 1 is a real regression I introduced, and it is the kind that never shows up in a test that was written by the same process that wrote the bug. Point by point.

1. [HIGH] Rows re-allocated wholesale → silent lane-selection drop. Fixed, and pinned.

Accepted in full, including the diagnosis: this was harmless while scrubGridRowMetadata only ran at the persistence boundary, and this PR is what put it on every close. 3af2bc55 returns the ORIGINAL row object for every row the pass did not change.

I took the regression test to where the race lives rather than to pool.test.ts — laneSelectionWake.renderer.test.tsx already owns the duringWake harness, so the new case commits a real workspaceWithoutSessions close of an expanded parent in another row while the wake is in flight, and asserts the woken agent still lands (written === [1]). It runs with a control row where the close scrubs nothing, because that case has always passed and it is what makes the first one mean something. Reverting the fix: 1 failed | 10 passed, the control green.

2. [HIGH — process] Five survivors, and M4 contradicting the body.

Correct on both counts, and the M4 row is the one that bothers me most: I reported "8 failed" for a mutation that is genuinely equivalent — the two helpers write disjoint stage fields — so the most impressive number in that table was measuring nothing. I have corrected the table in the PR body and marked M4 as an equivalent mutant rather than quietly dropping it.

Re-run after the fixes, whole renderer+unit workspace suites (1224 tests):

mutation	result
re-allocate every row (your blocker)	2 failed
delete the legacy fold	1 failed
delete the toast and the overlay close	2 failed
keep the toast, drop only the overlay close	2 failed
one copy for both callers	1 failed
never scrub (the original bug)	7 failed
restore the emptied.size guard	1 failed
drop the expandedParents prune	3 failed
delete the expectCoherent binding loop	passes — see 5

3. [MEDIUM] Comments claiming properties the code lacks.

(a) Fixed: toEqual → toBe(prev.stage.rows?.[0]). You are right that this is the assertion that would have caught finding 1, and it is now the one that fails when the fix is reverted.

(b) Fixed, with your sentence rather than mine: this is the one place a close removes a project; mergeProjectTabs re-points instead of scrubbing, rehydrate scrubs upstream, and a fourth removal site inherits nothing.

(c) Fixed, and I wrote down the Reload-All consequence you found rather than just softening the claim — a fully successful reload returns early, so live expandedParents keep pre-reload ids and row metadata has no equivalent of remapTiledLanes. Harmless today (an unknown id collapses a group a reload collapses anyway), noted as the same divergence one function away. Not fixed here: remapping row metadata is a different change than scrubbing it, and I would rather not smuggle it in.

4. [MEDIUM] The toast fixed half the symptom.

Accepted — the latched-overlay detail is what makes this a real finding rather than a nit: advice the user cannot act on is worse than silence. The failure path now calls closeNewAgentPlacement(), unconditionally, and the copy splits on projectOverride so an MCP agents.create or the New Agent In dialog is not told to try another lane.

It is now covered, which it was not: the new case in controlPlacement.renderer.test.tsx runs both callers. The lane-resolved half builds the authentic #863 state — an empty focused lane whose row is still bound to a closed project, since resolveDispatchSpawnTarget treats a row binding as outranking the active project. My first attempt set activeTabId to a ghost, and it passed for the wrong reason: the focused lane's occupant resolves first, so the create quietly succeeded under the lane's real project. That comment is in the test now.

I left "should be unreachable" out of the comment rather than claiming it, per your activeTabId === '' reading.

5. [LOW] expectCoherent vacuous.

Right, and the fix is the fixture, not the loop: the base workspace() now binds its rows (['a'], ['b','c']), so eight of nine call sites stop iterating an empty list. It also checks the legacy projectTabId, which it did not.

Deleting the loop still passes on correct code — as an invariant assertion should; it only fires when an invariant breaks. The measurement that shows it earns its place is the combined run: with the original bug reintroduced, pool.test.ts fails 7, and with the loop also deleted it fails 4. Three tests that say nothing about bindings on their own catch the bug through that loop.

6. [LOW/latent] Legacy projectTabId deleted, not folded.

Fixed — folded into projectTabIds the way normalizeRowProjects and mergeProjectTabs do, with a test. I agree it is dead in live state and I kept your reasoning for why it is dead in the comment, because "safe only because normalizeStage runs first, and nothing enforces that" is exactly the thing a third caller should not have to rediscover.

Not changed: M4's ordering (equivalent, now labelled as such), and your undoClose observation — a binding not restored by undo is unchanged by this PR and the post-fix state is strictly better, so it belongs in its own issue.

npx tsc -b clean; 171 files / 1224 tests pass.