Repository navigation
Codex agent detection (extension of #6631 agent-session tracking) - #6655
Merged
Merged
Conversation
…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>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughReplaces heuristic title/mtime-based agent session detection with deterministic hook-token binding. Introduces a ChangesAgent Session Tracking — Single Source of Truth
Sequence Diagram(s)sequenceDiagram
rect rgba(100, 180, 255, 0.5)
Note over WorkspaceDetailView, AgentChatSessionRegistry: iOS foreground reconciliation
end
participant WorkspaceDetailView
participant MobileChatEventSource
participant TerminalController
participant AgentChatTranscriptService
participant AgentChatSessionRegistry
WorkspaceDetailView->>WorkspaceDetailView: scenePhase → .active, foreground epoch changes chatRefreshKey
WorkspaceDetailView->>MobileChatEventSource: session(sessionID:)
MobileChatEventSource->>TerminalController: RPC "mobile.chat.session" {session_id}
TerminalController->>AgentChatTranscriptService: await refreshSessionBindings(sessionID:)
AgentChatTranscriptService->>AgentChatSessionRegistry: async refreshBindingsFromHookStore (Task.detached read)
AgentChatSessionRegistry-->>AgentChatTranscriptService: updated AgentChatSessionRecord (with version)
AgentChatTranscriptService-->>TerminalController: AgentChatSessionRecord?
TerminalController-->>MobileChatEventSource: MobileChatSessionResponse {session: ChatSessionDescriptor}
MobileChatEventSource-->>WorkspaceDetailView: ChatSessionDescriptor (authoritative, versioned)
sequenceDiagram
rect rgba(150, 230, 150, 0.5)
Note over TerminalSurface, codex: Codex hook injection via PATH shim
end
participant TerminalSurface
participant installCodexCommandShimIfPossible
participant codexShimScript as codex shim (per-surface)
participant cmuxCodexWrapper as cmux-codex-wrapper
participant codex as real codex
TerminalSurface->>installCodexCommandShimIfPossible: claudeWrapperURL, shimDirectory
installCodexCommandShimIfPossible-->>codexShimScript: write + chmod executable shim
Note over codexShimScript: PATH prepended at terminal launch
codexShimScript->>cmuxCodexWrapper: exec wrapper (CMUX_SURFACE_ID set)
cmuxCodexWrapper->>cmuxCodexWrapper: find_real_codex, check socket, classify argv
cmuxCodexWrapper->>cmuxCodexWrapper: export CMUX_CODEX_PID, CMUX_LAUNCH_KIND, build TOML hooks
cmuxCodexWrapper->>codex: exec with --enable-hooks, --dangerously-bypass-hook-trust, -c overrides
codex-->>cmuxCodexWrapper: (hook event fires per lifecycle)
cmuxCodexWrapper-->>AgentChatSessionRegistry: cmux hooks codex <event> (via cmux CLI)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (5 errors, 1 warning)
✅ Passed checks (17 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 |
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
Greptile SummaryThis PR adds Codex agent session detection to cmux by mirroring the existing Claude wrapper pattern: a new
Confidence Score: 4/5Safe to merge pending the workspace-not-found empty-list edge case in the new mobile.chat.sessions workspace-scoped path being confirmed or addressed. The wrapper, hook injection, and registry wiring are all solid — every failure path falls through to plain Sources/TerminalController+MobileChat.swift — the workspace-not-found early return emitting an empty session list rather than an error deserves a second look. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant User as User (types codex)
participant Shim as codex shim (per-surface PATH)
participant Wrapper as cmux-codex-wrapper
participant CLI as cmux CLI (inject-args)
participant Codex as Real Codex
participant Hook as Codex SessionStart Hook
participant Feed as feed.push / noteHookEvent
participant Registry as AgentChatSessionRegistry (@MainActor)
User->>Shim: exec codex [args]
Shim->>Wrapper: exec cmux-codex-wrapper [args]
Wrapper->>Wrapper: find_real_codex, cmux_socket_available
Wrapper->>Wrapper: should_inject_codex_hooks?
Wrapper->>CLI: cmux hooks codex inject-args (NUL stream)
CLI-->>Wrapper: "--enable hooks --dangerously-bypass-hook-trust -c hooks.SessionStart=[...] ..."
Wrapper->>Codex: exec real codex [injected-args] [user-args]
Note over Wrapper,Codex: CMUX_CODEX_PID=$$, CMUX_SURFACE_ID exported
Codex->>Hook: fires SessionStart hook (real session_id)
Hook->>Feed: cmux hooks codex session-start (surface_id, transcript_path)
Feed->>Registry: noteHookEvent(WorkstreamEvent with surfaceId, transcriptPath)
Registry->>Registry: bind surfaceID + transcriptPath, stamp version
Registry-->>Feed: "onRecordChanged -> descriptorChanged push to iOS"
Codex->>Hook: fires Stop hook on exit
Hook->>Feed: cmux hooks codex stop
Feed->>Registry: "noteHookEvent -> state=ended"
%%{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 User as User (types codex)
participant Shim as codex shim (per-surface PATH)
participant Wrapper as cmux-codex-wrapper
participant CLI as cmux CLI (inject-args)
participant Codex as Real Codex
participant Hook as Codex SessionStart Hook
participant Feed as feed.push / noteHookEvent
participant Registry as AgentChatSessionRegistry (@MainActor)
User->>Shim: exec codex [args]
Shim->>Wrapper: exec cmux-codex-wrapper [args]
Wrapper->>Wrapper: find_real_codex, cmux_socket_available
Wrapper->>Wrapper: should_inject_codex_hooks?
Wrapper->>CLI: cmux hooks codex inject-args (NUL stream)
CLI-->>Wrapper: "--enable hooks --dangerously-bypass-hook-trust -c hooks.SessionStart=[...] ..."
Wrapper->>Codex: exec real codex [injected-args] [user-args]
Note over Wrapper,Codex: CMUX_CODEX_PID=$$, CMUX_SURFACE_ID exported
Codex->>Hook: fires SessionStart hook (real session_id)
Hook->>Feed: cmux hooks codex session-start (surface_id, transcript_path)
Feed->>Registry: noteHookEvent(WorkstreamEvent with surfaceId, transcriptPath)
Registry->>Registry: bind surfaceID + transcriptPath, stamp version
Registry-->>Feed: "onRecordChanged -> descriptorChanged push to iOS"
Codex->>Hook: fires Stop hook on exit
Hook->>Feed: cmux hooks codex stop
Feed->>Registry: "noteHookEvent -> state=ended"
Reviews (3): Last reviewed commit: "agent-session: scope mobile chat session..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+4
to
+10
| # When running inside a cmux terminal (CMUX_SURFACE_ID is set), this wrapper | ||
| # makes `codex` carry cmux's hooks for THIS invocation only (nothing is written | ||
| # to ~/.codex), so Codex's own SessionStart/UserPromptSubmit/Stop/PreToolUse/ | ||
| # PostToolUse/PermissionRequest fire back into cmux with Codex's real session_id. | ||
| # It also fires a best-effort one-way `cmux hooks codex session-start` BEFORE | ||
| # exec so a session is detected at launch even if Codex's own SessionStart is | ||
| # delayed; the registry dedups by session id so the two are idempotent. |
Contributor
There was a problem hiding this comment.
cmux hooks codex session-start BEFORE exec", but the code explicitly does not fire any wrapper-initiated session-start (the NOTE at line 253 explains why it was removed to prevent phantom duplicate sessions). The contradicting header will mislead future readers about the wrapper's actual behavior.
Suggested change
| # When running inside a cmux terminal (CMUX_SURFACE_ID is set), this wrapper | |
| # makes `codex` carry cmux's hooks for THIS invocation only (nothing is written | |
| # to ~/.codex), so Codex's own SessionStart/UserPromptSubmit/Stop/PreToolUse/ | |
| # PostToolUse/PermissionRequest fire back into cmux with Codex's real session_id. | |
| # It also fires a best-effort one-way `cmux hooks codex session-start` BEFORE | |
| # exec so a session is detected at launch even if Codex's own SessionStart is | |
| # delayed; the registry dedups by session id so the two are idempotent. | |
| # When running inside a cmux terminal (CMUX_SURFACE_ID is set), this wrapper | |
| # makes `codex` carry cmux's hooks for THIS invocation only (nothing is written | |
| # to ~/.codex), so Codex's own SessionStart/UserPromptSubmit/Stop/PreToolUse/ | |
| # PostToolUse/PermissionRequest fire back into cmux with Codex's real session_id. | |
| # No wrapper-fired pre-exec session-start is sent; Codex's own injected | |
| # SessionStart carries the real session_id and is the single authoritative signal. |
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!
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
CLI/cmux.swift (1)
29515-29518: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPass
surfaceIdthrough teardown telemetry.
performAgentSessionTeardown()still emits telemetry with onlyworkspaceId. In non-turn-boundary teardown/finalize paths, this dropssurface_idfromfeed.push, making binding less deterministic than the other updated hook paths.💡 Proposed fix
- sendAgentFeedTelemetry(workspaceId: mapped.workspaceId) + sendAgentFeedTelemetry( + workspaceId: mapped.workspaceId, + surfaceId: mapped.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 29515 - 29518, The performAgentSessionTeardown() function currently calls sendAgentFeedTelemetry() with only the workspaceId parameter, but it is missing the surfaceId which is required for deterministic binding in the telemetry feed.push. Extract or retrieve the surfaceId from the available context (likely from the mapped lookup result or env parameter) and update the sendAgentFeedTelemetry() call to pass both workspaceId and surfaceId as parameters to ensure surface_id is included in the telemetry, consistent with other updated hook paths.Sources/TerminalController+MobileChat.swift (1)
300-304: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUse refreshed workspace ID after binding refresh.
Line 300 refreshes the session record, but Lines 302 and 304 still use the stale pre-refresh
workspaceID. If the session moved workspaces, the refreshedsurfaceIDwon’t resolve under the old workspace, producing falsenot_foundfailures for mobile send/interrupt/answer.💡 Suggested fix
- if let refreshed = await service.refreshSessionBindings(sessionID: sessionID), - let surfaceID = refreshed.surfaceID, - mobileChatBindingResolves(workspaceID: workspaceID, surfaceID: surfaceID), - mobileChatBindingIsCurrentAgent(refreshed) { - return ["workspace_id": workspaceID, "surface_id": surfaceID] + if let refreshed = await service.refreshSessionBindings(sessionID: sessionID), + let refreshedWorkspaceID = refreshed.workspaceID, + let surfaceID = refreshed.surfaceID, + mobileChatBindingResolves(workspaceID: refreshedWorkspaceID, surfaceID: surfaceID), + mobileChatBindingIsCurrentAgent(refreshed) { + 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 300 - 304, The session binding refresh returns updated workspace and surface information in the refreshed object, but the code is still using the stale pre-refresh workspaceID variable in both the mobileChatBindingResolves call and the return statement. Extract the workspace ID from the refreshed object in the same manner that surfaceID is extracted (adding it as a condition in the guard statement), then use this refreshed workspace ID instead of the stale workspaceID variable in the mobileChatBindingResolves function call on line 302 and in the returned dictionary on line 304.Sources/Mobile/AgentChat/AgentChatTranscriptService.swift (1)
221-233: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winClear failed resolution state when a transcript path arrives.
If
ensureTailerfailed earlier,failedResolutionsstill contains the session id. When async backfill later makestranscriptPathnon-nil, Line 233 callsensureTailer, but Line 161 returns nil, so subscribers never start tailing until a later explicit retry.Proposed fix
let stateChanged = previous?.state != record.state let transcriptBecameAvailable = previous?.transcriptPath == nil && record.transcriptPath != nil + if transcriptBecameAvailable { + failedResolutions.remove(record.sessionID) + } if stateChanged, record.state == .ended { if let tailer = tailers.removeValue(forKey: record.sessionID) {🤖 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 221 - 233, When a transcript path becomes available (transcriptBecameAvailable is true), clear any failed resolution state for that session before calling ensureTailer. Specifically, in the condition block where transcriptBecameAvailable is true and record.state is not ended, remove the record.sessionID from the failedResolutions collection before calling ensureTailer(for: record). This ensures that ensureTailer will attempt to create the tailer again instead of returning nil due to the session still being marked as failed from an earlier attempt.
🤖 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`:
- Around line 355-363: The documentation for Slice F regarding codex hook
auto-setup in the lines describing the deferred status is out of sync with the
Codex plan file in this PR that indicates wrapper-based detection has been
implemented. Update the Slice F status text (currently stating it is deferred
and needs a product decision) to reflect that the wrapper-based detection
approach for Codex has been implemented, ensuring the documentation aligns with
the actual rollout state described in the Codex plan file.
In `@docs/codex-agent-detection-plan.md`:
- Around line 7-14: The document describes two materially different Codex
detection mechanisms: hook injection via `--enable hooks
--dangerously-bypass-hook-trust -c hooks.<event>=...` (lines 7-14) and
wrapper-emitted `session-start` without Codex hooks (lines 58-79). Identify
which mechanism matches the actual shipped implementation and remove the
documentation describing the alternative approach that is not used. Ensure the
entire document consistently describes only the one detection mechanism that is
currently implemented to prevent incorrect follow-on implementations.
In
`@Packages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Store/ChatSessionListReducer.swift`:
- Around line 41-53: The version gating guard currently protects only
`.descriptorChanged` updates, but `.stateChanged` frames bypass this check and
can still apply out-of-order updates unconditionally. Identify where
`.stateChanged` is handled in the ChatSessionListReducer and apply the same
version comparison logic that exists for `.descriptorChanged` (checking that the
incoming version is >= the current version before allowing the update) to ensure
both descriptor and state changes respect the monotonic reconciliation
guarantee.
In `@Sources/Mobile/AgentChat/AgentChatSessionRegistry.swift`:
- Line 211: The liveness check using `kill(pid_t($0), 0) == 0` on line 211
incorrectly treats any failure from the kill syscall as indicating a dead
process, while the proper logic should only treat ESRCH (process not found) as
dead. Replace the inline kill check in that line with a call to the existing
`processIsDead(_:)` function that correctly implements the ESRCH-only check,
ensuring that inaccessible-but-existing processes are not incorrectly seeded as
ended.
- Around line 327-336: Create a new mutating method named adoptMissingBindings
on the AgentChatSessionRecord type that conditionally assigns fields from the
entry parameter only when the current fields are nil, rather than
unconditionally overwriting them as the current adoptBindings method does. Then
replace both calls to adoptBindings in the guard block and update closure with
calls to the new adoptMissingBindings method to prevent deferred store reads
from overwriting live hook-provided session bindings like surfaceID,
workspaceID, transcriptPath, and workingDirectory.
- Around line 173-175: In the
AgentChatTranscriptService.descriptorChangedMeaningfully method, modify the
comparison logic to exclude the version field when determining if a descriptor
has meaningfully changed. The current comparison treats any version difference
as a meaningful change, but since version is stamped on every update regardless
of actual descriptor content changes, you should compare all other descriptor
properties while ignoring version differences. This prevents false positives for
pure lastActivityAt bumps and preserves the tool-storm suppression logic.
- Around line 72-79: In the syncProcessExitWatch method, the early return
condition (when existing.pid equals record.pid) executes before checking whether
the record has ended or its pid is nil. This causes the cleanup code that
cancels and removes the exit watcher to be skipped when a process has exited.
Move the early return condition to also include checks that the record state is
not .ended and the pid is not nil, ensuring that exit watchers are properly
cancelled and removed from the dictionary even when the record has ended,
preventing the process source from being retained indefinitely.
---
Outside diff comments:
In `@CLI/cmux.swift`:
- Around line 29515-29518: The performAgentSessionTeardown() function currently
calls sendAgentFeedTelemetry() with only the workspaceId parameter, but it is
missing the surfaceId which is required for deterministic binding in the
telemetry feed.push. Extract or retrieve the surfaceId from the available
context (likely from the mapped lookup result or env parameter) and update the
sendAgentFeedTelemetry() call to pass both workspaceId and surfaceId as
parameters to ensure surface_id is included in the telemetry, consistent with
other updated hook paths.
In `@Sources/Mobile/AgentChat/AgentChatTranscriptService.swift`:
- Around line 221-233: When a transcript path becomes available
(transcriptBecameAvailable is true), clear any failed resolution state for that
session before calling ensureTailer. Specifically, in the condition block where
transcriptBecameAvailable is true and record.state is not ended, remove the
record.sessionID from the failedResolutions collection before calling
ensureTailer(for: record). This ensures that ensureTailer will attempt to create
the tailer again instead of returning nil due to the session still being marked
as failed from an earlier attempt.
In `@Sources/TerminalController`+MobileChat.swift:
- Around line 300-304: The session binding refresh returns updated workspace and
surface information in the refreshed object, but the code is still using the
stale pre-refresh workspaceID variable in both the mobileChatBindingResolves
call and the return statement. Extract the workspace ID from the refreshed
object in the same manner that surfaceID is extracted (adding it as a condition
in the guard statement), then use this refreshed workspace ID instead of the
stale workspaceID variable in the mobileChatBindingResolves function call on
line 302 and in the returned dictionary on line 304.
🪄 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: 94b84513-a760-4cf9-a007-f2d3b642de12
📒 Files selected for processing (25)
CLI/cmux.swiftPackages/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.swiftPackages/macOS/CMUXAgentLaunch/Sources/CMUXAgentLaunch/Workstream/WorkstreamEvent.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Spawn/TerminalSurface+StartupEnvironment.swiftPackages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface.swiftPackages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/SurfaceValues/TerminalSurfaceCodexCommandShim.swiftResources/Localizable.xcstringsResources/bin/cmux-codex-wrapperSources/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.mddocs/codex-agent-detection-plan.md
💤 Files with no reviewable changes (4)
- Sources/Mobile/AgentChat/AgentChatTranscriptService+TitleDetection.swift
- cmuxTests/AgentChatTranscriptResolverTests.swift
- Sources/TerminalController+MobileWorkspaceList.swift
- Sources/Mobile/AgentChat/AgentChatTranscriptResolver.swift
Comment on lines
+355
to
+363
| - **F (codex hook auto-setup): deferred, needs a product decision.** The only | ||
| way to "guarantee" codex hooks is to silently edit the user's | ||
| `~/.codex/config.toml` trust entries + `hooks.json` at launch — a standing | ||
| modification to external tooling config that should not be done silently and | ||
| warrants its own PR. Codex is tracked today for anyone who ran | ||
| `cmux hooks setup` (the existing onboarding step). Safe future options: an | ||
| explicit "install codex hooks" affordance, or a one-line launch hint when a | ||
| codex agent starts without hooks installed. | ||
| - **Slice C — Off-main parsing.** Move the hook-store JSON read off `@MainActor` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Sync Slice F status with the implemented Codex wrapper path
Line 355–Line 363 still says Slice F is deferred and gated on a product decision, which conflicts with the Codex plan file in this PR that marks wrapper-based detection as implemented. Please align this status text so the two docs don’t describe opposite rollout states.
🤖 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` around lines 355 - 363, The
documentation for Slice F regarding codex hook auto-setup in the lines
describing the deferred status is out of sync with the Codex plan file in this
PR that indicates wrapper-based detection has been implemented. Update the Slice
F status text (currently stating it is deferred and needs a product decision) to
reflect that the wrapper-based detection approach for Codex has been
implemented, ensuring the documentation aligns with the actual rollout state
described in the Codex plan file.
Comment on lines
+7
to
+14
| Done: `cmux-codex-wrapper` (PATH shim, Claude-parity per-invocation `[hooks]` | ||
| injection via `--enable hooks --dangerously-bypass-hook-trust -c hooks.<event>=...`), | ||
| codex PATH-shim install sibling to the claude shim, `WorkstreamEvent` + | ||
| `feed.push` + `noteHookEvent` now carry `surface_id`/`transcript_path`. Two | ||
| preflight-caught bugs fixed: phantom `fallback-*` duplicate (removed the | ||
| wrapper-fired empty-stdin launch signal) and unbound-surface on live | ||
| session-start. Live debug-socket proof: a real `codex exec` produced exactly one | ||
| codex session, surface + transcript bound, `idle -> ended` on exit. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document one detection mechanism consistently
Line 7–Line 14 describes hook injection (--enable hooks ... -c hooks...), but Line 58–Line 79 describes a different mechanism (wrapper-emitted session-start with no Codex hooks required). These are materially different contracts; keep only the path that matches shipped behavior to prevent wrong follow-on implementation.
Also applies to: 58-79
🤖 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 7 - 14, The document
describes two materially different Codex detection mechanisms: hook injection
via `--enable hooks --dangerously-bypass-hook-trust -c hooks.<event>=...` (lines
7-14) and wrapper-emitted `session-start` without Codex hooks (lines 58-79).
Identify which mechanism matches the actual shipped implementation and remove
the documentation describing the alternative approach that is not used. Ensure
the entire document consistently describes only the one detection mechanism that
is currently implemented to prevent incorrect follow-on implementations.
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 | 🏗️ Heavy lift
Version gating is bypassed by unversioned stateChanged frames
This guard protects only .descriptorChanged; an out-of-order .stateChanged can still clobber newer pulled state because it applies unconditionally. That breaks the monotonic reconciliation guarantee introduced here. Consider carrying version on stateChanged (or consolidating to versioned descriptor updates) and gating that path too.
🤖 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 version gating guard currently protects only
`.descriptorChanged` updates, but `.stateChanged` frames bypass this check and
can still apply out-of-order updates unconditionally. Identify where
`.stateChanged` is handled in the ChatSessionListReducer and apply the same
version comparison logic that exists for `.descriptorChanged` (checking that the
incoming version is >= the current version before allowing the update) to ensure
both descriptor and state changes respect the monotonic reconciliation
guarantee.
Comment on lines
+72
to
+79
| private func syncProcessExitWatch(for record: AgentChatSessionRecord) { | ||
| let sessionID = record.sessionID | ||
| if let existing = exitWatchers[sessionID], existing.pid == record.pid { | ||
| return | ||
| } | ||
| exitWatchers[sessionID]?.source.cancel() | ||
| exitWatchers[sessionID] = nil | ||
| guard record.state != .ended, let pid = record.pid else { return } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Cancel same-pid watchers when the record has ended.
The early return on Line 74 runs before the .ended/nil-pid guard, so handleProcessExit calling update { $0.state = .ended } leaves the exited process source retained in exitWatchers forever.
Proposed fix
private func syncProcessExitWatch(for record: AgentChatSessionRecord) {
let sessionID = record.sessionID
- if let existing = exitWatchers[sessionID], existing.pid == record.pid {
- return
- }
+ guard record.state != .ended, let pid = record.pid else {
+ exitWatchers[sessionID]?.source.cancel()
+ exitWatchers[sessionID] = nil
+ return
+ }
+ if 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 }📝 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
| private func syncProcessExitWatch(for record: AgentChatSessionRecord) { | |
| let sessionID = record.sessionID | |
| if let existing = exitWatchers[sessionID], existing.pid == record.pid { | |
| return | |
| } | |
| exitWatchers[sessionID]?.source.cancel() | |
| exitWatchers[sessionID] = nil | |
| guard record.state != .ended, let pid = record.pid else { return } | |
| private func syncProcessExitWatch(for record: AgentChatSessionRecord) { | |
| let sessionID = record.sessionID | |
| guard record.state != .ended, let pid = record.pid else { | |
| exitWatchers[sessionID]?.source.cancel() | |
| exitWatchers[sessionID] = nil | |
| return | |
| } | |
| if let existing = exitWatchers[sessionID], existing.pid == pid { | |
| return | |
| } | |
| exitWatchers[sessionID]?.source.cancel() | |
| exitWatchers[sessionID] = nil |
🤖 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, In the syncProcessExitWatch method, the early return condition (when
existing.pid equals record.pid) executes before checking whether the record has
ended or its pid is nil. This causes the cleanup code that cancels and removes
the exit watcher to be skipped when a process has exited. Move the early return
condition to also include checks that the record state is not .ended and the pid
is not nil, ensuring that exit watchers are properly cancelled and removed from
the dictionary even when the record has ended, preventing the process source
from being retained indefinitely.
Comment on lines
+173
to
+175
| stampVersion(&record) | ||
| records[sessionID] = record | ||
| syncProcessExitWatch(for: record) |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Keep version bumps out of the “meaningful descriptor change” comparison.
Because every update now stamps a new version, AgentChatTranscriptService.descriptorChangedMeaningfully will see record.descriptor differ on version alone and emit descriptor pushes for pure lastActivityAt bumps, defeating its tool-storm suppression.
Proposed fix in `AgentChatTranscriptService.descriptorChangedMeaningfully`
guard var normalizedPrevious = previous else { return true }
normalizedPrevious.lastActivityAt = current.lastActivityAt
+ normalizedPrevious.version = current.version
return normalizedPrevious.descriptor != current.descriptor🤖 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 173 -
175, In the AgentChatTranscriptService.descriptorChangedMeaningfully method,
modify the comparison logic to exclude the version field when determining if a
descriptor has meaningfully changed. The current comparison treats any version
difference as a meaningful change, but since version is stamped on every update
regardless of actual descriptor content changes, you should compare all other
descriptor properties while ignoring version differences. This prevents false
positives for pure lastActivityAt bumps and preserves the tool-storm suppression
logic.
| for entry in hookStore.entries(agentSource: source) { | ||
| for entry in entries { | ||
| guard records[entry.sessionID] == nil else { continue } | ||
| let alive = entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use the same ESRCH-only liveness check during seeding.
Line 211 treats any kill(pid, 0) failure as dead, while this file’s watcher path correctly treats only ESRCH as dead. Reuse processIsDead(_:) so inaccessible-but-existing processes are not seeded as .ended.
Proposed fix
- let alive = entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false
+ let alive = entry.pid.map { !processIsDead($0) } ?? false📝 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
| let alive = entry.pid.map { kill(pid_t($0), 0) == 0 } ?? false | |
| let alive = entry.pid.map { !processIsDead($0) } ?? 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` at line 211, The
liveness check using `kill(pid_t($0), 0) == 0` on line 211 incorrectly treats
any failure from the kill syscall as indicating a dead process, while the proper
logic should only treat ESRCH (process not found) as dead. Replace the inline
kill check in that line with a call to the existing `processIsDead(_:)` function
that correctly implements the ESRCH-only check, ensuring that
inaccessible-but-existing processes are not incorrectly seeded as ended.
Comment on lines
+327
to
+336
| var candidate = current | ||
| candidate.adoptBindings(from: entry, includingPID: current.pid == nil) | ||
| guard candidate.surfaceID != current.surfaceID | ||
| || candidate.workspaceID != current.workspaceID | ||
| || candidate.transcriptPath != current.transcriptPath | ||
| || candidate.workingDirectory != current.workingDirectory | ||
| || candidate.pid != current.pid else { return } | ||
| update(sessionID: sessionID) { record in | ||
| record.adoptBindings(from: entry, includingPID: record.pid == nil) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not let deferred store backfill overwrite live hook fields.
The comment says the deferred read fills only still-nil fields, but adoptBindings overwrites any non-nil surfaceID, workspaceID, transcriptPath, and workingDirectory from the store. A lagging hook-store read can therefore clobber the immediate surfaceId/transcriptPath carried by the live event and rebind the session to stale terminal data.
Proposed direction
- candidate.adoptBindings(from: entry, includingPID: current.pid == nil)
+ candidate.adoptMissingBindings(from: entry, includingPID: current.pid == nil)
guard candidate.surfaceID != current.surfaceID
|| candidate.workspaceID != current.workspaceID
|| candidate.transcriptPath != current.transcriptPath
|| candidate.workingDirectory != current.workingDirectory
|| candidate.pid != current.pid else { return }
update(sessionID: sessionID) { record in
- record.adoptBindings(from: entry, includingPID: record.pid == nil)
+ record.adoptMissingBindings(from: entry, includingPID: record.pid == nil)
}Add a missing-only helper on AgentChatSessionRecord:
mutating func adoptMissingBindings(
from entry: AgentChatHookSessionStore.Entry,
includingPID: Bool = true
) {
if surfaceID == nil { surfaceID = entry.surfaceID }
if workspaceID == nil { workspaceID = entry.workspaceID }
if transcriptPath == nil { transcriptPath = entry.transcriptPath }
if workingDirectory == nil { workingDirectory = entry.workingDirectory }
if includingPID, pid == nil { pid = entry.pid }
}🤖 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 327 -
336, Create a new mutating method named adoptMissingBindings on the
AgentChatSessionRecord type that conditionally assigns fields from the entry
parameter only when the current fields are nil, rather than unconditionally
overwriting them as the current adoptBindings method does. Then replace both
calls to adoptBindings in the guard block and update closure with calls to the
new adoptMissingBindings method to prevent deferred store reads from overwriting
live hook-provided session bindings like surfaceID, workspaceID, transcriptPath,
and workingDirectory.
azooz2003-bit
changed the title
… 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>
This was referenced Jun 24, 2026
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #6631. Base is
feat-agent-session-sot, notmain— Codex is consolidated as an extension of the agent-session-tracking work, so this diff is only the Codex commits on top of that branch. It merges intofeat-agent-session-sot(then #6631 carries everything to main as one consolidated change). Plan:docs/codex-agent-detection-plan.md. Base/return point: tagagent-session-sot-landmark.DO NOT MERGE until dogfood approval. Runtime change; gated on the owner confirming Codex tracking on-device.
The problem
Codex wasn't detected: cmux's codex hooks weren't installed in
~/.codex, codex's single legacynotifyslot was taken by Computer Use, and the title/mtime fallback was (intentionally) deleted in #6631. So a hook-less codex was invisible.The fix — the Claude primitive, generalized
cmux-codex-wrapper, a PATH-shim mirroringcmux-claude-wrapper. When you typecodexin a cmux terminal, the shim runs the wrapper, which injects codex's own[hooks]per-invocation (--enable hooks --dangerously-bypass-hook-trust -c 'hooks.SessionStart=...'etc.) so codex firesSessionStart/Stop/PermissionRequest/… with its real session id, which already flowscmux hooks codex <event>→feed.push→noteHookEvent→ the registry (source-agnostic, identical to Claude). Nothing is written to~/.codex; per-invocation only. Detection depends only on what cmux controls (the wrapper + injected env + parented pid). Passthrough-safe: every failure path execs the real codex unchanged; non-session subcommands pass through.Two bugs caught by live preflight and fixed here:
fallback-*duplicate session.WorkstreamEvent/feed.pushdidn't carrysurface_id/transcript_path, and the registry store-backfill is suppressed on session-start. Now threaded through the live event (also fixes Claude's new-session binding latency).Live verification (debug socket)
Real
codex execthrough the wrapper on a tagged build:Exactly one codex session (no phantom), surface + transcript bound,
idle → endedon exit.🤖 Generated with Claude Code