fix: release-review findings, verified over three passes - #115
Merged
Conversation
Independent review of v0.6.0..HEAD raised 10 findings. This fixes the six that are release-blocking or self-inflicted; #8 (DbGate per-line throughput) and #9 (File.Replace portability) are deferred by agreement. 1. HIGH — DisposeAndWaitForExitAsync guarded on pty.IsRunning, which is `_hProcess != IntPtr.Zero` and only cleared in Dispose. It does NOT mean the child is alive, so a pane whose claude already exited (user typed `exit`, or it crashed) subscribed to Exited AFTER MonitorExitAsync had already raised it, then waited the full 10s for an event that never comes. That compounded with the 15s shutdown budget added in #101: two stale panes consume the whole allowance and every remaining LIVE Claude session is then force-disposed with no exit wait — losing exactly the ~/.claude.json serialization the loop exists to provide. A slow shutdown became a correctness risk. Added PseudoTerminal.HasExited, latched before Exited is raised so a subscriber can never observe "not exited" for a dead process. Caller guards on it and re-checks after subscribing to close the in-between window. 2. MEDIUM — RestartSessionAsync disposed the old session and relaunched immediately, recreating the concurrent-config-writer race that the launch stagger and shutdown loop both exist to prevent, and making --resume read a session index the outgoing process had not finalised. Claude sessions now wait for real exit plus config quiesce; non-Claude keep the cheap teardown. 3. MEDIUM — SessionType_Changed cleared NameBox unconditionally. In edit mode that silently wrote Name = "" (AutoFillName has nothing to refill from for a remote session). Now returns early in edit mode. 4. MEDIUM — no working-folder validation in edit mode. An empty folder was persisted and LaunchSessionAsync silently fell back to %USERPROFILE%, so a session appeared reconfigured while running somewhere else, with git info and accent colour keyed off an empty path. Flipping Remote -> Local hit this every time. Validated in edit mode only; create mode keeps the useful home-folder default. 5. MEDIUM — self-inflicted in #105. IsRunnable rejected any zero-byte reparse point to skip Store App Execution Alias stubs, but a WORKING Store install of PowerShell 7 is exactly that shape. Store-PS7 users were silently downgraded to 5.1, losing the profile functions that are the entire reason for preferring pwsh — on every session launch, since the locators merged. Traded a rare failure for a common one. Now: metadata fast path for ordinary executables (no spawn), and an actual bounded execution probe only for the ambiguous alias shape. 6. LOW — self-inflicted in #110. RegisterWaitForSingleObject's callback can run before the assignment to `registration` completes when the handle is already signalled, leaving the wait unregistered and disposing the event under a live registration. Published through a lock with a once-only release flag. Also corrected a stale comment in MainViewModel describing a mouse-report filter that no longer exists, and added the _sessionUiVersion bump in RestartSessionAsync that the field's own doc says belongs there. 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
…alias shape is ambiguous Follow-up to the six release-review fixes. - Waiting for a PTY: check HasExited, never IsRunning. Written down because the distinction is invisible at the call site and getting it wrong turned the shutdown budget into a correctness risk rather than a latency guard. - RestartSessionAsync waits for real exit + config quiesce for Claude sessions. - PwshLocator: a zero-byte reparse point is AMBIGUOUS, not bad. A working Store install of PowerShell 7 is the same shape as a dead stub, so only that case is settled by probing. Recorded because rejecting the shape looks obviously correct and silently downgraded Store users. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
From the release security audit — reported below its severity bar, but it is a real robustness bug. ApplyProfileOverrides called JsonSerializer.Deserialize<JsonElement> on ProfileColorSchemeJson with no guard. That value is normally produced by SchemeMapper, but ImportExportService deserializes a whole AppState from any file the user opens, so it can be arbitrary. A JsonException there propagates out of the launch path and takes down an otherwise fine session. Now caught and logged; the session launches with the default palette. Losing a theme is survivable, failing to start is not. The audit found nothing at or above its bar across the whole v0.6.0..HEAD diff: the PostRunUrl scheme guard holds (no parse divergence between Uri and ShellExecute), SQL parameterisation is intact including the FTS5 and LIKE paths, and nothing is interpolated into the page — host->page traffic is exclusively PostWebMessageAsString of serialized JSON, with no ExecuteScriptAsync or NavigateToString anywhere. One hardening suggestion deliberately NOT taken: PwshLocator validates the absolute path from where.exe then returns the bare name for CreateProcess to re-resolve. Returning the validated path would close a currently-unreachable search-order gap, but the value is interpolated into a command line by BuildCmdLine without quoting, so an absolute path containing spaces would break every wrapped session. Not worth that risk immediately before a release for a gap the audit itself judged unreachable. 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
An independent verification of the six fixes confirmed four outright and found three problems, one of which silently defeated the fix it was checking. FUNCTIONAL — RestartSessionAsync branched on the WRONG command. EditSessionAsync calls SessionConfigEditor.Apply BEFORE RestartSessionAsync, and Apply mutates session.Command in place. So the branch read the NEW command: editing a Claude session to something else tore down the running claude.exe with a bare Dispose, no exit wait and no config quiesce — exactly the ~/.claude.json race the fix was written to close, still open on the one edit that needed it. The old command is now captured pre-Apply and passed in. UX — the sidebar row vanished for up to 11s during a Claude restart. The row is removed early, and the new exit wait was inserted BEFORE the placeholder that exists to stop the row blinking out. Placeholder moved above the wait. PERF — PwshLocator.Executable is a Lazy first forced from PseudoTerminal.Start, which runs on the UI thread. With the Store-alias probe behind it that is up to ~7s of frozen window on first launch, and a realistic ~1s during restore. This is the same class of UI-thread stall #107 and #110 spent four rounds removing, and I reintroduced it. Now warmed on the pool in OnLoaded, so the Lazy is already resolved before any session starts. Also from the verification: - HasExited is now a volatile field. It was safe as an auto-property only because the field-like Exited event's add accessor is an Interlocked CAS and supplied the fence — too subtle to rely on for a future reader that polls. - The setOptions re-fit uses requestAnimationFrame. Neither fonts API works here: fonts.ready is permanently resolved after page load, and fonts.load only matches CSS-connected FontFace objects — there are no @font-face rules and the families are OS-installed, so it resolved having matched nothing. rAF fires after the style change is applied and measured. - Corrected the PwshLocator class doc, which still argued against the probe the code now performs, and a comment in ApplyEditMode that described NameBox being blanked and repaired. Verified correct without change: HasExited latching on every exit path (including the DuplicateHandle early return), the guard/re-check ordering, the edit-mode early return, the folder validation branch and placement, and the WaitForHandleAsync lock (no inline dispatch, so no deadlock; Dispose strictly after Unregister). 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
Third verification pass: corrections A (restart command branch) and B (sidebar placeholder) verified correct; C was incomplete. Starting the warm task in OnLoaded is necessary but not sufficient. PwshLocator's Lazy uses ExecutionAndPublication, so a UI thread reaching .Value while the pool thread is still inside the factory takes the Lazy's monitor and blocks for the REMAINING factory duration. That converts a guaranteed stall into a raced one and shrinks it by the head start — it does not remove it. Residual was quantified at up to ~5-6.5s in the pathological case (slow where.exe plus a live Store alias paying a full PS7 cold start). PublicationOnly would not have fixed it either: the UI thread would stop blocking on the monitor and simply run its own copy of the factory, paying the same cost. The task is now awaited before the restore loop. Awaiting yields rather than blocking, so the window stays responsive, and Task.WhenAny with an 8s ceiling — above Resolve's own 7s bound of 2s where.exe + 5s alias probe — means a wedged probe delays restore rather than preventing it. In the ordinary MSI case Resolve finishes in ~10-50ms and this is a no-op. Also dropped a superseded comment in terminal-init.js: the old fonts.load rationale was left above the new requestAnimationFrame one and directly contradicted it. Verified correct without change in this pass: launchedCommand is captured genuinely pre-Apply with no re-mutation before use and only one call site exists; the placeholder is added exactly once and, critically, still AFTER the _sessionUi and _vm.Sessions removals so RebuildSidebarOrder's Resolve takes the placeholder branch rather than a stale live item; HasExited is latched on all three exit paths through the outer finally; and the rAF re-fit cannot throw. 314/314 pass, 0 warnings. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be
AThraen
added a commit
that referenced
this pull request
Sep 6, 2026
…nt (#117) Two problems, both found by running it rather than reasoning about it. 1. The adaptive config gate overshoots at shutdown too. #111 reverted it on the launch path but KEPT it here, on my reasoning that "the machine is quiet at shutdown, so polling is reliable". Measurement falsified that. A real run logged: SHUTDOWN 'SiteAnalysis': exit=4656ms cfgSettle=8731ms against a 1000ms cap — 8.7x over, and 56% of the entire shutdown budget spent in one session, which is what forced the remaining six to be killed with no exit wait. Same disease as the launch path: when the thread stalls, the gate measures the stall rather than the file. Now a flat Task.Delay, on both the shutdown loop and the restart path. Recomputing that run flat gives 8969ms instead of 15506ms with nothing force-disposed — the gate's typical ~300ms beats a flat 1000ms right up until it doesn't, and the tail is what costs. ClaudeConfigGate now has no callers and is deleted along with its tests, rather than left as dead code for someone to reintroduce. 2. The 15s budget was sized from the wrong data. It was chosen when the only measurements available were idle sessions exiting in 460-770ms. Busy sessions measure 2.3-4.7s each, so nine of them need roughly 30s, and 15s force-disposed over half the fleet on an ordinary close. Raised to 30s: a clean exit lets Claude finish writing its config, and ShutdownOverlay is already on screen explaining the wait. The budget is there to bound a wedged session, not to hurry a healthy one. Confirmed while here: the HasExited fix from #115 works. No exit= value is near the 10s cap (max observed 4656ms), so nothing is timing out on a process that already died — which was the whole point. CLAUDE.md updated: the gate is gone from both paths, and the budget carries its measurements plus a note to re-measure exit= before shrinking it, since the summary line cannot distinguish slow exits from waits that never return. 304/304 pass (10 fewer — ClaudeConfigGateTests removed), 0 warnings. Claude-Session: https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 6, 2026
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.
Closes out the v0.7.0 release review. An independent review raised 10 findings; this fixes the eight that are release-blocking, self-inflicted, or cheap. #8 (DbGate per-line throughput) and #9 (File.Replace portability) are deferred by agreement.
Three verification passes ran over these fixes. Two of them found real problems in my own corrections, which is why this PR is larger than the original finding list.
The blocker
DisposeAndWaitForExitAsyncguarded onIsRunning— which is_hProcess != IntPtr.Zero, released only inDispose. It does not mean the child is alive, so a pane whose Claude had already exited subscribed toExitedafter it had fired and waited the full 10s.That compounded with the 15s shutdown budget from #101: two stale panes consume the entire allowance, and every remaining live Claude session is then force-disposed with no exit wait — losing exactly the
~/.claude.jsonserialization the loop exists for. A latency guard had become a correctness risk.Added
PseudoTerminal.HasExited, latched beforeExitedis raised, with a re-check after subscribing.The rest
%USERPROFILE%PwshLocatorno longer rejects working Store PowerShell installsRegisterWaitForSingleObjectregistration raceProfileColorSchemeJsonfrom an imported state file no longer aborts a session launch#5 is worth singling out — it was mine, from #105, and it was worse than the bug it fixed. A working Store install of PS7 is a zero-byte
AppExecLinkreparse point, identical in shape to a dead stub. Rejecting the shape silently downgraded those users to 5.1 on every session launch, losing the profile functions that are the entire reason for preferring pwsh. I'd traded a rare failure for a common one.What the verification passes caught in my fixes
RestartSessionAsyncfix didn't work on the case it was written for.SessionConfigEditor.Applymutatessession.Commandbefore the restart, so the branch read the new command — a claude→non-claude edit still hit the config race. Now the pre-Applycommand is threaded through.PwshLocator.Executableis forced fromPseudoTerminal.Starton the UI thread; with the probe behind it, up to ~7s frozen. Warming it on the pool wasn't enough either —LazyinExecutionAndPublicationmode makes a UI thread arriving mid-factory block for the remainder. Now awaited before the restore loop, bounded at 8s.HasExitedwas safe only by accident — via the field-likeExitedevent'sInterlockedadd accessor supplying a fence. Now an explicitvolatilefield.document.fonts.readyis permanently resolved after page load;fonts.load()only matches CSS-connectedFontFaceobjects and there are no@font-facerules here. NowrequestAnimationFrame.Security audit: clean
No findings at or above the bar across
v0.6.0..HEAD. It confirmed thePostRunUrlscheme guard has noUri/ShellExecute parse divergence, SQL parameterisation holds including the FTS5 andLIKEpaths, and nothing is interpolated into the page — noExecuteScriptAsyncorNavigateToStringanywhere.One suggested hardening declined: returning
PwshLocator's validated absolute path instead of the bare name would close a currently-unreachable search-order gap, but that value is interpolated into a command line unquoted, so a path with spaces (i.e. the normal install) would break every wrapped session.Known-unresolved
#113 was probably a no-op. It shipped as the fix for the reported initial-render fault, but relied on
document.fonts.ready— which, per the above, doesn't defer. Nothing regressed, but that rendering issue should be treated as still open; thesetTimeout(50/250)fits are what actually correct it, and the cause may be the 0×0-container case rather than font timing.🤖 Generated with Claude Code
https://claude.ai/code/session_01NDsEfog5kVkT5NmX1Ya5be