Repository navigation
Agent-session tracking: single source of truth (no title/mtime heuristics) - #6631
Closed
azooz2003-bit wants to merge 16 commits into
Closed
azooz2003-bit wants to merge 16 commits into
azooz2003-bit wants to merge 16 commits into
Conversation
…(Slice A) Foundation for the reliable agent-session tracking redesign (see docs/agent-session-tracking-spec.md). Makes the host the single source of truth and gives the client an authoritative pull path so a missed or out-of-order best-effort push self-heals. - ChatSessionDescriptor + AgentChatSessionRecord gain a monotonic `version`, stamped by AgentChatSessionRegistry on every write (one chokepoint, counter not hash, so strict monotonicity holds even when a change reverts a field). - New `mobile.chat.session` RPC: authoritative single-session snapshot pull for reconnect / foreground / version-gap / manual-refresh. - ChatSessionListReducer version-gates descriptor upserts: a lower-version push never clobbers newer state from a later push or a snapshot pull. Equal version passes through (counter guarantees equal == identical content; keeps unversioned payloads upserting as before). +1 unit test. - iOS MobileChatEventSource.session(sessionID:) pull primitive + response type. Verified: CmuxAgentChat builds + 128 tests pass; CmuxMobileShell builds; full macOS app builds (tag agentsot). No heuristics removed yet; no behavior removed. iOS pull-trigger wiring and the process-exit backstop are next. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Completes Slice A's client side. The list seed via `source.sessions(...)` is already an authoritative pull that re-runs on reconnect (the connection epoch in `chatRefreshKey`). Add a foreground epoch so returning from `.background` re-subscribes and re-pulls: pushes are best-effort and can be dropped while the app is suspended, so on foreground we re-read the host's authoritative list rather than trust that every push arrived. Transient `.inactive` (control center, a banner) does not churn the subscription; only real background does. Pairs with the version-gated reducer so a pull that races a late push converges. The single-session `mobile.chat.session` pull primitive remains available for finer-grained version-gap healing in the conversation view. Verified: CmuxMobileShellUI builds for iOS Simulator (BUILD SUCCEEDED). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Replace the per-`sessions()` `kill(pid,0)` polling sweep with an event-driven `DispatchSourceProcess` (`.exit`) watcher per agent pid. cmux does not own a `Process` handle for a terminal agent (it is a child in the pty), so the deterministic exit signal is a process source on the pid cmux already knows from hook events / the store. On exit, the session flips to `.ended` on the main actor, but only if the exited pid is still the record's current pid, so a `claude --resume` under a new pid is never ended by its predecessor's exit. - `syncProcessExitWatch(for:)` reconciles the watcher with the record's pid at every store path (idempotent; cancels on pid change / clear / end). A pid already dead at registration ends the session on a fresh main-actor turn rather than waiting for an `.exit` that never comes. - `ended` stays retained: the GUI keeps showing the session and the input bar disables; only the watcher is torn down. - `sessions()` no longer sweeps on every read; the per-bound-session `kill(pid,0)` guard in `liveSession` stays as a cheap correctness backstop. A watcher unit test needs a real child process (timing-dependent, app test target only), so this is verified by build + dogfood rather than a flaky unit test. macOS app builds (tag agentsot). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… already exists Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Remove the unreliable agent-session detection layer (terminal-TITLE matching and the newest-.jsonl-by-MTIME scan, plus their claim/forced-retry/provisional machinery) while preserving the reliable path: hook events, the hook-store as cmux-written persistence/seed, and transcript resolution keyed by the exact recorded path or session id. Dropping detection of agents that never fire a hook is intended. Deleted: - Sources/Mobile/AgentChat/AgentChatTranscriptService+TitleDetection.swift - cmuxTests/AgentChatTranscriptResolverTests.swift (only covered newestClaudeTranscript) - Resolver: newestClaudeTranscript, cwdCandidates, claudeTranscriptTitle(at:)/(in:), normalizedClaudeTitle + title-read constants - Service: adoptDetectedClaudeSession, private newestClaudeTranscript, observeAgentTitleChanges, ghosttyTitleSubscription, titleAdoptionHandler, all title-detection state vars + constants, provisional/title-key helpers, the PendingTitleChange/ClaudeTranscriptResolutionKey typealiases, the provisional branch in history(), and the clearTitleDetectionState call. start(adoptDetectedAgentSession:) -> start() (just seedFromHookStores). - Registry: claimedSessionIDs(), adoptDetectedSession() - TerminalController+MobileChat: adoptDetectedAgentSession(s) variants; v2MobileChatSessions now just lists registry sessions filtered by mobileChatBindingIsCurrentAgent. - TerminalController+MobileWorkspaceList: the adoptDetectedAgentSessions calls - AppDelegate: start() no-arg call site Kept (reliable): hook path, hook-store seed/refresh/adoptBindings, transcript resolution by recorded path + claudeFallbackPath/codexFallbackPath, encodeClaudeProjectDir, the GhosttyTitleChange(+Subscription) types (used for tab titles), and Slice A/B work. RestorableAgentSession.swift's newestClaudeTranscript is KEPT: it is the session-restore mechanism keyed by the recorded session id (workflow-container resolution), not the unreliable mobile-chat detection heuristic. Verified: CmuxAgentChat builds + 128 tests pass; macOS app Build complete (tag agentsotd); iOS CmuxMobileShellUI BUILD SUCCEEDED. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… rationale Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ce C) Per the owner's directive "no jsonl parsing or heavy work on the main thread." After Slice D the only heavy main-actor parse left in this subsystem was the hook-store whole-file JSON read. Move all three read sites off-main: - seedFromHookStores is now async; the Data(contentsOf:)+JSONSerialization runs in a utility Task.detached, only the (cheap) record application touches main state. start() kicks it off and returns. - noteHookEvent no longer reads the store inline. When a binding is still missing (throttled to once per 30s/session) it returns immediately and defers an off-main backfill (backfillBindingsFromStore) that applies only still-nil fields via update() — so the live event stays authoritative and the hot tool- storm path never parses JSON on main. applyStoreBackfill no-ops when it learns nothing new, avoiding a spurious version bump / descriptor push. - refreshBindingsFromHookStore is async (off-main read); the send/interrupt/ answer + history RPC chain is threaded async to match. The transcript tailer already parses off its own actor; descriptor wire-encoding on main is small, not a whole-file parse. macOS app builds. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR implements a deterministic single-source-of-truth agent session tracking model. It replaces heuristic title-detection session adoption with hook-driven binding, adds monotonic ChangesAgent Session Tracking: Versioned Single Source of Truth
Codex Hook Injection: Fire-and-Forget Architecture
Sequence Diagram(s)sequenceDiagram
rect rgba(173, 216, 230, 0.5)
note over iOS as iOS Client,AgentChatSessionRegistry: Authoritative Pull Reconciliation
end
participant iOS as iOS Client
participant EventSource as MobileChatEventSource
participant TerminalCtl as TerminalController
participant Service as AgentChatTranscriptService
participant Registry as AgentChatSessionRegistry
iOS->>EventSource: session(sessionID:)
EventSource->>TerminalCtl: RPC mobile.chat.session {session_id}
TerminalCtl->>Service: sessionRecord(sessionID:)
Service->>Registry: lookup sessionID
Registry-->>Service: AgentChatSessionRecord (or nil)
Service-->>TerminalCtl: record
alt record found
TerminalCtl-->>EventSource: {session: ChatSessionDescriptor (with version)}
EventSource-->>iOS: ChatSessionDescriptor
else record not found
TerminalCtl-->>EventSource: error not_found {session_id}
EventSource-->>iOS: throws sessionNotFound
end
sequenceDiagram
rect rgba(255, 228, 196, 0.5)
note over Hook as Hook Event,DispatchSource as DispatchSourceProcess: Process-exit Backstop (replaces sweep)
end
participant Hook as Hook Event
participant Registry as AgentChatSessionRegistry
participant DispatchSource as DispatchSourceProcess
participant HookStore as HookStore (off-main)
Hook->>Registry: noteHookEvent(pid, sessionID)
Registry->>Registry: compute shouldConsultStore predicate
Registry->>Registry: update(sessionID) → stampVersion + syncProcessExitWatch
Registry->>DispatchSource: syncProcessExitWatch(pid, sessionID)
DispatchSource-->>Registry: handleProcessExit(pid, sessionID) on .exit
Registry->>Registry: guard PID matches → transition .ended, stampVersion
Registry->>HookStore: Task.detached { read hook store entry }
HookStore-->>Registry: parsed bindings (back on main actor)
Registry->>Registry: applyStoreBackfill (only non-nil changed fields) → stampVersion
sequenceDiagram
rect rgba(255, 182, 193, 0.5)
note over Codex as Codex invocation,CMux as cmux CLI: Hook Injection Flow
end
participant Codex as Codex invocation
participant Wrapper as cmux-codex-wrapper
participant CMux as cmux CLI
participant RealCodex as real codex binary
Codex->>Wrapper: codex [args]
Wrapper->>Wrapper: detect if session entrypoint
alt not session entrypoint or hooks disabled or not in cmux
Wrapper->>RealCodex: exec codex [original args]
else session entrypoint + in cmux + hooks enabled
Wrapper->>Wrapper: export CMUX_CODEX_PID, launch identity env vars
Wrapper->>CMux: cmux hooks codex inject-args (NUL-separated)
CMux-->>Wrapper: inject args (e.g., -c hook1.toml -c hook2.toml)
Wrapper->>RealCodex: exec codex [inject args] [original args]
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~105 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 2 warnings, 1 inconclusive)
✅ Passed checks (18 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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 |
…y holds for terminal agents) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR replaces the heuristic agent-session detection layer (terminal-title matching, newest-
Confidence Score: 5/5The deletion of heuristic code is safe because the reliable hook-event path already existed; versioning and pull are additive. The known exit-watcher leak (flagged in a prior thread) is a resource-usage concern rather than a session-state correctness hole. The architectural rework is well-scoped and thoroughly commented. Off-main JSON reads, version-gating, the snapshot-pull RPC, and the foreground re-pull all fit together cleanly. The only new finding is missing locale coverage for one new string, which does not affect runtime behavior. Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift — the exit-watcher leak on normal process exit (flagged in the prior review thread) still needs a follow-up fix. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Agent as Codex/Claude Agent
participant Hook as cmux Hook (CLI)
participant Reg as AgentChatSessionRegistry
participant Svc as AgentChatTranscriptService
participant iOS as iOS MobileChatEventSource
Note over Agent,Reg: Session start
Agent->>Hook: SessionStart fires (CMUX_SURFACE_ID in env)
Hook->>Svc: noteHookEvent(WorkstreamEvent + surfaceId + transcriptPath)
Svc->>Reg: noteHookEvent → stampVersion → syncProcessExitWatch
Reg-->>Svc: onRecordChanged(record, nil)
Svc-->>iOS: push descriptorChanged (version N)
Note over Reg: Exit detection (new in this PR)
Agent->>Reg: DispatchSourceProcess(.exit) fires
Reg->>Reg: "handleProcessExit → update { state = .ended } → version N+1"
Reg-->>Svc: onRecordChanged(record, previous)
Svc-->>iOS: push descriptorChanged(ended, version N+1)
Note over iOS,Svc: Foreground / reconnect pull (new in this PR)
iOS->>Svc: "mobile.chat.session { session_id }"
Svc->>Reg: sessionRecord(sessionID)
Reg-->>Svc: record (version N+1)
Svc-->>iOS: "{ session: descriptor(version N+1) }"
iOS->>iOS: "version-gated upsert (replace if version >= current)"
%%{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"}}}%%
sequenceDiagram
participant Agent as Codex/Claude Agent
participant Hook as cmux Hook (CLI)
participant Reg as AgentChatSessionRegistry
participant Svc as AgentChatTranscriptService
participant iOS as iOS MobileChatEventSource
Note over Agent,Reg: Session start
Agent->>Hook: SessionStart fires (CMUX_SURFACE_ID in env)
Hook->>Svc: noteHookEvent(WorkstreamEvent + surfaceId + transcriptPath)
Svc->>Reg: noteHookEvent → stampVersion → syncProcessExitWatch
Reg-->>Svc: onRecordChanged(record, nil)
Svc-->>iOS: push descriptorChanged (version N)
Note over Reg: Exit detection (new in this PR)
Agent->>Reg: DispatchSourceProcess(.exit) fires
Reg->>Reg: "handleProcessExit → update { state = .ended } → version N+1"
Reg-->>Svc: onRecordChanged(record, previous)
Svc-->>iOS: push descriptorChanged(ended, version N+1)
Note over iOS,Svc: Foreground / reconnect pull (new in this PR)
iOS->>Svc: "mobile.chat.session { session_id }"
Svc->>Reg: sessionRecord(sessionID)
Reg-->>Svc: record (version N+1)
Svc-->>iOS: "{ session: descriptor(version N+1) }"
iOS->>iOS: "version-gated upsert (replace if version >= current)"
Reviews (2): Last reviewed commit: "Merge pull request #6655 from manaflow-a..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+74
to
+76
| if let existing = exitWatchers[sessionID], existing.pid == record.pid { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
syncProcessExitWatch early-returns when existing.pid == record.pid, which is always true immediately after handleProcessExit calls update { $0.state = .ended } — the record's pid didn't change, only its state. So every session that ends via the happy-path (process exits, source fires, state flips to .ended) leaves a live DispatchSourceProcess entry in exitWatchers forever. Over time the dictionary grows unboundedly, and each stale source holds a EVFILT_PROC kqueue slot. Worse, if the OS recycles the pid and a new unrelated process inherits it, the stale source wakes the handler again (the record.pid == pid guard in handleProcessExit blocks false-ending only while the new pid hasn't been adopted by a SessionStart hook).
Suggested change
| if let existing = exitWatchers[sessionID], existing.pid == record.pid { | |
| return | |
| } | |
| if let existing = exitWatchers[sessionID], existing.pid == record.pid { | |
| if record.state == .ended { | |
| existing.source.cancel() | |
| exitWatchers[sessionID] = nil | |
| } | |
| return | |
| } |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift (1)
211-220: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCold-seed liveness currently forces
pid == nilsessions to.ended.Line 211 uses
?? false, so missing PID always becomes “not alive” and Line 219 seeds.ended. That conflicts with the file’s own assumption (Line 271) that PID can legitimately be absent. It also treatsEPERMas dead here, while Line 80 treats onlyESRCHas dead.💡 Suggested fix
- let alive = entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false + let alive = entry.pid.map { !processIsDead($0) } ?? true var record = AgentChatSessionRecord( sessionID: entry.sessionID, agentKind: kind, workspaceID: entry.workspaceID, surfaceID: entry.surfaceID, workingDirectory: entry.workingDirectory, transcriptPath: entry.transcriptPath, state: alive ? .idle : .ended,🤖 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/Mobile/AgentChat/AgentChatSessionRegistry.swift` around lines 211 - 220, The liveness check in AgentChatSessionRegistry uses `entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false` which defaults nil PIDs to "not alive", causing sessions without PIDs to be marked as `.ended` state. This conflicts with line 271 which assumes PIDs can legitimately be absent, and also differs from the error handling at line 80 which distinguishes between ESRCH (process not found) and EPERM (permission denied) rather than treating all failures as dead. Refactor the alive computation to handle nil PIDs separately from dead processes, either by preserving nil as a valid state or by aligning the error handling to match line 80's logic of only treating ESRCH as indicating a dead process.
🤖 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 `@docs/agent-session-tracking-spec.md`:
- Line 335: Resolve the status inconsistency for Slice D in the document. The
status note confirms that Slices A, B, D are implemented and build-green on all
layers, but Slice D is currently marked as "IN PROGRESS" elsewhere in the
document. Locate the section where Slice D status is defined and update its
status from "IN PROGRESS" to "DONE" to align with the implementation status
stated in the status note and confirmed by the PR context.
In
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`:
- Around line 219-231: The chatRefreshKey calculation currently includes
scenePhase state that changes immediately when entering background, which causes
.task(id:) to re-run and re-trigger subscription work during
backgrounding—opposite of the intended "tear down to save battery" behavior.
Instead of directly using scenePhase == .background to determine the foreground
value, only update the foreground component of the key when transitioning from
background back to active/foreground state. This requires tracking the previous
scene phase or using a separate counter that increments only when returning from
background, not when entering it, ensuring the task key remains stable during
background and only changes on foreground return to re-establish the
subscription.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatSessionListReducer.swift`:
- Around line 41-53: The guard condition checking descriptor.version >=
updated[index].version allows equal versions to pass through, which causes a
race condition where a delayed old .descriptorChanged with the same version can
overwrite newer state applied by .stateChanged (which doesn't advance the
version). Change the guard statement to use strict greater-than comparison (>)
for versioned payloads, but preserve the equality check only for legacy
unversioned payloads where both versions are 0. This prevents equal-version
stale descriptors from clobbering newer state while maintaining backward
compatibility.
---
Outside diff comments:
In `@Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift`:
- Around line 211-220: The liveness check in AgentChatSessionRegistry uses
`entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false` which defaults nil PIDs to
"not alive", causing sessions without PIDs to be marked as `.ended` state. This
conflicts with line 271 which assumes PIDs can legitimately be absent, and also
differs from the error handling at line 80 which distinguishes between ESRCH
(process not found) and EPERM (permission denied) rather than treating all
failures as dead. Refactor the alive computation to handle nil PIDs separately
from dead processes, either by preserving nil as a valid state or by aligning
the error handling to match line 80's logic of only treating ESRCH as indicating
a dead process.
🪄 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: ecbf1344-1d38-4be6-befe-2050b9ac0d21
📒 Files selected for processing (18)
Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Model/ChatSessionDescriptor.swiftPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatSessionListReducer.swiftPackages/Shared/CmuxAgentChat/Tests/CmuxAgentChatTests/ChatSessionListReducerTests.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatEventSource.swiftPackages/iOS/CmuxMobileShell/Sources/CmuxMobileShell/MobileChatSessionResponse.swiftPackages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/Mobile/AgentChat/AgentChatSessionRecord.swiftSources/Mobile/AgentChat/AgentChatSessionRegistry.swiftSources/Mobile/AgentChat/AgentChatTranscriptResolver.swiftSources/Mobile/AgentChat/AgentChatTranscriptService+TitleDetection.swiftSources/Mobile/AgentChat/AgentChatTranscriptService.swiftSources/TerminalController+MobileChat.swiftSources/TerminalController+MobileWorkspaceList.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentChatTranscriptResolverTests.swiftdocs/agent-session-tracking-spec.md
💤 Files with no reviewable changes (5)
- Sources/TerminalController+MobileWorkspaceList.swift
- Sources/Mobile/AgentChat/AgentChatTranscriptService+TitleDetection.swift
- cmuxTests/AgentChatTranscriptResolverTests.swift
- Sources/Mobile/AgentChat/AgentChatTranscriptResolver.swift
- cmux.xcodeproj/project.pbxproj
| `ended` stays retained (disables the input bar, stays visible). `DispatchSource` | ||
| is an event source, not a timer, so it does not fall under the asyncAfter ban | ||
| and is cancellable. | ||
| - **Status note (after A/B/D):** Slices A, B, D are implemented, build-green on |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Resolve Slice D status inconsistency.
Line 335 states "Slices A, B, D are implemented, build-green on all layers, and committed," but line 363 labels Slice D as "IN PROGRESS." Per the PR objectives and AI summary, Slice D (delete unreliable heuristics) is implemented and included in this PR. Update line 363 to reflect "DONE" status to align with the status note and PR context.
🔧 Proposed fix
-- **Slice D — Delete the unreliable heuristics. IN PROGRESS.** Remove
+- **Slice D — Delete the unreliable heuristics. DONE.** RemoveAlso applies to: 363-363
🤖 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 `@docs/agent-session-tracking-spec.md` at line 335, Resolve the status
inconsistency for Slice D in the document. The status note confirms that Slices
A, B, D are implemented and build-green on all layers, but Slice D is currently
marked as "IN PROGRESS" elsewhere in the document. Locate the section where
Slice D status is defined and update its status from "IN PROGRESS" to "DONE" to
align with the implementation status stated in the status note and confirmed by
the PR context.
Comment on lines
+219
to
+231
| /// Identity for the session refetch: workspace, connection epoch, and a | ||
| /// foreground epoch. A change re-runs `.task(id:)`, which re-subscribes to | ||
| /// the push stream and re-pulls the authoritative session list. | ||
| /// | ||
| /// The foreground epoch flips only on `.background` (not transient | ||
| /// `.inactive` like control center or a banner), so a real | ||
| /// background-then-foreground re-pulls while momentary inactivity does not | ||
| /// churn the subscription. `.background` tears the stream down to save | ||
| /// battery; returning to the foreground re-establishes and reconciles. | ||
| private var chatRefreshKey: String { | ||
| "\(workspace.id.rawValue)#\(store.connectionState == .connected ? 1 : 0)" | ||
| let connected = store.connectionState == .connected ? 1 : 0 | ||
| let foreground = scenePhase == .background ? 0 : 1 | ||
| return "\(workspace.id.rawValue)#\(connected)#\(foreground)" |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Background transition currently re-triggers chat refresh work.
Line 230 flips the task key on .background, so .task(id:) restarts and can re-run subscription/pull while backgrounded—the opposite of the “tear down to save battery” intent.
Suggested fix
private func refreshChatSessions() async {
+ guard scenePhase != .background else { return }
guard let source = store.makeChatEventSource() else {
chatSessions = []
applyChatModeFallback()
return
}🤖 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
`@Packages/iOS/CmuxMobileShellUI/Sources/CmuxMobileShellUI/WorkspaceDetailView.swift`
around lines 219 - 231, The chatRefreshKey calculation currently includes
scenePhase state that changes immediately when entering background, which causes
.task(id:) to re-run and re-trigger subscription work during
backgrounding—opposite of the intended "tear down to save battery" behavior.
Instead of directly using scenePhase == .background to determine the foreground
value, only update the foreground component of the key when transitioning from
background back to active/foreground state. This requires tracking the previous
scene phase or using a separate counter that increments only when returning from
background, not when entering it, ensuring the task key remains stable during
background and only changes on foreground return to re-establish the
subscription.
Comment on lines
+41
to
+53
| // Version-gated upsert: best-effort pushes can arrive out of | ||
| // order, be duplicated, or race an authoritative pull. The host | ||
| // stamps a strictly increasing `version` on every change, so a | ||
| // descriptor whose version is LOWER than the one already | ||
| // applied is stale (or out of order) and must not clobber newer | ||
| // state the client got from a later push or a snapshot pull. | ||
| // Equal version is allowed through (a no-op in practice: the | ||
| // monotonic counter guarantees equal version == identical | ||
| // content), which also keeps unversioned (version 0) payloads | ||
| // upserting as before. | ||
| guard descriptor.version >= updated[index].version else { | ||
| return sessions | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Equal-version upserts can re-apply stale descriptor payloads after a newer state transition.
Line 51 accepts descriptor.version == updated[index].version. But Line 66 applies .stateChanged without advancing version, so a delayed old .descriptorChanged at the same version can overwrite the newer state. For versioned payloads, this should be strict >; keep equality only for legacy 0/0.
💡 Suggested fix
if let index = updated.firstIndex(where: { $0.id == descriptor.id }) {
- guard descriptor.version >= updated[index].version else {
+ let currentVersion = updated[index].version
+ let incomingVersion = descriptor.version
+ let allowLegacyUnversionedEqual = (currentVersion == 0 && incomingVersion == 0)
+ guard incomingVersion > currentVersion || allowLegacyUnversionedEqual else {
return sessions
}
updated[index] = descriptor
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| // Version-gated upsert: best-effort pushes can arrive out of | |
| // order, be duplicated, or race an authoritative pull. The host | |
| // stamps a strictly increasing `version` on every change, so a | |
| // descriptor whose version is LOWER than the one already | |
| // applied is stale (or out of order) and must not clobber newer | |
| // state the client got from a later push or a snapshot pull. | |
| // Equal version is allowed through (a no-op in practice: the | |
| // monotonic counter guarantees equal version == identical | |
| // content), which also keeps unversioned (version 0) payloads | |
| // upserting as before. | |
| guard descriptor.version >= updated[index].version else { | |
| return sessions | |
| } | |
| // Version-gated upsert: best-effort pushes can arrive out of | |
| // order, be duplicated, or race an authoritative pull. The host | |
| // stamps a strictly increasing `version` on every change, so a | |
| // descriptor whose version is LOWER than the one already | |
| // applied is stale (or out of order) and must not clobber newer | |
| // state the client got from a later push or a snapshot pull. | |
| // Equal version is allowed through (a no-op in practice: the | |
| // monotonic counter guarantees equal version == identical | |
| // content), which also keeps unversioned (version 0) payloads | |
| // upserting as before. | |
| if let index = updated.firstIndex(where: { $0.id == descriptor.id }) { | |
| let currentVersion = updated[index].version | |
| let incomingVersion = descriptor.version | |
| let allowLegacyUnversionedEqual = (currentVersion == 0 && incomingVersion == 0) | |
| guard incomingVersion > currentVersion || allowLegacyUnversionedEqual else { | |
| return sessions | |
| } | |
| updated[index] = descriptor | |
| } else { |
🤖 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
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatSessionListReducer.swift`
around lines 41 - 53, The guard condition checking descriptor.version >=
updated[index].version allows equal versions to pass through, which causes a
race condition where a delayed old .descriptorChanged with the same version can
overwrite newer state applied by .stateChanged (which doesn't advance the
version). Change the guard statement to use strict greater-than comparison (>)
for versioned payloads, but preserve the equality check only for legacy
unversioned payloads where both versions are 0. This prevents equal-version
stale descriptors from clobbering newer state while maintaining backward
compatibility.
…bal install Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… injection (Slice F)
Make Codex sessions track in the iOS GUI as reliably as Claude, without
installing anything into the user's ~/.codex and without clobbering their
existing notify/config.
cmux-codex-wrapper mirrors cmux-claude-wrapper: when inside a cmux terminal
(CMUX_SURFACE_ID + live socket) and a session entrypoint (bare codex, a
prompt, or codex exec/e), it execs the real codex with per-invocation hooks:
--enable hooks --dangerously-bypass-hook-trust -c 'hooks.SessionStart=[{hooks=[{type="command",command='''<gated>''',timeout=...}]}]' (and UserPromptSubmit/Stop/PreToolUse/PostToolUse/PermissionRequest)
The injected command is the exact gated shape cmux installs for persisted
codex hooks (resolve cmux CLI, require surface+socket+not-disabled, run
'cmux hooks codex <event>', else echo '{}'), carried as a TOML multi-line
literal string so its single quotes need no escaping. Verified empirically
against codex-cli 0.141.0: all hooks fire and codex passes session_id +
transcript_path on stdin, binding the transcript by real session id.
Belt-and-suspenders: the wrapper also fires a one-way 'cmux hooks codex
session-start' (surface/pid/cwd, empty stdin) BEFORE exec, so detection
happens at launch even if codex's own SessionStart is delayed; the registry
dedups by session id so the two reconcile.
Passthrough safety mirrors the claude wrapper exactly: every gate (opt-out
via CMUX_CODEX_HOOKS_DISABLED, outside cmux, dead socket, non-session
subcommand like resume/doctor/--help) and find_real_codex failure exec the
real codex unchanged, so installing the wrapper can never break codex.
A per-surface 'codex' PATH shim is written into the same cmux-cli-shims dir
as the claude shim (already on PATH), resolving+exec'ing the wrapper, else
stripping the shim dirs and exec'ing real codex. Bundled into the app
Resources/bin via the Copy CLI phase alongside cmux-claude-wrapper.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ed.push event (fix) A live codex/claude session record from chat.sessions.dump showed surface_id=None / transcript_path=None even though the hook store had both. The feed.push event carried workspace_id/cwd but no surface_id or transcript_path, and the .sessionStart store-backfill was suppressed and 30s-throttled, so a fresh (or short-lived `codex exec`) session stayed unbound until the next consult. Option A (timing-independent): carry the hook-resolved surface/transcript in the event itself. - WorkstreamEvent: add surfaceId (surface_id) and transcriptPath (transcript_path), mirroring workspaceId exactly (default nil, decodeIfPresent, encodeIfPresent, and via CodingKeys.allCases they stay in the knownKeys set). - AgentChatSessionRegistry.noteHookEvent: apply event.surfaceId and event.transcriptPath onto the record alongside workspaceId/cwd, so a live event binds immediately without waiting on the throttled store consult. - CLI sendFeedTelemetry: add surfaceId param and write surface_id + transcript_path (from parsedInput.transcriptPath) into the feed.push event. Thread the hook-RESOLVED target.surfaceId through sendAgentFeedTelemetry / sendAgentFeedTelemetryUnlessSuppressed at every agent-hook call site that has a resolved target in scope (session-start, prompt-submit, stop/notification, session-end via mapped.surfaceId). Verified live: a real `codex exec` session 019ef2cc-... appeared as a single non-fallback codex record with non-null surface_id and transcript_path in state idle, then transitioned to ended after the process exited. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
… on cmux
The codex wrapper injected per-invocation [hooks] whose command called
`cmux hooks codex <sub>` SYNCHRONOUSLY. Codex runs hooks synchronously and
blocks until they return, so every launch hung ~35s on "Running SessionStart
hook" and every prompt lagged on UserPromptSubmit while the cmux call did
socket round-trips.
Reuse cmux's proven fire-and-forget shape (CMUXCLI.codexFireAndForget-
AgentHookShellCommand): capture codex's stdin payload to a temp file, nohup-
background the cmux call with a 30s watchdog, and `echo '{}'` back to codex
instantly. Detection still binds the real session_id/transcript_path because
the backgrounded call gets codex's real stdin.
Implementation: a hidden, socket-free `cmux hooks codex inject-args` emits the
exact codex arg list (NUL-terminated) to enable + inject the fire-and-forget
hooks for all six events (SessionStart, UserPromptSubmit, Stop, PreToolUse,
PostToolUse, PermissionRequest), each fire-and-forget command carried in a
TOML multi-line literal. The wrapper reads that stream into a bash array and
execs codex with it, replacing the hand-rolled TOML/quoting in bash. All
passthrough-safety gates (not in cmux / dead socket / hooks-disabled /
non-session subcommand / emit fails) still fall back to plain `exec codex`.
Two bugs found and fixed during live verification: the CLI emitted args
NUL-SEPARATED (dropped the final PermissionRequest arg at EOF) -> now
NUL-terminated; and the wrapper read via `raw="$(...)"` command substitution,
which bash strips NUL bytes from, collapsing the stream and silently dropping
the whole injection -> now reads the command directly via process
substitution. Verified live: hook returns {} in ~0.01s, the codex session
binds (surface_id + transcript_path) and goes ended after exit.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…ace, not stale stored workspace_id cmux workspace ids regenerate on every Mac relaunch while surface ids are stable and rehydrate verbatim, so a chat session created before the last relaunch carries a stale stored workspace_id and was dropped from its terminal's current workspace (no iOS chat toggle). Scope the workspace- filtered mobile.chat.sessions listing by the surface's CURRENT workspace: resolve the requested workspace, return every session whose surface is a live terminal panel there and that matches its agent against that workspace+panel, and re-stamp each returned record to the requested workspace so the seed and live descriptorChanged pushes both scope to it. Also exposes mobile.chat.sessions over the local control/debug socket for dogfood verification of this path. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Codex agent detection (extension of #6631 agent-session tracking)
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift (2)
72-79: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel ended-session exit sources before the same-pid fast path.
Line 74 returns before Line 79 checks
.ended, sohandleProcessExitat Line 107 stores.endedwhile leaving the firedDispatchSourceProcessretained inexitWatchers. That leaks one source per ended session; derive the desired pid/state first, then fast-path only live watchers.Suggested fix
private func syncProcessExitWatch(for record: AgentChatSessionRecord) { let sessionID = record.sessionID - if let existing = exitWatchers[sessionID], existing.pid == record.pid { + let desiredPID = record.state == .ended ? nil : record.pid + if let pid = desiredPID, + let existing = exitWatchers[sessionID], + existing.pid == pid { return } exitWatchers[sessionID]?.source.cancel() exitWatchers[sessionID] = nil - guard record.state != .ended, let pid = record.pid else { return } + guard let pid = desiredPID else { return }🤖 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/Mobile/AgentChat/AgentChatSessionRegistry.swift` around lines 72 - 79, The early return in syncProcessExitWatch for matching pids prevents cleanup of exit watchers when a session has ended, causing DispatchSourceProcess sources to leak. Reorder the logic to check the session state and extract the pid before the fast-path return condition: first validate that record.state is not .ended and extract the pid, then only return early for the same-pid case if the session is still live (not ended), ensuring that ended sessions always proceed to cancel and remove their watchers from the exitWatchers dictionary.
211-219: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same ESRCH-only dead check during seeding.
Line 211 seeds any nonzero
kill(pid, 0)result as.ended, but the watcher path correctly treatsEPERMas alive. A live-but-unsignalable pid would be hidden as ended after relaunch until another hook arrives.Suggested fix
- let alive = entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false + let alive = entry.pid.map { pid in + guard pid > 0 else { return false } + return !processIsDead(pid) + } ?? 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 `@Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift` around lines 211 - 219, The dead process check on line 211 using kill(pid_t($0), 0) treats all non-zero results as dead, but the watcher path correctly distinguishes between ESRCH (process not found) and EPERM (permission denied, meaning process is alive). Update the alive computation to check errno and only mark the process as .ended when kill returns an error specifically due to ESRCH, treating EPERM and other errors as indicating a live but unsignalable process, so that live processes with restricted permissions are not incorrectly hidden after relaunch.Sources/Mobile/AgentChat/AgentChatTranscriptService.swift (1)
48-52: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDon’t let authoritative session pulls race startup seeding.
start()now returns before hook-store seeding finishes, whileseedFromHookStoresonly inserts records and does not emitonRecordChangedpushes. An immediatemobile.chat.sessionspull can therefore return an empty authoritative snapshot with no later correction until the client refreshes again; expose/await a seed task before serving session snapshots, or make startup await the seed before accepting mobile pulls. As per coding guidelines, “Flag fire-and-forgetTask { ... }work with meaningful lifecycle in cmux-owned Swift code that is not stored, cancelled, or tied to a caller-owned operation.”🤖 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/Mobile/AgentChat/AgentChatTranscriptService.swift` around lines 48 - 52, The fire-and-forget Task in the start() method that calls registry.seedFromHookStores() creates a race condition where mobile.chat.sessions pulls can execute and return empty snapshots before seeding completes. Either refactor start() to await seedFromHookStores() before returning, or expose the seed task and ensure mobile.chat.sessions pulls wait for its completion before serving authoritative snapshots. Additionally, per Swift coding guidelines for cmux-owned code, this fire-and-forget Task must be stored, cancelled, or tied to a caller-owned lifecycle operation rather than left dangling.Source: Coding guidelines
Sources/TerminalController+MobileChat.swift (2)
207-212: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResolve the terminal binding only once for
send.
v2MobileChatSendfirst capturesterminalParams, then callsmobileChatTerminalPanel, which performs another async lookup. If the registry refreshes between those calls, the prompt can be cleared on one terminal while paste/image delivery uses another. Return(params, panel)from a single binding-resolution helper and use that snapshot for the whole send.🤖 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/TerminalController`+MobileChat.swift around lines 207 - 212, The v2MobileChatSend method makes two separate async lookups (mobileChatTerminalParams and mobileChatTerminalPanel) which can become inconsistent if the registry refreshes between the calls, potentially causing the prompt to be cleared on one terminal while paste/image delivery uses another. Create a single helper function that atomically resolves and returns both the terminal params and panel as a tuple in one operation, replacing the two separate guard statements that call mobileChatTerminalParams and mobileChatTerminalPanel with a single call to this new helper function, ensuring the entire send operation uses a consistent snapshot of the terminal binding.
348-352: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse the refreshed record’s workspace after hook-store backfill.
After
refreshSessionBindings, this still resolvesrefreshed.surfaceIDagainst the pre-refreshworkspaceID. That is exactly the value documented as stale after relaunch, so send/interrupt/answer can continue failing even when refresh found the current workspace.Proposed fix
- if let refreshed = await service.refreshSessionBindings(sessionID: sessionID), - let surfaceID = refreshed.surfaceID, - mobileChatBindingResolves(workspaceID: workspaceID, surfaceID: surfaceID), + if let refreshed = await service.refreshSessionBindings(sessionID: sessionID), + let refreshedWorkspaceID = refreshed.workspaceID ?? record.workspaceID, + let surfaceID = refreshed.surfaceID, + mobileChatBindingResolves(workspaceID: refreshedWorkspaceID, surfaceID: surfaceID), mobileChatBindingIsCurrentAgent(refreshed) { - return ["workspace_id": workspaceID, "surface_id": surfaceID] + return ["workspace_id": refreshedWorkspaceID, "surface_id": 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 `@Sources/TerminalController`+MobileChat.swift around lines 348 - 352, The code is using the stale pre-refresh workspaceID parameter instead of the updated workspace ID from the refreshed record returned by refreshSessionBindings. Extract the workspace ID from the refreshed record and use it in both the mobileChatBindingResolves method call and the returned dictionary instead of the workspaceID parameter. This ensures the current workspace value after the hook-store backfill is used for binding resolution and the return value, preventing stale workspace references that can cause send/interrupt/answer operations to fail.
🤖 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/CMUXCLI`+CodexFireAndForgetHooks.swift:
- Line 32: The unconditional emission of `--dangerously-bypass-hook-trust` on
line 32 bypasses hook review for all hooks in Codex (including any pre-existing
user/project hooks in ~/.codex/hooks.json and ~/.codex/config.toml), not just
cmux-injected ones, which violates cmux's principle of not clobbering user Codex
config. To fix this, either verify that the cmux-owned Codex wrapper injection
scopes the bypass only to cmux-injected hooks via the `-c` arguments, or gate
the `--dangerously-bypass-hook-trust` flag by checking the Codex version
(introduced in v0.131.0) and fail closed if user hook sources are detected in
the Codex config. Additionally, document the security boundary in code comments
explaining which hooks this flag applies to and what assumptions are being made
about the scope of the bypass.
In `@docs/codex-agent-detection-plan.md`:
- Around line 51-63: The design section describing the cmux-codex-wrapper (lines
51-63) currently prescribes that the wrapper emits a pre-exec session-start
signal via `cmux hooks codex session-start`, but the actual implementation
intentionally removed this behavior to avoid creating phantom fallback-*
duplicate sessions. Update the design description to remove the reference to the
wrapper firing the pre-exec signal and instead document the actual runtime
behavior where no pre-exec signal is emitted. Also apply the same correction to
lines 130-131 which describe the same outdated signal pattern, ensuring the
documentation reflects what was actually shipped rather than the original design
intent.
In `@Sources/TerminalController`+MobileChat.swift:
- Around line 81-88: The code is returning session descriptors without
re-resolving them against the current workspace, which can result in stale
workspace information being returned to the client. Create or use a shared
helper function that takes a session descriptor and returns an updated
descriptor with the current workspace properly resolved. In the no-filter path
within the guard block (where descriptors are being encoded with
service.wirePayload), apply this helper to restamp each descriptor before
encoding. Also apply the same helper in the single-session endpoint path (around
lines 139-150) to ensure both the list and individual session endpoints use the
same authoritative mechanism for resolving current workspace information before
returning encoded payloads to the client.
- Around line 400-411: The mobileChatRecordMatchesAgent function currently uses
terminal title/name heuristics (extracting, normalizing, and matching title
strings) to determine agent identity, which is incorrect for
correctness-critical decisions. Remove the title extraction and normalization
logic that prepares the terminal title for downstream matching. Instead,
validate the agent match using only structured agent binding and context
information obtained from WorkspaceContentView.terminalAgentContext. The
function should fail closed (return false) unless the structured context
definitively proves the agent matches the record, not based on terminal title
similarity.
---
Outside diff comments:
In `@Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift`:
- Around line 72-79: The early return in syncProcessExitWatch for matching pids
prevents cleanup of exit watchers when a session has ended, causing
DispatchSourceProcess sources to leak. Reorder the logic to check the session
state and extract the pid before the fast-path return condition: first validate
that record.state is not .ended and extract the pid, then only return early for
the same-pid case if the session is still live (not ended), ensuring that ended
sessions always proceed to cancel and remove their watchers from the
exitWatchers dictionary.
- Around line 211-219: The dead process check on line 211 using kill(pid_t($0),
0) treats all non-zero results as dead, but the watcher path correctly
distinguishes between ESRCH (process not found) and EPERM (permission denied,
meaning process is alive). Update the alive computation to check errno and only
mark the process as .ended when kill returns an error specifically due to ESRCH,
treating EPERM and other errors as indicating a live but unsignalable process,
so that live processes with restricted permissions are not incorrectly hidden
after relaunch.
In `@Sources/Mobile/AgentChat/AgentChatTranscriptService.swift`:
- Around line 48-52: The fire-and-forget Task in the start() method that calls
registry.seedFromHookStores() creates a race condition where
mobile.chat.sessions pulls can execute and return empty snapshots before seeding
completes. Either refactor start() to await seedFromHookStores() before
returning, or expose the seed task and ensure mobile.chat.sessions pulls wait
for its completion before serving authoritative snapshots. Additionally, per
Swift coding guidelines for cmux-owned code, this fire-and-forget Task must be
stored, cancelled, or tied to a caller-owned lifecycle operation rather than
left dangling.
In `@Sources/TerminalController`+MobileChat.swift:
- Around line 207-212: The v2MobileChatSend method makes two separate async
lookups (mobileChatTerminalParams and mobileChatTerminalPanel) which can become
inconsistent if the registry refreshes between the calls, potentially causing
the prompt to be cleared on one terminal while paste/image delivery uses
another. Create a single helper function that atomically resolves and returns
both the terminal params and panel as a tuple in one operation, replacing the
two separate guard statements that call mobileChatTerminalParams and
mobileChatTerminalPanel with a single call to this new helper function, ensuring
the entire send operation uses a consistent snapshot of the terminal binding.
- Around line 348-352: The code is using the stale pre-refresh workspaceID
parameter instead of the updated workspace ID from the refreshed record returned
by refreshSessionBindings. Extract the workspace ID from the refreshed record
and use it in both the mobileChatBindingResolves method call and the returned
dictionary instead of the workspaceID parameter. This ensures the current
workspace value after the hook-store backfill is used for binding resolution and
the return value, preventing stale workspace references that can cause
send/interrupt/answer operations to fail.
🪄 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: 93123e8a-a6bd-46f5-a09d-a46e2c9658c1
📒 Files selected for processing (15)
CLI/CMUXCLI+CodexFireAndForgetHooks.swiftCLI/cmux.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamEvent.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/MobileHost/ControlCommandCoordinator+MobileHost.swiftPackages/macOS/CmuxControlSocket/Sources/CmuxControlSocket/Coordinator/MobileHost/ControlMobileHostContext.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Spawn/TerminalSurface+StartupEnvironment.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/SurfaceValues/TerminalSurfaceCodexCommandShim.swiftResources/bin/cmux-codex-wrapperSources/Mobile/AgentChat/AgentChatSessionRegistry.swiftSources/Mobile/AgentChat/AgentChatTranscriptService.swiftSources/TerminalController+ControlMobileHostContext.swiftSources/TerminalController+MobileChat.swiftcmux.xcodeproj/project.pbxprojdocs/codex-agent-detection-plan.md
| guard let codexDef = Self.agentDef(named: "codex") else { | ||
| throw CLIError(message: "Codex hook integration is unavailable.") | ||
| } | ||
| var args: [String] = ["--enable", "hooks", "--dangerously-bypass-hook-trust"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
❓ Verification inconclusive
🌐 Web query:
For the OpenAI Codex CLI version range cmux supports, when was --dangerously-bypass-hook-trust introduced, and does it bypass review for all enabled hook sources in an invocation or only hooks injected via command-line -c arguments?
💡 Result:
The flag was introduced in OpenAI Codex via PR #21768 (May 8, 2026) and first stable in Codex 0.131.0 (after being merged May 13, 2026). [1][2] Effect: it is an invocation-scoped runtime-only override that bypasses the “persisted hook trust” requirement for enabled hooks for that specific Codex invocation, not just hooks injected via command-line -c arguments. Evidence: - PR #21768 states the flag “sets a runtime-only bypass_hook_trust override” and is “one invocation-scoped escape hatch,” plus explicitly notes it affects “enabled hooks” being allowed to run without requiring persisted hook trust for that invocation. [1] - The OpenAI Developers “Hooks” docs describe that for “one-off automation… pass --dangerously-bypass-hook-trust to run enabled hooks without requiring persisted hook trust for that invocation.” This implies the behavior is based on which hooks are enabled, not whether they were provided via -c arguments. [3] - Additional Codex issue discussion confirms there are separate CLI/non-interactive vs TUI behaviors, but in the described working behavior the bypass suppresses the startup trust prompt and “Enabled hooks run without requiring per-session trust (still respecting explicit disabled hooks).” [2] So, for a given Codex invocation where the flag is actually honored (e.g., not lost across a TUI/app-server path), it bypasses review/trust gating for all hooks that are enabled in that invocation (subject to explicit disabled hooks), regardless of whether they were supplied through -c arguments or otherwise discovered/loaded. [1][3][2] Note: there were reported cases where TUI mode startup initially didn’t respect the flag (blocking on the interactive “Hooks need review” prompt), but that was later fixed to skip the prompt appropriately; also, persistent/resumed threads may keep review prompt when the bypass override can’t be safely applied. [2][4]
Citations:
- 1: add --dangerously-bypass-hook-trust CLI flag openai/codex#21768
- 2:
--dangerously-bypass-hook-trustis ignored in TUI mode (codex 0.131.0–0.133.0): "Hooks need review" prompt still blocks startup openai/codex#24093 - 3: https://developers.openai.com/codex/hooks
- 4: Respect hook trust bypass during TUI startup openai/codex#24317
🏁 Script executed:
# Find and inspect the file with the flag usage
fd -t f "CMUXCLI+CodexFireAndForgetHooks.swift" -x wc -l {}Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Read the file with context around line 32
fd -t f "CMUXCLI+CodexFireAndForgetHooks.swift" -x cat -n {} | head -50Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for Codex version constraints in cmux
rg "version|Codex" -t swift --max-count=5 -B 2 -A 2 | head -80Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for hook configuration and bypass flag usage elsewhere
rg "dangerously-bypass-hook-trust|hook" -t swift -B 3 -A 3 | head -100Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Find the file in the entire repo
find . -name "*CodexFireAndForget*" -type f 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 198
🏁 Script executed:
# Search for the bypass flag usage in any file
rg "dangerously-bypass" -t swift -B 5 -A 5Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for Codex version or requirements
rg "Codex|codex" -t swift --max-count=10 | head -50Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look at file structure to understand where CLI files are
find . -path "*/CLI/*.swift" -type f 2>/dev/null | head -20Repository: manaflow-ai/cmux
Length of output: 857
🏁 Script executed:
# Read the CodexFireAndForgetHooks file to understand context
cat -n ./CLI/CMUXCLI+CodexFireAndForgetHooks.swiftRepository: manaflow-ai/cmux
Length of output: 5363
🏁 Script executed:
# Search for any version constraints or requirements for Codex
rg -i "codex.*version|requirement|support" -t swift --max-count=20Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look for how hooks are loaded/configured in cmux
rg -i "hook.*config|load.*hook|enabled.*hook" -t swift -B 2 -A 2 | head -80Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for Codex minimum version requirements or constraints
rg -i "0\.[0-9]+\.[0-9]+" -t swift -B 2 -A 2 | grep -i codexRepository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look for any configuration or user hook loading
rg "user.*hook|project.*hook|~/.config|\.cmux|hook.*source" -t swift -B 2 -A 2 | head -60Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check if there are any conditions/gates around the bypass flag usage
rg "bypass.*hook|hook.*bypass" -t swift -B 10 -A 10Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for Codex agent definition to understand supported versions
rg "agentDef|codex" -t swift | grep -i "version\|require\|support" | head -20Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look for agent definitions or Codex version info
find . -name "*.swift" -type f -exec grep -l "agentDef\|Agent.*Definition" {} \; | head -10Repository: manaflow-ai/cmux
Length of output: 500
🏁 Script executed:
# Check for any Codex-specific configuration or minimum version checks
rg "Codex\|codex" -t swift --context 3 | head -100Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Search for wrapper-related code that might show context
rg "wrapper|cmux-codex-wrapper|emitCodex" -t swift -B 2 -A 5 | head -80Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look at git history/blame for this file to understand intent
cd . && git log --oneline -20 -- CLI/CMUXCLI+CodexFireAndForgetHooks.swift 2>/dev/null || echo "No git history available"Repository: manaflow-ai/cmux
Length of output: 226
🏁 Script executed:
# Look at the agent hook definitions to understand version requirements
cat -n ./CLI/CMUXCLI+AgentHookDefinitions.swift | head -80Repository: manaflow-ai/cmux
Length of output: 5085
🏁 Script executed:
# Check the merge commit that added this feature
git show --stat d8e980f2 2>/dev/null | head -50Repository: manaflow-ai/cmux
Length of output: 3107
🏁 Script executed:
# Look for any documentation about Codex support or version requirements
rg -i "codex.*version|minimum.*codex|require.*codex" -t swift -t md -t txt | head -40Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check if there are any discussions or TODOs about the bypass flag
rg "dangerously|bypass|hook.*trust|trust.*hook" -t swift -B 3 -A 3Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Get more of the agent definitions to find Codex config
cat -n ./CLI/CMUXCLI+AgentHookDefinitions.swift | grep -A 20 "\"codex\""Repository: manaflow-ai/cmux
Length of output: 4311
🏁 Script executed:
# Search for how this function is called and whether there are any guards
rg "emitCodexWrapperInjectArgs" -t swift -B 5 -A 5Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look for any tests or documentation about Codex hook integration
find . -name "*.swift" -o -name "*.md" | xargs grep -l "CodexFireAndForget\|codex.*hook\|bypass.*hook" 2>/dev/nullRepository: manaflow-ai/cmux
Length of output: 1492
🏁 Script executed:
# Check if there's a README or docs about supported agent versions
find . -name "README*" -o -name "*SETUP*" -o -name "*AGENT*" | head -20Repository: manaflow-ai/cmux
Length of output: 962
🏁 Script executed:
# Look at the Codex agent detection plan
cat -n ./docs/codex-agent-detection-plan.md | head -100Repository: manaflow-ai/cmux
Length of output: 6068
🏁 Script executed:
# Look at agent hooks documentation
cat -n ./docs/agent-hooks.md | head -150Repository: manaflow-ai/cmux
Length of output: 11116
🏁 Script executed:
# Check the CLI contract for version constraints
rg "version|support|require" ./docs/cli-contract.md -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 5294
🏁 Script executed:
# Check if there's any version checking or gating for the bypass flag
rg "0\.131|codex.*version|version.*codex" -t swift -B 3 -A 3Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Look for any discussions or decision records about this trade-off
rg "bypass.*security|security.*bypass|hook.*trust|trust.*hook" -t md -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 154
🏁 Script executed:
# Check if there are any guards or conditions around enabling the bypass
ast-grep outline ./CLI/CMUXCLI+CodexFireAndForgetHooks.swiftRepository: manaflow-ai/cmux
Length of output: 355
🏁 Script executed:
# Look at how the wrapper is configured and whether user hooks are involved
rg "codex.*hook|hook.*codex" -t swift -B 3 -A 3 | head -100Repository: manaflow-ai/cmux
Length of output: 154
Gate Codex hook-trust bypass by version and scope.
Line 32 unconditionally emits --dangerously-bypass-hook-trust on every Codex invocation. The flag (introduced in Codex v0.131.0, May 2026) bypasses hook review for all enabled hooks in that invocation—not just the cmux-injected ones via -c arguments. Since Codex persists hooks in ~/.codex/hooks.json and config in ~/.codex/config.toml, any user or project hooks already installed there would bypass review too, even though cmux did not inject them. The design correctly states cmux should not clobber user Codex config, so a user with existing project/personal Codex hooks would have those hooks automatically authorized without their knowledge. Confirm that cmux-owned Codex wrapper injection scopes the bypass only to cmux-injected hooks, or gate the flag by minimum Codex version and fail closed if user hook sources are detected. Document the security boundary in the code.
🤖 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/CMUXCLI`+CodexFireAndForgetHooks.swift at line 32, The unconditional
emission of `--dangerously-bypass-hook-trust` on line 32 bypasses hook review
for all hooks in Codex (including any pre-existing user/project hooks in
~/.codex/hooks.json and ~/.codex/config.toml), not just cmux-injected ones,
which violates cmux's principle of not clobbering user Codex config. To fix
this, either verify that the cmux-owned Codex wrapper injection scopes the
bypass only to cmux-injected hooks via the `-c` arguments, or gate the
`--dangerously-bypass-hook-trust` flag by checking the Codex version (introduced
in v0.131.0) and fail closed if user hook sources are detected in the Codex
config. Additionally, document the security boundary in code comments explaining
which hooks this flag applies to and what assumptions are being made about the
scope of the bypass.
Comment on lines
+51
to
+63
| ## Design: PATH-shim wrapper that emits its own session-start | ||
|
|
||
| A `cmux-codex-wrapper`, mirroring `cmux-claude-wrapper`: | ||
|
|
||
| 1. cmux prepends a shim dir to `PATH` when it spawns its terminal shells (the | ||
| same mechanism Claude already uses). Typing `codex` resolves to the shim, not | ||
| the real binary. | ||
| 2. Before exec'ing the real codex, the wrapper emits the launch signal itself: | ||
| `cmux hooks codex session-start` carrying `CMUX_SURFACE_ID`, the cwd, and its | ||
| child pid. THIS is the reliable detection signal, and it needs nothing from | ||
| Codex — the wrapper, which cmux controls, is the source. (More robust than the | ||
| Claude path, where the signal comes from Claude's own hook.) | ||
| 3. The wrapper execs the real codex (resolved by skipping the shim on `PATH`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the design section with the implemented no-pre-exec signal behavior.
Line 51-63 and Line 130-131 still prescribe a wrapper-fired pre-exec session-start, but the shipped wrapper intentionally removed that path to avoid phantom fallback-* duplicate sessions. Please update this section so the doc’s source-of-truth matches runtime behavior.
Suggested doc patch
-## Design: PATH-shim wrapper that emits its own session-start
+## Design: PATH-shim wrapper with injected Codex hooks as the authoritative start signal
@@
-2. Before exec'ing the real codex, the wrapper emits the launch signal itself:
- `cmux hooks codex session-start` carrying `CMUX_SURFACE_ID`, the cwd, and its
- child pid. THIS is the reliable detection signal, and it needs nothing from
- Codex — the wrapper, which cmux controls, is the source. (More robust than the
- Claude path, where the signal comes from Claude's own hook.)
+2. The wrapper injects cmux Codex hooks per invocation and then execs real codex.
+ Codex's injected `SessionStart` hook is the authoritative start signal and
+ carries Codex's real `session_id`. (A pre-exec wrapper-fired start signal was
+ removed because it produced phantom fallback sessions.)
@@
-3. Wrapper emits `cmux hooks codex session-start` (surface / pid / cwd) before
- exec, and no-ops cleanly outside cmux.
+3. Wrapper injects hook args and no-ops cleanly outside cmux.Also applies to: 130-131
🤖 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 `@docs/codex-agent-detection-plan.md` around lines 51 - 63, The design section
describing the cmux-codex-wrapper (lines 51-63) currently prescribes that the
wrapper emits a pre-exec session-start signal via `cmux hooks codex
session-start`, but the actual implementation intentionally removed this
behavior to avoid creating phantom fallback-* duplicate sessions. Update the
design description to remove the reference to the wrapper firing the pre-exec
signal and instead document the actual runtime behavior where no pre-exec signal
is emitted. Also apply the same correction to lines 130-131 which describe the
same outdated signal pattern, ensuring the documentation reflects what was
actually shipped rather than the original design intent.
Comment on lines
+81
to
+88
| guard let workspaceID else { | ||
| // No filter: return all current-agent sessions across workspaces, | ||
| // resolving each via its stored binding as before. | ||
| let descriptors = service.sessionRecords(workspaceID: nil) | ||
| .filter { mobileChatBindingIsCurrentAgent($0) } | ||
| .map(\.descriptor) | ||
| let encoded = descriptors.compactMap { service.wirePayload($0) } | ||
| return .ok(["sessions": encoded]) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Re-resolve the current workspace before returning authoritative snapshots.
The scoped list path acknowledges record.workspaceID can be stale after relaunch, but the no-filter path and mobile.chat.session still trust record.descriptor as-is. A no-filter pull or single-session pull can therefore omit a live session or return a descriptor with a stale workspace, so iOS reconciliation does not actually self-heal. Use one shared “current descriptor for session surface” helper for both list modes and the single-session endpoint, and restamp before encoding.
As per path instructions, reliability-critical session/workspace identity should use a reliable structured source of truth rather than stale or disagreeing sources.
Also applies to: 139-150
🤖 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/TerminalController`+MobileChat.swift around lines 81 - 88, The code
is returning session descriptors without re-resolving them against the current
workspace, which can result in stale workspace information being returned to the
client. Create or use a shared helper function that takes a session descriptor
and returns an updated descriptor with the current workspace properly resolved.
In the no-filter path within the guard block (where descriptors are being
encoded with service.wirePayload), apply this helper to restamp each descriptor
before encoding. Also apply the same helper in the single-session endpoint path
(around lines 139-150) to ensure both the list and individual session endpoints
use the same authoritative mechanism for resolving current workspace information
before returning encoded payloads to the client.
Source: Path instructions
Comment on lines
+400
to
+411
| /// Agent-match core: whether an already-resolved `(workspace, terminalPanel)` | ||
| /// still looks like the agent the record represents. Resolution-free so the | ||
| /// workspace-filtered listing path can call it with the surface's CURRENT | ||
| /// workspace rather than the record's stale stored one. | ||
| private func mobileChatRecordMatchesAgent( | ||
| record: AgentChatSessionRecord, | ||
| workspace: Workspace, | ||
| terminalPanel: TerminalPanel | ||
| ) -> Bool { | ||
| let title = workspace.panelTitle(panelId: terminalPanel.id) ?? terminalPanel.displayTitle | ||
| let normalizedTitle = title.trimmingCharacters(in: .whitespacesAndNewlines).lowercased() | ||
| let context = WorkspaceContentView.terminalAgentContext(panel: terminalPanel, workspace: resolved.workspace) | ||
| let context = WorkspaceContentView.terminalAgentContext(panel: terminalPanel, workspace: workspace) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Do not use terminal title/name heuristics for agent identity.
This helper feeds correctness-critical chat-toggle and prompt-routing decisions, but it still prepares the terminal title for downstream "claude" / "codex" / prefix matching. A reused plain shell with a matching title can be treated as the session’s agent; fail closed unless the structured agent binding/context proves the match.
As per path instructions, correctness-critical agent/session identity must not be derived from terminal title/name heuristics or “better than nothing” fallbacks.
🤖 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/TerminalController`+MobileChat.swift around lines 400 - 411, The
mobileChatRecordMatchesAgent function currently uses terminal title/name
heuristics (extracting, normalizing, and matching title strings) to determine
agent identity, which is incorrect for correctness-critical decisions. Remove
the title extraction and normalization logic that prepares the terminal title
for downstream matching. Instead, validate the agent match using only structured
agent binding and context information obtained from
WorkspaceContentView.terminalAgentContext. The function should fail closed
(return false) unless the structured context definitively proves the agent
matches the record, not based on terminal title similarity.
Source: Path instructions
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
3 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.
Reworks how the iOS GUI ("coding agent" mode) tracks whether a terminal has an agent session, per docs/agent-session-tracking-spec.md. The old design fused three lossy signals (hook events, an on-disk store, and terminal-title / newest-jsonl-by-mtime guessing) and reconciled them heuristically, which showed the wrong conversation or none.
Dogfood status
Owner dogfooded on-device (iPhone): detection feels better than before. Landing this as a good starting point; known follow-ups remain (see below). Not auto-merged — merge at owner's discretion.
What changed (Slices A–D)
versionper session (one registry chokepoint); newmobile.chat.sessionsnapshot-pull RPC; the session-list reducer version-gates upserts so a stale/out-of-order push can't clobber newer state; iOS re-pulls the list on reconnect and foreground. Push is best-effort, pull is authoritative — a missed push self-heals.ended. ADispatchSourceProcess(.exit)watcher on the agent pid replaces thekill(pid,0)polling sweep; the session flips toendedthe instant the process exits (guarded againstclaude --resumere-using the id). The iOS GUI already disables the input bar onended, so it now disables reliably.seedFromHookStores, a deferrednoteHookEventbackfill,refreshBindingsFromHookStore); the send/interrupt/answer RPC chain threaded async to match..jsonl-by-mtime scan, the claim/forced-retry machinery, and provisional sessions. Kept the reliable hook-store persistence and by-session-id transcript resolution. (KeptRestorableAgentSessionIndex's session-id-keyed restore scan — not the unreliable guess.)Known follow-ups (not blocking this starting point)
~/.codexconfig. Codex is tracked today aftercmux hooks setup.Verification
CmuxAgentChatbuilds + 128 unit tests pass (incl. new version-gating test);CmuxMobileShell/CmuxMobileShellUIbuild for iOS; full macOS app builds.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements