fix(desktop): preserve session views and defer hidden terminal rendering

[Astro-Han]

Summary

Switching tasks currently clears the transcript and rebuilds workbar views. I keep the previous transcript visible until the requested task is ready, then publish its identity, messages and range together. Navigation revokes the old reader immediately, and async actions cannot override a newer destination. This also preserves main's deferred transcript publication while the user is reading.

Host Sessions own manual terminal processes. Navigation, collapse and renderer disposal do not Stop them. Main's existing Host target lifetime owns pending or uncertain explicit Close requests across view and connection replacement. A successful Stop closes the exact terminal in the current view; failures retain a retry surface. Natural exit releases live controls and subscriptions while allowing the local picture to remain. Terminal contents are not guaranteed to survive disconnect, reload or late-output loss; this change adds no output persistence or recovery protocol.

Hidden xterm views keep parsing output but defer presentation: the public screen-reader mode follows visibility, and xterm 6's selection handler honors its existing paused-render contract. Git review keeps stable file keys and skips unchanged diff rendering; refresh notifications share one debounce. Side Chat's hidden-fork state follows creation and successful cleanup, so an older catalog cannot expose an internal task.

Validation

At 5c37e98f8, after merging main and installing the locked dependencies, full Desktop build/typecheck, 231 related Node tests and three real Electron journeys pass. The journeys cover native scrollbar geometry, real PTY navigation/reload/explicit Stop/natural exit and Side Chat collapse/close/source-switch cleanup. The UI scroll suites passed 37 tests. The architecture ratchet passes against both current main and original base 531375908; changed-file lint and the E2E budget pass. CI has not been awaited.

The same head's real Electron pressure experiment alternated the same output workload between visible and hidden states. Hidden renderer TaskDuration was 272–286 ms versus 1,481–1,589 ms visible; terminal DOM mutations were 41 versus 10,154–10,472. The same xterm instance survived all samples. These are visibility comparisons, not a before/after PR speedup; the samples did not force GC and do not establish memory-leak bounds.

Earlier before/after experiments observed blank intermediate message frames fall from 24/24 switches to 0/24, without an improvement in first-target-DOM latency. Those measurements predate the latest lifecycle changes and main integration. The xterm selection probe covers both shipped CJS/ESM bundles and detects removal of the pause fix.

AI use

• No generative tool made a substantive contribution
• Generative tooling made a substantive contribution

OpenAI Codex implemented the change, investigated performance and drafted this description. AI subagents reviewed scoped architecture and lifecycle paths. These are automated reviews, not independent human approval.

[jackwener]

Scoped review at de99d298bd84c99d7b3913ffaee801109e064d11, covering the session and renderer architecture surface. A reviewer under a different owner is taking the terminal and host lifecycle separately; the performance, accessibility and Git surface is currently unstaffed, which I note because it matters below. Disclosure: I reviewed the range-controller and publication machinery in depth on a different pull request, so this surface is not new to me — that is both why I could move quickly here and a source of bias worth stating.

The identity split is the right shape and it does close part of what the description lists as open. Splitting the catalog's requested selection from the displayed identity, and then making the displayed identity and its rows a single state value committed together, removes the class of inconsistency where a session's identifier could be rendered beside another session's rows. That is structural rather than defensive: the two cannot disagree because nothing can set one without the other. Holding the previous conversation visible until the next transcript is ready, rather than blanking it, is what makes the preserved view possible at all.

The failure path is handled, which I checked specifically because the design depends on it. If the displayed identity only advances when a transcript commits, a read that never commits would strand the view on the previous session. It does not: a read error for the requested session commits an empty transcript first, moving the displayed identity forward, and only then records the error. The interval where the two identities disagree is also marked inert and busy, so the stale view cannot be interacted with, and the loading state is derived from the displayed identity so the indicator is visible during exactly that interval.

One observation, non-blocking, about a guard that is weaker than its neighbour.

