Repository navigation
fix(browser): preserve modal controls and expose bounded SPA readiness - #21
Conversation
…rols Keep hidden and inert background subtrees out of dialog snapshots without dropping reachable offscreen controls. Preserve global actionable indices across bounded pages and expose scope, totals, and continuation in both native backends. Add real Chromium regression coverage, malformed-option tests, Rust formatter coverage, native e2e checks, and CI/documentation updates. Fixes munimtechnologies#18.
Wait for an expected visible CSS selector on state-returning actions or read-only snapshots instead of treating a loaded tab as application readiness. Preserve action success separately from readiness met, timeout, error, or not_requested. Forward the contract and render status consistently in Rust and Swift, with site-policy checks on every observation. Add a real Chromium regression proven to fail on deb4c7a, delayed-dialog MCP integration checks, contract tests, and safe retry documentation. Verified Windows and Linux Rust suites, 16 full browser integration checks, extension/build tests, and tool schema parity.
|
Note 🤖 gpt-6.1-sol responding on behalf of Elliot The exact PR head This includes the macOS release Swift build, bridge check with 120 connected peers, and 18 real-Chrome browser integration checks against the Swift server. Windows/Linux Rust and extension/parity checks also passed. Both original regressions were independently proven failing on untouched upstream source and passing after the fix; the PR body has commands, evidence and limits. The upstream workflow is awaiting maintainer approval: https://github.com/munimtechnologies/munim-computer-use/actions/runs/37636339482 . This account cannot grant that approval. Please approve the upstream CI run; no CI security policy has been changed or bypassed. |
There was a problem hiding this comment.
Note
🤖 Opus 5.5 responding on behalf of Elliot
Independent review of f2a466b3d5c1cb01241d251332116a737b2669bb (base deb4c7a)
Coverage: read the PR body, #18/#19 (no comments on either), and every changed file: background.js, background.test.mjs, main.swift, browser.rs, tools.rs, e2e-browser.mjs, snapshot-dom.test.mjs, ci.yml, README.md. I also read the surrounding CLICK_JS, checkTab, credential fill and settle/withState code.
Run locally (Windows 11, branded Chrome): node --test chrome-extension/background.test.mjs passed 49/49. check-tool-parity.mjs reported 32 tools OK. tool-defs.mjs --check round-tripped OK. A reviewer probe used CDP against headless Chrome and evaluated the PR's own SNAPSHOT_JS and visibleSelectorInPage (evidence is inline). I did not run snapshot-dom.test.mjs: without TMPDIR it writes into the checkout (R3), and the sandbox would not let me set the variable. I did not run Swift/Rust builds, cargo tests or e2e. For those I relied on the source and the fork CI run cited in the PR body.
Claim verdicts
- Partly holds. Scope is chosen before the 250-control budget. Native top layer, painted stacking, the nested dialog, hidden/inert ancestry and the background-tab fallback all behave correctly in my probe. However, an ARIA
role=dialogwithoutaria-modalis treated as modal. A non-covering banner or widget therefore hides every page control (R1). - Holds. Global indices are tagged across the whole selected scope.
nextOffset/truncated/offset > totalare correct, stale tags are cleared on every snapshot, and password values stay redacted. Paging a changed UI silently remaps indices, but this is documented as "unchanged UI". - Holds, with one regression in the shared rule. The selector wait polls independently of
tab.statusand returnsmet/timeout/errorwithout replaying the action. Site policy is rechecked on each poll.JSON.stringifycloses the selector-injection path. In my probe, a link click that navigated mid-poll queued behind the commit and then returnedmet; it did not error. The shared visibility predicate adds an element-levelopacity:0exclusion, which drops common real controls (R2). - Holds. Rust
normaliseand SwiftwithReturnStateforwardlimit/wait_for_selector/wait_timeout_mson click/type/press/navigate/snapshot. Headroom is +15 s. Readiness and pagination rendering match on both backends, and the schemas and README match the extension's validation. - Mostly holds. The DOM regressions run the real extracted source, and
--sourcereproduces the baseline failures. The suite does not pin either problem above. Test-profile scratch defaults to the repo (R3). The fix is otherwise the smallest complete root fix: no new deps and no new tools.
Findings
- R1 (High): a
role=dialogwithoutaria-modalscopes the snapshot and hides all page controls.chrome-extension/background.js:592. #21 (comment) - R2 (Medium): an element's own
opacity:0drops operable transparent inputs (file upload, custom checkbox).chrome-extension/background.js:567. #21 (comment) - R3 (Low): the DOM test profile falls back to
<repo>/.snapshot-testwhenTMPDIRis unset.scripts/snapshot-dom.test.mjs:22. #21 (comment)
Fork-main CI exposed ENOTEMPTY after all DOM assertions passed. A launcher-only kill left POSIX Chromium children writing to the disposable profile. Own a private process group and terminate it even if the launcher has already exited. Four deterministic regressions were proven failing before the repair; run them in CI. (cherry picked from commit 655fc19)
- Snapshot modal scoping kept only controls inside the dialog, so a select or combobox whose options render in a portal at the end of <body> (Radix, MUI, Headless UI) lost every option. Roots the modal's controls point at via aria-controls/aria-owns, open popovers, and controls painted on top of the modal stay in scope. - wait_for_selector refuses selectors that test the value attribute. React mirrors controlled inputs' values there, password fields included, so met/timeout answered a password one guessed character at a time. Escapes and comments are undone before the check. - null for an optional argument means absent again, as it did before offset/limit/wait_for_selector existed. - A transient evaluate error (a navigation destroying the context) polls again until the deadline instead of ending the wait. - When an extension older than the server ignores the new arguments, both hosts say so and ask for a reload instead of printing nothing. - The native-host relay exits on backend EOF on Linux too (#16): it blocks on Chrome's stdin the same way the Windows relay did. - e2e: click a prompt's button after the evaluation has answered, since the click closes the window; report the live targets when no prompt answers. - browser_click / browser_press_key / browser_close_all_tabs descriptions cover child tabs and Tab's focus report.
2 participants
Summary
Closes #18.
Closes #19.
This is one combined PR because the defects overlap in the browser-state path and the identical Swift/Rust schemas. The implementation keeps the existing 250-control budget and tool count. It adds no runtime dependencies and no arbitrary-script tool.
Root causes and fixes
SNAPSHOT_JScollected page controls in DOM order and silently sliced the first 250, even when those controls were hidden or covered by a modal. Scope to the topmost visible modal first, exclude hidden/inert subtrees, then paginate the selected scope. Return scope, total, truncation and next offset explicitly. Global indices remain actionable across pages of an unchanged UI. Native modeless dialogs and explicitaria-modal=falsedo not scope the page. Implicit ARIA dialogs require viewport-centre coverage and paint-order evidence, so corner chat widgets and bottom cookie banners retain page controls. Password values remain redacted.tabs.statuscannot establish same-document application readiness. Add a bounded explicitwait_for_selectorcondition to state-returning browser actions and read-onlybrowser_snapshot. Return readiness separately from action success (met,timeout,error,not_requested). A timeout does not roll back or replay a click. Re-observe withbrowser_snapshot, not another non-idempotent action.Regression evidence
The same tests were exercised against untouched upstream
deb4c7a5243d2024b7a627a305e0eecc6bab8c7c, independently:--suite snapshot: fails. The visible dialog's Domain/Next/Verify controls are missing; the snapshot returns 250 Background controls instead.--suite readiness: fails. One click returns after approximately 363 ms;Next=false,dialogRendered=false, thensnapshot returned before the delayed dialog rendered.node --test chrome-extension/background.test.mjs node scripts/snapshot-cleanup.test.mjs node scripts/build-extension.test.mjs node scripts/check-tool-parity.mjs node scripts/tool-defs.mjs --check node scripts/snapshot-dom.test.mjs --chrome /path/to/chrome node scripts/e2e-browser.mjs --server /path/to/munim-computer-use --chrome /path/to/chromium cargo test --locked --manifest-path windows-linux/Cargo.toml cargo check --locked --manifest-path windows-linux/Cargo.tomlVerified locally:
cargo check --lockedpassed on both; Linux native build passed.Limits / compatibility
An explicit selector checks a visible match in the top-level document, not arbitrary application completion, clickability, occlusion, frames or shadow roots. Use a selector specific to the newly expected UI. Timeout defaults to 8000 ms, maximum 10000 ms; the budget excludes browser/API latency and approval prompts. Without a selector, state-returning actions preserve the existing short pause/bounded navigation wait and report
not_requestedrather than pretending that the app is ready.These options need both the updated native server and extension. macOS cannot be built locally on the Windows host; the actual release build, bridge and real-Chrome integration were verified in fork CI below. Upstream approval/execution is a separate gate.
Opus 5.5 audit disposition
The independent subscription-authenticated reviewer used
claude-opus-5-5, reviewedf2a466b, and posted three findings directly on this PR. All three were independently reproduced as RED tests before fixes in signed commit1646abc1dfef109a2d15b83995517a1556f863df:TMPDIR || os.tmpdir(). Tests cover an absent override and an explicit override; an actual native Windows run withTMPDIRunset also passed without checkout artifacts.Current CI verification
Current head:
98606830b5a4e81e436bc5d7fad7be5efeb1077f. This adds only the test-cleanup follow-up from fork-main commit655fc19132449429da55fd90d04bddc6696ff0f3: the DOM runner owns a private POSIX process group, signals surviving descendants even after the launcher exits, and runs four deterministic Linux/macOS teardown regressions in CI. No production/browser-state behavior changed in this follow-up.Current-head local checks passed: all four cleanup regressions, 49 extension checks, three extension-build checks, 32-tool parity, definition round-trip on an LF checkout, runner JS syntax, and Git whitespace checks. The Windows checkout inherited CRLF conversion; the unchanged definition emitter was separately exercised on exact tracked LF bytes. No installed browser/profile or server was used for these local checks.
Current-head CI is being exercised in the verification-only fork PR against the actual upstream base
deb4c7a5243d2024b7a627a305e0eecc6bab8c7c:ElliotDrel#3
Fork integration is already verified independently: merging this head into fork main
655fc19132449429da55fd90d04bddc6696ff0f3is conflict-free and reproduces its exact tree552e466b46c42ec44105e6aed26d47c74c64ca73. The EOF fixec3dac7936eb97f4074ba6476ba16b2fc97f3badand integration mergebf0469ec044b587a6793aae77c295eafd2ccb982remain ancestors of fork main, but neither is imported into this PR. Fork main was not changed.All four fork-main CI jobs passed at
655fc19132449429da55fd90d04bddc6696ff0f3, including Windows/Linux Rust, the Windows backend EOF/restart regression, extension/parity/cleanup gates, Linux real-Chrome DOM/native integration, and the macOS release build/120-peer bridge/real-Chrome native integration:https://github.com/ElliotDrel/munim-computer-use/actions/runs/37655803451
Historical verification at previous PR head
1646abc1dfef109a2d15b83995517a1556f863dfpassed all four jobs, including 18 real-Chrome native Swift integration checks:https://github.com/ElliotDrel/munim-computer-use/actions/runs/37643965060
That previous-head synthetic merge tree equaled
157e0e00d5c2cd760628f82b9206a95d2e0b545b. Its first macOS attempt failed the existing credential-cancellation harness check withno live DevTools target answered; one unchanged rerun passed all 18 checks. This is historical evidence, not a claim that the new head has completed CI.Current upstream CI still requires maintainer approval (
action_required) at the new head:https://github.com/munimtechnologies/munim-computer-use/actions/runs/37657800741
The account has read-only upstream repository permissions and cannot approve that workflow. No approval policy was bypassed or weakened. The PR is conflict-free; upstream CI approval/execution remains the final gate. No installed server, native host, Chrome user profile, MCP configuration or site policy was changed.
Latest-head CI closeout
Verified head
98606830b5a4e81e436bc5d7fad7be5efeb1077f: fork CI passed all four jobs, including macOS release build, multi-peer bridge and real Chrome browser integration, plus Windows/Linux and extension checks. The first macOS attempt failed in the credential-cancellation harness withno live DevTools target answered; one unchanged-head rerun passed. This demonstrates a successful rerun, not a repair of that intermittent harness race. Fork verification is separate from upstream maintainer approval and merge.