feat(mcp): lint_url accepts real http(s) URLs via ChromiumDriver - #131
Conversation
Branch on `is_fake_url` inside the `lint_url` tool: `plumb-fake://` URLs continue to serve `PlumbSnapshot::canned()` for tests, while `http(s)://` URLs are routed through `ChromiumDriver` against a fixed desktop target (1280x800, DPR 1.0). Driver failures surface as `CallToolResult::error` with the typed `CdpError` message — no unwrap, no panic. Refs #38. The `http://localhost:<N>/fixture.html` protocol test that boots Chromium is deferred to a follow-up milestone since it requires a Chromium binary on the runner; the existing `mcp_lint_url_returns_structured_content` happy path (using `plumb-fake://hello`) keeps CI green. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No description provided. |
|
I have everything I need for a complete review. Here's my assessment: Review:
|
| # | Severity | Location | Issue |
|---|---|---|---|
| 1 | BLOCKER | plumb-mcp/src/lib.rs:111 |
New ChromiumDriver per call violates the documented anti-pattern; each call cold-starts Chromium |
| 2 | REQUEST_CHANGES | .agents/rules/dependency-hierarchy.md:16 |
Rule doc says plumb-mcp depends on plumb-core, plumb-format — plumb-cdp not listed; update the normative rule |
| 3 | REQUEST_CHANGES | crates/plumb-cli/tests/mcp_stdio.rs |
http(s):// code path has no test; deferred work needs a feature-gated skeleton or tracking issue reference, not an unlinked commit-message note |
Verdict: BLOCK
Resolves the conflicts that arose after PR #131 (lint_url over real http(s)://) and PR #133 (delegation-guard merge bypass) merged into main while this branch carried PR #132 (get_config tool). - crates/plumb-mcp/src/lib.rs: keep both new use lines — plumb_cdp::{BrowserDriver, ChromiumDriver, ChromiumOptions, Target, is_fake_url} from main and plumb_config::ConfigError from this branch. Bring in ViewportKey alongside Config/PlumbSnapshot/etc. - Cargo.lock: union plumb-cdp and plumb-config under plumb-mcp. Note: local cargo check is blocked by a disk-space failure in the sandbox toolchain ('No space left on device' while syncing the 1.95.0 channel). Verification deferred to CI.
Merge main containing PR #130 (explain_rule) and PR #131 (lint_url real http URLs) into the list_rules branch. Also resolve doc-only conflict in docs/src/mcp.md. Conflicts in plumb-mcp/src/lib.rs and tests/mcp_protocol.rs are mechanical: both branches added a new tool (`explain_rule` on main, `list_rules` on this branch). The resolution unions both: 4 tools (echo, lint_url, explain_rule, list_rules) registered in list_tools, dispatched in call_tool, and exercised by the test suite. register_builtin import switched to the top-level re-export (plumb_core::register_builtin) introduced on main; the list_rules test now uses the same import rather than the deprecated plumb_core::rules::register_builtin path. Local cargo check skipped — host disk full while syncing toolchain 1.95.0; CI will verify. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Target branch
mainSpec
Fixes #38
Summary
lint_urlnow branches on the URL scheme:plumb-fake://keeps the canned-snapshot path;http(s)://bootsChromiumDriverwith a fixed desktop target (1280x800, DPR 1.0) and runs the rules engine on the real snapshot.CdpError(Chromium-not-found / unsupported version / malformed snapshot / driver fault) surfaces asCallToolResult::errorwith theDisplaytext —isError: true, deterministic message, no panic path.docs/src/mcp.md, the protocol-test description assertion, and the scopedplumb-mcp/AGENTS.mdfollow.Crates touched
plumb-coreplumb-formatplumb-cdpplumb-configplumb-mcpplumb-cli(test fixture only)xtaskdocs/.agents/or.claude/.github/System impact
# Errorssection if fallible)tools/listentry + protocol test)register_builtinentry)cargo xtask schema+ commit result)plumb-mcpnow depends onplumb-cdp(layer 5 → layer 3, permitted bydependency-hierarchy.md)..agents/rules/determinism.md)Architectural compliance
plumb-corehas no internal deps; unsafe only inplumb-cdp;println!/eprintln!only inplumb-cli. The newplumb-mcp -> plumb-cdpedge respects the layered hierarchy.thiserror-derived in libs;anyhowonly inplumb-cli::main.unwrap/expect/panic!in library crates.SystemTime::now/Instant::nowinplumb-core.HashMapin observable output paths (useIndexMap).#[allow(...)]is local and has a one-line rationale.Test plan
just validate(or component checks) pass locally —cargo fmt,cargo build --workspace --all-features,cargo clippy --workspace --all-targets --all-features -- -D warnings,cargo nextest run -p plumb-mcp --all-features,cargo nextest run -p plumb-cli --all-features --test mcp_stdioall green.cargo xtask pre-releasepasses (if rule or schema changed) — N/A.just determinism-checkpasses (3× byte-diff clean) — to run in CI.cargo deny checkpasses — to run in CI.mcp_lint_url_returns_structured_contentstill coversplumb-fake://. See "Reviewer notes" for the deferred protocol test.Documentation
LintUrlArgs::urlupdated; no new public items.# Errorssection on every new public fallible fn —run_stdioalready documented; no new fallible public fns.docs/src/updated when user-visible behavior changed —docs/src/mcp.mdreflects http(s)/fake schemes.docs/src/rules/<category>-<id>.mdwritten for new rules — N/A.Breaking change?
Checklist
codex/<primary>-<type>-<slug>/gh-review --local-diff main...HEADrun locally — pending.Reviewer notes
This is the skeleton initial PR for issue #38. It satisfies two of the three acceptance criteria:
ChromiumDriverand produce real-rule violations.CallToolResult::errorwith deterministic text.http://localhost:<N>/fixture.htmlis deferred to a follow-up milestone — that test needs a Chromium binary on the runner and exercises the real CDP path end-to-end. Out of scope for this skeleton.The existing
mcp_lint_url_returns_structured_contenthappy-path test (usesplumb-fake://hello) keeps CI green without Chromium. The--executable-pathplumbing through MCP args and pre-warmed browser reuse onPlumbServerare tracked separately in batch 3B.