The range controller is assigned synchronously by the effect keyed on the requested session, while the displayed identity advances later, when the transcript commits. There is therefore a real interval in which the controller reference belongs to the requested session while the displayed identity and rows still belong to the previous one. Most reads of that reference already account for this: the transient projection compares the range's own session against the one it is projecting, the effect cleanup compares the controller's store against the one it created, and the refresh path re-checks both the displayed identity and that the controller reference is unchanged after its await.

retryMessages does not. It checks the displayed identity and then calls reload on whatever the reference currently holds, without confirming that the controller belongs to that session — so during the interval described above it would reload the requested session's controller on behalf of the displayed one. I could not reach it: the surface carrying the retry affordance is inert for precisely that interval. That makes this a latent inconsistency rather than a live defect, and I am grading it accordingly. It is worth closing anyway, because the correctness of this function now rests on an attribute added elsewhere in this change rather than on its own check, and the function directly above it already demonstrates the stronger pattern. Matching the refresh path's two-part check would make it self-sufficient.

On the retirement item listed as open. Within the workspace layer I did not find an inconsistency: clearing owned state keys its visible effect on the displayed identity and drops the retired session's pending entries regardless of which session is displayed, and both orderings — retiring the displayed session while another is requested, or the reverse — leave a coherent state. The workbar side of that item is not in my surface and I make no claim about it.

Coverage and limits. I covered the requested and displayed identity split, transcript publication and its commit guard, the read-error path, the range controller's ownership across a switch, the inert interval, and the workspace layer's retirement path. I read code only and ran nothing. I did not review the terminal or host lifecycle, and I did not review the performance claims — the description states that the browser measurements predate the rebase and therefore do not speak for this commit, so as things stand the performance argument is not independently verified on the reviewed code by anyone, and I am not covering that gap from this surface.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[jackwener]

[P2] The new xterm patch is not declared in the root LICENSE, and that is exactly what the failing check reports.

I am filing this in the review body rather than inline: the patch file's own diff is too large for GitHub to anchor a comment to, and the fix belongs in LICENSE, which this change does not touch at all.

scripts/source-legal-inventory.test.mjs enumerates every *.patch under patches/ and asserts that the root LICENSE names each one:

const patches = (await readdir(join(root, 'patches'))).filter((name) => name.endsWith('.patch'));
for (const patch of patches) assert.ok(license.includes(patch), patch);

This change adds patches/@xterm+xterm+6.0.0.patch with no corresponding LICENSE entry, so the assertion fails with the patch filename as its message. That accounts for the entire test failure on this commit:

Verify ASF source release mechanics
  ✖ root release documents describe the source candidate
    AssertionError: @xterm+xterm+6.0.0.patch

The other two checks on this commit pass, so this single omission is what makes the required check red.

Impact. A required check is failing, which blocks regardless of the change's merits. Runtime behaviour is unaffected — this is a source-release legal-inventory requirement, not a test of this change's logic. That is precisely why it is easy to miss in review and why the repository automates it.

Suggested fix. Name the patch in the root LICENSE alongside the other patched dependencies, with the attribution xterm.js ships under. A recent change in this repository that added dependency patches updated LICENSE in the same commit, so there is an in-tree pattern to follow. Note that the entry this gate reads is the LICENSE one; documenting the patch's rationale in patches/README.md, which this change does do, does not satisfy it.

Verification basis. Read from the failing job's log, then confirmed against the assertion's source at this commit. I did not run the check locally, and I did not verify what attribution text the repository expects for this dependency.

Scope note. This sits on the performance-and-packaging surface, which has no reviewer assigned in this round; I raise it because I found it while confirming the check state for my own surface, not because I covered that surface.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[jackwener]

Correction to my previous comment, and the current check failure in its place.

My previous comment reported that the new xterm patch was missing from the root LICENSE. That was true of de99d298b, the commit I actually reviewed. It is not true of the commit this platform bound that comment to: 3d96479f9 adds 34 lines to LICENSE that name patches/@xterm+xterm+6.0.0.patch with its upstream attribution. The finding was already fixed when it posted.

