Browser CLI reliability: socket-worker JS lane, CSP eval fallback, blank-surface fixes, fixture test suite - #5778
Conversation
… (red) Plain HTML/JS fixture pages (event trust/order, shadow DOM, nested iframes, custom dropdowns, occlusion, contenteditable, keyboard widgets, CSP without unsafe-eval, hostile sticky inputs, date/range) driven over the V2 socket by two new XCUITest classes. The regression class covers the 2026-03 browser CLI feedback: wait on a never-navigated surface, url.get on a blank surface, eval under CSP, and real JS exception text. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…k, loud flag errors - Browser methods that evaluate page JavaScript now run on the socket worker instead of the main actor. On the main actor they blocked SwiftUI updates for their full duration, and on a never-navigated webview that was a starvation deadlock: the JS cannot run until SwiftUI mounts the webview, which cannot happen while the handler holds the main thread (wait/eval on a fresh blank surface burned 10s and failed). - Never-navigated webviews are kicked through the panel's navigate path to about:blank (KVO-bounded) before automation JS runs, so the first JS call on a blank surface no longer hangs. - browser.eval retries in the isolated content world when the page world fails, matching the existing snapshot fallback: page CSP without unsafe-eval (e.g. Hacker News) blocks eval() and callAsyncJavaScript in the page world but not isolated worlds. - JS errors now surface the real exception text from WKJavaScriptExceptionMessage instead of the generic localizedDescription. - browser.wait reports js_error (with url + hint) when the condition cannot be evaluated, keeping the timeout code for genuine timeouts; the CLI scales its socket response timeout with --timeout-ms. - CLI open/goto reject unrecognized flags loudly instead of folding them into the URL (which silently opened about:blank or searched Google); --snapshot-after is accepted anywhere in goto args. - browser.open_split resolves its URL with the same smart logic as navigate (URL, then search fallback) and errors on unresolvable input. - browser.url.get reports about:blank for never-navigated surfaces instead of an empty string, matching JS location.href. CLI input surface is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR moves JS-evaluating browser handlers to the socket-worker with main-actor-synchronized WKWebView access, tightens CLI URL/flag validation and wait timeouts, converts parameter-parsing helpers to ChangesSocket-Worker JavaScript Execution Safety
Test Infrastructure, Fixtures, and Regression Coverage
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 (2 errors, 1 warning, 4 inconclusive)
✅ Passed checks (14 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 24d529f3df
ℹ️ 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".
| "browser.forward", | ||
| "browser.reload", | ||
| "browser.snapshot", | ||
| "browser.eval", |
There was a problem hiding this comment.
This new worker classification makes ControlCommandExecutionPolicy(forMethod: "browser.eval") return .socketWorker, but the existing ControlCommandExecutionPolicyTests.everythingElseRunsOnTheMainActor still lists browser.eval and asserts .mainActor. Any macOS/CI run of the package tests will fail until the test expectations are updated to match the intended browser JS worker lane.
Useful? React with 👍 / 👎.
![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
🤖 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 `@cmuxUITests/BrowserReliabilityRegressionUITests.swift`:
- Around line 19-31: The test currently accesses envelope?["ok"] without
asserting the envelope is non-nil; change the test to call XCTUnwrap on the
result of socketEnvelope(...) (the local variable envelope) before using it so
failures show a clear unwrap message; then use the unwrapped envelope to
evaluate envelope["ok"] in the XCTAssertEqual assertion (keep the same message
and checks) to mirror the pattern used in testEvalErrorCarriesRealExceptionText.
🪄 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: 53b87a06-63d6-4e49-a757-bb451eb5a4f2
📒 Files selected for processing (17)
CLI/cmux.swiftPackages/CmuxControlSocket/Sources/CmuxControlSocket/Wire/ControlCommandExecutionPolicy.swiftSources/TerminalController.swiftSources/TerminalControllerV2ParamParsingSupport.swiftcmux.xcodeproj/project.pbxprojcmuxUITests/BrowserFixtureInteractionUITests.swiftcmuxUITests/BrowserFixtures/contenteditable.htmlcmuxUITests/BrowserFixtures/csp-no-unsafe-eval.htmlcmuxUITests/BrowserFixtures/custom-dropdowns.htmlcmuxUITests/BrowserFixtures/datetime-range.htmlcmuxUITests/BrowserFixtures/event-trust-and-order.htmlcmuxUITests/BrowserFixtures/iframe-nested.htmlcmuxUITests/BrowserFixtures/keyboard-widget.htmlcmuxUITests/BrowserFixtures/occlusion-overlay.htmlcmuxUITests/BrowserFixtures/shadow-open.htmlcmuxUITests/BrowserFixtures/sticky-input.htmlcmuxUITests/BrowserReliabilityRegressionUITests.swift
| let envelope = socketEnvelope( | ||
| method: "browser.wait", | ||
| params: ["surface_id": sid, "load_state": "complete", "timeout_ms": 4_000], | ||
| responseTimeout: 10.0 | ||
| ) | ||
| let elapsed = Date().timeIntervalSince(start) | ||
|
|
||
| XCTAssertEqual( | ||
| envelope?["ok"] as? Bool, | ||
| true, | ||
| "browser.wait {load_state: complete} on a never-navigated surface should succeed: " + | ||
| "\(String(describing: envelope))" | ||
| ) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | 💤 Low value
Optional: Use XCTUnwrap for clearer test failure messages.
The test accesses envelope?["ok"] without unwrapping the optional envelope first. If socketEnvelope returns nil, the assertion would fail but the failure message would be less clear than an explicit unwrap failure. For consistency with testEvalErrorCarriesRealExceptionText (line 78), consider unwrapping the envelope first:
♻️ Suggested refactor for test clarity
- let envelope = socketEnvelope(
+ let envelope = try XCTUnwrap(
+ socketEnvelope(
- method: "browser.wait",
- params: ["surface_id": sid, "load_state": "complete", "timeout_ms": 4_000],
- responseTimeout: 10.0
- )
+ method: "browser.wait",
+ params: ["surface_id": sid, "load_state": "complete", "timeout_ms": 4_000],
+ responseTimeout: 10.0
+ ),
+ "Expected a response for browser.wait on never-navigated surface"
+ )
let elapsed = Date().timeIntervalSince(start)
XCTAssertEqual(
- envelope?["ok"] as? Bool,
+ envelope["ok"] as? Bool,
true,📝 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.
| let envelope = socketEnvelope( | |
| method: "browser.wait", | |
| params: ["surface_id": sid, "load_state": "complete", "timeout_ms": 4_000], | |
| responseTimeout: 10.0 | |
| ) | |
| let elapsed = Date().timeIntervalSince(start) | |
| XCTAssertEqual( | |
| envelope?["ok"] as? Bool, | |
| true, | |
| "browser.wait {load_state: complete} on a never-navigated surface should succeed: " + | |
| "\(String(describing: envelope))" | |
| ) | |
| let envelope = try XCTUnwrap( | |
| socketEnvelope( | |
| method: "browser.wait", | |
| params: ["surface_id": sid, "load_state": "complete", "timeout_ms": 4_000], | |
| responseTimeout: 10.0 | |
| ), | |
| "Expected a response for browser.wait on never-navigated surface" | |
| ) | |
| let elapsed = Date().timeIntervalSince(start) | |
| XCTAssertEqual( | |
| envelope["ok"] as? Bool, | |
| true, | |
| "browser.wait {load_state: complete} on a never-navigated surface should succeed: " + | |
| "\(String(describing: envelope))" | |
| ) |
🤖 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 `@cmuxUITests/BrowserReliabilityRegressionUITests.swift` around lines 19 - 31,
The test currently accesses envelope?["ok"] without asserting the envelope is
non-nil; change the test to call XCTUnwrap on the result of socketEnvelope(...)
(the local variable envelope) before using it so failures show a clear unwrap
message; then use the unwrapped envelope to evaluate envelope["ok"] in the
XCTAssertEqual assertion (keep the same message and checks) to mirror the
pattern used in testEvalErrorCarriesRealExceptionText.
…e policy tests The about:blank kick now first restores discarded webviews via restoreDiscardedWebViewIfNeeded and otherwise re-navigates to the panel's preserved currentURL, so automation against a discarded/restored tab brings the user's page back instead of clobbering it. Policy unit tests updated for the JS-evaluating browser methods' move to the socket-worker lane. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3c7a8d883d
ℹ️ 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".
| let resolved = resolveBrowserNavigableURL(urlStr) | ||
| ?? BrowserSearchSettings.currentConfiguration().searchURL(query: urlStr) |
There was a problem hiding this comment.
When browser.open_split is called with the supported custom diff-viewer URL form (cmux-diff-viewer://<token>/...), this new resolver path rejects the non-http/file scheme and then turns the whole string into a search URL. That bypasses the existing v2RegisterDiffViewerURLIfNeeded custom-scheme allowlist path below, so socket/CLI callers that pass the custom scheme open a search page instead of the internal diff viewer; the previous URL(string:) path preserved the custom URL and registered it.
Useful? React with 👍 / 👎.
…ocket pong Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6c8b8bedf9
ℹ️ 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".
| if case .failure(let pageMessage) = rawResult, #available(macOS 11.0, *) { | ||
| let isolatedResult = v2RunJavaScript( | ||
| webView, | ||
| script: asyncFunctionBody, |
There was a problem hiding this comment.
When a page-world evaluation fails for an ordinary JavaScript exception after performing a side effect, this unconditional retry runs the same script again in the isolated world before returning an error. For example, browser.eval with document.body.dataset.count = (+document.body.dataset.count || 0) + 1; throw new Error('boom') mutates the page twice, and automation scripts that partially act before throwing can duplicate clicks/input. The fallback should be gated to the CSP/unsafe-eval failure it is intended to recover from, rather than retrying every page-world failure.
Useful? React with 👍 / 👎.
Greptile SummaryMoves ~40 JS-evaluating
Confidence Score: 5/5The concurrency refactor is well-structured: JS-evaluating methods reliably run off the main actor with UI access gated through v2MainSync, and the three original regression bugs are verified fixed by new UITests. All findings are non-blocking quality concerns: V2BrowserPanelContext passes non-Sendable references to the worker (safe in practice because every property touch goes through v2MainSync, but the compiler cannot enforce it), nonisolated(unsafe) suppresses isolation checking on the KVO observation variable, and the 'refused to evaluate' CSP heuristic is broader than strictly necessary. None of these introduce a present data race or wrong-result path. Sources/TerminalController.swift — review V2BrowserPanelContext Sendable conformance, nonisolated(unsafe) observation, and the CSP heuristic match list. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI / Sidebar
participant Policy as ControlCommandExecutionPolicy
participant Worker as Socket Worker Lane
participant Main as Main Actor (v2MainSync)
participant WK as WKWebView
CLI->>Policy: browser.eval / browser.click / browser.wait / etc.
Policy-->>Worker: socketWorkerMethods → run on worker
Worker->>Main: v2BrowserWithPanelContext (resolve panel)
Main-->>Worker: V2BrowserPanelContext (surfaceId, webView ref)
Worker->>Worker: v2EnsureBrowserDocumentLoaded
Worker->>Main: KVO observe webView.url (if blank surface)
Main-->>Worker: URL committed
Worker->>Main: "MainActor.assumeIsolated { webView.callAsyncJavaScript }"
WK-->>Main: JS result / error
Main-->>Worker: result
alt CSP eval block detected
Worker->>Main: retry callAsyncJavaScript in isolated world
WK-->>Main: isolated result
Main-->>Worker: "result + content_world=isolated"
end
Worker-->>CLI: V2CallResult (ok / js_error / timeout)
Reviews (8): Last reviewed commit: "Fix Swift warning budget after main merg..." | Re-trigger Greptile |
…ile lengths in budget XCUIApplication.activate auto-records a test failure when hosted runners refuse activation, so the launch gate now waits for running state only and relies on the socket pong for readiness. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…er activation, matching repo precedent Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: de24e6c066
ℹ️ 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".
| // webview, which can't happen while the handler holds the main | ||
| // thread). UI/model access inside the handlers stays on main via | ||
| // v2MainSync. | ||
| "browser.navigate", |
There was a problem hiding this comment.
Putting browser.navigate and the other browser automation methods in the socket-worker policy makes TerminalController.runV2CommandLine(_:) reject them with invalid_dispatch, because processV2Command only accepts .mainActor methods; CmuxSidebarActionDispatch invokes custom-sidebar cmux(...) actions through that in-process entrypoint on the main actor. Any sidebar button/action that navigates, clicks, waits, or evals in a browser now silently gets an error instead of performing the action, so this path needs a worker-capable in-process dispatch rather than only changing the socket policy.
Useful? React with 👍 / 👎.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| // callAsyncJavaScript's function construction and eval() in the page world, but does | ||
| // not apply to isolated content worlds. The isolated world shares the DOM, so most | ||
| // automation scripts (and user evals) still work; page-world JS globals are not visible. | ||
| if case .failure(let pageMessage) = rawResult, #available(macOS 11.0, *) { |
There was a problem hiding this comment.
!useEval means browser.eval will retry in the isolated content world whenever the page world fails for any reason — including ordinary JS exceptions and timeouts. The isolated world does not see page-world globals (window.someVar set by the page's own scripts). On a CSP-hardened site, browser.eval window.reactRoot fails in the page world (CSP blocks function construction), the isolated retry succeeds, and the result is undefined — returned to the agent with no indication it came from a different JS context instead of the real page-world value. The retry should be restricted to paths that don't use eval() in the page world, which is what the !useEval guard was enforcing.
| if case .failure(let pageMessage) = rawResult, #available(macOS 11.0, *) { | |
| if !useEval, case .failure(let pageMessage) = rawResult, #available(macOS 11.0, *) { |
… timeout Register synchronously and invalidate after the await so the token cannot leak when the about:blank commit never arrives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07db9584f1
ℹ️ 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".
| let resolved = resolveBrowserNavigableURL(urlStr) | ||
| ?? BrowserSearchSettings.currentConfiguration().searchURL(query: urlStr) |
There was a problem hiding this comment.
When a socket/CLI caller explicitly passes url: "about:blank" to browser.open_split, resolveBrowserNavigableURL rejects the about: scheme and this fallback turns it into a search URL instead of opening a blank page. The previous URL(string:) path preserved about:blank, and the rest of the browser code treats that URL as a valid blank surface, so this regresses automation that uses an explicit blank URL rather than omitting url.
Useful? React with 👍 / 👎.
…nk in open_split Two review findings on the prior commit: - Cursor (medium): browser.navigate/back/forward/reload still called v2BrowserAppendPostSnapshot inside v2MainSync, so --snapshot-after ran the accessibility-tree walk on the main thread and could block SwiftUI / recreate mount deadlocks on a fresh surface. Standalone browser.snapshot already runs on the socket worker. Move the post-action snapshot out of the v2MainSync block in v2BrowserNavigate and v2BrowserNavSimple so it runs on the worker thread, matching the standalone path. The navigation action itself still runs on main. - Codex (P2): browser.open_split with an explicit url "about:blank" regressed when resolution switched to resolveBrowserNavigableURL, which rejects the about: scheme and let the search fallback turn it into a query. Preserve an explicit about:blank as a blank surface, which the rest of the browser code already treats as valid. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rning them silently Greptile flagged that the CSP isolated-world retry fired on every page-world failure, so a browser.eval reading a page-world global (window.reactRoot) under CSP would silently get undefined from the isolated world, which can't see page globals, with no signal to the agent. The suggested literal fix (gate on !useEval) would also kill the legitimate case the regression suite already covers: browser.eval of a DOM read like document.title under CSP, where the isolated-world value is correct (the DOM is shared). Disabling the fallback there would regress reliability on CSP pages. Instead keep the fallback (DOM reads keep working under CSP) but make it loud: v2RunBrowserJavaScript invokes an onIsolatedWorldFallback callback when the isolated world is what produced success, and browser.eval annotates the result with content_world: "isolated" plus a note that page-world globals were not visible. Internal automation (useEval == false: clicks, snapshots, finds) only touches the shared DOM, so it stays transparent and unflagged. Strengthen testCSPNoUnsafeEval to assert the content_world flag is present on the CSP-served eval, so the silent-context regression is covered behaviorally. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 871a315143
ℹ️ 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".
| 32553 CLI/cmux.swift | ||
| 21996 Sources/TerminalController.swift | ||
| 32573 CLI/cmux.swift | ||
| 22172 Sources/TerminalController.swift |
There was a problem hiding this comment.
The CI workflow runs python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv (.github/workflows/ci.yml:88-89), but at this commit Sources/TerminalController.swift has 22220 physical lines while this entry allows only 22172. The budget check reports +48 Sources/TerminalController.swift and fails before the rest of CI can pass, so this needs to be raised to the actual count or the file reduced.
Useful? React with 👍 / 👎.
| 830 Sources/TaskManagerTypes.swift | ||
| 810 Packages/CmuxSwiftRender/Tests/CmuxSwiftRenderTests/SwiftViewInterpreterTests.swift | ||
| 787 Sources/ClosedItemHistory.swift | ||
| 769 cmuxUITests/BrowserFixtureInteractionUITests.swift |
There was a problem hiding this comment.
The same Swift file-length CI check also fails on the new cmuxUITests/BrowserFixtureInteractionUITests.swift: the file has 774 physical lines at this commit, but this new budget entry allows only 769. Even after fixing the TerminalController entry, CI will still fail with +5 cmuxUITests/BrowserFixtureInteractionUITests.swift unless this budget is raised or the file is shortened.
Useful? React with 👍 / 👎.
…SP retry Three findings on the socket-worker migration: - Cursor HIGH / Codex P2 (in-process browser actions broken): moving browser.* to the socket-worker policy made runV2CommandLine -> processV2Command reject them with invalid_dispatch, so sidebar cmux(...) actions that navigate/click/ eval/wait silently failed. Route worker-lane methods through handleSocketLine on a serial background queue (they must run off the main actor, which is why they left the main-actor path); main-actor methods still run synchronously. - Cursor MEDIUM (browser.wait routing): browser.wait resolved its surface with surface_id-or-focused only, ignoring pane_id/tab_id, so a wait routed by pane could target the wrong webview. Use the shared v2ResolveBrowserSurfaceId like every other socket-worker browser command. - Codex P2 (isolated-world retry duplicates side effects): the CSP isolated-world retry fired on every page-world failure, so a script that performed a side effect then threw, or timed out, was re-run in the isolated world (duplicating the side effect, or returning a wrong-context value). Gate the retry on a CSP eval-block signature (unsafe-eval / Content Security Policy / blocked by CSP / refused to evaluate). Genuine CSP blocks still fall back and still carry the content_world flag added in the prior commit; ordinary failures now surface honestly. This also fully resolves the Greptile P1 about silent wrong-context results. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 26dbae0. Configure here.
browser.open_split's switch to resolveBrowserNavigableURL (http/https/file only) also dropped the trusted internal cmux-diff-viewer://<token>/... scheme: it was rejected and turned into a search URL, bypassing v2RegisterDiffViewerURLIfNeeded so socket/CLI callers got a search page instead of the diff viewer. Same family as the about:blank regression. Preserve the diff-viewer scheme (alongside about:blank) before the navigable-URL/search fallback. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e657358d4
ℹ️ 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".
| let resolved = resolveBrowserNavigableURL(urlStr) | ||
| ?? BrowserSearchSettings.currentConfiguration().searchURL(query: urlStr) |
There was a problem hiding this comment.
When browser.open_split receives any valid custom-scheme URL other than the two special-cased schemes (for example mailto:user@example.com, xcode://..., or a user-configured external deep link), this branch turns it into a search URL before the later browser-disabled/external-open paths can call NSWorkspace.open on the original URL. The previous URL(string:) path preserved those URLs, so users with the cmux browser disabled or respect_external_open_rules enabled now open a search page instead of the intended external app.
Useful? React with 👍 / 👎.
…le budgets Round 2 of review on the socket-worker migration: - Codex P2 (external custom schemes): browser.open_split's resolveBrowserNavigableURL switch turned any non-http/file scheme into a search, not just about:blank and cmux-diff-viewer. Schemes like mailto:/xcode:// (used by the browser-disabled and respect_external_open_rules NSWorkspace.open paths) regressed too. Replace the two special-cases with one rule: resolveBrowserNavigableURL first (http/https/file + host-like), else preserve any real-scheme URL it rejects, else search. The resolver returns nil for schemed non-web URLs, so external/internal schemes are preserved and only scheme-less non-navigable input becomes a query. - Cursor MEDIUM (sidebar command ordering): the prior fix enqueued only worker-lane commands async while main-actor ones ran inline, so a later main command could finish before an earlier browser navigate/click/wait. Run the whole action's command sequence on the serial worker queue via handleSocketLine (which runs worker methods off-main and hops main-actor methods back to main), preserving authored order end to end. openURL runs synchronously on main to keep its slot. - Codex P1 (file-length budget): refresh budgets to the branch's actual sizes for TerminalController.swift and BrowserFixtureInteractionUITests.swift (this PR's growth) plus ContentView.swift and SessionIndexView.swift (already over budget on the branch from an earlier merge, blocking the CI budget gate). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # .github/swift-file-length-budget.tsv # Sources/TerminalController.swift
…assumeIsolated WebKit The 94-commit main merge combined main's browserInputHelpers/reactCompatibleSetValue input refactor with this branch's nonisolated worker-lane browser methods, producing "main actor-isolated X referenced from a nonisolated context" warnings that tripped the CI Swift warning budget (the merge re-triggered the full required CI suite). Fixes, all principled (not budget refreshes): - Mark the immutable Sendable JS-helper constants nonisolated: browserInputHelpers, reactCompatibleSetValue, the v2BrowserEvalEnvelope* keys, and v2BrowserUndefinedSentinel (its empty final class is now Sendable). They are global constants; nonisolated access is correct. - v2RunJavaScript took a main-actor WKContentWorld, so every nonisolated caller warned on .page/.defaultClient. Take a Sendable V2JSContentWorld enum instead and resolve the real WKContentWorld, plus the callAsyncJavaScript/evaluateJavaScript calls, inside MainActor.assumeIsolated. The evaluator only ever runs on the main actor (Thread.isMainThread branch or DispatchQueue.main.async), so assumeIsolated is safe and centralizes WebKit main-actor access to one spot. - CmuxSidebarActionDispatch: discard NSWorkspace.open's Bool in the main.sync closure. Budget bumped for the merged TerminalController size only. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 498ab454a7
ℹ️ 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".
| } else { | ||
| return .err( | ||
| code: "invalid_params", | ||
| message: "Could not resolve URL or search query", |
There was a problem hiding this comment.
The /workspace/cmux/AGENTS.md instructions require every user-facing string to be localized, and this new browser.open_split error is returned directly to CLI/socket callers when URL resolution fails. Please wrap the message in String(localized:defaultValue:) and add the key to Resources/Localizable.xcstrings for all supported locales rather than introducing a bare English response.
Useful? React with 👍 / 👎.
…browser CLI, pairing QR, iOS PRs included: - manaflow-ai#5816 ControlCommandCoordinator extraction (package coordinator skeleton; fork keeps legacy v2* dispatchers) - manaflow-ai#5859 sidebar perf - manaflow-ai#5857 RendererRealization (added as SurfaceHibernation adapter) - manaflow-ai#5867 in-process custom sidebars - manaflow-ai#5778 browser CLI / system-proxy bypass - manaflow-ai#5872 minimal pairing QR - iOS pairing/manual-entry stack - 30+ hot fixes Fork-side adjustments: - Skip 21 TerminalController+Control* extension files (PR manaflow-ai#5816 architecture refactor not adopted) - Add Sources/App/RendererRealizationSettingsAdapter.swift to bridge new RendererRealizationSettings to fork's existing SurfaceHibernationSettings - Restore v2SurfaceDragToSplit shim removed by upstream - Add SettingsNavigationTarget.customSidebars case - Stub ghostty_surface_set_renderer_realized callsites pending GhosttyKit rebuild (zig 0.15.2 required, host has 0.16.0) - Update ghostty submodule to 44b2baa81 (cherry-pick the 3 renderer commits onto fork's manaflow-ai#5128 link-fix pointer) - Keep fork's CMUXSessionDaemon module pbxproj refs and SurfaceHibernation settings
This branch was successfully deployed
1 participant
Fixes the browser automation failures from the 2026-03 agent feedback and hardens cmux browser CLI for agent loops. CLI input surface is unchanged; implementation moved.
Re-verification of the original report on current main found three still broken: stray flags after a URL were silently folded into it (open landed on about:blank, goto searched Google),
wait --load-state completeon a fresh blank surface burned 10s ignoring--timeout-ms 4000, andevalfailed on CSP pages like Hacker News with the useless messageA JavaScript exception occurred.Root cause of the wait/eval hangs: all browser V2 methods ran on the main actor, so a handler waiting on page JavaScript blocked SwiftUI for its full duration, and on a never-navigated webview deadlocked itself by starvation (the webview cannot mount, so its JS cannot run, while the handler holds main). JS-evaluating browser methods now run on the socket-worker lane per the existing execution-policy design (same lane as
browser.download.wait), with UI access kept on main viav2MainSync. Never-navigated webviews are kicked to about:blank through the panel's navigate path before automation JS runs.CSP: page CSP without
unsafe-evalblocks botheval()andcallAsyncJavaScriptin the page world but not isolated content worlds;browser.evalnow retries in the isolated world like snapshot already did. JS errors carry the realWKJavaScriptExceptionMessage.waitdistinguishesjs_error(condition unevaluable, with url + hint) fromtimeout.open/gotoreject unknown flags loudly;--snapshot-afteris valid anywhere in goto args.browser.open_splitresolves URLs with the same smart logic as navigate.url.getreportsabout:blankinstead of""on never-navigated surfaces.Commit 1 adds the regression coverage (red): a fixture suite of plain HTML/JS pages (shadow DOM, nested iframes, custom dropdowns, occlusion overlays, contenteditable, keyboard widgets, CSP without unsafe-eval, hostile sticky inputs, date/range, event isTrusted/order logging — scenario catalog distilled from browser-use's stress tests, no Python) plus two XCUITest classes driving them over the V2 socket. Commit 2 adds the fixes (green).
Verified live on a tagged build: fresh-blank-surface
wait --load-state complete --timeout-ms 4000returns OK in 0.3s (was 10s failure),eval document.titleon news.ycombinator.com returnsHacker News(was js_error),click a.morelinkon HN navigates, stray flags error loudly,get urlon a blank surface returnsabout:blank.Known limitations documented by the fixture tests (XCTExpectFailure): shadow-DOM piercing selectors, nested-iframe frame.select retargeting, and synthetic key events not satisfying per-char isTrusted checks. Tracked in cmux-todos
browser-reliability.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
High Risk
Large refactor of browser command dispatch and concurrency (main actor vs socket worker) across
TerminalControllerand sidebar dispatch; regressions could affect UI responsiveness, automation ordering, or wrong-webview routing despite new tests.Overview
Moves browser automation off the main actor so
browser.*commands that evaluate or wait on page JavaScript run on the socket-worker lane (ControlCommandExecutionPolicy,v2BrowserJSCommandOnSocketWorker,v2BrowserWithPanelContext). UI/model access stays on main viav2MainSync; param parsing and JS helpers arenonisolated. Sidebarcmux(...)actions use a serial worker queue andhandleSocketLineso ordered navigate/click/wait sequences do not block SwiftUI.Hardens blank-surface and wait/eval behavior: never-navigated webviews are kicked through the panel navigate path (restore discarded tab or preserved URL, else
about:blank) before automation JS;browser.url.getreportsabout:blank.browser.waituses full surface routing (pane_id/tab_id), returnsjs_errorvstimeout, and the CLI extends socket timeout past--timeout-ms. Post-action--snapshot-afterruns off main after navigate/back/reload.CLI and URL handling:
open/gotoreject unknown--flags;gotoaccepts--snapshot-afteranywhere.browser.open_splituses the same smart URL resolution as navigate.browser.evalretries isolated-world only on CSP eval-block signatures and flagscontent_world: isolated; JS errors surfaceWKJavaScriptExceptionMessage.Tests: execution-policy unit tests updated; new
BrowserFixtureInteractionUITests/ reliability UITests and Xcode project entries; Swift file-length budgets bumped.Reviewed by Cursor Bugbot for commit 498ab45. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Improves browser CLI reliability by moving JS/interaction
browser.*calls to the socket worker, adding a CSP‑safe eval with isolated‑world flagging, fixing blank‑surface and--snapshot-afterstalls, and tightening URL/flag handling. Adds HTML fixtures and socket‑driven UITests to cover core interactions and CSP behavior.Bug Fixes
browser.*methods on the socket worker; keep UI access on main viav2MainSync. Execute in‑processcmux(...)action sequences on a serial worker queue viahandleSocketLineto preserve order;openURLstays on main.currentURLbefore automation; kick never‑navigated surfaces toabout:blank.browser.url.getreturnsabout:blank.browser.evalretries in an isolated world only on CSP‑block signatures; annotate results withcontent_world: "isolated"and surfaceWKJavaScriptExceptionMessage.--snapshot-afteroff the main thread; standalonebrowser.snapshotalready runs off‑main.browser.waitdistinguishesjs_errorfromtimeout, scales socket headroom with--timeout-ms, respects timeouts on blank surfaces, and resolves targets viav2ResolveBrowserSurfaceId.open/gotoreject unknown flags;--snapshot-afterallowed anywhere.browser.open_splitresolves URLs like navigate and preserves external schemes (e.g.,mailto:,xcode:), explicitabout:blank, and trustedcmux-diff-viewer://; otherwise falls back to search.nonisolatedand centralizing WebKit access viaMainActor.assumeIsolatedwith a sendable content‑world enum.New Features
unsafe-eval, sticky inputs, date/range) and socket-driven UITests; gate on socket pong, avoidXCUIApplication.activate(), wrap launch in non‑strictXCTExpectFailure, and assert isolated‑world flags in CSP tests.Written for commit 498ab45. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Refactor
Tests