fix(mcp): drain a synthesized navigation before the tool replies - #745
Open
ntdatt812 wants to merge 1 commit into
Open
fix(mcp): drain a synthesized navigation before the tool replies#745ntdatt812 wants to merge 1 commit into
ntdatt812 wants to merge 1 commit into
Conversation
A submit click runs the page's form glue and leaves the navigation as pending state. It becomes a request only when a driving layer converts it. The CDP path converts it, in Input.dispatchMouseEvent and after Runtime.evaluate, which is why the same click POSTs over CDP and does nothing over MCP. The transport's own pump cannot cover this. It is armed after every dispatch, but it sits in a biased select behind read_line, so a client that sends its next tool call immediately, which an agent does, wins that race every time. The reply is also written before any pump turn could run, so browser_click answered "Clicked" while nothing had left the process, and location.href already reported the destination, telling the agent the submit had happened. Add settle_synthetic_navigation and call it from every tool that can synthesize a click, a keypress, or run a script, so the invariant holds for the surface rather than for one tool. It is a no-op when nothing is pending. Fixes h4ckf0r0day#618.
Author
|
Obstacle course, for the CONTRIBUTING pre-PR item I could not tick from a unit test. Companion repo at the ref your CI pins (
No delta. The single failure is the same stage on both, and it fails on untouched So it is a property of this host rather than of the change, and I have not chased it further since it is outside the diff. It is deterministic rather than flaky here: re-running just that stage with Everything else in this PR is unchanged. |
3 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #618.
What changed
settle_synthetic_navigation()onBrowserState: consume the pending navigation, and if one landed, clear the ref table (it names elements in a document that is gone) and give the new document one event-loop slice, so a tool that reads the URL or the text next sees the landed page rather than an empty one. It is a no-op when nothing is pending.It is called from
browser_click,browser_fill,browser_type,browser_press_keyandbrowser_evaluate, not frombrowser_clickalone. The invariant @yinnho describes is the useful thing here: every tool that can synthesize a click, a keypress, or run a script drains pending navigation before it replies.browser_evaluaterunninglocation.href = ...orform.submit(), and an Enter throughbrowser_press_key, land in the same pending state, so fixing only the reported tool would leave the same bug one call to the left. Those five tools becomeasync; two existing tests that called them directly gained.await.@yinnho's framing in the issue is right and holds here exactly: pumping is not draining. A submit click runs the page's own form glue and leaves the navigation as pending state. It becomes a request only when a driving layer converts it. The CDP path converts it, in two places:
crates/obscura-cdp/src/domains/input.rs:227after a dispatched mouse event, andcrates/obscura-cdp/src/domains/runtime.rs:32afterRuntime.evaluate. MCP had no equivalent, which is the whole of your Path A versus Path B.Why the existing pump does not already cover it
BrowserState::advance_active_page_tasksalready pumps and drains, and it is armed after every dispatch. It still never gets a turn here, for two independent reasons:The select is
biasedwithread_linefirst, so a client that already has its next tool call queued, which an agent always does, wins that race every time. And the response is written before the loop comes back around at all, so even an unbiased select would reply first.That is also where the misleading part of the report comes from.
location.hrefreports the destination because the pending navigation has already updated the page's URL state. The agent is told the submit happened while no request has left the process, which is worse than an outright failure.Validation
New test
click_on_a_submit_button_issues_the_request_before_replying, built on your repro page: a recording server serving the form,browser_fillon#q,browser_clickon#go. The assertion is at the wire, not onlocation.href, because the URL was the thing that lied.Red with the drain removed from
tool_click, green with it:Full four-configuration sequence from CONTRIBUTING, in a
rust:latestcontainer so the result matches CI rather than my Windows host:cargo build --release -p obscura-cli --bins --features rendercargo nextest run --release --features render --no-fail-fastcargo build --release -p obscura-cli --bins --no-default-featurescargo nextest run --release -p obscura --no-fail-fastcargo nextest run --release --workspace --exclude obscura --exclude obscura-render --no-default-features --no-fail-fastcargo check --release -p obscura-render --no-default-featuresRendering
Not applicable. No layout, paint, screenshot, screencast or PDF code is touched.
browser_screenshotandbrowser_pdfare not among the five tools changed.Performance
The added work is one
process_pending_navigation()per call to those five tools, which returns immediately when nothing is pending, so the cost on the common path is a check. When a navigation is pending the tool now waits for one event-loop slice before replying, which is work the client was going to pay for on its next call anyway, moved to where it is correct. The other MCP tools are untouched.Relationship to the other issues on this
#640's framing, that the transport never pumps the page task queue, is not what breaks this: the pump exists and is armed. It loses a race and, more fundamentally, runs after the reply. So this fix is orthogonal to any pump change and does not close #640.
I have not touched
location.hrefreporting a destination for a navigation that has not been converted. After this change the two agree for these tools, but the underlying "URL state moves before the request" behaviour is still reachable from anywhere that does not drain, and it deserves its own issue rather than a rider here.Checklist