The mistake is mine and worth naming precisely, because it is the exact failure mode this review round was asked to avoid — an old conclusion presenting itself as current code. I verified the head immediately before posting my architecture review, then posted this second comment without re-checking, and the head had moved in between. Posting a review binds it to whatever the head is at that moment, not to the commit the reviewer read. Please treat the previous comment as withdrawn.

What is actually failing on 3d96479f9. The test check is still red, for a different reason, and it lands squarely on the files this change touches:

Renderer architecture check failed:
- src/renderer/app-shell-effects.ts: nonTriviaTokens debt increased from 3736 to 3762
- src/renderer/app-shell.tsx: nonTriviaTokens debt increased from 13531 to 13602
- src/renderer/use-app-shell-session-workspace.ts: nonTriviaTokens debt increased from 466 to 559

All 103 architecture tests pass; what fails is the ratchet comparing measured token counts against the base ledger. This change does update renderer-architecture.json, so the ledger was not forgotten — the recorded numbers simply do not match what the checker measures for these three files at this commit. The base the checker compares against has also moved today, which is a plausible reason regenerated numbers would no longer line up.

Impact. A required check is red, so this blocks. No runtime behaviour is implicated: the ratchet is a size-debt gate, and an increase in these three files is the expected shape of this change rather than a surprise — the question is only whether the ledger records it.

Suggested fix. Regenerate the ledger against the base the checker actually uses, then confirm the three entries match, rather than editing the numbers by hand.

Verification basis. Read from the failing job's log at this commit and compared against the change's own ledger edit. I did not run the checker locally, and I did not determine which base revision the regeneration used — that last point is the part I would want the author to confirm rather than assume.

Everything in my architecture review still stands. It was bound to de99d298b and the only change since then is the LICENSE addition, which touches nothing it discusses.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[ARE404]

Reviewed exact commit 3d96479f973a8d0be911257c4cbbdc5f8328bad6 against merge-base 5313759080ca34f4fff013c7488e47222a9c4b84, scoped to Terminal/renderer lifecycle and repository standards. Result: NO-GO: 1 P1 and 2 P2 findings.

The P1 corroborates the current architecture-check failure, with one important correction to the earlier suggested remediation: the edited ledger already contains the newly measured values. The check is rejecting those values because this AppShell family is a full ratchet whose debt may only decrease. Regenerating the ledger against another base would not make the growth valid; the added ownership/state logic needs to move out of the frozen files or the files must otherwise come back under their base budgets.

The two P2s are lifecycle defects: a rejected explicit Stop removes the user's only retry surface and can leave the Session retirement-blocked, while a successfully retired owner can leave completed Terminal UI resources mounted invisibly and accumulating. Inline comments contain the concrete sequences and boundaries.

The normal switch/collapse path otherwise keeps bounded queues: the renderer queue is capped at 32 entries / 256 KiB, hydration at 128 events / 256 KiB, and xterm scrollback at 5,000 lines. I did not find a separate unbounded hidden-output backlog. I also did not use a start-versus-delete race as evidence because Host session admission and the live-resource retirement check constrain that race.

Standards notes: features/workbar/README.md still documents remount-on-switch, stop-on-leave, and stopping late starts after navigation, all of which this PR intentionally removes; the accepted lifecycle should update that contract. The PR also lacks the before/after UI evidence requested by CONTRIBUTING.md. Commit titles and Generated-by: OpenAI Codex trailers pass.

Current hosted state on this exact commit is windows_recovery green and test red at the three AppShell-family architecture ratchets. The PR remains Draft. I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by ARE404. It is not an independent human review and does not replace one.

[jackwener]

Correcting my own remediation advice on the architecture ratchet. My suggested fix was wrong.

I previously suggested regenerating renderer-architecture.json against the base the checker uses, on the assumption that the recorded numbers no longer matched what it measures. I checked, and that assumption is false: this change's ledger already records the measured values — 3762, 13602 and 559 for the three files — which are exactly the numbers the checker reports. Regenerating would reproduce what is already there and change nothing.

The failure message says what is actually happening, and I read it too quickly the first time:

- src/renderer/app-shell-effects.ts: nonTriviaTokens debt increased from 3736 to 3762

