Repository navigation
Move diff viewer backend boundary to a Rust sidecar - #7804
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:
📝 WalkthroughWalkthroughAdds a Rust DiffSidecar with typed protocol handling, secure resource serving, branch RPCs, browser transport implementations, native macOS packaging, benchmark coverage, and CI routing. It also updates bundled agent-session assets. ChangesDiff sidecar integration
Agent session bundle updates
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DiffViewer
participant DiffTransport
participant DiffSidecar
participant CmuxCLI
DiffViewer->>DiffTransport: Request branchList or branchChange
DiffTransport->>DiffSidecar: Send typed RPC
DiffSidecar->>CmuxCLI: Execute hidden branch command
CmuxCLI-->>DiffSidecar: Return refs or navigation URL
DiffSidecar-->>DiffTransport: Return typed response
DiffTransport-->>DiffViewer: Update picker or navigate
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
Warning Review the following alerts detected in dependencies. According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Greptile SummaryThis PR moves the diff viewer backend into a bundled Rust sidecar. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (85): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if let override = ProcessInfo.processInfo.environment["CMUX_DIFF_SIDECAR_PATH"]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !override.isEmpty { | ||
| let url = URL(fileURLWithPath: override, isDirectory: false) | ||
| if FileManager.default.isExecutableFile(atPath: url.path) { | ||
| return url.standardizedFileURL.resolvingSymlinksInPath() | ||
| } |
There was a problem hiding this comment.
When CMUX_DIFF_SIDECAR_PATH is present, the CLI launches that executable as the diff viewer server with the full user environment and only checks that the path is executable. A normal production cmux open run can therefore be steered to an arbitrary sidecar binary, moving the loopback resource server and branch RPC boundary outside the bundled app code.
There was a problem hiding this comment.
Fixed in 3fb54f7. Production resolution now accepts only the bundled sidecar next to the cmux executable, with the existing cmux executable fallback. The environment override was removed.
— Claude Code
| let Some(path) = scheme_url.strip_prefix(&format!("cmux-diff-viewer://{token}/")) else { | ||
| return Err(()); | ||
| }; | ||
| Ok(format!( | ||
| "http://127.0.0.1:{}/{token}/{path}#cmux-diff-viewer", |
There was a problem hiding this comment.
change_branch trusts the path returned by the delegated cmux command after only stripping the custom-scheme prefix, then emits it as a loopback redirect. If that command returns a path containing ../, encoded separators, or another malformed resource path, the browser follows a redirect that was never revalidated by the sidecar’s resource-path checks, so branch changes can navigate to an invalid or unintended loopback URL instead of the regenerated diff.
There was a problem hiding this comment.
Fixed in 3fb54f7. Delegated output is parsed as a URL, constrained to the expected custom scheme and token host with no credentials, port, query, or fragment, validated as a request path, and required to resolve through the token manifest before navigation. The integration test rejects traversal output.
— Claude Code
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
🤖 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/ci.yml:
- Around line 337-369: Add a Rust build cache step to the diff-sidecar-check job
after Rust installation and before the cargo commands, using a pinned rust-cache
action scoped to Native/DiffSidecar (including its Cargo manifest/lockfile as
appropriate). Ensure the existing cargo fmt, clippy, test, and benchmark steps
reuse the cached registry and target artifacts.
- Around line 343-344: Update the checkout step identified by uses:
actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd to explicitly set
persist-credentials: false, preventing the GitHub token from remaining available
to subsequent cargo and bun dependency scripts.
In `@CLI/cmux_open.swift`:
- Around line 7499-7506: Update the in-process server implemented by
runDiffViewerHTTPServer and started by startDiffViewerHTTPServer to handle POST
requests at /__cmux_diff_rpc, matching the transport payload’s endpoint.
Implement the branch-picker RPC request/response behavior expected by the React
adapter, including request parsing and appropriate errors, so calls succeed when
the sidecar is unavailable.
In `@Native/DiffSidecar/src/benchmark.rs`:
- Around line 37-52: Add a happy-path test in the benchmark test module that
invokes run with a small size and one iteration, asserting it returns Ok. Verify
the hand-built manifest fixture in run uses field names and casing matching the
serde attributes on manifest::Manifest and its nested types, so schema drift is
caught by cargo test.
- Around line 26-71: Add an RAII cleanup guard immediately after creating `root`
in the benchmark function so it removes the temporary directory on drop,
covering all subsequent fallible operations and early returns. Ensure the guard
owns or references `root` safely, and remove the manual
`std::fs::remove_dir_all(root)` call at the end.
In `@Native/DiffSidecar/src/main.rs`:
- Around line 37-50: Reject leftover command-line arguments in the command
dispatch handling: after parsing `handshake`, ensure the iterator has no
remaining values, and after parsing `benchmark`’s `sample_bytes` and
`iterations`, likewise error if any arguments remain. Use the existing error
propagation style so unexpected parameters cause the command to fail.
- Around line 72-74: Validate the threshold returned by
environment_number::<f64> for finiteness and non-negativity before comparing it
with report.sequential_read_mib_per_second. Reject NaN, infinities, and negative
values with an error so invalid CMUX_DIFF_BENCH_MIN_READ_MIBPS settings cannot
disable the benchmark budget.
In `@Native/DiffSidecar/src/server.rs`:
- Around line 261-311: Replace the wildcard arm in the request-command match
with explicit arms for every currently defined DiffCommand variant, including
SessionOpen and SessionClose, returning the existing unsupportedMethod response
where appropriate. This ensures future DiffCommand additions trigger a compiler
exhaustiveness error and require an explicit decision.
- Around line 350-372: Update load_branch_refs and change_branch to capture
child-process stderr instead of discarding it, and log a bounded, redacted
snippet with tracing for non-zero exits, timeouts, command errors, and JSON
deserialization failures before returning Err(()). Preserve the existing timeout
and error behavior while ensuring sensitive token or repository data is not
emitted.
- Around line 554-582: Improve resolve_allowed_file cache robustness by
validating manifest content with a monotonic version or content hash rather than
relying only on ManifestFingerprint byte length and modified time, ensuring
identical-size rewrites cannot serve stale file lists. Replace the
manifests.clear() capacity handling with least-recently-used eviction, updating
recency on cache hits and evicting only the oldest entry when
MAX_CACHED_MANIFESTS is reached.
- Around line 341-372: Pass group_id through BranchListRequest and the
branch_refs query parameters into load_branch_refs, updating all callers and
protocol definitions. Refactor authorization to use the exact session filename
derived from group_id, matching authorize_branch_change, instead of
authorize_repo_for_token’s directory scan; retain token/repository validation
and update related handlers/tests.
In `@Native/DiffSidecar/tests/server_integration.rs`:
- Around line 9-14: Ensure the integration test cleanup runs during unwinding:
guard the spawned child process and temporary root directory with an RAII
cleanup type or equivalent drop guard that kills/waits for the child and removes
the directory. Apply this around the runtime.block_on calls and existing
verify_resources, verify_rpc, and verify_websocket assertions so cleanup occurs
even when they panic.
In `@webviews/bench/diff-stream.bench.ts`:
- Line 6: Correct the p95 index calculations in the benchmark’s percentile
reporting logic, including the corresponding sections around the initial
iteration handling and later result reporting. Use nearest-rank semantics (for
example, a 1-based rank clamped to the sample count) so small sets such as five
iterations select the slowest sample, and apply the same correction consistently
to the Rust benchmark’s indexing pattern if covered by this change.
- Around line 5-6: Validate the `fileCount` and `iterations` values in
`diff-stream.bench.ts` after parsing them from the environment, rejecting values
that are non-finite, non-positive, or non-integer. Add a clear validation
failure that stops execution before the benchmark runs when either input is
invalid.
🪄 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: 07a3d01d-e0ab-41b6-9187-1536d2b6adc5
⛔ 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 (35)
.github/workflows/ci.ymlCLI/cmux_open.swiftNative/DiffSidecar/.gitignoreNative/DiffSidecar/Cargo.tomlNative/DiffSidecar/README.mdNative/DiffSidecar/src/benchmark.rsNative/DiffSidecar/src/bin/generate_types.rsNative/DiffSidecar/src/lib.rsNative/DiffSidecar/src/main.rsNative/DiffSidecar/src/manifest.rsNative/DiffSidecar/src/protocol.rsNative/DiffSidecar/src/server.rsNative/DiffSidecar/tests/server_integration.rsNative/DiffSidecar/tests/support/test_host.rsResources/agent-session-react/assets/app.jsResources/agent-session-react/index.htmlResources/agent-session-solid/assets/app.jsResources/agent-session-solid/index.htmlResources/markdown-viewer/webviews-app/chunks/diffSurface.mjscmux.xcodeproj/project.pbxprojcmuxTests/CMUXOpenCommandTests.swiftscripts/benchmark-diff-viewer.shscripts/build-diff-sidecar.shscripts/generate-diff-sidecar-types.shtests/test_ci_change_areas.pywebviews/bench/diff-stream.bench.tswebviews/package.jsonwebviews/src/App.tsxwebviews/src/BranchBasePicker.tsxwebviews/src/agent-session/shared/bridge.tswebviews/src/diff/transport.tswebviews/src/global.d.tswebviews/src/types.tswebviews/test/branch-base-picker.test.tsxwebviews/test/diff-transport.test.ts
💤 Files with no reviewable changes (1)
- webviews/src/agent-session/shared/bridge.ts
| diff-sidecar-check: | ||
| needs: changes | ||
| if: ${{ needs.changes.outputs.macos == 'true' }} | ||
| runs-on: ${{ vars.LINUX_RUNNER || 'blacksmith-4vcpu-ubuntu-2404' }} | ||
| timeout-minutes: 15 | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 | ||
|
|
||
| - name: Install Rust | ||
| run: ./scripts/install-rust-ci.sh | ||
|
|
||
| - name: Setup Bun | ||
| uses: oven-sh/setup-bun@0c5077e51419868618aeaa5fe8019c62421857d6 # v2 | ||
|
|
||
| - name: Install webview dependencies | ||
| working-directory: webviews | ||
| run: bun install --frozen-lockfile | ||
|
|
||
| - name: Check Rust sidecar | ||
| run: | | ||
| cargo fmt --manifest-path Native/DiffSidecar/Cargo.toml --all --check | ||
| cargo clippy --manifest-path Native/DiffSidecar/Cargo.toml --all-targets -- -D warnings | ||
| cargo test --manifest-path Native/DiffSidecar/Cargo.toml --all-targets | ||
|
|
||
| - name: Verify generated protocol types | ||
| run: ./scripts/generate-diff-sidecar-types.sh --check | ||
|
|
||
| - name: Enforce diff-viewer performance budgets | ||
| env: | ||
| CMUX_DIFF_WEB_BENCH_ITERATIONS: "10" | ||
| run: ./scripts/benchmark-diff-viewer.sh | ||
|
|
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial
Consider caching Rust build artifacts for this job.
diff-sidecar-check runs cargo fmt, cargo clippy --all-targets, cargo test --all-targets, and then a further benchmark step against the same crate, all within a 15-minute timeout and with no visible cargo registry/target caching. A cold cache on a busy runner could push this close to (or past) the timeout.
Consider adding a Rust cache step (e.g. Swatinem/rust-cache) scoped to Native/DiffSidecar to keep this job fast and reduce flaky timeouts.
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 343-344: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[info] 337-337: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
🤖 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 @.github/workflows/ci.yml around lines 337 - 369, Add a Rust build cache step
to the diff-sidecar-check job after Rust installation and before the cargo
commands, using a pinned rust-cache action scoped to Native/DiffSidecar
(including its Cargo manifest/lockfile as appropriate). Ensure the existing
cargo fmt, clippy, test, and benchmark steps reuse the cached registry and
target artifacts.
| let Ok(_permit) = state.child_processes.try_acquire() else { | ||
| return Err(()); | ||
| }; | ||
| let mut command = Command::new(&state.config.cmux_executable); | ||
| command | ||
| .arg("__diff-viewer-refs") | ||
| .arg("--repo") | ||
| .arg(repo) | ||
| .arg("--token") | ||
| .arg(token) | ||
| .stdin(Stdio::null()) | ||
| .stderr(Stdio::null()) | ||
| .kill_on_drop(true); | ||
| if let Some(base) = base { | ||
| command.arg("--base").arg(base); | ||
| } | ||
| match tokio::time::timeout(CHILD_PROCESS_TIMEOUT, command.output()).await { | ||
| Ok(Ok(output)) if output.status.success() => { | ||
| serde_json::from_slice(&output.stdout).map_err(|_| ()) | ||
| } | ||
| _ => Err(()), | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Subprocess stderr is discarded with no logging on failure.
load_branch_refs/change_branch set .stderr(Stdio::null()) and collapse every failure mode (non-zero exit, timeout, bad JSON) to a bare Err(()). This makes diagnosing real CLI failures (bad repo path, git errors, timeouts) effectively impossible from sidecar logs. Consider capturing stderr and logging a bounded/redacted snippet on failure via tracing.
Also applies to: 406-428
🤖 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 `@Native/DiffSidecar/src/server.rs` around lines 350 - 372, Update
load_branch_refs and change_branch to capture child-process stderr instead of
discarding it, and log a bounded, redacted snippet with tracing for non-zero
exits, timeouts, command errors, and JSON deserialization failures before
returning Err(()). Preserve the existing timeout and error behavior while
ensuring sensitive token or repository data is not emitted.
| async fn resolve_allowed_file( | ||
| state: &AppState, | ||
| resource_path: &str, | ||
| ) -> Option<(String, AllowedFile)> { | ||
| let normalized = format!("/{}", resource_path.trim_start_matches('/')); | ||
| let (token, request_path) = split_resource_path(&normalized)?; | ||
| let path = state.config.root.join(format!(".manifest-{token}.json")); | ||
| let metadata = tokio::fs::metadata(path).await.ok()?; | ||
| let fingerprint = ManifestFingerprint { | ||
| byte_length: metadata.len(), | ||
| modified: metadata.modified().ok(), | ||
| }; | ||
| if let Some(cached) = state.manifests.read().await.get(token) | ||
| && cached.fingerprint == fingerprint | ||
| { | ||
| let file = cached.files.get(&request_path)?.clone(); | ||
| return Some((token.to_owned(), file)); | ||
| } | ||
|
|
||
| let manifest = Manifest::load(&state.config.root, token).await.ok()?; | ||
| let files = Arc::new(manifest.files_by_path().ok()?); | ||
| let file = files.get(&request_path)?.clone(); | ||
| let mut manifests = state.manifests.write().await; | ||
| if manifests.len() >= MAX_CACHED_MANIFESTS && !manifests.contains_key(token) { | ||
| manifests.clear(); | ||
| } | ||
| manifests.insert(token.to_owned(), CachedManifest { fingerprint, files }); | ||
| Some((token.to_owned(), file)) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
Manifest cache freshness relies on size+mtime, not content.
ManifestFingerprint (byte_length + modified time) is a reasonable freshness check, but two manifest rewrites with identical size within the same mtime tick (filesystem-resolution dependent) would be treated as unchanged and serve a stale file list. Also, eviction at capacity (manifests.clear(), line 578) drops the entire cache rather than the least-recently-used entry, which can cause cache-thrashing if the session count hovers near MAX_CACHED_MANIFESTS. Neither is likely to bite in practice (macOS APFS mtimes are sub-second resolution, and 64 concurrent sessions is a high bar), but a monotonic version/content-hash and an LRU eviction would be more robust.
🤖 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 `@Native/DiffSidecar/src/server.rs` around lines 554 - 582, Improve
resolve_allowed_file cache robustness by validating manifest content with a
monotonic version or content hash rather than relying only on
ManifestFingerprint byte length and modified time, ensuring identical-size
rewrites cannot serve stale file lists. Replace the manifests.clear() capacity
handling with least-recently-used eviction, updating recency on cache hits and
evicting only the oldest entry when MAX_CACHED_MANIFESTS is reached.
| let root = std::env::temp_dir().join(format!( | ||
| "cmux-diff-sidecar-test-{}-{}", | ||
| std::process::id(), | ||
| uuid::Uuid::new_v4() | ||
| )); | ||
| std::fs::create_dir_all(&root).expect("create root"); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Cleanup only runs on success; leaks the child process and temp dir on assertion failure.
child.kill() and remove_dir_all(root) are only reached after runtime.block_on(...) returns normally. Any panicking assertion inside verify_resources/verify_rpc/verify_websocket skips this cleanup, leaking the spawned sidecar process and the /tmp test directory on every failing run.
♻️ Guard-based cleanup that runs even on panic
+struct SidecarGuard {
+ child: std::process::Child,
+ root: std::path::PathBuf,
+}
+
+impl Drop for SidecarGuard {
+ fn drop(&mut self) {
+ let _ = self.child.kill();
+ let _ = self.child.wait();
+ let _ = std::fs::remove_dir_all(&self.root);
+ }
+}
+
let mut child = Command::new(env!("CARGO_BIN_EXE_cmux-diff-sidecar"))
...
.spawn()
.expect("start sidecar");
+ let mut guard = SidecarGuard { child, root: root.clone() };
+ let child = &mut guard.child;
let stdout = child.stdout.take().expect("sidecar stdout");
...
runtime.block_on(async {
...
});
- let _ = child.kill();
- let _ = child.wait();
- let _ = std::fs::remove_dir_all(root);Also applies to: 58-84
🤖 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 `@Native/DiffSidecar/tests/server_integration.rs` around lines 9 - 14, Ensure
the integration test cleanup runs during unwinding: guard the spawned child
process and temporary root directory with an RAII cleanup type or equivalent
drop guard that kills/waits for the child and removes the directory. Apply this
around the runtime.block_on calls and existing verify_resources, verify_rpc, and
verify_websocket assertions so cleanup occurs even when they panic.
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)
cmux.xcodeproj/project.pbxproj (1)
5062-5062: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Build Diff Sidecar" phase's
inputPathslist may go stale as the Rust crate grows.The phase enumerates individual source files (
benchmark.rs,lib.rs,main.rs,manifest.rs,protocol.rs,server.rs) rather than the wholeNative/DiffSidecar/srctree. Since Xcode only reruns a script build phase when a listed input is newer than the output, adding a new module file to the crate (e.g. splittingserver.rsfurther) without updating this list means Xcode could skip rebuildingcmux-diff-sidecareven thoughcargowould have picked up the change — producing a stale bundled binary. This mirrors the same fragile pattern already used by "Build Command Palette Nucleo FFI", so it's not a new problem introduced here, but the risk grows with a larger crate (protocol/manifest/server modules, tests support, etc.).Consider watching the whole
Native/DiffSidecar/srcdirectory (or settingalwaysOutOfDate = 1like "Write Extension Point" does) so future file additions can't silently produce a stale sidecar binary.Also applies to: 5417-5444
🤖 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 `@cmux.xcodeproj/project.pbxproj` at line 5062, Update the “Build Diff Sidecar” PBXShellScriptBuildPhase (D1FF50000000000000000001) input tracking so it watches the entire Native/DiffSidecar/src directory rather than enumerating individual Rust files, or mark the phase always out of date like “Write Extension Point”; apply the same change to the corresponding phase definition around the additional referenced section to ensure new crate modules always trigger a rebuild.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@CLI/CMUXCLI`+DiffSidecar.swift:
- Around line 33-37: In the Process.run() error handler, update the CLIError
message construction to use String(describing: error) instead of
error.localizedDescription, preserving the structured underlying failure
details.
---
Outside diff comments:
In `@cmux.xcodeproj/project.pbxproj`:
- Line 5062: Update the “Build Diff Sidecar” PBXShellScriptBuildPhase
(D1FF50000000000000000001) input tracking so it watches the entire
Native/DiffSidecar/src directory rather than enumerating individual Rust files,
or mark the phase always out of date like “Write Extension Point”; apply the
same change to the corresponding phase definition around the additional
referenced section to ensure new crate modules always trigger a rebuild.
🪄 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: 5825bcc4-ebb1-4b7f-9399-605ad692810d
⛔ Files ignored due to path filters (2)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsvNative/DiffSidecar/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.github/workflows/ci.ymlCLI/CMUXCLI+DiffSidecar.swiftCLI/cmux_open.swiftNative/DiffSidecar/Cargo.tomlNative/DiffSidecar/README.mdNative/DiffSidecar/src/server.rsNative/DiffSidecar/tests/server_integration.rsResources/Localizable.xcstringsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjscmux.xcodeproj/project.pbxproj
| do { | ||
| try process.run() | ||
| } catch { | ||
| throw CLIError(message: "Failed to start diff viewer server: \(error.localizedDescription)") | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use String(describing:) for the CLI error cause.
error.localizedDescription can collapse Process.run() failures (e.g. CocoaError/NSError) to a generic string and hide the real cause. Prefer the structured description for CLI diagnostics.
🩹 Proposed fix
do {
try process.run()
} catch {
- throw CLIError(message: "Failed to start diff viewer server: \(error.localizedDescription)")
+ throw CLIError(message: "Failed to start diff viewer server: \(String(describing: error))")
}Based on learnings: "Use String(describing: error) instead of error.localizedDescription when formatting errors in the cmux Swift CLI (CLI/**/*.swift)."
📝 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.
| do { | |
| try process.run() | |
| } catch { | |
| throw CLIError(message: "Failed to start diff viewer server: \(error.localizedDescription)") | |
| } | |
| do { | |
| try process.run() | |
| } catch { | |
| throw CLIError(message: "Failed to start diff viewer server: \(String(describing: error))") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@CLI/CMUXCLI`+DiffSidecar.swift around lines 33 - 37, In the Process.run()
error handler, update the CLIError message construction to use
String(describing: error) instead of error.localizedDescription, preserving the
structured underlying failure details.
Source: Learnings
fb2de5a to
a488d25
Compare
a488d25 to
9fd3996
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
Native/DiffSidecar/src/main.rs (2)
17-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract shared
--root/--cmuxargument parsing.The
serveandrpcsubcommands have identical argument parsing logic (~10 lines each). Extract a small helper to reduce duplication and ensure both stay in sync if the CLI contract changes.♻️ Optional: extract shared arg parser
+fn parse_root_and_cmux(args: &mut impl Iterator<Item = String>, subcommand: &str) -> Result<(PathBuf, PathBuf), String> { + let mut root = None; + let mut cmux = None; + while let Some(argument) = args.next() { + match argument.as_str() { + "--root" => root = args.next().map(PathBuf::from), + "--cmux" => cmux = args.next().map(PathBuf::from), + _ => return Err(format!("unexpected argument: {argument}")), + } + } + let root = root.ok_or_else(|| format!("{subcommand} requires --root"))?; + let cmux_executable = cmux.ok_or_else(|| format!("{subcommand} requires --cmux"))?; + Ok((root, cmux_executable)) +}🤖 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 `@Native/DiffSidecar/src/main.rs` around lines 17 - 56, Duplicate parsing of --root and --cmux exists in the serve and rpc command branches. Extract this logic into a shared helper that consumes the argument iterator, rejects unexpected arguments, and returns the validated root and cmux paths; update both branches to call it while preserving their command-specific error messages and behavior.
83-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject non-finite or negative read budgets Native/DiffSidecar/src/main.rs:92 accepts
f64thresholds as-is, soNaN,±inf, or a negative value can silently bypassCMUX_DIFF_BENCH_MIN_READ_MIBPS. Fail closed on invalid input before comparing.🤖 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 `@Native/DiffSidecar/src/main.rs` around lines 83 - 101, The enforce_benchmark_budget function accepts invalid read-throughput thresholds that can bypass enforcement. Validate the value returned for CMUX_DIFF_BENCH_MIN_READ_MIBPS before comparing, rejecting negative, NaN, and infinite values with an error so invalid configuration fails closed.
♻️ Duplicate comments (1)
Native/DiffSidecar/src/main.rs (1)
57-70: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLeftover arguments still not validated for
handshakeandbenchmark.This was previously flagged and the code is unchanged.
handshakeignores all remaining arguments, andbenchmarksilently ignores arguments after the two expected values, allowing misspelled or stale CI parameters to appear successful.🤖 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 `@Native/DiffSidecar/src/main.rs` around lines 57 - 70, Validate that no arguments remain after processing the `handshake` and `benchmark` subcommands. Update the command-dispatch logic in `main` so `handshake` rejects leftover arguments and `benchmark` rejects anything beyond its optional sample size and iteration count, returning a clear error instead of silently succeeding.
🤖 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 501-509: Add a stdio/RPC integration test alongside
rpc_uses_stdio_without_server_state that starts the sidecar in rpc mode, sends a
branchChange request through stdin, and parses the response. Assert
result.value.url is the raw cmux-diff-viewer:// URL and does not contain an
http://127.0.0.1 prefix, covering the state.port == 0 path.
---
Outside diff comments:
In `@Native/DiffSidecar/src/main.rs`:
- Around line 17-56: Duplicate parsing of --root and --cmux exists in the serve
and rpc command branches. Extract this logic into a shared helper that consumes
the argument iterator, rejects unexpected arguments, and returns the validated
root and cmux paths; update both branches to call it while preserving their
command-specific error messages and behavior.
- Around line 83-101: The enforce_benchmark_budget function accepts invalid
read-throughput thresholds that can bypass enforcement. Validate the value
returned for CMUX_DIFF_BENCH_MIN_READ_MIBPS before comparing, rejecting
negative, NaN, and infinite values with an error so invalid configuration fails
closed.
---
Duplicate comments:
In `@Native/DiffSidecar/src/main.rs`:
- Around line 57-70: Validate that no arguments remain after processing the
`handshake` and `benchmark` subcommands. Update the command-dispatch logic in
`main` so `handshake` rejects leftover arguments and `benchmark` rejects
anything beyond its optional sample size and iteration count, returning a clear
error instead of silently succeeding.
🪄 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: 0c28358d-6cd7-43f5-abf1-c7d8a7f69be6
📒 Files selected for processing (12)
CLI/cmux_open.swiftNative/DiffSidecar/README.mdNative/DiffSidecar/src/main.rsNative/DiffSidecar/src/protocol.rsNative/DiffSidecar/src/server.rsNative/DiffSidecar/tests/server_integration.rsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Panels/BrowserPanel.swiftSources/Panels/DiffCommentsBridge.swiftcmuxTests/CMUXOpenCommandTests.swiftwebviews/src/App.tsxwebviews/test/app.test.tsx
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)
Native/DiffSidecar/src/server.rs (1)
124-173: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
port == 0as an implicit "stdio mode" flag is fragile.
app_state(config, 0)inrun_rpc(line 160) and thestate.port == 0check inchange_branch(line 501) repurpose a plainu16port field as a mode discriminator. It's correct today only becauserun()always assigns a real OS-ephemeral port (never 0) before constructingAppState. Any future change to howportis populated (e.g., a configurable/fixed port) would silently break this branch with no compiler error, returning malformed navigation URLs.Consider an explicit
AppStatefield (e.g.transport_mode: TransportModeoris_stdio: bool) set once at construction, so the intent is visible at the type level instead of inferred from a numeric sentinel.♻️ Suggested direction
struct AppState { config: Arc<ServerConfig>, client: reqwest::Client, port: u16, + is_stdio: bool, manifests: Arc<RwLock<HashMap<...>>>, child_processes: Arc<Semaphore>, } -fn app_state(config: ServerConfig, port: u16) -> Result<AppState, String> { +fn app_state(config: ServerConfig, port: u16, is_stdio: bool) -> Result<AppState, String> { Ok(AppState { config: Arc::new(config), client: ..., port, + is_stdio, ... }) }And in
change_branch:if state.is_stdio { ... } else { ... }.Also applies to: 501-509
🤖 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 `@Native/DiffSidecar/src/server.rs` around lines 124 - 173, Replace the implicit stdio-mode sentinel based on port value with an explicit AppState field, such as is_stdio or a TransportMode enum. Initialize it appropriately in app_state callers, including run_rpc and the network transport setup, then update change_branch to branch on the explicit mode field instead of state.port == 0 while retaining the existing port for URL construction.
🤖 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/main.rs`:
- Around line 37-56: Extract the duplicated --root/--cmux parsing, validation,
and current_exe lookup from the serve and rpc match arms into a shared
parse_server_args helper. Have both branches call this helper and retain only
their distinct server invocation and subcommand-specific error context, updating
serve and rpc consistently for any future argument changes.
In `@webviews/test/app.test.tsx`:
- Line 82: Replace the fixed-duration wait before assertions in the test with a
real completion signal: use React’s act or await a deadline-bounded poll of the
relevant state predicate. Update the affected test flow in the surrounding test
case so assertions run only after the effect or async operation has actually
completed.
---
Outside diff comments:
In `@Native/DiffSidecar/src/server.rs`:
- Around line 124-173: Replace the implicit stdio-mode sentinel based on port
value with an explicit AppState field, such as is_stdio or a TransportMode enum.
Initialize it appropriately in app_state callers, including run_rpc and the
network transport setup, then update change_branch to branch on the explicit
mode field instead of state.port == 0 while retaining the existing port for URL
construction.
🪄 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: baee3ea4-e902-4a20-873a-097646fd64eb
📒 Files selected for processing (13)
CLI/cmux_open.swiftNative/DiffSidecar/README.mdNative/DiffSidecar/src/main.rsNative/DiffSidecar/src/protocol.rsNative/DiffSidecar/src/server.rsNative/DiffSidecar/tests/server_integration.rsResources/markdown-viewer/webviews-app/chunks/diffSurface.mjsSources/Panels/BrowserPanel.swiftSources/Panels/DiffCommentsBridge.swiftSources/TerminalController.swiftcmuxTests/CMUXOpenCommandTests.swiftwebviews/src/App.tsxwebviews/test/app.test.tsx
| Some("rpc") => { | ||
| let mut root = None; | ||
| let mut cmux = None; | ||
| while let Some(argument) = args.next() { | ||
| match argument.as_str() { | ||
| "--root" => root = args.next().map(PathBuf::from), | ||
| "--cmux" => cmux = args.next().map(PathBuf::from), | ||
| _ => return Err(format!("unexpected argument: {argument}")), | ||
| } | ||
| } | ||
| let root = root.ok_or_else(|| "rpc requires --root".to_owned())?; | ||
| let cmux_executable = cmux.ok_or_else(|| "rpc requires --cmux".to_owned())?; | ||
| let executable_path = std::env::current_exe().map_err(|error| error.to_string())?; | ||
| server::run_rpc(ServerConfig { | ||
| root, | ||
| cmux_executable, | ||
| executable_path, | ||
| }) | ||
| .await | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extract shared argument parsing for serve and rpc.
The rpc block is a near-exact copy of the serve block (lines 17–36) — identical --root/--cmux parsing, validation, and current_exe() lookup, differing only in the subcommand name in error messages and the final server call. A shared helper would eliminate the duplication and ensure both paths stay consistent if flags are added later.
♻️ Proposed refactor: extract a shared `parse_server_args` helper
+ fn parse_server_args(
+ args: &mut impl Iterator<Item = String>,
+ subcommand: &str,
+ ) -> Result<(PathBuf, PathBuf, PathBuf), String> {
+ let mut root = None;
+ let mut cmux = None;
+ while let Some(argument) = args.next() {
+ match argument.as_str() {
+ "--root" => root = args.next().map(PathBuf::from),
+ "--cmux" => cmux = args.next().map(PathBuf::from),
+ _ => return Err(format!("unexpected argument: {argument}")),
+ }
+ }
+ let root = root.ok_or_else(|| format!("{subcommand} requires --root"))?;
+ let cmux_executable = cmux.ok_or_else(|| format!("{subcommand} requires --cmux"))?;
+ let executable_path = std::env::current_exe().map_err(|error| error.to_string())?;
+ Ok((root, cmux_executable, executable_path))
+ }Then both serve and rpc become:
Some("serve") => {
- let mut root = None;
- let mut cmux = None;
- while let Some(argument) = args.next() {
- match argument.as_str() {
- "--root" => root = args.next().map(PathBuf::from),
- "--cmux" => cmux = args.next().map(PathBuf::from),
- _ => return Err(format!("unexpected argument: {argument}")),
- }
- }
- let root = root.ok_or_else(|| "serve requires --root".to_owned())?;
- let cmux_executable = cmux.ok_or_else(|| "serve requires --cmux".to_owned())?;
- let executable_path = std::env::current_exe().map_err(|error| error.to_string())?;
+ let (root, cmux_executable, executable_path) = parse_server_args(&mut args, "serve")?;
server::run(ServerConfig { root, cmux_executable, executable_path }).await
}
Some("rpc") => {
- let mut root = None;
- let mut cmux = None;
- while let Some(argument) = args.next() {
- match argument.as_str() {
- "--root" => root = args.next().map(PathBuf::from),
- "--cmux" => cmux = args.next().map(PathBuf::from),
- _ => return Err(format!("unexpected argument: {argument}")),
- }
- }
- let root = root.ok_or_else(|| "rpc requires --root".to_owned())?;
- let cmux_executable = cmux.ok_or_else(|| "rpc requires --cmux".to_owned())?;
- let executable_path = std::env::current_exe().map_err(|error| error.to_string())?;
+ let (root, cmux_executable, executable_path) = parse_server_args(&mut args, "rpc")?;
server::run_rpc(ServerConfig { root, cmux_executable, executable_path }).await
}📝 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.
| Some("rpc") => { | |
| let mut root = None; | |
| let mut cmux = None; | |
| while let Some(argument) = args.next() { | |
| match argument.as_str() { | |
| "--root" => root = args.next().map(PathBuf::from), | |
| "--cmux" => cmux = args.next().map(PathBuf::from), | |
| _ => return Err(format!("unexpected argument: {argument}")), | |
| } | |
| } | |
| let root = root.ok_or_else(|| "rpc requires --root".to_owned())?; | |
| let cmux_executable = cmux.ok_or_else(|| "rpc requires --cmux".to_owned())?; | |
| let executable_path = std::env::current_exe().map_err(|error| error.to_string())?; | |
| server::run_rpc(ServerConfig { | |
| root, | |
| cmux_executable, | |
| executable_path, | |
| }) | |
| .await | |
| } | |
| fn parse_server_args( | |
| args: &mut impl Iterator<Item = String>, | |
| subcommand: &str, | |
| ) -> Result<(PathBuf, PathBuf, PathBuf), String> { | |
| let mut root = None; | |
| let mut cmux = None; | |
| while let Some(argument) = args.next() { | |
| match argument.as_str() { | |
| "--root" => root = args.next().map(PathBuf::from), | |
| "--cmux" => cmux = args.next().map(PathBuf::from), | |
| _ => return Err(format!("unexpected argument: {argument}")), | |
| } | |
| } | |
| let root = root.ok_or_else(|| format!("{subcommand} requires --root"))?; | |
| let cmux_executable = cmux.ok_or_else(|| format!("{subcommand} requires --cmux"))?; | |
| let executable_path = std::env::current_exe().map_err(|error| error.to_string())?; | |
| Ok((root, cmux_executable, executable_path)) | |
| } | |
| Some("serve") => { | |
| let (root, cmux_executable, executable_path) = parse_server_args(&mut args, "serve")?; | |
| server::run(ServerConfig { | |
| root, | |
| cmux_executable, | |
| executable_path, | |
| }) | |
| .await | |
| } | |
| Some("rpc") => { | |
| let (root, cmux_executable, executable_path) = parse_server_args(&mut args, "rpc")?; | |
| server::run_rpc(ServerConfig { | |
| root, | |
| cmux_executable, | |
| executable_path, | |
| }) | |
| .await | |
| } |
🤖 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 `@Native/DiffSidecar/src/main.rs` around lines 37 - 56, Extract the duplicated
--root/--cmux parsing, validation, and current_exe lookup from the serve and rpc
match arms into a shared parse_server_args helper. Have both branches call this
helper and retain only their distinct server invocation and subcommand-specific
error context, updating serve and rpc consistently for any future argument
changes.
9fd3996 to
6cafd02
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. |
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. |
# Conflicts: # 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. |
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. |
react-apps-check fails because #7804 committed a bundled chunk one line out of sync with its webviews sources (reproduces on plain main). Regenerated with ./scripts/build-webviews-app.sh as the check instructs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
react-apps-check fails because #7804 committed a bundled chunk one line out of sync with its webviews sources (reproduces on plain main). Regenerated with ./scripts/build-webviews-app.sh as the check instructs. Co-authored-by: austinpower1258 <austinwang115@gmail.com> Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
WKScriptMessageHandlerWithReplycmux-diff-viewer://handler, with no production TCP listener or persistent sidecarPortable production build
opt-level = "z", fat LTO, one codegen unit, panic abort, and stripped symbolsSize
Performance
Verification
diff-sidecar-check, remote daemon, web typecheck, React apps, and database migration jobs passmainThe speculative merge workflow is currently stopped by
ios/cmux.xcworkspace/xcshareddata/swiftpm/Package.resolved, an unexpected lockfile already present on currentmainand unchanged by this PR. Its dedicated sidecar and routed jobs pass.