Repository navigation
Fix Codex transcript fallback session binding - #6993
Closed
austinywang wants to merge 53 commits into
Closed
austinywang wants to merge 53 commits into
austinywang wants to merge 53 commits into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCodex transcript lookup now validates rollout metadata through a shared locator and resolver path. The mobile service resolves Codex transcripts before tailing, and several iOS UI paths now require newer Swift compiler checks or main-actor annotations. ChangesCodex transcript lookup and resolution
iOS compiler guards
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (21 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR fixes a correctness bug (issue #5550) where Codex transcript fallback could bind a chat pane to the wrong conversation by scanning rollout directories and matching session IDs via substring heuristics. The fix removes the fallback scan entirely — both the mobile
Confidence Score: 5/5Safe to merge; the behavioral change is deliberate and well-tested — Codex transcript resolution now fails closed rather than scanning directories, eliminating the wrong-conversation binding bug. The core fix (removing the Codex directory-scan fallback) is straightforward and correct. A focused CI regression gate is in place and correctly bypasses the XCTest summary parser that silently swallows Swift Testing results. The iOS/Swift 6.2 gating and @mainactor additions address real compiler requirements without introducing new isolation hazards. The only finding is a naming redundancy between two now-identical methods, which has no runtime impact. No files require special attention. The resolver and CLI changes are deliberate, the test coverage directly exercises the fixed invariant, and the iOS compatibility edits are mechanical. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Hook as Hook Event
participant Service as AgentChatTranscriptService
participant Resolver as AgentChatTranscriptResolver
participant FS as Filesystem
Note over Hook,FS: Before fix (bug - #5550)
Hook->>Service: session record has no transcriptPath
Service->>Resolver: transcriptPath(for: codexRecord)
Resolver->>FS: scan ~/.codex/sessions for sessionID substring
FS-->>Resolver: rollout matching substring (may be wrong session)
Resolver-->>Service: wrong rollout path returned
Note over Hook,FS: After fix (this PR)
Hook->>Service: session record has hook-recorded transcriptPath
Service->>Resolver: transcriptPath(for: codexRecord)
Resolver->>Resolver: check recordedTranscriptPath
alt hook-recorded path exists on disk
Resolver-->>Service: correct hook-recorded path
else no hook-recorded path
Resolver-->>Service: nil returned (fail closed)
end
%%{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 Hook as Hook Event
participant Service as AgentChatTranscriptService
participant Resolver as AgentChatTranscriptResolver
participant FS as Filesystem
Note over Hook,FS: Before fix (bug - #5550)
Hook->>Service: session record has no transcriptPath
Service->>Resolver: transcriptPath(for: codexRecord)
Resolver->>FS: scan ~/.codex/sessions for sessionID substring
FS-->>Resolver: rollout matching substring (may be wrong session)
Resolver-->>Service: wrong rollout path returned
Note over Hook,FS: After fix (this PR)
Hook->>Service: session record has hook-recorded transcriptPath
Service->>Resolver: transcriptPath(for: codexRecord)
Resolver->>Resolver: check recordedTranscriptPath
alt hook-recorded path exists on disk
Resolver-->>Service: correct hook-recorded path
else no hook-recorded path
Resolver-->>Service: nil returned (fail closed)
end
Reviews (36): Last reviewed commit: "Fix duplicate test source wiring" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
austinywang
force-pushed
the
issue-5550-token-count-shows-climbing-phantom-values-for
branch
from
June 27, 2026 01:59
41744fd to
24bb84a
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Mobile/AgentChat/AgentChatTranscriptResolver.swift (1)
185-196: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftUnbounded recursive scan now performs file I/O per rollout — bound it to recent day directories.
The enumerator walks the entire
~/.codex/sessions/**tree with no recency ordering or limit. Previously each candidate was rejected with a cheap filename substring check (no I/O); now every.jsonlis opened and parsed viaCodexTranscriptIdentityMatcheruntil a match is found. For an unknown/wedged session id (the exact scenario this PR targets), the loop reads and parses every rollout the user has ever accumulated. The doc comment on Line 176 says "scan recent day directories," but the code does not actually bound to recent days.Consider enumerating the
YYYY/MM/DDdirectories newest-first with an explicit day/file cap so per-resolution work stays bounded as the sessions tree grows. Also hoist the matcher out of the loop (one instance instead of per-file allocation on Line 192).As per path instructions,
.github/review-bot-rules/algorithmic-complexity.md: confirm "recent" Codex rollout scans are bounded (explicit limit/size cap) and not repeatedly filtering the same large set.🤖 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/AgentChatTranscriptResolver.swift` around lines 185 - 196, The scan in AgentChatTranscriptResolver’s transcript lookup is unbounded and does per-file I/O across the entire sessions tree, so update the directory traversal to only walk recent YYYY/MM/DD rollout directories with an explicit day/file cap and newest-first ordering. Reuse a single CodexTranscriptIdentityMatcher instance inside the lookup helper instead of constructing one for every JSONL candidate, and keep the search bounded so the transcript(at:matchesSessionID:) path does not repeatedly parse the full ~/.codex/sessions hierarchy.Source: Path instructions
🤖 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
`@Packages/macOS/CmuxFoundation/Sources/CmuxFoundation/CodexTranscriptIdentityMatcher.swift`:
- Around line 21-24: The authoritative session check in
CodexTranscriptIdentityMatcher should rely only on session_meta.payload.id and
not on url.lastPathComponent substring matching. Update the matching logic so
the method that compares against normalizedSessionID uses sessionMetaID(at:) as
the sole source of truth, and returns false only when that metadata is missing
or does not match. Keep the rest of the matcher behavior unchanged.
---
Outside diff comments:
In `@Sources/Mobile/AgentChat/AgentChatTranscriptResolver.swift`:
- Around line 185-196: The scan in AgentChatTranscriptResolver’s transcript
lookup is unbounded and does per-file I/O across the entire sessions tree, so
update the directory traversal to only walk recent YYYY/MM/DD rollout
directories with an explicit day/file cap and newest-first ordering. Reuse a
single CodexTranscriptIdentityMatcher instance inside the lookup helper instead
of constructing one for every JSONL candidate, and keep the search bounded so
the transcript(at:matchesSessionID:) path does not repeatedly parse the full
~/.codex/sessions hierarchy.
🪄 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: a892623c-f7af-4aac-b6fa-470ddc5e2d9a
📒 Files selected for processing (4)
CLI/cmux.swiftPackages/macOS/CmuxFoundation/Sources/CmuxFoundation/CodexTranscriptIdentityMatcher.swiftSources/Mobile/AgentChat/AgentChatTranscriptResolver.swiftcmuxTests/AgentChatTranscriptResolverTests.swift
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/test-ios.yml:
- Line 113: The iOS workflow is using a floating default runner alias, which
makes the Xcode/iOS SDK baseline nondeterministic. Update the runner selection
in the test-ios workflow so the existing vars.MACOS_RUNNER_IOS override still
works, but the fallback for the jobs using that runner is a pinned macOS image
instead of depot-macos-latest; apply this in the affected job definitions around
the runs-on settings.
🪄 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: a57171aa-aa32-474a-bcaa-b776d749efae
📒 Files selected for processing (2)
.github/workflows/test-ios.ymlPackages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Transcript/Rows/ChatFileEditCardView.swift
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Mobile/AgentChat/AgentChatTranscriptService`+TitleDetection.swift:
- Around line 323-327: The nil-fallback path in ensureTailer(for:) is leaving
stale entries in failedResolutions even after
resolver.recordedTranscriptPath(for:) becomes available. Update the guard-let
fallback handling to clear any existing failedResolutions entry for the current
sessionID when a recorded path now exists, and only keep the failure marker when
there is still no recorded transcript path. Keep the logic localized around
ensureTailer(for:), failedResolutions, and
resolver.recordedTranscriptPath(for:).
🪄 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: 97a7553a-39a8-4783-99d3-4bf885df05ee
📒 Files selected for processing (5)
.github/workflows/test-ios.ymlPackages/Shared/CmuxAgentChat/Sources/CmuxAgentChat/Parsing/CodexTranscriptLocator.swiftSources/Mobile/AgentChat/AgentChatTranscriptResolver.swiftSources/Mobile/AgentChat/AgentChatTranscriptService+TitleDetection.swiftSources/Mobile/AgentChat/AgentChatTranscriptService.swift
added 3 commits
June 26, 2026 23:34
…t-shows-climbing-phantom-values-for # Conflicts: # Packages/iOS/CmuxAgentChatUI/Sources/CmuxAgentChatUI/Screen/ChatKeyboardTrackingViewController.swift
`applyCodexTranscriptResolution` only re-checked the pending-resolution key, which is not updated when the registry later receives a real `transcriptPath` from a hook. So a detached fallback scan scheduled while `transcriptPath == nil` could, on completion, overwrite an authoritative path recorded in the meantime — moving the chat tailer to an older/different rollout for the same session. Before applying the scan result, bail out if the record now has a valid recorded transcript path; the hook-recorded path is authoritative and the fallback is only meant to fill the gap when none exists. Found by autoreview (Codex). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
austinywang
force-pushed
the
issue-5550-token-count-shows-climbing-phantom-values-for
branch
from
June 30, 2026 05:02
52c3188 to
3b7b621
Compare
…t-shows-climbing-phantom-values-for # Conflicts: # .github/swift-file-length-budget.tsv
…values-for Resolved conflicts: - .github/swift-file-length-budget.tsv: regenerated via swift_file_length_budget.py --write-budget - ChatScrollEdgeCoordinator.swift, ChatTranscriptTableView.swift: took origin/main (main superseded this branch's inline #if compiler(>=6.2) glass-API guards with the applyScrollEdgeEffects helper + scroll-momentum work in #7072/#7109; branch predated it) - GhosttySurfaceView.swift: took origin/main and removed the now-orphaned GhosttySurfaceHandle.swift; main's GhosttySurfaceWorkQueue redesign (#7098) supersedes this branch's GhosttySurfaceHandle Sendable-wrapper approach for the same surface-pointer safety concern. Codex transcript payload (resolver realpath fix, service shutdown()/race guard, MobileShellComposite isolated deinit) preserved. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-shows-climbing-phantom-values-for # Conflicts: # .github/swift-file-length-budget.tsv
A hook can record the authoritative Codex transcript path while an off-main fallback scan is in flight. When that stale scan result lands, applyDirectCodexTranscriptResolution overwrites the recorded path because it only compares `record.transcriptPath != resolved`. Expose the apply method as internal and add a deterministic test that drives it with a stale scan result while the registry already holds an existing recorded path. This commit intentionally omits the fix so CI shows the test red; the guard follows in the next commit. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 19 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
The regression test from the previous commit is Swift Testing (@Test/#expect). The app-host "Run unit tests" step tolerates failures by parsing only the XCTest "Executed ... (0 unexpected)" summary, which does not count Swift Testing issues — so that test passed CI on the previous commit even though it must fail without the fix. The intended two-commit red proof was silently swallowed (the "silently ignored test" pitfall). Follow the repo's established non-tolerant pattern (BrowserSystemProxy / Option-Alt sided-modifier): add the suite to FOCUSED_GATE_SELECTORS so it is not folded into the tolerant shard, guard that exclusion in test_ci_cmux_unit_test_shard.py, and run the suite through run-app-host-xcodebuild.sh in a focused -only-testing step that propagates xcodebuild's exit code. With this wiring and no fix yet, CI goes genuinely red on the app-host focused shard, proving the test catches the bug. The guard fix follows in the next commit (CI back to green). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…path Move the recorded-transcript-path guard out of the scheduled wrapper (applyCodexTranscriptResolution) and into the shared applyDirectCodexTranscriptResolution, which both real callers funnel through. The direct path used by history(...) previously had no guard: it checks recordedTranscriptPath == nil, then awaits an off-main fallback scan, and during that suspension a hook can record the authoritative path — so the post-await apply could overwrite the freshly-recorded authoritative transcript with a stale scan result. Placing the guard on the shared path fixes that race for history(...) too and removes the duplicated guard (addresses Greptile P1). Behavior is unchanged except the exact bug case: when a recorded path already exists on disk, the scan result no longer overwrites it and resolution is treated as succeeded (failedResolutions cleared). All recorded-path-absent cases are identical to before. With the fix, the non-tolerant focused gate added in the previous commit (app-host shard 4) goes back to green — the regression test now passes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…t-shows-climbing-phantom-values-for # Conflicts: # .github/swift-file-length-budget.tsv
…t-shows-climbing-phantom-values-for # Conflicts: # .github/swift-file-length-budget.tsv
…t-shows-climbing-phantom-values-for # Conflicts: # .github/swift-file-length-budget.tsv
…t-shows-climbing-phantom-values-for # Conflicts: # .github/swift-file-length-budget.tsv # Sources/Mobile/AgentChat/AgentChatTranscriptResolver.swift # cmux.xcodeproj/project.pbxproj
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.
Fixes #5550
Summary
session_meta.payload.idand fail closed when metadata is missing or mismatchedTesting
git diff --checkpython3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsvscripts/check-pbxproj.shpython3 scripts/check-workspace-package-groups.py --checkpython3 scripts/check-package-resolved-policy.pyDemo Video
N/A: bug fix for transcript file selection with no UI flow change.
Review Trigger
Ready for review on the current head after CI completes.
Checklist
Fixes #5550xcodebuildrunLocalization
No user-facing strings changed.
Summary by CodeRabbit