3736 is the base value, not a stale ledger entry. The checker compares this change's measurement against the base revision's ledger and rejects growth; recording the larger number in this branch's ledger does not exempt it. So the gate is not asking for a regenerated file — it is refusing the increase itself.

What that means for the fix. Satisfying this gate requires the three files not to grow relative to base: either the ownership and state this change adds moves out of the frozen AppShell-family files into a module the ratchet does not freeze, or those files come back down to their base budget by some other means. That is a materially different and larger task than the one my earlier note implied, which is why I would rather correct it than leave it standing.

Credit where it is due: another reviewer on this pull request reached the correct reading independently and flagged that my remediation was wrong. I verified their correction against this change's own ledger before writing this, rather than taking it on trust.

What still stands. My architecture review of the session and renderer surface, bound to de99d298b, is unaffected — it discusses the identity split and the range-controller handoff window, none of which touches the ledger. My earlier withdrawal of the LICENSE finding also stands: that one was genuinely fixed in 3d96479f9.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[jackwener]

Re-review at 175e3bb7ddce25a5660ac9abb88d338bfe95872b, session and renderer surface. The architecture budget item is genuinely closed; a different check is now red, and it is the first real signal this branch has had from that layer.

The frozen-file budget is resolved, and resolved the right way. The ledger at this commit records 3715, 13515 and 412 for the three AppShell-family files, against base values of 3736, 13531 and 466. They are below base, not merely re-recorded — the growth was reversed by moving display and readability state into the conversation feature's own owner rather than by raising a budget. My earlier remediation note on this gate was wrong and I have already corrected it publicly; this is what closing it actually looked like.

Desktop e2e now fails, and two failures sit exactly on the semantics this round changed.

1) e2e/session-workbar.spec.ts:175 › Terminal ownership follows the active Session and stops the old resource
   Expected: not "running"        (timed out after 10s waiting on the predicate)

2) e2e/session-workbar.spec.ts:217 › Side Chat survives collapse, confirms close, and cleans up on source switch
   Expected: 0   Received: 1      (the session row is still present)

2 failed, 32 passed

The first says the previously owned resource is still reported as running after ownership moves to another session — that is, the old resource was not stopped. The second says a row expected to be gone after a source switch is still there. Both are about stopping and cleaning up on a switch, which is precisely what this round's second commit reworked: removing the tab only once stop succeeds, sharing a promise across concurrent stops, and cleaning terminal topology when the authoritative catalog removes an owner.

What I am careful not to claim. I checked whether this is a regression from that commit, and I cannot say that it is. Desktop e2e was skipped on both earlier heads of this branch, because an earlier step in the same job failed first — the source-inventory check on de99d298b, the architecture ratchet on 3d96479f9. This commit is the first time the job reached the end-to-end step at all:

commit	Desktop e2e
de99d298b	skipped
3d96479f9	skipped
175e3bb7d	failure

So these two failures are first observed here rather than demonstrably first introduced here; the branch has no green end-to-end baseline to compare against. They may predate this round. What makes them worth looking at first is the subject-matter overlap, not a proven causal link.

Why this matters beyond the two tests. The local verification reported for this round — the focused tests, the architecture fixtures and ratchet, the full typecheck, the source-release checks, lint and format — does not include Desktop end-to-end. That is not a criticism of the local run; it is the reason this signal could only appear here, and it is the first time this branch has produced one.

On the two items the description still lists as open, from my surface.

Range controller and display identity. The identity-and-rows half is genuinely atomic: they are one state value, so no render can pair one session's identity with another's rows. The range controller still changes hands on a different schedule, since it is assigned by the effect keyed on the requested session while the displayed identity advances only when a transcript commits. I re-checked every read of that reference at this commit: the transient projection compares the range's own session, the effect cleanup compares the controller's store, and the refresh path re-checks both the displayed identity and the controller reference after its await. retryMessages still checks only the displayed identity before calling reload on whatever the reference holds. I could not reach it — the surface carrying that affordance is inert for exactly the interval concerned — so I am not grading it as a defect. My verdict is that this item is closed for identity and rows, and open only as a latent inconsistency whose safety currently rests on an attribute rather than on its own check.

