Repository navigation
Hibernation: correct notification lifecycle for all agent types - #6721
Closed
lawrencecchen wants to merge 27 commits into
Closed
lawrencecchen wants to merge 27 commits into
lawrencecchen wants to merge 27 commits into
Conversation
…+ robust restorability Two systemic gaps kept non-codex agents from hibernating, found while mapping the hibernation path for every coding-agent kind (claude, codex, grok, gemini, opencode, cursor, kiro, antigravity, rovodev, copilot, codebuddy, factory, qoder, hermes-agent, pi, amp, custom vault agents). 1. Notification clobber was per-lane, not just claude. A routine "waiting for input" reminder classified as needs-input and flipped the hibernation lifecycle to `.needsInput`, clobbering the `.idle` the Stop hook recorded — so the pane never hibernated. This was the dedicated claude bug AND the generic agent-hook lane bug (grok, antigravity, custom). Fix: one shared `agentNotificationIsBlockingPrompt` (permission/approval/error) used by BOTH lanes. A notification only asserts `.needsInput` when genuinely blocking; otherwise it leaves the lifecycle untouched (the Stop hook is the single idle source). The user-facing notification, bell, and status are unchanged. 2. Claude restorability failed silently. `hookRecordIsRestorable` hard-required the claude transcript `.jsonl` on disk (claude-only; every other agent trusts the `isRestorable` flag). When transcript resolution missed for a genuinely restorable session (custom CLAUDE_CONFIG_DIR, account-scoped/forked roots, project-dir encoding drift), claude was dropped from the restorable index and silently never hibernated while codex did — the classic "only codex hibernates" report. Fix: trust the authoritative `record.isRestorable == true` when the transcript isn't found (transcript becomes a strong hint, not a hard gate), and log a breadcrumb so the gap is diagnosable instead of silent. Tests: AgentNotificationBlockingClassifierTests pins the shared blocking classifier that every agent's lifecycle decision funnels through (permission/ approval/error → blocking; waiting/completion/attention/empty → not blocking; signal field also considered). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughAdds blocking classification to agent notifications, threads ChangesisBlocking Notification Classification and Lifecycle
Hibernation diagnostics, tests, and CI coverage
Sequence Diagram(s)sequenceDiagram
participant AgentHook as Agent hook
participant Summarizer as summarizeClaudeHookNotification
participant Classifier as classifyAgentHookNotification
participant Session as session persistence
participant Lifecycle as setAgentLifecycle
AgentHook->>Summarizer: signal + message
Summarizer->>Classifier: subtitle, body, isBlocking
Classifier->>Session: persist blocking lifecycle when blocking
Classifier->>Lifecycle: setAgentLifecycle(.needsInput) when blocking
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 24 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (24 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
… silent Hibernation gave zero signal when a restorable agent failed to hibernate, making diagnosis a manual spelunk (it took hours to trace one such case). Emit one concise line per planner evaluation — restorable/live/maxLive/idle/protected/ unconfirmedInput/selected — whenever any restorable agent exists, so the exclusion reason is readable straight from the debug log. Quiet on idle systems (only logged when there are restorable records). Pairs with the transcript-drop breadcrumb. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The live setAgentLifecycle gate skipped only the in-memory mutation; the generic agent-hook lane still persisted agentLifecycle(for: .needsInput) to the durable hook store for routine waiting reminders, clobbering the Stop hook's stored .idle. On the index fallback (after restart / no live state) the pane was then treated as non-hibernatable. Persisted lifecycle now mirrors the live decision: blocking -> needsInput, completion -> idle, routine waiting / attention -> nil (preserve). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…tion lane These agents carried turn-end on Stop but routed Notification (their permission/attention channel) to the stop subcommand, which forced the pane .idle and let it hibernate while a permission prompt was live. Route Notification through the notification lane so the shared blocking classifier asserts .needsInput for permission/approval/error and preserves idle for routine reminders. Worst case after this change is no-hibernation (status quo for many agents), strictly safer than hibernating mid-permission-prompt. Adds AgentNotificationRoutingTests to pin the wiring. Bumps file-length budget for the touched files. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
CLI/cmux.swift (1)
26198-26208: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCompletion branch passes computed
blocking, but sibling completion paths hardcodefalse.Lines 26206-26207 thread
isBlocking: blockinginto the completion branch, whereas thesummarizeAgentHookNotificationcompletion returns (Lines 25929-25930, 25962-25963) hardcodeisBlocking: false. A completion message containingerror/failedwould computeblocking == truehere. It's currently moot because this branch emitsstatus: .idleand only the.needsInput?switch case acts onisBlocking, but it's a latent trap ifisBlockingis later consumed outside that switch. RecommendisBlocking: falsefor the completion branch for consistency and intent clarity.Suggested change
subtitle: String(localized: "agent.generic.notification.subtitle.completed", defaultValue: "Completed"), body: truncate(body, maxLength: 180), status: .idle, isFallback: isFallback, - isBlocking: blocking + isBlocking: false )🤖 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 `@CLI/cmux.swift` around lines 26198 - 26208, The completion branch within the containsCompletionCue condition currently passes isBlocking: blocking to the AgentHookNotificationSummary initializer, but other completion paths in the summarizeAgentHookNotification function hardcode isBlocking: false. Change the isBlocking parameter in the AgentHookNotificationSummary initialization within the containsCompletionCue block from isBlocking: blocking to isBlocking: false to maintain consistency with the sibling completion paths and clarify that completion notifications should not be blocking regardless of the computed blocking value.
🤖 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 92-98: The agentNotificationIsBlockingPrompt function contains a
redundant keyword check. Remove the `lower.contains("permission_prompt")`
condition since any string containing "permission_prompt" will already be caught
by the existing `lower.contains("permission")` check on the previous line,
making the additional check unnecessary.
In `@Sources/RestorableAgentSession.swift`:
- Around line 1281-1283: In the cmuxDebugLog call for the
agentHib.restorable.claudeTranscriptMissing message, redact the sensitive
transcript path information from record.transcriptPath. Instead of logging the
actual path value which may contain the user's home directory or account config
details, replace the transcriptPath parameter with a non-sensitive indicator
such as whether the path is nil or simply a generic placeholder that preserves
the log's usefulness without exposing sensitive file system information. Keep
the rest of the log message structure intact including the session ID prefix and
the isRestorable indicator.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 26198-26208: The completion branch within the
containsCompletionCue condition currently passes isBlocking: blocking to the
AgentHookNotificationSummary initializer, but other completion paths in the
summarizeAgentHookNotification function hardcode isBlocking: false. Change the
isBlocking parameter in the AgentHookNotificationSummary initialization within
the containsCompletionCue block from isBlocking: blocking to isBlocking: false
to maintain consistency with the sibling completion paths and clarify that
completion notifications should not be blocking regardless of the computed
blocking value.
🪄 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: 701dc52f-db6f-4e2a-acd3-99d9d05e6de0
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
CLI/cmux.swiftSources/App/AgentHibernationController.swiftSources/RestorableAgentSession.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentNotificationBlockingClassifierTests.swift
Comment on lines
+1281
to
+1283
| cmuxDebugLog( | ||
| "agentHib.restorable.claudeTranscriptMissing session=\(record.sessionId.prefix(8)) " | ||
| + "transcriptPath=\(record.transcriptPath ?? "<nil>") — trusting isRestorable=true" |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Redact the transcript path from the debug log.
record.transcriptPath can include the user’s home/account config root and encoded project names; keep the breadcrumb but log only non-sensitive metadata.
🛡️ Proposed fix
if record.isRestorable == true {
+ let hasTranscriptPath = normalizedNonEmptyValue(record.transcriptPath) != nil
+ let transcriptPathBytes = record.transcriptPath?.utf8.count ?? 0
cmuxDebugLog(
"agentHib.restorable.claudeTranscriptMissing session=\(record.sessionId.prefix(8)) "
- + "transcriptPath=\(record.transcriptPath ?? "<nil>") — trusting isRestorable=true"
+ + "hasTranscriptPath=\(hasTranscriptPath ? 1 : 0) "
+ + "transcriptPathBytes=\(transcriptPathBytes) — trusting isRestorable=true"
)
return true
}🤖 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 `@Sources/RestorableAgentSession.swift` around lines 1281 - 1283, In the
cmuxDebugLog call for the agentHib.restorable.claudeTranscriptMissing message,
redact the sensitive transcript path information from record.transcriptPath.
Instead of logging the actual path value which may contain the user's home
directory or account config details, replace the transcriptPath parameter with a
non-sensitive indicator such as whether the path is nil or simply a generic
placeholder that preserves the log's usefulness without exposing sensitive file
system information. Keep the rest of the log message structure intact including
the session ID prefix and the isRestorable indicator.
Source: Coding guidelines
Contributor
Greptile SummaryThis PR fixes the "only codex hibernates" bug by correcting the notification lifecycle for all 16 supported coding-agent types. A routine "waiting for input" reminder after every turn was classified as
Confidence Score: 5/5The change is safe to merge: the shared blocking classifier is deliberately conservative (false positives keep panes live, never kill them), the fail-closed behavior of AgentBackgroundWorkStatus protects live background tasks from accidental SIGTERM, and the copilot/codebuddy/factory re-routing is strictly safer than the prior stop-lane routing. All lifecycle mutations are gated behind isBlockingPrompt, the authoritative Stop hook remains the sole idle source, and the fix applies symmetrically to both the live in-memory state and the durable hook-store record. The new test suites are thorough and the CI isolation for AgentHibernationPlannerTests is correctly reasoned. Previous thread concerns have been acknowledged or addressed in the design notes; no new correctness or safety gaps were found. No files require special attention. CLI/cmux.swift has a minor redundancy (multiple AgentNotification instantiations with the same inputs across classifyClaudeNotification and summarizeClaudeHookNotification) but this is a pure struct with no side effects and is not a correctness issue. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Agent Hook Event] --> B{Event type?}
B -->|Stop| C[hasActiveClaudeBackgroundWork?]
B -->|Notification| D[summarize notification]
B -->|SessionStart/End| E[other lanes]
C -->|yes| F[lifecycle = .running]
C -->|no| G[lifecycle = .idle]
D --> H[AgentNotification.isBlockingPrompt]
H -->|true - permission/approval/error/?| I[lifecycle = .needsInput]
H -->|false - routine waiting reminder| J[lifecycle = nil — preserve Stop's .idle]
I --> K[setAgentLifecycle + upsert durable store]
J --> L[skip lifecycle mutation\nuser-facing bell/status unchanged]
G --> M[AgentHibernationPlanner evaluation ~30s]
K --> N[pane stays live]
L --> M
M --> O{lifecycle == .idle\nAND restorable\nAND outside idle window\nAND not protected?}
O -->|yes - excess over cap| P[SIGTERM process group\nhibernate pane]
O -->|no| Q[pane remains live]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[Agent Hook Event] --> B{Event type?}
B -->|Stop| C[hasActiveClaudeBackgroundWork?]
B -->|Notification| D[summarize notification]
B -->|SessionStart/End| E[other lanes]
C -->|yes| F[lifecycle = .running]
C -->|no| G[lifecycle = .idle]
D --> H[AgentNotification.isBlockingPrompt]
H -->|true - permission/approval/error/?| I[lifecycle = .needsInput]
H -->|false - routine waiting reminder| J[lifecycle = nil — preserve Stop's .idle]
I --> K[setAgentLifecycle + upsert durable store]
J --> L[skip lifecycle mutation\nuser-facing bell/status unchanged]
G --> M[AgentHibernationPlanner evaluation ~30s]
K --> N[pane stays live]
L --> M
M --> O{lifecycle == .idle\nAND restorable\nAND outside idle window\nAND not protected?}
O -->|yes - excess over cap| P[SIGTERM process group\nhibernate pane]
O -->|no| Q[pane remains live]
Reviews (20): Last reviewed commit: "perf: gate the per-record hibernation re..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
94
to
100
| return lower.contains("permission") || lower.contains("approve") || lower.contains("approval") | ||
| || lower.contains("permission_prompt") | ||
| || lower.contains("error") || lower.contains("failed") || lower.contains("failure") | ||
| || lower.contains("exception") | ||
| } | ||
|
|
||
| #if DEBUG |
Contributor
There was a problem hiding this comment.
agentNotificationIsBlockingPrompt is a file-scope free function called by both the Claude hook lane and the generic agent-hook lane, and imported directly by the test suite via @testable import. The cmux no-ambient-global-state rule flags new top-level funcs used as API — the correct shape is either a private/fileprivate file-scope helper (passing to the rule) or a static func on a named classifier type (e.g., AgentNotificationClassifier) that owns the signal set and can be tested at the type boundary without widening the whole-module surface.
Rule Used: Flag new ambient global state in production Swift:... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Comment on lines
97
to
99
| || lower.contains("exception") | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
"error" match risks false-positives that keep panes live when they should hibernate
lower.contains("error") will match benign informational messages an agent might send — e.g. "0 errors found", "error handling complete", "no error occurred" — and classify them as isBlocking: true. For agents (grok, antigravity) that use completion notifications as turn boundaries, a final "task finished with 0 errors" message would cause setAgentLifecycle(.needsInput) to fire after the Stop hook already wrote .idle, blocking hibernation. Tighter matching — anchoring on structured signal values or word-boundary matching for "error" — would eliminate the false positives without losing the real blocking signal.
Comment on lines
+1281
to
+1284
| cmuxDebugLog( | ||
| "agentHib.restorable.claudeTranscriptMissing session=\(record.sessionId.prefix(8)) " | ||
| + "transcriptPath=\(record.transcriptPath ?? "<nil>") — trusting isRestorable=true" | ||
| ) |
Contributor
There was a problem hiding this comment.
transcriptPath in debug log may include macOS username
record.transcriptPath expands to an absolute path under /Users/<username>/…. Logging it raw through cmuxDebugLog risks leaking a personally identifiable filesystem path to the unified log where it can be captured by Console.app or crash reporters. The session ID prefix already provides enough correlation signal; transcriptPath could be replaced with a redacted form (e.g., the last 2 path components) or a component count.
Rule Used: Flag production Swift diagnostics that bypass unif... (source)
Fixes two build regressions caught by review: 1. Release build: cmuxDebugLog is declared only under #if DEBUG, but the new observability calls in AgentHibernationController and RestorableAgentSession were unconditional, so a Release archive could not resolve the symbol. Both call sites are now #if DEBUG guarded (the whole observability block in the controller, the breadcrumb in the restorability path). 2. Test target: agentNotificationIsBlockingPrompt and CMUXCLI.agentDef live in the CLI executable target, which cmuxTests does not link (documented in RemotesClientTests). The unit test referencing them could not compile. Moved the pure classifier into the CMUXAgentLaunch SwiftPM package as a public function and relocated its test to that package's test target, where it runs via without launching the app (verified: 3/3 pass). The CLI now calls the package function. Removed the cmuxTests test file + its pbxproj wiring. The routing test (copilot/codebuddy/factory Notification mapping) is dropped: it referenced CLI-target agentDefs that no available test target links; the reroute stays covered by the classifier test and PR description. Refreshes RestorableAgentSession length budget. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses review policy: package API must be owned by a type (not a top-level free function), and new non-UI package tests use Swift Testing. Wraps the classifier as AgentNotificationClassifier.isBlockingPrompt and updates CLI call sites; converts the test suite to import Testing / @suite / @test / #expect, matching the 24 other Swift Testing suites in CMUXAgentLaunch. Verified: 3/3 pass via swift test. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ns-lint) The all-static enum AgentNotificationClassifier tripped the repo namespace-type rule (no all-static public types in packages). Remodel as an instantiated value: AgentNotification(signal:message:).isBlockingPrompt, which satisfies both that lint and the review's 'owned by a type' rule. Updates CLI call sites and the Swift Testing suite. Verified locally: package tests 3/3 pass and lint-ios-package-conventions.sh exits 0 with no AgentNotification findings. Also clears the cascading ios-tests routing gate, which only failed because the lint did. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…able fallback Review flagged that trusting isRestorable==true with no transcript proof could hibernate a claude session that then fails to resume. Tighten the fallback to fail closed unless the hook actually recorded a transcript path: the legitimate lookup-miss cases (custom CLAUDE_CONFIG_DIR, forked roots, encoding drift) all still record a path that just isn't resolvable from the gate, while a pathless record never proved a transcript and is no longer trusted. Bounds the residual to 'path recorded but file later deleted', where claude --resume exits cleanly to a shell. (The companion finding about confirm/continue prompts hibernating is a misread of the model: a non-blocking notification never forces .idle, it only skips asserting .needsInput; the Stop hook is the sole idle source, so a mid-turn prompt with no Stop stays non-hibernatable.) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Concede the review concern: trusting sticky isRestorable when no transcript resolves risks SIGTERMing a live claude pane and then failing claude --resume into a dead shell. Revert to the original fail-closed gate (restorable only when a transcript file is actually found) and instead emit a DEBUG diagnostic when a record claims isRestorable but no transcript resolved. This makes hard-to-locate-transcript never-hibernation observable rather than guessed at; the evidence-driven fix, if it ever reproduces, is to resolve those roots in the transcript lookup, not to fail open. The systemic 'only codex hibernates' cause is the notification-lifecycle clobber fixed elsewhere in this change, which does not need this gate relaxed. Restorability behavior is now identical to before plus the diagnostic. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
changed the title
Adds AgentHibernationPlannerTests (cmuxTests, app-host) exercising the pure selection logic AgentHibernationPlanner.selectedPanelKeys: idle restorable agents over the cap are hibernated oldest-first; needs-input / running / unknown / protected / unconfirmed-input / within-idle-window agents are never hibernated; non-live and non-restorable are ignored; disabled hibernates nothing. The needs-input exclusion is the payoff of the notification-lifecycle fix (a finished agent reaches .idle here instead of being clobbered to .needsInput). Also pins that every supported coding-agent type (amp, antigravity, claude_code, codebuddy, codex, copilot, cursor, factory, gemini, grok, hermes-agent, kiro, opencode, pi, qoder, rovodev) is an allowed lifecycle status key, so an agent added without wiring its hibernation lifecycle fails the suite. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
Sources/RestorableAgentSession.swift (1)
1287-1291: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winRedact transcript path in DEBUG diagnostics.
At Line 1290, the log emits
record.transcriptPathverbatim, which can expose user/home/project-identifying filesystem data. Keep only non-sensitive metadata (presence + size).As per coding guidelines, "Do not log secrets, tokens, passwords, private keys, customer content, or personal data without explicit private redaction."
Proposed fix
`#if` DEBUG if record.isRestorable == true { + let hasTranscriptPath = normalizedNonEmptyValue(record.transcriptPath) != nil + let transcriptPathBytes = record.transcriptPath?.utf8.count ?? 0 cmuxDebugLog( "agentHib.restorable.claudeTranscriptUnresolved session=\(record.sessionId.prefix(8)) " - + "transcriptPath=\(record.transcriptPath ?? "<nil>") — failing closed (no transcript found)" + + "hasTranscriptPath=\(hasTranscriptPath ? 1 : 0) " + + "transcriptPathBytes=\(transcriptPathBytes) — failing closed (no transcript found)" ) } `#endif`🤖 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 `@Sources/RestorableAgentSession.swift` around lines 1287 - 1291, The cmuxDebugLog call in the isRestorable block directly exposes record.transcriptPath, which contains sensitive filesystem data including user/home/project paths. Replace the transcriptPath parameter in the log message with redacted metadata that indicates only the presence and size of the transcript file, rather than the full path string. This ensures the diagnostic log complies with the guideline against logging personal or customer data without explicit redaction.Source: Coding guidelines
🤖 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.
Duplicate comments:
In `@Sources/RestorableAgentSession.swift`:
- Around line 1287-1291: The cmuxDebugLog call in the isRestorable block
directly exposes record.transcriptPath, which contains sensitive filesystem data
including user/home/project paths. Replace the transcriptPath parameter in the
log message with redacted metadata that indicates only the presence and size of
the transcript file, rather than the full path string. This ensures the
diagnostic log complies with the guideline against logging personal or customer
data without explicit redaction.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6eca9a7d-4a54-43d2-828b-6f6bd4873a4f
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
CLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentNotificationBlockingClassifier.swiftPackages/macOS/CMUXAgentLaunch/Tests/CMUXAgentLaunchTests/AgentNotificationBlockingClassifierTests.swiftSources/App/AgentHibernationController.swiftSources/RestorableAgentSession.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentHibernationPlannerTests.swift
…agents # Conflicts: # .github/swift-file-length-budget.tsv
…cused gate
The provisional-navigation-race suite runs real WebProcesses plus a local
race server. Under a shared shard it can exhaust WebKit and get its
WebProcess killed ('WebProcess does not exist'), aborting the app-host and
cascading to later suites in that shard. Its shard membership is also
sharding-order-dependent, so adding unrelated test files (e.g. a new
hibernation test class) reshuffles it into a shard where it crashes —
which is what turned app-host 2/4 red after merging main into this branch.
Move it into FOCUSED_GATE_SELECTORS so it is excluded from the four shared
shards and runs isolated in its own app-host (like the system-proxy-mirror
and Option/Alt gates), where WebProcess pressure and sharding order can't
affect it.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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 @.github/workflows/ci.yml:
- Around line 527-546: The browser session history restore step duplicates the
same focused-gate app-host/xcodebuild boilerplate as the other focused-gate
jobs; extract the shared `run-in-console-session.sh` and
`run-app-host-xcodebuild.sh` invocation into a reusable helper (script or
composite action) and parameterize it with the suite description and
`-only-testing` selector. Update this job to call the new helper so the unique
`BrowserSessionHistoryRestoreTests` selector is the only job-specific part,
matching the existing focused-gate pattern used by the other steps.
🪄 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: e9915bb8-9fd1-44f6-8b1b-56546a140e82
📒 Files selected for processing (2)
.github/workflows/ci.ymlscripts/ci/cmux_unit_test_shard.py
Comment on lines
+527
to
+546
| - name: Run browser session history restore regression | ||
| if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }} | ||
| run: | | ||
| # Heavy WKWebView provisional-navigation-race suite, isolated in its own | ||
| # app-host so WebProcess exhaustion can't cascade into a shared shard and | ||
| # so its pass/fail is independent of sharding order. See the note in | ||
| # scripts/ci/cmux_unit_test_shard.py FOCUSED_GATE_SELECTORS. | ||
| set -euo pipefail | ||
| SOURCE_PACKAGES_DIR="$PWD/.ci-source-packages" | ||
| scripts/ci/run-in-console-session.sh \ | ||
| scripts/ci/run-app-host-xcodebuild.sh \ | ||
| -project cmux.xcodeproj -scheme cmux-unit -configuration Debug \ | ||
| -derivedDataPath "$CMUX_DERIVED_DATA_PATH" \ | ||
| -clonedSourcePackagesDirPath "$SOURCE_PACKAGES_DIR" \ | ||
| -disableAutomaticPackageResolution \ | ||
| -destination "platform=macOS" \ | ||
| CMUX_SKIP_ZIG_BUILD=1 \ | ||
| -only-testing:cmuxTests/BrowserSessionHistoryRestoreTests \ | ||
| test | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
Third copy of near-identical focused-gate boilerplate.
This step duplicates the structure of the two preceding focused-gate steps (browser system proxy mirror, Option/Alt sided-modifier) almost verbatim — only the comment and -only-testing selector differ. Consider extracting the common run-in-console-session.sh + run-app-host-xcodebuild.sh invocation into a small reusable script/composite action parameterized by selector and description, to avoid a fourth copy-paste next time a suite needs isolating.
🤖 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 @.github/workflows/ci.yml around lines 527 - 546, The browser session history
restore step duplicates the same focused-gate app-host/xcodebuild boilerplate as
the other focused-gate jobs; extract the shared `run-in-console-session.sh` and
`run-app-host-xcodebuild.sh` invocation into a reusable helper (script or
composite action) and parameterize it with the suite description and
`-only-testing` selector. Update this job to call the new helper so the unique
`BrowserSessionHistoryRestoreTests` selector is the only job-specific part,
matching the existing focused-gate pattern used by the other steps.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 11 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentNotificationBlockingClassifier.swift">
<violation number="1" location="Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentNotificationBlockingClassifier.swift:33">
P2: Loose substring matching for `error`, `failed`, `failure`, `exception` risks false-positive blocking on routine non-blocking notifications, directly undermining the classifier's purpose.</violation>
</file>
<file name=".github/workflows/ci.yml">
<violation number="1" location=".github/workflows/ci.yml:527">
P3: This is the third near-identical focused-gate step (after `BrowserSystemProxyMirrorTests` and `GhosttyOptionAsAltModsTests`). The only differences between these blocks are the comment and the `-only-testing:` selector. Consider extracting the common `run-in-console-session.sh` + `run-app-host-xcodebuild.sh` invocation into a small reusable composite action or parameterized script to reduce copy-paste maintenance burden when the next isolated suite is added.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
| let lower = "\(signal) \(message)".lowercased() | ||
| return lower.contains("permission") || lower.contains("approve") || lower.contains("approval") | ||
| || lower.contains("permission_prompt") | ||
| || lower.contains("error") || lower.contains("failed") || lower.contains("failure") |
There was a problem hiding this comment.
P2: Loose substring matching for error, failed, failure, exception risks false-positive blocking on routine non-blocking notifications, directly undermining the classifier's purpose.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Packages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/AgentNotificationBlockingClassifier.swift, line 33:
<comment>Loose substring matching for `error`, `failed`, `failure`, `exception` risks false-positive blocking on routine non-blocking notifications, directly undermining the classifier's purpose.</comment>
<file context>
@@ -0,0 +1,36 @@
+ let lower = "\(signal) \(message)".lowercased()
+ return lower.contains("permission") || lower.contains("approve") || lower.contains("approval")
+ || lower.contains("permission_prompt")
+ || lower.contains("error") || lower.contains("failed") || lower.contains("failure")
+ || lower.contains("exception")
+ }
</file context>
| -only-testing:cmuxTests/GhosttyOptionAsAltModsTests \ | ||
| test | ||
|
|
||
| - name: Run browser session history restore regression |
There was a problem hiding this comment.
P3: This is the third near-identical focused-gate step (after BrowserSystemProxyMirrorTests and GhosttyOptionAsAltModsTests). The only differences between these blocks are the comment and the -only-testing: selector. Consider extracting the common run-in-console-session.sh + run-app-host-xcodebuild.sh invocation into a small reusable composite action or parameterized script to reduce copy-paste maintenance burden when the next isolated suite is added.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/ci.yml, line 527:
<comment>This is the third near-identical focused-gate step (after `BrowserSystemProxyMirrorTests` and `GhosttyOptionAsAltModsTests`). The only differences between these blocks are the comment and the `-only-testing:` selector. Consider extracting the common `run-in-console-session.sh` + `run-app-host-xcodebuild.sh` invocation into a small reusable composite action or parameterized script to reduce copy-paste maintenance burden when the next isolated suite is added.</comment>
<file context>
@@ -524,6 +524,26 @@ jobs:
-only-testing:cmuxTests/GhosttyOptionAsAltModsTests \
test
+ - name: Run browser session history restore regression
+ if: ${{ matrix.shard == fromJSON(env.CMUX_APP_HOST_FOCUSED_REGRESSION_SHARD) }}
+ run: |
</file context>
…on == main Adding this new suite reshuffled the greedy unit-test shard packing, which moved a heavy order/load-sensitive WKWebView suite into a shard where it exhausted WebKit (WebProcess killed) and aborted the app-host — turning an app-host shard red purely from reshuffling, not from any real regression. Isolating one WebKit suite just moved the reshuffle to another shard. Since this PR's only new cmuxTests class is AgentHibernationPlannerTests (pure, fast planner-selection tests), exclude IT from the four shared shards instead. That leaves the shard composition byte-identical to main (where app-host is green) and runs the suite isolated in its own app-host, like the other focused gates. Reverts the earlier browser-suite gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Autoreview: the isBlocking gate only matched permission/approval/error, so a
genuine user prompt surfaced only through a Notification hook could hit the
notification lane with a non-blocking classification, skip
setAgentLifecycle(.needsInput), and leave the Stop hook's `.idle` — letting the
pane hibernate (and get SIGTERMed) while an agent waited for the user's answer.
Detect a genuine prompt from a high-confidence signal: a direct interrogative
(the message contains `?`), or the literal `question` token paired with an
interaction word. Wire it in two places that were the actual gap:
- CLI classifier `containsWaitingCue` now classifies an interrogative as
`.needsInput`, so the generic lane (which requires status == .needsInput AND
isBlocking) reaches the lifecycle write.
- `AgentNotification.isBlockingPrompt` treats the same cue as blocking, so both
the claude lane (lifecycle = isBlocking ? .needsInput : nil) and the generic
lane flip for real questions.
Deliberately NOT keyed on bare verbs (confirm/choose/continue/proceed): those
collide with routine agent chatter ("continue when ready") and blocking on them
would clobber `.idle` so the pane never hibernates — the bug this lane prevents.
Routine idle/waiting reminders carry no `?`, so they stay non-blocking. A
punctuation-free imperative ("Choose an option") is intentionally left to the
authoritative structured signal (notification_type), not this free-text heuristic.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lawrencecchen
force-pushed
the
feat-hibernation-all-agents
branch
from
July 1, 2026 05:46
51d6a72 to
565e197
Compare
cmux-policy-check follow-up on the hibernation classifier/background-work types: - Add Swift-DocC `///` docs to the public memberwise initializers. - Move the pure helpers (arrayOfObjects, terminalStatuses, containsGenuineQuestionCue) off the value types to file-scope private funcs/lets, so the types carry only instance members (no static-as-namespace surface). Public API unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review follow-up: the Stop lane recorded lifecycle `.running` when background tasks were still alive but painted the status pill "Idle" (pause icon), so the user-visible state contradicted the hibernation state. Show "Running" (bolt.fill, #4C8DFF) in that case, matching the antigravity background-work lane's display. Existing localized key, no new strings. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… closed Autoreview round-5 findings, both verified real: P1: classifyAgentHookNotification checks completion cues before waiting cues, so "Task completed. Continue?" classified status=.idle with isBlocking=true, and the generic notification lane's persisted path (agentLifecycle(for: .idle)) durably wrote .idle while the agent waited on a user answer — hibernatable mid-question. Guard the completion branch with !blocking so blocking text falls through to the waiting branch (.needsInput); persisted and live paths then both record .needsInput. Same guard on the claude display classifier for subtitle consistency. Side effect (correct): grok/antigravity no longer treat "completed + question" as a turn boundary. P2: AgentBackgroundWorkStatus treated a present-but-unreadable background_tasks value (schema drift to a keyed object, an array of non-objects) as "no work", failing open to hibernation's group SIGTERM. Parse now distinguishes ABSENT (older clients — inactive, they must keep hibernating) from PRESENT-but- unreadable (fail closed as one active task; unreadable evidence of work must keep the pane alive, never authorize the kill). Verified against the built binary: drifted payload -> running; completed+question notification -> needsInput; all prior lifecycle scenarios unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 157ef0a. Configure here.
cmux-policy: one major type per file. AgentHookFieldParse was a private parsing helper, not a second domain type; replace it with a file-scope private function returning (objects, unreadableCount). Behavior identical. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…agents PR #7129 landed the same claude Stop-lane background-work gate this branch carries, plus notification category gating. Resolution unifies the two: - One background-work parser: hasActiveClaudeBackgroundWork now delegates to AgentBackgroundWorkStatus(hookObject:) (unit-tested, fail-closed on present-but-unreadable payloads; non-terminal statuses count as live). Main's inline predicate (exact "running" only, fails open on malformed) is replaced. Main's tests only pin running/empty/cron/absent, all compatible. - Stop lane: main's shape kept (hasPendingBackgroundWork, localized Running/Idle pill, hadPendingBackgroundWorkAtStop cache). - Notification lane: union of main's category gating (notifyCategory, suppressNeedsInputState) and this branch's blocking gate — lifecycle flips to .needsInput only for genuinely blocking prompts (summary.isBlocking && !suppressNeedsInputState). Main's unconditional .needsInput write would have reintroduced the routine-reminder clobber this PR fixes. - Generic lane: persisted lifecycle composes suppressPendingWaitingState (.running) > isBlocking (.needsInput) > completion (.idle) > preserve. - AgentHookNotificationSummary carries both isBlocking and notifyCategory. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…agents # Conflicts: # .github/swift-file-length-budget.tsv # cmux.xcodeproj/project.pbxproj
The `agentHib.restorable.claudeTranscriptUnresolved` diagnostic I added fired once per claude record on every ~30s planner reload over the whole session store. Stale cross-worktree records (deleted worktree, benign) routinely fail closed there, so it flooded the debug log (~1700 lines/min observed under a multi-agent dogfood) and added file-write churn on the reload's background threads. It is `#if DEBUG` only (never ships), but it degrades the dogfood build. Gate it behind CMUX_DEBUG_HIBERNATION_VERBOSE (off by default, read once via a thread-safe global initializer) so the diagnostic stays available when investigating a real never-hibernation report without the default storm. The ~30s uncached full-index reload itself is pre-existing (Add Agent Hibernation #4165, not this PR); filing that as a separate perf follow-up. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
9 tasks
lawrencecchen
added
the
stale-revisit
This branch was successfully deployed
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
2 participants
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.
Problem
Mapping the hibernation path for every coding-agent kind (claude, codex, grok, gemini, opencode, cursor, kiro, antigravity, rovodev, copilot, codebuddy, factory, qoder, hermes-agent, pi, amp, custom vault agents) surfaced why non-codex agents silently never hibernated.
Notification clobber was general, not claude-specific
A routine "waiting for input" reminder classifies as needs-input and flipped the hibernation lifecycle to
.needsInput, clobbering the.idlethe Stop hook recorded, so the pane never became hibernation-eligible. This existed in both the dedicated Claude hook lane and the generic agent-hook lane (grok, antigravity, and custom agents), in both the live in-memory state and the durable hook-store record used by the index fallback. This is the systemic cause of "only codex hibernates."Three agents could hibernate mid permission-prompt
copilot, codebuddy, and factory carried their turn boundary on
Stopbut routedNotification(their permission/attention channel) to thestopsubcommand, forcing the pane.idlewhile a live permission prompt was on screen.Fix
AgentNotification(signal:message:).isBlockingPrompt(permission/approval/error), drives the lifecycle decision in both lanes. A notification only asserts.needsInputwhen genuinely blocking; otherwise it leaves the lifecycle untouched (the Stop hook is the single authoritative idle source). This applies to the live mutation and the durable persisted lifecycle, so a routine reminder can no longer clobber stored.idleon the index fallback (after restart / no live state). The user-facing notification, bell, and sidebar status are unchanged.Notificationroutes through thenotificationlane instead ofstop. Worst case after the change is no-hibernation (status quo for many agents), strictly safer than hibernating mid permission-prompt.Stopremains the turn boundary.AgentHibernationControllerlogs oneagentHib.evaluateline per cycle when restorable records exist (restorable/live/maxLive/idle/protected/unconfirmedInput/selected), and the claude restorability gate logs when a record claimsisRestorablebut no transcript resolved — so a non-hibernating fleet is never silent again. Both are DEBUG-only.Claude restorability is unchanged behaviorally: it remains fail-closed (restorable only when a transcript file is actually found), now with the diagnostic above.
Tests
AgentNotification(CMUXAgentLaunch package,swift test): the shared blocking classifier — permission/approval/error → blocking; waiting/completion/attention/empty → not blocking; signal field considered.AgentHibernationPlannerTests(cmuxTests, app-host): the pure hibernate-selection logicAgentHibernationPlanner.selectedPanelKeys— idle restorable agents over the cap are hibernated oldest-first; needs-input / running / unknown / protected / unconfirmed-input / within-idle-window agents are never hibernated; non-live and non-restorable are ignored; disabled hibernates nothing. Also pins that all 16 supported agent types are allowed lifecycle status keys.Verified on the built app (tag, deterministic hook drive)
Drove
cmux hooks <agent> <event>against the running tagged build and read the persisted lifecycle back, for all 16 agent types (claude, codex, copilot, codebuddy, factory, grok, gemini, cursor, qoder, rovodev, pi, amp, hermes-agent, antigravity, kiro, opencode):stop → idle, a routine "waiting for input" notification keepslifecycle = idle(display status still shows needs-input), and a permission notification flips toneedsInput. A/B contrast against a build without the fix showed the routine notification clobberingidle → needsInput, confirming the gate is what preserves idle. Restorability fail-closed was checked against the live hook store: restorable records with an existing transcript are hibernatable, transcript-less ones are rejected.Design notes (addressed review concerns)
A non-blocking notification never forces
.idle. The lifecycle has one authoritative idle source: the Stop hook. A notification only asserts.needsInputwhen blocking; when non-blocking it skips the live mutation and persistsnil(preserve). A question asked in claude's response means the turn ended (Stop fired → idle); hibernating that idle, restorable session is correct, since the question is in the transcript and resume reconstructs it. A genuinely synchronous permission prompt fires a keyword-matched Notification →needsInput→ not hibernated. The classifier is deliberately narrow to permission/approval/error because widening it to "waiting/confirm/continue/request" would re-clobber.idle(claude's own idle reminder is "Claude is waiting for your input", and completion text contains "continue"/"request") and reintroduce the never-hibernate bug this PR fixes.Claude restorability stays fail-closed. An earlier revision trusted
isRestorable == truewhen no transcript resolved, to cover hard-to-locate-transcript cases (customCLAUDE_CONFIG_DIR, forked roots, encoding drift). That was reverted: sticky hook state is not proof the conversation can replay, and a stale/deleted path would turn a live pane into a failedclaude --resume. The gate logs the unresolved case instead, so if that never-hibernation is ever real it is observable, and the evidence-driven fix is to resolve those roots in the lookup rather than fail open.Per-agent status
on_error → stopis correct; after an error the agent has genuinely stopped and is idle, and the session is restorable.Notificationevent, onlyStop → stop; no mid-prompt risk.Relationship to #6694
This subsumes #6694 (the claude-only notification fix) by generalizing it to a shared classifier across all agent lanes. If this lands, #6694 can be closed.
🤖 Generated with Claude Code
Note
High Risk
Changes hibernation eligibility and persisted lifecycle for all agent hook lanes; incorrect classification could SIGTERM live work or leave panes never hibernating, and unreadable Claude background-task payloads now fail closed to stay alive.
Overview
Fixes the systemic “only Codex hibernates” behavior: routine post-turn “waiting for input” notifications were flipping the hibernation lifecycle to
.needsInput, overwriting the Stop hook’s.idlein both the live state and the durable hook store.Shared blocking classifier — Adds
AgentNotification.isBlockingPromptin CMUXAgentLaunch and wires it through the Claude and generic agent-hook notification paths. Only permission/approval/error (and high-confidence question cues) may assert.needsInput; routine idle reminders leave lifecycle unchanged while user-facing bell/status behavior stays the same. Completion classification is skipped when text is still blocking (e.g. “Task completed. Continue?”).Hook routing — Copilot, CodeBuddy, and Factory map
Notificationto thenotificationsubcommand instead ofstop, so permission prompts are not forced idle mid-prompt.Claude background work — Replaces inline
background_tasks/session_cronschecks withAgentBackgroundWorkStatus, treating non-terminal or unreadable payload evidence as active so hibernation does not SIGTERM silent background tasks.Diagnostics & tests — DEBUG
agentHib.evaluatelogging inAgentHibernationController; optional verbose logging when Claude claims restorable but no transcript resolves (still fail-closed). New package and app-host tests for the classifier, background-work parser, andAgentHibernationPlannerselection; CI runs planner tests in an isolated focused gate.Reviewed by Cursor Bugbot for commit 2ed1744. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit