MCP server: default-toolset compatibility (foundations, canonical schemas, full default tools)#105
Conversation
marcmuon
left a comment
marcmuon
left a comment
There was a problem hiding this comment.
This is a genuinely strong increment — I ran the full suite twice locally (113 passed, both runs; the body's "105 tests" is stale, and the branch carries a fifth browser_drop commit the description doesn't mention), and the foundations are the real thing: the file policy does O_EXCL/O_NOFOLLOW creation, symlink-component parent walks, and lstat/realpath re-checks; the resolver's honor-or-error behavior held up under probing (unknown and misspelled keys rejected at top level and nested; canonical wins alias conflicts); and I runtime-verified the eval opt-out fully unregisters the tool. Test quality is real — hundreds of specific assertions, not vacuous checks.
Withholding approval for two fix-first items, both verified against the code and both worth fixing in this PR: the upload-failure retry flag that can make an unrelated click execute twice (a determinism break, which is this project's core promise), and the escape-cleanup path that deletes the file a hostile symlink pointed at (destructive cleanup outside the confinement root). Both have small fixes; details inline. The remaining comments are hardening/completeness — the kind of thing worth catching while the toolset surface is still young.
One more body-level note: CI never runs mcp/tests/ (the workflow's test list doesn't include it and root pytest discovery is scoped to tests/), so all 113 of these tests are green only on contributors' machines. Wiring them into the test workflow is probably the single highest-leverage follow-up in this PR.
| _, chooser = _pending_modals(page) | ||
| if chooser is None: | ||
| # A cancelled chooser after a validation failure is suppressed once | ||
| # by some backends. Replay this already-authorized input click once. |
There was a problem hiding this comment.
Fix before merge — determinism: file_chooser_retry_needed is a page-wide flag set by a failed upload, and the next browser_click on that page replays its own action if no chooser appears — without verifying the target is the file input the flag was set for. Sequence: upload validation fails → flag set → agent clicks any unrelated button → that button is clicked twice while the tool reports one success. A double-executed action on a stateful page (submit, pay, delete) is the exact class of bug this surface exists to prevent. Minimal fix: bind the retry to the originating chooser's input element identity (replay only when the incoming click resolves to that same element), or drop the automatic replay and surface a retry hint instead.
| and resolved is not None | ||
| and not _is_within(resolved, self.output_root) | ||
| ): | ||
| self._remove_entry(resolved) |
There was a problem hiding this comment.
Fix before merge — destructive cleanup: when finalize_output detects a symlink whose resolved target escaped the root, this branch unlinks the resolved external target — i.e. the policy deletes a file outside its confinement root, which is precisely what confinement promises never to touch. If the symlink pointed at a pre-existing user file, cleanup destroys it (and the adversarial test can't see this because it removes the evidence it should assert survives). Fix: remove only the in-root symlink entry, never the resolved external path — and add a sentinel test asserting a pre-existing external target survives the escape attempt unchanged.
| root = Path(output_root).expanduser() | ||
| if not root.is_absolute(): | ||
| root = Path.cwd() / root | ||
| root.mkdir(mode=0o700, parents=True, exist_ok=True) |
There was a problem hiding this comment.
Pointing RUSTWRIGHT_MCP_OUTPUT_DIR at an existing directory makes the policy chmod 0700 it and — via _artifact_files() rglob — treat every pre-existing regular file beneath it as an evictable artifact when the total-byte cap is exceeded. A user who sets the env var to ~/Screenshots can have unrelated files silently deleted by quota eviction. Suggest a per-session subdirectory under the configured root (keeps the UX, scopes chmod and eviction to files this policy created), or track created paths and evict only those.
| self.page_registries[key] = registry | ||
| return registry | ||
|
|
||
| def register_page_handlers(self, page: Any) -> PageRegistry: |
There was a problem hiding this comment.
Popup coverage gap: handlers are installed only at explicit registration sites (navigate/tabs/etc.) — there's no context-level page handler — so a window.open popup's early console/network/dialog/download events are lost until some tool call happens to touch it, and a dialog raised by a fresh popup can sit outside the pending-modal system the no-hang guarantee depends on. The existing multi-tab tests register their second page manually, which masks this. Fix: register a context-level page-created handler at attach time (and remove it at teardown) so every page is covered from birth.
|
|
||
|
|
||
| def _allow_eval() -> bool: | ||
| raw = os.environ.get("RUSTWRIGHT_MCP_ALLOW_EVAL") |
There was a problem hiding this comment.
Runtime-verified: RUSTWRIGHT_MCP_ALLOW_EVAL=0 cleanly unregisters browser_evaluate — but the gate fails open for anything not in the falsy set, including typos (ALLOW_EVAL=flase → tool enabled). For a security toggle, unknown values should be rejected loudly at startup (the RUSTWRIGHT_MCP_TOOLSET parser right below already warns on unknown values — same treatment, or stricter). Registered-by-default is a defensible parity call given the SECURITY note; fail-open on misconfiguration is harder to defend.
| return None | ||
|
|
||
|
|
||
| def compare_tool_schema( |
There was a problem hiding this comment.
The comparator makes this fixture a compatibility floor, not a wire-format pin: extra tools and extra optional params pass silently, fixture-required fields aren't required to stay required, and nested schemas/additionalProperties aren't compared. Concretely, the fixture pins 23 tools while the default inventory registers 25 (browser_reload and browser_get_text are unpinned), so a schema regression in those — or any additive drift — sails through. Since the whole point of the clean-room fixture is catching unintended surface change, suggest comparing the exact normalized tools/list output (names, full schemas, required sets) with an explicit allow-list for intentional additions.
| navigation_start_request_index: int = 1 | ||
| navigation_pending: bool = False | ||
| last_known_title: str | None = None | ||
| console_evictions: dict[int, int] = field(default_factory=dict) |
There was a problem hiding this comment.
Small leak for the long-lived-session case: console_evictions/network_evictions are keyed by navigation epoch and incremented on every ring-buffer eviction, but _begin_navigation bumps the epoch without ever pruning old keys — so a busy session accumulates one dict entry per navigation forever. network_evictions is only ever read for the current epoch, and console_evictions history is only used as a sum, so the history is pure dead weight. Fix is one line each: prune stale epochs in _begin_navigation (network), and fold console into a running total. Worth doing since long-lived parallel sessions are exactly this server's target workload.
marcmuon
left a comment
marcmuon
left a comment
There was a problem hiding this comment.
All seven review items verified on this head — approving. The two fix-before-merge items check out in code: chooser retry binds to the originating input's element identity so an unrelated click can no longer double-execute, and escape cleanup takes a lexical-containment guard with a byte-exact sentinel test proving an external target survives untouched. Strict eval parsing, created-paths-only eviction scoping, context-level popup registration, the 25-tool contract pin, and bounded counters all landed. Suite: 120 passed, run twice, on db5bcb1. Disclosure: an independent review lane rates the residual identity-check race (inline below) and same-path file replacement inside the output root as blocking; I don't — the replacement scenario requires a same-UID writer inside the 0700 root, which is outside this policy's threat model, and the race is a narrow hardening item with the one-line fix suggested inline.
| retry_same_file_input = False | ||
| if retry_element is not None: | ||
| try: | ||
| retry_same_file_input = bool( |
There was a problem hiding this comment.
Non-blocking hardening: the identity check and the replay both go through the live Locator, which re-resolves by selector — so a DOM swap in the ~50ms between check and replay could still hit a different node. Clicking the replay through the captured element handle instead closes that window and fails loudly (detached handle) rather than clicking the wrong element, which is the right failure mode for this surface.
Add RUSTWRIGHT_MCP_CDP_ENDPOINT / _CDP_HEADERS / _CDP_TIMEOUT_MS so the server can attach to an externally-owned browser over CDP instead of launching a local Chromium. In remote mode the local launch options are ignored, teardown detaches without terminating the remote browser, and a dead or unreachable remote fails loudly — it never silently falls back to a local browser. Endpoint and headers are never printed or logged. Docs cover the vendor-neutral usage with a hosted provider (x-api-key header) as an example; tests exercise a live remote (a second local browser) and a dead-endpoint failure. Co-Authored-By: Claude <noreply@anthropic.com>
…er, contract harness Groundwork for default-toolset compatibility (no tool behavior change yet except the documented screenshot-path confinement): - Typed SessionState with an exactly-once per-page handler registrar (console, request/response, filechooser, dialog, download) at every page-acquisition point; a dedicated event lock so callbacks never contend the tool lock; navigation epochs and stable 1-based per-epoch request indices; bounded immutable event records with oldest-first eviction; modal/download slots. - Safe file policy: outputs confined to RUSTWRIGHT_MCP_OUTPUT_DIR (0700 dir, 0600 files, exclusive create, per-file and total byte caps with eviction, workspace-relative artifact links). Post-write finalization rejects a non-regular or escaped output. browser_take_screenshot is migrated to it; a path outside the root now returns a structured error instead of writing (documented breaking change; see mcp/README.md). - Strict ref-or-selector resolver: refs resolve only via the current snapshot registry (never fall through to CSS); selectors must match exactly one node. - serverInfo now reports the package name and version (was the SDK version). - Neutral contract-comparator harness (clean-room fixture: names/types/enums/ defaults/required only) seeded with the already-matching tools. 43 tests pass, including an independent reviewer's adversarial regressions (finalization integrity, workspace-relative links, evicted-record handling).
…files Wire-level compatibility for the ten schema-divergent default tools, with canonical camelCase parameters advertised and snake_case accepted as hidden aliases (canonical wins on conflict). All advertised tools now reject unknown parameters (additionalProperties: false; honor-or-error). - click: element/doubleClick/button/modifiers; type: submit/slowly (clear kept as a documented extension); select_option: values[] (scalar normalized); snapshot: target/filename/depth/boxes (integer enclosing-bounds geometry; zero-area nodes unannotated); screenshot: element/target/type/filename/ fullPage/scale with mutual-exclusion checks; evaluate: function alias, element-context execution, JSON results; handle_dialog: promptText alias; wait_for: time (seconds, capped 30) / textGone; tabs: action enum, close-current default, list in every response. - Sectioned response envelope (Result / Page / Tabs / Snapshot), one snapshot per mutating call, no codegen section. - RUSTWRIGHT_MCP_CAPS env + --caps argv accepted (warn, never crash); RUSTWRIGHT_MCP_TOOLSET=mirror|lean profiles. - browser_evaluate is now registered by default; RUSTWRIGHT_MCP_ALLOW_EVAL=0 disables it (SECURITY note in mcp/README.md; top-level README aligned). - Clean-room contract fixture extended to all ten adapted tools. 85 tests pass, including an independent reviewer's adversarial suite (alias conflicts, schema purity, unknown-parameter rejection, box geometry, caps precedence, envelope ordering).
MCP section now leads with zero-edit setup blocks for Claude Code, Claude Desktop (with config-file locations), and any MCP client, plus a three-row decision table (bundled browser / headed / disabling eval) and the upcoming PyPI shorthand. CLI section gets a try-it-in-60-seconds loop. mcp/README.md puts install + verify on the first screen.
…loads Adds the remaining default tools and the observability/modal/download layer: - browser_resize (number dimensions, device-pixel rounding), browser_drag, browser_fill_form (five field types, first-failure by field name, documented non-transactional), browser_find (snapshot-backed, actionable refs, /pattern/flags regex with i,m,s). - browser_console_messages (severity threshold, per-navigation scoping, artifact output) and the network pair: browser_network_requests (session-monotonic indices — never reused across navigations; static-asset filtering; regex filter preserving indices) + browser_network_request (per-part detail, lazy bounded body reads, base64 for binary, stale-navigation indices rejected with the valid range). - browser_file_upload on pending file-chooser modals (workspace-confined paths, cancel via omission), and dialogs flipped to pending-modal semantics across ALL tabs (no arm-next; no-hang guarantee: tools report modal state instead of evaluating on blocked pages; tabs list uses cached titles). - Automatic downloads (sanitized names, caps) reported exactly once each via per-download flags, plus envelope sections Console (new-since-last, warning+, capped) / Modal / Downloads. - Clean-room contract fixture extended to the full default set. 105 tests pass twice, including two independent adversarial validation suites (cross-tab modals, index stability across navigations, out-of-order download completion, fixture-vs-target-schema checks).
…fixtures Completes the default toolset. Validates sources via the shared file policy (workspace-confined, non-symlink, byte-capped), synthesizes a DataTransfer with File objects and MIME string entries, and dispatches dragenter/dragover/drop on a strictly-resolved unique target. The synthetic nature (untrusted events) is disclosed in the tool description. A machine-readable conformance artifact records observed behavior across four dropzone styles for the future native-drop decision. Mirror profile only. Independently validated: file content fidelity via FileReader, mixed files+data, custom MIME retrieval, preventDefault-style handlers, reject-path security (no page-world leakage on rejects).
… scoping, popups, eval toggle, exact contract, counter bounds Seven review fixes (two 'fix before merge'): - File-chooser retry now binds to the originating input element's identity and replays only when the incoming click resolves to that exact element; otherwise no replay (fixes double-executing an unrelated side-effecting click). - Symlink-escape cleanup removes only the in-root link entry, never the resolved external target; sentinel test asserts a pre-existing external file survives byte-for-byte (the prior adversarial test wrongly required its deletion). - FilePolicy tracks only paths it created; chmod 0700 and eviction never touch pre-existing files under a user-configured RUSTWRIGHT_MCP_OUTPUT_DIR. - Context-level page handler registers every popup (window.open) from birth, so its early console/network/dialog/download events and the no-hang guarantee hold. - RUSTWRIGHT_MCP_ALLOW_EVAL parses strictly (1/true/yes/on|0/false/no/off); unknown values fail loudly at startup instead of silently enabling eval. - Contract comparator now pins the exact normalized tools/list (names + full schemas + required sets) with an explicit additions allow-list; fixture covers all 25 registered tools (adds browser_reload, browser_get_text). - Navigation eviction counters are bounded (network cleared per navigation; console folded to a running total), fixing an unbounded per-epoch dict. 120 tests pass; new regressions for each fix; no adversarial test weakened (the symlink test was corrected to assert the confinement invariant).
db5bcb1 to
9e9f3f8
Compare
|
Synced to rustwright-cloud: https://github.com/Skyvern-AI/rustwright-cloud/pull/108 |
2 participants
Builds the MCP server's default-toolset compatibility (stacked on the remote-CDP branch of #93 — that commit appears here until #93 merges):
RUSTWRIGHT_MCP_OUTPUT_DIRconfinement, 0700/0600, byte caps, post-write integrity checks — screenshot paths outside the root now return a structured error, a documented breaking change); a strict ref-or-selector resolver (refs never fall through to CSS; selectors must match exactly one element);serverInfonow reports the package name/version; a clean-room contract-comparator harness.additionalProperties: false); a sectioned response envelope (Result/Page/Tabs/Snapshot);--caps/env acceptance (warn, never crash);RUSTWRIGHT_MCP_TOOLSET=mirror|leanprofiles;browser_evaluateis registered by default with aRUSTWRIGHT_MCP_ALLOW_EVAL=0opt-out (SECURITY note in mcp/README.md).Verification
120 tests, run twice, all green — covering cross-tab modal handling, index stability across navigations, out-of-order download completion, path-escape and symlink-swap attempts, alias-conflict resolution, and exact fixture-vs-schema checks. The adversarial regression suites are retained under
mcp/tests/validation/.Review updates
Addressed review feedback: the file-chooser retry now binds to the originating input element's identity (no double-executing an unrelated click); symlink-escape cleanup removes only the in-root link, never the resolved external target (a sentinel test asserts the external file survives); the file policy tracks only paths it creates, so a user-configured output dir's pre-existing files are never chmodded or evicted; a context-level page handler covers
window.openpopups from birth;RUSTWRIGHT_MCP_ALLOW_EVALparses strictly and fails loudly on unknown values; the contract comparator now pins the exact normalizedtools/list(all 25 tools + full schemas + required sets) with an explicit additions allow-list; navigation eviction counters are bounded.Notes for reviewers
mcp/tests/contract/fixtures/default_toolset.json) is clean-room: names/types/enums/defaults/required flags only.