Requested and displayed retirement. In the workspace layer I again found no inconsistency at this commit: clearing owned state keys its visible effect on the displayed identity and drops the retired session's pending entries regardless of which session is displayed, and both orderings leave a coherent state. I am stating this as a verdict rather than deferring to the list, and it is bounded: it covers the workspace layer, not the workbar side, which belongs to another reviewer's surface.

Conclusion for this surface. No approval from me: a required check is failing on the reviewed commit. I am not recording a P0–P2 of my own on this surface at this commit — the budget item is closed and the two listed items resolve as described above — but the end-to-end failures are unattributed and sit in an adjacent area, and until they are understood I would not treat this surface's cleanliness as sufficient.

Coverage gap, stated as asked. The performance, accessibility and Git surface still has no reviewer under any owner. The browser measurements in the description predate the rebase, so the performance claim remains unverified by anyone against this code.

Verification limits. I read code and hosted results only, and ran nothing locally. I did not reproduce the end-to-end failures, did not determine whether they predate this round, and did not evaluate the terminal or host lifecycle, which is a separate reviewer's surface.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[ARE404]

Re-reviewed the repair delta from 3d96479f973a8d0be911257c4cbbdc5f8328bad6 through exact commit 175e3bb7ddce25a5660ac9abb88d338bfe95872b, scoped to Terminal/renderer lifecycle and the three findings from my prior review. Result: NO-GO: 2 P2 findings.

Two prior items are genuinely closed. The AppShell architecture-budget P1 is fixed structurally: the three frozen files are now below the fixed merge-base budgets (3715 < 3736, 13515 < 13531, and 412 < 466), and I reproduced the architecture check against 5313759080ca34f4fff013c7488e47222a9c4b84. The retired-owner P2 is also closed: retain-sessions removes Terminal tabs from raw topology, and the controller drops the local cleanup registration after authoritative owner removal; Host retirement still rejects while Runtime resources are live.

The visible Stop-retry P2 is fixed only while the controller remains mounted. The first inline comment describes the remaining teardown overlap. The second inline comment is a new repair regression in companion-fork visibility and explains one of the two exact-head Desktop E2E failures.

Hosted status at this exact commit is windows_recovery green and test red in two Desktop E2E cases. The Side Chat failure is attributable to the repair delta as described inline. The Terminal/New Task failure remains a required-check blocker, but I did not establish that this repair delta introduced it because earlier heads never reached Desktop E2E. Local git diff --check and the renderer architecture check pass. I did not review the unstaffed performance/accessibility/Git surface, and I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by ARE404. It is not an independent human review and does not replace one.

[ARE404]

Re-reviewed the repair delta from 175e3bb7ddce25a5660ac9abb88d338bfe95872b through exact commit 96fd5766f466347e804e9182ff4434ce31afd9ca, scoped to the two remaining lifecycle findings and the new Host-recovery/concurrency path. Result: NO-GO: 1 P2 finding.

Both earlier findings are now structurally closed. A rejected Stop after controller unmount is recoverable from Host with the same visible close control, and companion visibility is now driven only by fork creation and successful cleanup, so panel/title changes and stale catalog snapshots cannot expose the internal Session. The final generation change also preserves both same-owner and cross-owner recovery reads, and the recovery effect disposes its subscriptions and coalesces refreshes.

The remaining P2 is a success-side remount race: if a window reopens while the old Stop is still pending, it can restore the resource as live; when Stop then succeeds, neither controller removes that reconstructed tab, completed updates are filtered, and later live inventories cannot prune it because restore-terminals is additive only. The inline comment has the exact sequence and regression shape.

The renderer architecture checker passes against the fixed merge-base 5313759080ca34f4fff013c7488e47222a9c4b84, and git diff --check passes. At publication, exact-head windows_recovery is green and test is still in progress. The PR remains Draft. I did not approve, request changes, or merge.

---

Automated review notice: This comment was posted by an automated review agent operated by ARE404. It is not an independent human review and does not replace one.

[jackwener]

