Repository navigation
Conversation
…atus-after-interrupt
…atus-after-interrupt
…atus-after-interrupt
…atus-after-interrupt # Conflicts: # .github/swift-file-length-budget.tsv # CLI/FeedEventClassifier.swift # CLI/cmux.swift # Resources/Localizable.xcstrings # Resources/bin/cmux-claude-wrapper # cmuxTests/FeedEventClassificationTests.swift # tests/test_bash_integration_no_done_notifications.py # tests/test_claude_wrapper_hooks.py
Extend AgentHibernationLifecycleState with waiting, completed, and failed; wire generic and Claude hook stop/notification paths to emit the new pills (Completed green, Waiting orange, Failed red) and update regression tests.
|
Someone is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR expands lifecycle state handling with ChangesAgent lifecycle status
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Hook
participant Monitor
participant SessionStore
participant Terminal
Hook->>Monitor: report completion, waiting, failure, or stop-failure
Monitor->>SessionStore: persist runtime status and lifecycle
Monitor->>Terminal: publish localized status command
Possibly related issues
Possibly related PRs
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 passed)
✨ Finishing Touches🧪 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 adds explicit
Confidence Score: 4/5Safe to merge with one minor fix: a dead code branch in agentHookRuntimeStatus that returns .idle instead of .completed, inconsistent with the generic handler updated in the same PR. The lifecycle model change is coherent and well-tested. The hasRunningSession extension to include needsInput sessions when a surface is specified correctly prevents Completed from overwriting Waiting. The only gap is the new private agentHookRuntimeStatus function whose .idle branch is unreachable but would produce wrong session-store data if ever reached. CLI/cmux.swift — agentHookRuntimeStatus(for:) .idle case. Also cmuxTests/WorkspaceRemoteConnectionTests.swift where a Completed + pause.circle.fill icon conjunction was noted in a prior review thread. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Claude hook fires] --> B{subcommand?}
B -->|stop| C{hasPendingBackgroundWork?}
B -->|stop-failure| D[lifecycleAfterTurn = .idle / Gray pill]
B -->|notification| E{notificationStatus?}
C -->|yes| F[lifecycleAfterTurn = .running]
C -->|no| G[lifecycleAfterTurn = .completed / Green pill]
E -->|.idle| H[lifecycle = .completed]
E -->|.needsInput| I[lifecycle = .waiting / Orange]
E -->|.error| J[lifecycle = .failed / Red]
K[Generic agent hook] --> L{stopNotificationStatus?}
L -->|.idle| M[.completed]
L -->|.needsInput| N[.waiting]
L -->|.error| O[.failed]
P[Workspace aggregation priority] --> Q[running > failed > waiting > needsInput > completed > unknown > idle]
%%{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[Claude hook fires] --> B{subcommand?}
B -->|stop| C{hasPendingBackgroundWork?}
B -->|stop-failure| D[lifecycleAfterTurn = .idle / Gray pill]
B -->|notification| E{notificationStatus?}
C -->|yes| F[lifecycleAfterTurn = .running]
C -->|no| G[lifecycleAfterTurn = .completed / Green pill]
E -->|.idle| H[lifecycle = .completed]
E -->|.needsInput| I[lifecycle = .waiting / Orange]
E -->|.error| J[lifecycle = .failed / Red]
K[Generic agent hook] --> L{stopNotificationStatus?}
L -->|.idle| M[.completed]
L -->|.needsInput| N[.waiting]
L -->|.error| O[.failed]
P[Workspace aggregation priority] --> Q[running > failed > waiting > needsInput > completed > unknown > idle]
Reviews (2): Last reviewed commit: "merge: integrate PR #4390 reliability fi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -24225,9 +24227,9 @@ struct CMUXCLI { | |||
| client: client, | |||
Contributor
There was a problem hiding this comment.
agent.generic.status.completed
The key "agent.generic.status.completed" used here does not exist in Resources/Localizable.xcstrings. For non-English locales (at minimum Japanese is supported, with key "agent.generic.notification.subtitle.completed" → "完了"), String(localized:defaultValue:) will silently fall back to the English "Completed" string. There is already a translated key "agent.generic.notification.subtitle.completed" with the same English/Japanese values that could be reused, or a new "agent.generic.status.completed" entry with translations needs to be added to the catalog.
Rule Used: Flag production user-facing text that is not fully... (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!
…lish Cherry-pick upstream interrupt/Codex-monitor work (stop-failure hook, recordCodexMonitorIdleIfCurrent, surface-scoped running checks). Fix stop-failure to clear Running to Idle instead of Completed.
|
Deployment failed with the following error: |
AsifAd
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
CLI/cmux.swift (1)
31752-31767: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winWaiting pill color is inconsistent with the new orange standard.
This
.needsInput?notification branch now drivesagentLifecycle = .waiting, but the pill still uses--color=#4C8DFF`` (blue). Every other waiting/needs-input pill in this PR moved to orange#FF9500(Claude notify Line 24549, prompt-submit restore Line 30810, stop-waiting Line 31396). Per the PR objective ("waiting notifications display orange Waiting"), this path is now the outlier.🎨 Align the waiting pill color
- "set_status \(def.statusKey) \(statusValue) --icon=bell.fill --color=`#4C8DFF` --priority=100 --tab=\(workspaceId)\(socketPanelOption(surfaceId))", + "set_status \(def.statusKey) \(statusValue) --icon=bell.fill --color=`#FF9500` --priority=100 --tab=\(workspaceId)\(socketPanelOption(surfaceId))",🤖 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 31752 - 31767, Update the `.needsInput?` branch’s `sendV1Command` status color from blue `#4C8DFF` to the standard waiting orange `#FF9500`, while preserving its existing lifecycle, icon, priority, and tab parameters.cmuxTests/WorkspaceRemoteConnectionTests.swift (1)
4778-4782: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate stale
Idlefailure messages.These assertions now expect
Completed, but their failure messages still sayIdle, which makes failures misleading during triage.Proposed fix
- "Expected successful Codex turn to report Idle, saw \(state.commands)" + "Expected successful Codex turn to report Completed, saw \(state.commands)" - "Expected scoped assistant reply to suppress no-final-response error, saw \(state.commands)" + "Expected scoped assistant reply to report Completed and suppress no-final-response error, saw \(state.commands)" - "Expected stale unscoped error to leave Codex idle, saw \(state.commands)" + "Expected stale unscoped error to leave Codex Completed, saw \(state.commands)"Also applies to: 5004-5007, 5074-5077
🤖 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/WorkspaceRemoteConnectionTests.swift` around lines 4778 - 4782, Update the failure messages for the assertions near the successful Codex turn checks, including the corresponding cases near lines 5004–5007 and 5074–5077, to say “Completed” instead of “Idle” so they match the expected status.
🤖 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 24230-24232: Add the missing agent.generic.status.completed entry
to the Localizable.xcstrings string catalog, using “Completed” as its
default/localized value so the status configuration in CLI/cmux.swift resolves
through the catalog.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 31752-31767: Update the `.needsInput?` branch’s `sendV1Command`
status color from blue `#4C8DFF` to the standard waiting orange `#FF9500`, while
preserving its existing lifecycle, icon, priority, and tab parameters.
In `@cmuxTests/WorkspaceRemoteConnectionTests.swift`:
- Around line 4778-4782: Update the failure messages for the assertions near the
successful Codex turn checks, including the corresponding cases near lines
5004–5007 and 5074–5077, to say “Completed” instead of “Idle” so they match the
expected status.
🪄 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: e71e6ff0-b698-415d-9cc8-570dd906a918
📒 Files selected for processing (11)
CLI/cmux.swiftSources/AgentHibernation/AgentHibernationLifecycleState.swiftSources/Workspace+AgentLifecycle.swiftSources/Workspace+Todos.swiftcmuxTests/AgentHibernationTests.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/CLIGenericHookPersistenceTests.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/ClaudeBackgroundWorkNotifyTests.swiftcmuxTests/ClaudeHookLiveDeliveryTargetTests.swiftcmuxTests/WorkspaceRemoteConnectionTests.swift
Comment on lines
+24230
to
+24232
| value: String(localized: "agent.generic.status.completed", defaultValue: "Completed"), | ||
| icon: "checkmark.circle.fill", | ||
| color: "#34C759" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the new localization key exists across catalogs
rg -n "agent.generic.status.completed" --glob '*.xcstrings' -C1Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Locate string catalogs and inspect whether the new key exists anywhere.
printf '\n== catalogs ==\n'
fd -a '\.xcstrings$' . || true
printf '\n== key search ==\n'
rg -n --glob '*.xcstrings' 'agent\.generic\.status\.completed|Completed' . || true
printf '\n== locale markers in catalogs ==\n'
python3 - <<'PY'
import json, glob, os
for path in glob.glob('**/*.xcstrings', recursive=True):
try:
with open(path, 'r', encoding='utf-8') as f:
data = json.load(f)
except Exception as e:
print(f'{path}: ERROR {e}')
continue
print(f'\n{path}')
print(' localizations:', sorted(data.get('sourceLanguage', '') and [data.get('sourceLanguage')] or []))
strings = data.get('strings', {})
if 'agent.generic.status.completed' in strings:
print(' has_key: yes')
entry = strings['agent.generic.status.completed']
print(' entry keys:', sorted(entry.keys()))
locs = entry.get('localizations', {})
print(' localized locales:', sorted(locs.keys()))
else:
print(' has_key: no')
PYRepository: manaflow-ai/cmux
Length of output: 4320
Add agent.generic.status.completed to Resources/Localizable.xcstrings
CLI/cmux.swift:24230-24232 uses a localized status key, but the key isn’t present in the string catalog, so it falls back to the inline defaultValue instead of a catalog entry.
🤖 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 24230 - 24232, Add the missing
agent.generic.status.completed entry to the Localizable.xcstrings string
catalog, using “Completed” as its default/localized value so the status
configuration in CLI/cmux.swift resolves through the catalog.
Source: Path instructions
teamleaderleo
added
the
needs a call
This branch has not been 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
4 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.
Summary
Combines two high-impact improvements:
Reliability (from upstream PR Clear agent Running status on interrupt signals #4390) — fixes stale Running after interrupt
stop-failurehook clears Running → Idle (no false "done" notification)Lifecycle clarity — explicit sidebar states
stop-failure) → Idle (not Completed)Fixes #4389 (interrupt stale Running). Partially addresses #4276 and #3749 via Codex monitor fallback.
Test plan
./scripts/setup.sh && ./scripts/reload.sh --tag reliability --launchpython3 tests/test_claude_wrapper_hooks.py(passes locally)xcodebuild test -only-testing:cmuxTests/CLINotifyProcessIntegrationRegressionTestsNote
This PR integrates the open upstream fix from #4390 rather than reimplementing it.
Summary by CodeRabbit
cmux claude-hookcommand.