Fix Claude no-flicker sidebar status routing#6058
Open
austinywang wants to merge 61 commits into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe CLI now snapshots environment state once, threads provenance-aware terminal-binding data through Claude hook resolution, and tightens PID and surface handling across hook paths. New serialized tests cover no-flicker routing, stored-session fallback, and transient recovery cases. ChangesClaude hook routing and regression coverage
Test infrastructure and regression coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR fixes the no-flicker (
Confidence Score: 5/5Safe to merge; the fix is well-scoped to ambient no-flicker hook routing and does not regress normal sessions. All routing changes are correctly gated: the expensive system.top process-snapshot lookup is only triggered when allowProcessSnapshotBinding is true, which is only set when the hook live PID differs from the stored session PID. Stored-session workspace and surface are preserved on transient surface.list failures via allowUnknown:true. The set_agent_pid refresh on prompt-submit only fires when the resolved surface is authoritative. Four new serialized Swift test suites cover binding, routing, and transient-failure paths. No files require special attention; CLI/cmux.swift changes are well-bounded and the test files provide direct coverage of the new paths. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Claude hook fires] --> B{needsRecoveredHookTarget}
B -->|No - normal session| C[mappedHookTargetIsReachable returns true]
B -->|Yes - ambient no-flicker| D{Mapped session workspace and surface reachable?}
D -->|Yes| E[Use mapped session target]
D -->|No| F{PID or TTY binding found?}
F -->|Binding found| G[resolvePreferredWorkspaceId]
F -->|No binding| Z[Graceful no-op]
E --> G
C --> G
G --> H{Preferred UUID set and workspace accessible?}
H -->|Yes - PID snapshot, different workspace| J[Return live binding workspace]
H -->|Yes - same workspace or TTY| K[Return preferred workspace]
H -->|No - ambient fallback| L{Terminal binding found?}
L -->|Yes| M[Return binding workspace]
L -->|No| N[Default workspace resolution]
J --> O[resolvePreferredSurfaceDetailed]
K --> O
M --> O
N --> O
O --> P{Preferred surface accessible?}
P -->|Yes - PID snapshot overrides| Q[Live binding surface - isAuthoritative=true]
P -->|Yes - matches stored| R[Preferred surface - isAuthoritative=true]
P -->|No - ambient fallback with binding| Q
P -->|No binding| T[Focused panel fallback - isAuthoritative=false]
Q --> U[prompt-submit: set_agent_pid refresh then set status with pid]
R --> U
T --> V[setAgentLifecycle and status without pid]
Reviews (30): Last reviewed commit: "fix: tighten Claude hook routing and fee..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 23185-23210: The cache currently only tracks didResolve and
binding (ClaudeHookTerminalBindingCache) so
resolveClaudeHookTerminalBinding(...) may return a binding regardless of
differing agentPID or allowProcessSnapshotBinding; change the cache to include
the lookup key (store agentPID and allowProcessSnapshotBinding in
ClaudeHookTerminalBindingCache or add an assertion that the incoming key matches
the cached key), then update resolveClaudeHookTerminalBinding(...) to check the
cached key before returning cached.binding and to set both the key and binding
when populating the cache (or assert on mismatch), referencing the
ClaudeHookTerminalBindingCache and resolveClaudeHookTerminalBinding symbols and
the CallerTerminalBinding type so mixed-lookups cannot return
stale/over-permissive bindings.
In `@cmuxTests/ClaudeHookRoutingTestSupport.swift`:
- Around line 145-152: The timed-out subprocess may not exit after
process.terminate(), causing subsequent readDataToEndOfFile() calls to block;
update the timeout-handling block so that after calling process.terminate() you
check if the process is still running and if so escalate to SIGKILL (use
kill(process.processIdentifier, SIGKILL) or equivalent), wait again for the
exitSignal (e.g., another wait with a short timeout) until the process has
exited, and only then call stdoutPipe.fileHandleForReading.readDataToEndOfFile()
and stderrPipe...; modify the block around timedOut, terminate(),
exitSignal.wait(...), and the return that constructs ProcessRunResult so the
kill-and-wait escalation is performed before draining stdout/stderr and reading
process.terminationStatus.
- Around line 104-106: The write call inside response.withCString ignoring
Darwin.write's return can produce partial writes and truncated JSON; update the
code around response.withCString { ptr in Darwin.write(clientFD, ptr,
strlen(ptr)) } to loop until the full byte count is written: compute the total
length (e.g., with strlen(ptr)), call Darwin.write in a loop advancing the
pointer by the number of bytes written, handle short writes and retry on EINTR,
and treat negative return values as errors (break/log/close as appropriate);
reference the existing response.withCString closure, Darwin.write, and clientFD
when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2022bd44-7d07-4909-b9db-af3960322b13
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
CLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClaudeHookRoutingTestSupport.swiftcmuxTests/ClaudeNoFlickerHookRoutingTests.swift
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux.swift`:
- Around line 23053-23054: The fallback path is currently accepting PID-derived
bindings if claudeHookWorkspaceIsAccessible(...) is true even when
binding.isProcessSnapshotBound is true; require the bound workspace to also be
the currently selected workspace before returning a PID-derived binding. In
CLI/cmux.swift (lines 23053-23054) modify the resolver that returns
boundWorkspaceId to add a condition alongside
claudeHookWorkspaceIsAccessible(boundWorkspaceId, client: client) that the
workspace is the selected/current workspace (e.g.
workspaceIsSelected(boundWorkspaceId, client: client) or equivalent check) when
binding.isProcessSnapshotBound is true; and in CLI/cmux.swift (lines
23068-23069) apply the same selected-workspace gate in the fallback-only
resolver so that stop/cleanup paths cannot accept a background PID match (no
change needed to other logic—just add the selected-workspace predicate where
claudeHookWorkspaceIsAccessible is currently used for process-snapshot
bindings).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d3dd822c-5421-4399-9596-4ada1558e202
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (4)
CLI/cmux.swiftcmux.xcodeproj/project.pbxprojcmuxTests/ClaudeHookRoutingTestSupport.swiftcmuxTests/ClaudeNoFlickerHookRoutingTests.swift
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmuxTests/ClaudeNoFlickerHookRoutingTests.swift (1)
296-410: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winAdd missing stale workspace negative assertion.
The test seeds a stored session with
staleWorkspaceId(lines 302, 313, 321) and thesurface.listmock returns different surfaces based on workspace (lines 335, 339), but the assertions only verify that no commands targetstaleSurfaceId(lines 394-400). Tests 1–3 verify both--paneland--tabnegatives when checking stale bindings (lines 118-130, 196-201, 287-293). SincestaleWorkspaceIddoes not appear in thesystem.topresponse (lines 341-356), the CLI should never send commands to it.✅ Add workspace negative assertion
"Stored stale PID surface must not receive Claude's visible status or PID gate, saw \(commands)" ) + `#expect`( + !commands.contains { + ($0.hasPrefix("set_status claude_code Running ") || $0.hasPrefix("set_agent_pid claude_code ")) + && $0.contains("--tab=\(staleWorkspaceId)") + }, + "Stored session's stale workspace must not receive Claude's visible status or PID gate, saw \(commands)" + ) let systemTopCalls = commands.filter {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/ClaudeNoFlickerHookRoutingTests.swift` around lines 296 - 410, The test claudePromptSubmitPrefersLiveHookPIDOverStoredPID() validates that the CLI ignores a stale stored workspace and surface when a live PID hook is available. Currently, it asserts that commands don't target staleSurfaceId via the --panel parameter (line 394-400), but it's missing a parallel assertion to verify commands don't target staleWorkspaceId via the --tab parameter. Add a negative assertion after the existing staleSurfaceId check that verifies no commands contain --tab=staleWorkspaceId, following the pattern used in earlier tests like claudePromptSubmitFiltersSessionsByWorkspace() and claudePromptSubmitPrefersSessionFromActiveList() which validate both --tab and --panel negatives for stale bindings.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/ClaudeNoFlickerHookBindingTests.swift`:
- Line 446: The combined statement in ClaudeNoFlickerHookBindingTests should be
split into separate lines for readability; update the environment initialization
and the subsequent TTY assignment in the test setup so they are written as
distinct statements, using the existing support.baseHookEnvironment(context:)
and environment["TTY"] reference points.
- Line 423: The compound let declaration that declares staleWorkspaceId,
staleSurfaceId, and staleTTY on a single line exceeds 145 characters and reduces
readability. Split this into separate let declarations with each variable on its
own line to comply with Swift style guidelines.
---
Outside diff comments:
In `@cmuxTests/ClaudeNoFlickerHookRoutingTests.swift`:
- Around line 296-410: The test
claudePromptSubmitPrefersLiveHookPIDOverStoredPID() validates that the CLI
ignores a stale stored workspace and surface when a live PID hook is available.
Currently, it asserts that commands don't target staleSurfaceId via the --panel
parameter (line 394-400), but it's missing a parallel assertion to verify
commands don't target staleWorkspaceId via the --tab parameter. Add a negative
assertion after the existing staleSurfaceId check that verifies no commands
contain --tab=staleWorkspaceId, following the pattern used in earlier tests like
claudePromptSubmitFiltersSessionsByWorkspace() and
claudePromptSubmitPrefersSessionFromActiveList() which validate both --tab and
--panel negatives for stale bindings.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 14e71662-bc24-4e40-99ac-393f81485af1
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (3)
CLI/cmux.swiftcmuxTests/ClaudeNoFlickerHookBindingTests.swiftcmuxTests/ClaudeNoFlickerHookRoutingTests.swift
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/ClaudeNoFlickerHookTransientTests.swift`:
- Around line 300-363: The test
claudePromptSubmitDoesNotRepublishStoredPIDWithoutLiveHookPID is missing a
defensive assertion to verify that system.top is not called as part of the
hot-path optimization when using a stored PID without a live hook PID. Add an
assertion after the existing storedPIDWasRepublished check to verify that the
commands captured from context.state.snapshot() do not contain any invocation of
system.top, consistent with the pattern demonstrated in the sibling tests that
validate this optimization.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 16b40e7d-1b48-41a6-af5c-02265932db7b
📒 Files selected for processing (2)
CLI/cmux.swiftcmuxTests/ClaudeNoFlickerHookTransientTests.swift
…-sidebar-status # Conflicts: # .github/swift-file-length-budget.tsv
…-sidebar-status # Conflicts: # .github/swift-file-length-budget.tsv
Failing regression test (no fix) per the two-commit regression policy. An ambient no-flicker prompt-submit carrying a mapped session whose stored workspace was closed (and whose PID/TTY cannot be recovered) must no-op instead of borrowing the focused pane and publishing Running status / the PID gate onto it. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A mapped session record bypassed the recovered-target no-op unconditionally, so an ambient no-flicker prompt-submit / stop / notification / pre-tool-use whose stored target had been closed (and whose PID/TTY could not be recovered) fell through to the app-selected focused pane and published Running/Idle/Needs-input status and the PID gate onto it — the cross-pane routing class this PR fixes. Gate the bypass on the stored target still being reachable via a shared mappedHookTargetIsReachable() helper that requires BOTH the stored workspace AND the stored surface to still be present. When either is gone and PID recovery misses, the hook no-ops exactly as it does without a record. session-end is intentionally unchanged: it mutates the consumed record's own stored target, not the resolved focused fallback. Adds regression tests for the closed-workspace and closed-surface cases. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Fixes #6048
Summary
CMUX_SURFACE_IDwhileCLAUDE_CODE_NO_FLICKER=1is set--surfaceand mapped session surfaces authoritativeclaude_codePID ownership gate onprompt-submitbefore writing Running status so the sidebar badge has both required signals on the same panelMechanism
Claude hooks still fire in no-flicker mode; the failure path is that inherited ambient cmux routing can point visible status at one panel while the sidebar badge gate requires the recorded agent PID owner and status entry to match the same live panel. The fix treats live terminal binding as ground truth only for ambient fallback surfaces.
Verification
git diff --checkpython3 tests/test_claude_wrapper_hooks.pyLocal app builds intentionally not run per issue instructions; CI should compile and run the Swift regression test.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes sidebar badge/status mismatches in no‑flicker mode by routing Claude hook status through the pane that owns the live process, tightening PID/target validation, and aligning feed telemetry to the same target. Also no‑ops ambient hooks when a mapped session’s target is gone and recovery isn’t possible; adds serialized Swift tests for these cases.
Bug Fixes
set_status; honor explicit--surface/--workspace(including empty/equals); prefer the live PID over stored; never republish or persist stored PIDs (especially on fallback routes); avoid process snapshots on hot paths with an isolated, authenticated per‑hook socket.CLAUDE.mdseparators to avoid conflict marker scan false positives.Dependencies
.github/swift-file-length-budget.tsvto match current code size and tests.Written for commit c53e1d0. Summary will update on new commits.
Summary by CodeRabbit
--surface/--workspaceflags (including empty/equals forms), and correct telemetry when--surfaceis explicitly provided; refined PID attribution and snapshot binding behavior.