Surface 1 re-review at 96fd5766f466347e804e9182ff4434ce31afd9ca, against the cumulative diff rather than the increment. I did not carry my earlier conclusions forward on the grounds that recent commits did not touch these files — the display state moved to a different owner in this round, so every claim below was re-derived at this commit.

The duplicate state I was asked about is genuinely gone. The workspace hook previously held its own copies of the displayed identity ref, the message list and its ref, the transient projection and its per-session map, and the load-pending flag, all of which were then threaded outward. Those declarations are removed; one owner now holds them and derives the catalog and host session views beside them, with the shell wiring rather than re-deriving. This is a real reduction in duplicated state rather than a relocation of it.

The atomicity property survives the move. The displayed identity and its rows remain a single state value written together, so no render can pair one session's identity with another's rows. The switching predicate is now named and owned in the same place, defined as the displayed identity differing from the requested one — the same condition the shell previously computed inline, so the inert and busy interval is unchanged in meaning.

The recovery contract does not reach this surface, and I checked rather than assumed. The renderer-side owned-resources reference is gone from the tree entirely; recovery now lives in the workbar layout state and controller, which query the Host and restore terminal views. Nothing in that path writes the displayed session, the transcript or the load-pending flag. From this surface's point of view the recovery contract adds no competing writer, and the single-authority property for conversation display is intact. What that recovery does to terminal resource ownership and boundedness is a different surface's question and I make no claim about it.

The two items the description still lists as open, re-derived here.

Range controller and display identity. The identity-and-rows half is atomic, as above. The range controller still changes hands on a different schedule, being assigned by the effect keyed on the requested session while the displayed identity advances only when a transcript commits. I re-checked every read of that reference at this commit: the transient projection compares the range's own session before using it, the effect cleanup compares the controller's store against the one it created, and the refresh path re-checks both the displayed identity and the controller reference after its await. retryMessages still checks only the displayed identity and then calls reload on whatever the reference holds. I could not reach it — the surface carrying that affordance is inert for exactly that interval — so it remains a latent inconsistency rather than a defect. My verdict is unchanged but re-established: closed for identity and rows, open only as a guard weaker than its neighbour, whose safety rests on an attribute rather than on its own check.

Requested and displayed retirement. Re-read at this commit rather than carried forward: clearing owned state drops the retired session's pending entries unconditionally and keys its visible effect on the displayed identity, so retiring the displayed session while another is requested, and the reverse, both leave a coherent state. I find no inconsistency in this layer. The workbar side of that item is not mine.

Conclusion for this surface: no P0–P2. The one item I carry forward is the latent guard above, which I would grade P3 and which does not block.

I am not approving, and the reason is not this surface. Approval would have to speak for the whole change. Surface 2 is under independent review by a reviewer belonging to a different owner and has not reported yet. Surface 3 — hidden terminal performance, accessibility, Git incremental refresh and resource boundedness — has no reviewer under any owner, and I could not find an available one: the candidates are variously offline, out of quota, or belong to the same owner as the surface 2 reviewer, which would not make them independent. I am recording that as an open gap rather than covering it from here.

On performance specifically. The full A/B comparison has not been re-run on this commit. Any performance statement available today is quoted from an earlier measurement, not independently reproduced by me, and I am not treating it as verification of this code.

Verification limits. I read code and hosted results only and ran nothing. I did not exercise the recovery path, did not reproduce the inert interval, and did not review the terminal or host lifecycle or the performance surface.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[Sun-GLiang]

Review result: NO-GO (2 P2, 1 P3)

Reviewed commit 96fd5766f466347e804e9182ff4434ce31afd9ca.

The structural direction is sound: displayed Session identity and transcript rows now have one owner under the conversation feature, Workbar topology remains in its existing reducer, and the duplicate renderer Terminal ownership map is gone. The exact-head hosted knip stage is also clean. Two lifecycle sequences are still incomplete.

P2 — a Terminal restored during its previous Stop becomes a permanent dead tab

Reachable sequence: Close starts Stop; the window is disposed while Stop is pending; a new controller calls listLive() before Stop settles and restores the resource; then the old Stop succeeds.

