Repository navigation
Resolve last-turn diffs in a persistent Rust sidecar - #8255
lawrencecchen wants to merge 142 commits into
Conversation
|
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:
📝 WalkthroughWalkthroughThe diff viewer’s last-turn source now carries provider and session identity to the Rust sidecar. The sidecar resolves latest-turn patches from Codex, Claude, and OpenCode storage, while CLI, UI, asset handling, lifecycle behavior, RPC transport, and regression tests are updated. ChangesAgent-turn diff flow
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant DiffCLI
participant DiffSidecar
participant AgentStorage
participant DiffViewer
User->>DiffCLI: select last-turn with agent and session
DiffCLI->>DiffSidecar: open agentTurn session
DiffSidecar->>AgentStorage: resolve latest-turn trajectory
AgentStorage-->>DiffSidecar: unified patch and repository root
DiffSidecar->>DiffViewer: return session and patch
Possibly related issues
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (21 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 54d0cced5d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Greptile SummaryThis PR replaces prompt-time Git snapshots with on-demand agent trajectory patches delivered by a persistent Rust sidecar over stdio multiplexing. Swift sends only the agent provider and session ID; the Rust
Confidence Score: 4/5The core transport and trajectory logic is well-structured; one production Swift seam added for test observation needs to be moved out of Sources before shipping. The Files Needing Attention: Sources/Panels/BrowserPanel.swift — the Important Files Changed
Sequence DiagramsequenceDiagram
participant AppDelegate
participant BrowserPanel
participant DiffSidecarBridge
participant DiffSidecarProcessSupervisor
participant RustSidecar
participant TrajectoryRS
AppDelegate->>BrowserPanel: openBrowserOnRight(loadingURL)
AppDelegate->>SharedLiveAgentIndex: freshSnapshot(1s deadline)
SharedLiveAgentIndex-->>AppDelegate: SessionRestorableAgentSnapshot
AppDelegate->>AppDelegate: launchDiffViewerProcess(--agent, --session)
Note over DiffSidecarProcessSupervisor,RustSidecar: Lazy-started, reused across requests
DiffSidecarBridge->>DiffSidecarProcessSupervisor: run(request)
DiffSidecarProcessSupervisor->>RustSidecar: stdin frame (newline-delimited JSON)
RustSidecar->>TrajectoryRS: resolve_last_turn_patch
TrajectoryRS-->>RustSidecar: ResolvedTurnPatch (repo_root + patch)
RustSidecar->>DiffSidecarProcessSupervisor: stdout frame (newline-delimited JSON)
DiffSidecarProcessSupervisor-->>DiffSidecarBridge: Data response
DiffSidecarBridge-->>BrowserPanel: diff session opened
BrowserPanel->>BrowserPanel: closeDiffViewerLoadingOverlay
Reviews (70): Last reviewed commit: "fix: resolve final merge gate failures" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux_open.swift`:
- Line 1538: In the last-turn diff source handling near the current throw,
replace CLIError with EmptyDiffSourceError so completeDeferredDiffViewerSource
can catch it and continue the fallback loop on legacy hosts. Keep the separate
CLIError behavior in writeGitDiffViewerHTMLSet for explicit --last-turn requests
unchanged.
In `@CLI/cmux.swift`:
- Line 35297: Remove the duplicate diff command usage entry from the cmux help
output, keeping the existing documentation entry near line 35176 and deleting
the redundant entry shown here.
In `@Native/DiffSidecar/src/server.rs`:
- Around line 1129-1130: Update the lock-result handling in the lease-state
check so any outcome other than successfully acquiring the exclusive lock
preserves the file, including TryLockError::Error. Keep the existing WouldBlock
behavior indicating an active lease, and ensure only a successful lock
acquisition reports the lease as inactive.
In `@Native/DiffSidecar/src/trajectory.rs`:
- Around line 325-334: Update claude_transcript_cwd and the underlying
for_json_lines traversal so processing stops immediately after the first valid
cwd is found, rather than continuing through the transcript; preserve the
existing Option<PathBuf> result and error propagation. Alternatively, reuse cwd
extraction within claude_last_turn_patch so the transcript is scanned only once.
- Around line 460-465: Update the prompt-tracking logic around current_prompt
and patch so a latest user prompt without promptId clears the prior patch and
fails closed instead of retaining the preceding turn. Ensure malformed JSONL
prompt lines are detected and handled as invalid latest-turn input rather than
silently ignored or returning stale data. Add regressions covering both missing
promptId and malformed latest prompt lines.
- Around line 501-504: Update the create-file determination in the trajectory
diff-generation logic to rely exclusively on the explicit `"type": "create"`
value, removing the `oldLines == 0` fallback. Add a regression test covering an
insertion-only edit to an existing file and verify it is not emitted as a
new-file diff.
In `@Sources/AppDelegate.swift`:
- Around line 6059-6061: Update the agent-turn routing around
normalizedOpenDiffViewerPath so a missing snapshot.workingDirectory and
launchCommand.workingDirectory does not prevent the path from executing when
provider/session identity is valid. Pass fallbackCwd only as the CLI working
directory, while preserving the original snapshot source values and relying on
the structured identity for repository resolution.
🪄 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: a7facda4-56ab-4e01-b714-2c1d99efddb2
⛔ Files ignored due to path filters (2)
Native/DiffSidecar/Cargo.lockis excluded by!**/*.lockwebviews/src/diff/generated/protocol.tsis excluded by!**/generated/**
📒 Files selected for processing (26)
CLI/CMUXCLI+DiffSidecar.swiftCLI/CMUXCLI+TypedDiffViewer.swiftCLI/cmux.swiftCLI/cmux_open.swiftNative/DiffSidecar/Cargo.tomlNative/DiffSidecar/rust-toolchain.tomlNative/DiffSidecar/src/lib.rsNative/DiffSidecar/src/protocol.rsNative/DiffSidecar/src/server.rsNative/DiffSidecar/src/trajectory.rsNative/DiffSidecar/tests/server_integration.rsNative/DiffSidecar/tests/trajectory.rsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/AppDelegate.swiftSources/OpenDiffViewerAgentBaselineLookup.swiftSources/OpenDiffViewerAgentContextRequest.swiftSources/RestorableAgentTypes.swiftSources/Workspace+PanelLifecycle.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentHibernationTests.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/CMUXOpenCommandTests.swiftscripts/ci/cmux-unit-test-timings.jsonwebviews/src/App.tsxwebviews/test/app.test.tsx
💤 Files with no reviewable changes (5)
- Sources/OpenDiffViewerAgentContextRequest.swift
- scripts/ci/cmux-unit-test-timings.json
- Sources/OpenDiffViewerAgentBaselineLookup.swift
- cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift
- cmux.xcodeproj/project.pbxproj
54d0cce to
259ea62
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 259ea6223f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 9
♻️ Duplicate comments (1)
CLI/cmux.swift (1)
35297-35297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove duplicate command documentation.
The
diffcommand usage is listed twice in thecmux helpoutput (here and at line 35176). Consider removing this redundant entry to keep the help text clean.♻️ Proposed refactor
- diff [patch-file|-] [--source <unstaged|staged|branch|last-turn>] [--agent <codex|claude|opencode>] [--session <id>] [--cwd <path>] [--base <ref>] [--focus <true|false>] [--no-focus] [--title <text>] [--layout <split|unified>] [--font-size <points>] (open patch input or git source in a browser split)🤖 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` at line 35297, Remove the duplicate diff command usage entry from the cmux help output, keeping the canonical entry near line 35176 and deleting the later duplicate shown in the diff.
🤖 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 `@Native/DiffSidecar/src/server.rs`:
- Around line 354-360: Update the cancel-frame handling in the RPC request
parsing flow so a missing or empty requestId produces the same invalidRequest
response used for other malformed frames instead of propagating an error through
frame?. Preserve valid cancel frames as RpcRequestRead::Cancel and keep the
persistent RPC session running.
In `@Native/DiffSidecar/src/trajectory.rs`:
- Around line 349-354: Update the task_started handling in the trajectory
event-processing logic to require a present turn_id; when it is missing, clear
current_turn and patch, then return TrajectoryError::Invalid instead of
preserving prior state. Keep the existing initialization and successful
early-return behavior unchanged for events that include turn_id.
- Around line 155-184: Update the trajectory selection logic around
read_hook_record, database_record, transcript, and repo_root to choose one
authoritative record atomically: use the hook only when both transcript_path and
cwd are present, otherwise require and use both fields from the database record.
Remove the independent per-field fallback chains so transcript and repository
root can never be combined from different sources.
- Around line 141-148: Update validate_session_id to reject path-bearing session
IDs before they are used as directory components: permit only the providers’
identifier character set, or equivalently require a single normal path component
with no separators, absolute-path semantics, or traversal segments. Preserve the
existing empty, length, and control-character validation and return
TrajectoryError::Invalid for disallowed values.
- Around line 604-624: Update relative_patch_path and the code that writes Git
patch headers to Git-quote every path component or complete relative path before
interpolation, escaping spaces, quotes, backslashes, tabs, newlines, and other
required characters while preserving valid UTF-8. Ensure all emitted headers use
the encoded path so filenames cannot inject additional headers, while retaining
the existing path validation behavior.
In `@Sources/Panels/DiffSidecarBridge.swift`:
- Around line 211-216: Align the encoded response-size limit used by
DiffSidecarBridge and the sidecar trajectory framing logic. Update the visible
maximumResponseBytes contract and the corresponding limit in trajectory.rs to
the same value, ensuring valid responses are not rejected as invalidResponse and
no separate conflicting patch-size limit remains.
- Around line 25-29: Update DiffSidecarBridge.shutdown so it no longer launches
an unretained Task.detached operation; make shutdown caller-owned and awaitable
by exposing an async shutdown operation that directly awaits
sidecarProcess.shutdown, preserving the utility priority only if the surrounding
API requires it.
- Around line 402-405: Update outputDidEnd(_:) so a matching stdout EOF always
transitions the bridge to a failed/stopped state, including when
process?.isRunning is true; remove the running-process guard while retaining the
generation check and existing processExited error propagation, ensuring
outputTask is cleared through stopProcess.
In `@Sources/RestorableAgentTypes.swift`:
- Around line 103-111: Update diffTrajectoryProvider for the .opencode case to
return the contract-required "openCode" string instead of rawValue, while
preserving rawValue for .codex and .claude and nil for all other providers.
---
Duplicate comments:
In `@CLI/cmux.swift`:
- Line 35297: Remove the duplicate diff command usage entry from the cmux help
output, keeping the canonical entry near line 35176 and deleting the later
duplicate shown in the diff.
🪄 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: 51d3b435-eece-4051-9bc1-12d6648594c6
⛔ Files ignored due to path filters (2)
Native/DiffSidecar/Cargo.lockis excluded by!**/*.lockwebviews/src/diff/generated/protocol.tsis excluded by!**/generated/**
📒 Files selected for processing (28)
CLI/CMUXCLI+DiffSidecar.swiftCLI/CMUXCLI+TypedDiffViewer.swiftCLI/cmux.swiftCLI/cmux_open.swiftNative/DiffSidecar/Cargo.tomlNative/DiffSidecar/rust-toolchain.tomlNative/DiffSidecar/src/lib.rsNative/DiffSidecar/src/main.rsNative/DiffSidecar/src/protocol.rsNative/DiffSidecar/src/server.rsNative/DiffSidecar/src/trajectory.rsNative/DiffSidecar/tests/server_integration.rsNative/DiffSidecar/tests/trajectory.rsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/AppDelegate.swiftSources/OpenDiffViewerAgentBaselineLookup.swiftSources/OpenDiffViewerAgentContextRequest.swiftSources/Panels/DiffSidecarBridge.swiftSources/RestorableAgentTypes.swiftSources/Workspace+PanelLifecycle.swiftcmux.xcodeproj/project.pbxprojcmuxTests/AgentHibernationTests.swiftcmuxTests/CLICodexHookTimeoutRegressionTests.swiftcmuxTests/CLINotifyProcessIntegrationRegressionTests.swiftcmuxTests/CMUXOpenCommandTests.swiftscripts/ci/cmux-unit-test-timings.jsonwebviews/src/App.tsxwebviews/test/app.test.tsx
💤 Files with no reviewable changes (5)
- Sources/OpenDiffViewerAgentBaselineLookup.swift
- Sources/OpenDiffViewerAgentContextRequest.swift
- scripts/ci/cmux-unit-test-timings.json
- cmuxTests/CLINotifyProcessIntegrationRegressionTests.swift
- cmux.xcodeproj/project.pbxproj
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
CLI/cmux_open.swift (1)
3642-3648: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve
agentProviderwhen building alternate-repository contexts.This fresh context copies
sessionIdbut dropsagentProvider. Selecting an alternate repository’s last-turn page therefore emits an incompletesessionSource, preventing provider-specific sidecar resolution.🐛 Proposed fix
let pageContext = DiffSourceContext( workspaceId: selectedContext.workspaceId, surfaceId: selectedContext.surfaceId, sessionId: selectedContext.sessionId, repoRoot: option.repoRoot, - branchBaseRef: source == .branch ? repoBranchBaseRef : selectedContext.branchBaseRef + branchBaseRef: source == .branch ? repoBranchBaseRef : selectedContext.branchBaseRef, + agentProvider: selectedContext.agentProvider )Based on the PR contract that provider and session identity must reach the Rust sidecar.
🤖 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_open.swift` around lines 3642 - 3648, Update the DiffSourceContext construction for alternate-repository contexts to also copy selectedContext.agentProvider, alongside workspaceId, surfaceId, and sessionId. Preserve the existing repository and branch selection logic so provider and session identity both reach the Rust sidecar.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/cmux_open.swift`:
- Around line 6774-6779: Replace the hard-link-first logic in the bundled asset
cache population flow with an unconditional file copy, removing the Darwin.link
path and its fallback branching. Ensure each cache entry receives an independent
inode so later in-place .deflate rewrites cannot mutate existing
content-addressed assets or active viewers.
In `@cmuxTests/CMUXOpenCommandTests.swift`:
- Around line 1172-1173: Update the mock handling for browser.navigate in the
regression test to capture its parameters, then assert that the requested URL
matches redirectURL and that navigation targets the surface returned by
browser.open_split. Ensure the assertions validate both the destination URL and
target surface rather than accepting every navigation request.
---
Outside diff comments:
In `@CLI/cmux_open.swift`:
- Around line 3642-3648: Update the DiffSourceContext construction for
alternate-repository contexts to also copy selectedContext.agentProvider,
alongside workspaceId, surfaceId, and sessionId. Preserve the existing
repository and branch selection logic so provider and session identity both
reach the Rust sidecar.
🪄 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: 720ce371-1721-4dd8-bf5e-e6d97acb52aa
📒 Files selected for processing (6)
CLI/CMUXCLI+DiffViewerBundledAssets.swiftCLI/cmux_open.swiftSources/ContentView.swiftcmuxTests/CLIMockSocketServerSupport.swiftcmuxTests/CMUXOpenCommandTests.swiftscripts/compress-markdown-viewer-assets.sh
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…turn # Conflicts: # Sources/Panels/BrowserPanel.swift # Sources/Panels/DiffCommentsBridge.swift # Sources/RestorableAgentSession.swift # Sources/TerminalController.swift
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
1 similar comment
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
6834f09 to
8b97a9b
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
8b97a9b to
670d27a
Compare
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
3e55ccd to
1ad0354
Compare
1ad0354 to
2600f24
Compare
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
Replaces prompt-time Git snapshots with on-demand agent trajectory patches.
Swift now sends only the agent provider and session ID. The Rust diff sidecar resolves the latest Codex, Claude, or OpenCode turn, verifies its repository, and serves the resulting patch through the existing diff session protocol. The viewer treats
agentTurnas a first-class source and does not rewrite it when repository UI state changes.The packaged native bridge now owns one lazy-started Rust process for the app lifetime. Newline-delimited requests share its private stdin/stdout pipes, Rust multiplexes up to four jobs by request ID, cancellation aborts only the matching job, and the next request restarts the child after a crash. No HTTP listener or filesystem socket is used.
This removes the Git stash baseline, untracked-file hashing, JSON and SQLite baseline storage, legacy migration, pruning, and prompt-hook snapshot work. Hook state becomes visible before auxiliary work, and stale cleanup cannot clear a running replacement on the same panel.
Rust is pinned to 1.97.0 with edition 2024. Direct crates and Cargo.lock are current for Rust 1.97. The obsolete
fs2dependency is replaced by stablestd::fs::Filelocking.Supersedes #8205. Builds on the merged Rust-sidecar prerequisite #7804.
Validation:
cargo update --verbose: zero direct updates remainingcargo test --all-features --no-fail-fast: 26 passedcargo clippy --all-targets --all-features -- -D warnings: passedcargo build --release --all-features: passedbun test test/app.test.tsx: 16 passedbun run typecheck: passedbun run lint:ci: passedgit stash createalone: 110 to 140 ms per hook, before untracked-file workRegression commits remain test-first, then fix, in the PR history.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Switches last‑turn diffs to typed agent trajectory patches resolved by a persistent Rust sidecar over stdio. Adds immediate single‑skeleton loading with worker‑gated renderer init, trusted session reuse, and removes prompt‑time Git snapshotting.
New Features
agentTurn { provider, session_id }diffs with optionalviewerInstanceId; patches stream from Rusttrajectory.rsviarusqlite(Rust 1.97.1,worker_threads = 2).browser.navigateacceptsexpected_operation_id..cmux-asset-manifest.json; a new pruner removes stale asset directories.Bug Fixes
--agent/--sessionor a typed sidecar are missing;--last-turnworks without a repo/cwd and preserves agent identity; Codex patch sources inherit the active turn identity.Written for commit 2600f24. Summary will update on new commits.
Summary by CodeRabbit
diffCLI with--agent,--session, and--target-surface(supportsbrowser.navigate+ expected URL).