The disposed controller fences its success callback in apps/desktop/src/renderer/features/workbar/controller/use-workbar-controller.ts, so it cannot remove the reconstructed tab. The new controller does not know about that Stop. apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts:48-54,76-78 filters terminal/completed updates out of both recovery reads and subscriptions, while apps/desktop/src/renderer/features/workbar/model/workbar-layout.ts:208-219 only appends missing tabs and never removes a recovered tab absent from a later live inventory. The result is a visible Terminal whose attach/write operations can only fail.

Please add the missing Stop pending -> unmount -> remount while live -> Stop succeeds regression and reconcile recovered entries when they leave the Host live inventory, without removing deliberate completed-output tabs that were created and retained in the current window.

P2 — retiring the displayed Session cancels a newer requested Session

The new model intentionally allows Session A to remain displayed while Session B is requested and loading. During that interval, both retirement checks in apps/desktop/src/renderer/app-shell-effects.ts:177-180,230-233 inspect activeIdRef, which still names A. If A disappears from the refreshed catalog, retireSession(A) reaches apps/desktop/src/renderer/app-shell.tsx:1974-1977, where setActiveId(undefined) unconditionally clears the catalog selection. That overwrites the still-valid request for B and blanks the view instead of allowing B to finish loading.

Please make retirement identity-scoped: clear the requested selection only when the retired id is still the requested id, and retire displayed state independently. Add both orderings to regression coverage: displayed A retires while requested B is loading, and requested B retires while A remains displayed.

P3 — reuse the existing shell-runs namespace instead of restating seven pass-through methods

apps/desktop/src/renderer/platform/desktop/create-workbar-services.ts:67-73 wraps start, stop, attach, detach, write, subscribePtyData, and subscribeResync without changing arguments or results. The renderer adapter contract in apps/desktop/src/renderer/README.md explicitly keeps structural bridge subsets intact and hand-writes only rename, guard, or translation blocks. Reuse or narrow bridge.shellRuns once, then override only listLive and the filtered subscribeUpdates; this removes duplicated forwarding code and its maintenance surface without introducing a new abstraction.

Validation

git diff --check is clean. I reproduced the renderer architecture check against merge-base 5313759080ca34f4fff013c7488e47222a9c4b84. Hosted test and windows_recovery are green; the hosted test job includes build, typecheck, Desktop knip, and workspace test stages. I did not rerun the live Electron performance measurements.

Automated review notice: This review was produced by an AI agent and does not replace independent human review.

[jackwener]

Surface 1 re-review at 5c37e98f8938f3fa0eca3411087172ad0ce98c19, re-derived at this commit rather than carried forward. The one item I have been carrying across three heads is now closed, and closed structurally.

The range controller no longer changes hands on its own schedule. Through the previous heads I reported that identity and rows were published atomically while the range controller was installed separately by the effect keyed on the requested session, leaving an interval where the reference belonged to one session and the displayed identity to another. I noted that every read of that reference guarded itself except the retry path, which checked the displayed identity and then used whatever the reference held, and that its safety therefore rested on the surrounding surface being inert rather than on its own check.

At this commit the reference is installed in exactly one place: the same function that advances the displayed identity and publishes the rows, with the controller passed alongside them. Every other assignment clears it — on cleanup, on navigating away from the requested session, and on retirement. The effect no longer installs it at all. The interval is therefore gone, and the retry path's single check is now sufficient, because when the displayed identity is a session the reference necessarily holds that session's controller.

That is the better of the two available fixes. I had suggested matching the neighbouring guard; removing the divergence that made a guard necessary is stronger, because it cannot be undone by a future edit that forgets the guard.

The rest of my surface at this commit. The published unit is now identity, rows and range together. A commit refuses outright when the session it names is no longer the requested one and reports that refusal to its caller, so a callback left over from a retired or superseded navigation cannot write back. Retirement clears the displayed identity, the range reference and the rows together when the retired session is the displayed one, rather than leaving any of the three behind. The three frozen files remain below their base budget rather than merely at it.

No P0–P2 on this surface.

Why this is not an approval. A reviewer under a different owner raised an issue on the terminal lifecycle surface at the previous head — a close that was pending across a controller remount could restore an already-closed terminal as a permanently dead tab. This commit describes a change intended to address exactly that, by moving close intent into the host target lifecycle and preventing stale inventory from resurrecting a completed close. Whether it does is a question about their surface, on their evidence, and they are reviewing it. Approving now would mean letting a clean local surface stand in for a global conclusion, which is precisely what this review was asked not to do. I will approve once that surface reports, without needing to be asked again.

Verification limits. I read code and hosted results only and ran nothing; the author's local runs are not part of my evidence. I did not review the terminal, host or workbar timing, and I did not evaluate the performance surface, which still has no reviewer under any owner. The hosted check state on this commit is not something I am relying on for the conclusions above, which are about code.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one.

[jackwener]

Approve at f4bdc35cd222bdf5444b38c5cbb29a502a213782. This is a formal verdict on my own verification, not a deferral to another reviewer's pending one.

The increment since my last review changes nothing at runtime. An inline type reference became a top-level type import and the measured ledger was updated to match. Type imports are erased, so my conclusions from the previous commit carry without re-derivation, and I say so having checked the diff rather than assuming it.

My surface is clean, and the item I carried across four heads is closed structurally. Identity, rows and the range controller are now published as one unit by one function. That function refuses outright when the session it names is no longer the requested one and reports the refusal to its caller, so a callback left over from a retired or superseded navigation cannot write back. Retirement clears identity, range reference and rows together. The range reference is installed in exactly one place — the same commit that advances the displayed identity — and every other assignment clears it, which removes the interval I had reported rather than guarding against it. The three frozen files remain below their base budget.

On the terminal issue raised by the reviewer on the other surface, I verified the mechanism myself rather than waiting. Their finding was that a close left pending across a controller remount could restore an already-closed terminal as a permanently dead tab. The cause that made it possible was that the intent lived with something that remounts. It no longer does: intent is now held in the main process, keyed by session and resource reference, explicitly independent of both the renderer and connection leases. Concurrent stops share one operation rather than racing; success deletes the entry and announces closure; failure keeps the entry so an explicit retry still has an entry point; each completion re-checks that the entry it is about to mutate is still the one it registered, so a superseded attempt cannot overwrite a newer one; recovery reconciles against actual terminal status rather than trusting a snapshot; and retirement clears the session's entries. The lifetime dependency that produced the original failure is gone.

I want to be precise about what that does and does not settle. I verified the design removes the condition that made their sequence possible. I did not reproduce their sequence against it, and my verdict does not stand in for theirs — they found it, they know the exact ordering, and their formal conclusion is still owed. If they find it survives in some form, that supersedes this approval and I will say so.

One non-blocking observation. Retiring a session announces closure for every intent it holds, including ones that only ever reached pending or unknown. Nothing displays those tabs after retirement, so the practical effect is nil, and the change explicitly does not promise stop retries that outlive the desktop. It is still a stronger claim than the state supports, and if a future reader treats that announcement as evidence a resource stopped, it would mislead.

Coverage, and the gaps I am not papering over. I reviewed the session and renderer architecture, the publication seam with the merged upstream, the retirement path, and the close-intent mechanism described above. I did not review the workbar timing, the host transport, or the performance, accessibility and Git surface — that third surface has had no reviewer under any owner for the whole of this review, and a third independent owner was sought repeatedly without one becoming available. The performance argument in particular remains unverified against this code by anyone.

I ran nothing: no test, build, Storybook run or application session. The author's local results are theirs and are not my evidence. At the time of posting the hosted test check is still running while the recovery check has passed; this approval is a code-review conclusion and does not assert that checks pass, nor does it authorise a merge.

---

Automated review notice. This review was produced by an automated reviewer (agent seat kabi-opus, model Claude Opus) acting on its own review identity. It is posted from a shared machine account, so the seat is named here because the account alone does not identify the reviewer. It is not an independent human review and does not substitute for one. No merge was performed.