Skip to content

feat(cdp): chromium auto-fetch via chromiumoxide fetcher (#78) - #220

Merged
aram-devdocs merged 1 commit into
mainfrom
claude/issue-78-chromium-auto-fetch
May 6, 2026
Merged

feat(cdp): chromium auto-fetch via chromiumoxide fetcher (#78)#220
aram-devdocs merged 1 commit into
mainfrom
claude/issue-78-chromium-auto-fetch

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

Closes #78. Adds an opt-in --auto-fetch-chromium flag that downloads Chrome-for-Testing into a Plumb-managed cache directory when no system Chromium is detected and no --executable-path is given. The fetched binary is hash-pinned via a .plumb-sha256 sidecar so a later tampering attempt is detected at launch and refused.

What changed

  • plumb-cdp::fetcher (new module) — platform cache-dir resolution (XDG / Library/Caches / LOCALAPPDATA), SHA-256 hash verification, cache-root canonicalisation, and the ensure_chromium async entry that drives chromiumoxide::fetcher::BrowserFetcher.
  • CdpError gains AutoFetchFailed, HashMismatch, and CacheDirUnavailable variants.
  • ChromiumOptions gains auto_fetch_chromium: bool and auto_fetch_cache_dir: Option<PathBuf> (the latter for testability).
  • ChromiumDriver and PersistentBrowser resolve auto-fetch up front; an explicit executable_path always wins so the flag is a no-op when the user already supplied a binary.
  • CLI: lint --auto-fetch-chromium (default false).
  • Docs: install-chromium.md gains an auto-fetch section with the cache-path table and a trust-model note.

Trade-off

chromiumoxide_fetcher 0.8 does not re-export Milestone at the crate top level (only BrowserVersion, Channel, Revision, Version, VersionError), so we cannot pin the fetched binary to MIN_SUPPORTED_CHROMIUM_MAJOR via the typed BrowserVersion::Milestone(...) variant. We use Channel::Stable (Chrome-for-Testing's current stable build) instead — Plumb's existing validate_browser_version check fires at launch and refuses to proceed if the fetched binary's major falls outside MIN..=MAX_SUPPORTED_CHROMIUM_MAJOR, so a stable-channel drift surfaces as a typed UnsupportedChromium error rather than a mysterious launch failure. Documented in code at fetcher::ensure_chromium.

Security boundary

Auto-fetch downloads and executes a third-party binary. chromiumoxide does not ship signature verification for Chrome-for-Testing artifacts, so passing --auto-fetch-chromium is the user's explicit acknowledgement of trust. The SHA-256 sidecar protects against post-install tampering, not against a compromised upstream. Documented in fetcher.rs module docs and in docs/src/install-chromium.md.

Test plan

  • cargo fmt --all -- --check
  • cargo clippy --workspace --all-targets --all-features -- -D warnings
  • cargo nextest run --workspace --all-features (402 tests, all pass)
  • just determinism-check (3 runs byte-identical)
  • cargo deny check (advisories / bans / licenses / sources OK)
  • RUSTDOCFLAGS=-Dwarnings cargo doc -p plumb-cdp --all-features --no-deps
  • just check-agents
  • 20 new plumb_cdp::fetcher::tests units cover cache-path resolution (Linux/macOS/Windows + empty XDG + unknown OS), SHA-256 vector spot checks, sidecar record/verify/refuse-on-mismatch, sidecar trailing-newline tolerance, sibling-tempdir rejection by enforce_cache_root, and pinned_milestone == MIN_SUPPORTED_CHROMIUM_MAJOR.
  • 2 new CLI integration tests confirm --auto-fetch-chromium shows up in --help and that the FakeDriver path treats the flag as a no-op (so the test suite never hits the network).

🤖 Generated with Claude Code

Adds an opt-in `--auto-fetch-chromium` flag that downloads
Chrome-for-Testing into a Plumb-managed cache directory when no
system Chromium is detected and no `--executable-path` is given. The
fetched binary is hash-pinned via a `.plumb-sha256` sidecar so a
later tampering attempt is detected at launch and refused.

* New `plumb_cdp::fetcher` module with platform cache-dir resolution
  (XDG / `Library/Caches` / `LOCALAPPDATA`), SHA-256 verification,
  cache-root canonicalisation, and the `ensure_chromium` async entry
  that drives chromiumoxide's `BrowserFetcher`.
* New typed errors: `AutoFetchFailed`, `HashMismatch`,
  `CacheDirUnavailable`.
* `ChromiumOptions` gains `auto_fetch_chromium: bool` and
  `auto_fetch_cache_dir: Option<PathBuf>` so tests can scope the
  cache to a tempdir without hitting `$HOME`.
* `ChromiumDriver` and `PersistentBrowser` resolve auto-fetch up
  front; an explicit `executable_path` always wins so the flag is a
  no-op when the user already supplied a binary.
* CLI: `lint --auto-fetch-chromium` (default `false`).
* Docs: install-chromium.md gains an auto-fetch section with the
  cache-path table and a trust-model note.

20 fetcher unit tests cover cache path resolution across all
platforms (including empty `XDG_CACHE_HOME`), SHA-256 vector spot
checks, sidecar record/verify/refuse-on-mismatch, sidecar
trailing-newline tolerance, sibling-tempdir rejection by
`enforce_cache_root`, and pinning of `pinned_milestone` to
`MIN_SUPPORTED_CHROMIUM_MAJOR`. Two CLI integration tests confirm
the flag appears in `--help` and that the FakeDriver path treats
the flag as a no-op.

Closes #78.
@github-actions

github-actions Bot commented May 6, 2026

Copy link
Copy Markdown
Contributor

Now I have enough to write the full review.


PR Review: feat(cdp): chromium auto-fetch via chromiumoxide fetcher (#78)

1. Determinism

No violations. fetcher::resolve_cache_dir reads env vars, which is already an established pattern in plumb-cdp (ensure_executable_path calls std::env::current_dir at lib.rs:721). The disallowed-methods list in clippy.toml only bans SystemTime::now, Instant::now, and std::env::temp_dir — not std::env::var. The fetcher never feeds env-derived paths into rule-engine output.

HashMap use in #[cfg(test)] is fine; tests are not observable output.

2. Workspace layering

Clean. plumb-cdpplumb-core only. sha2 added to the workspace table correctly. tempfile already in workspace.

3. Error handling

No unwrap()/expect() in library code. clippy.toml sets allow-expect-in-tests = true, so test-only expect calls are correct. The one unwrap_or_else at fetcher.rs:253 is unwrap_or_else, not unwrap; not caught by clippy::unwrap_used. See warning below.

4. Blockers


[BLOCK 1] fetcher.rs:237enforce_cache_root is dead code in production

The function is documented as the path-traversal guard: "Defends against a --cache-dir-style override that resolves through a symlink to /etc or similar." It is tested (three test cases). But it is never called from any production path.

The full runtime chain is:

resolve_auto_fetch (lib.rs:1202)
  → fetcher::resolve_cache_dir()        // or options.auto_fetch_cache_dir.clone()
  → fetcher::ensure_chromium(&cache_dir) // writes into cache_dir without root check

Neither resolve_auto_fetch (lib.rs:1203-1211) nor ensure_chromium (fetcher.rs:292) calls enforce_cache_root. A Rust caller that sets options.auto_fetch_cache_dir = Some(PathBuf::from("/etc")) will get std::fs::create_dir_all("/etc") called silently. The protection is documented, tested, and inert.

Fix: in resolve_auto_fetch (lib.rs:1206-1209), validate the resolved cache_dir before passing it to ensure_chromium:

let default_root = fetcher::resolve_cache_dir()?;
let cache_dir = options
    .auto_fetch_cache_dir
    .as_deref()
    .map(|p| fetcher::enforce_cache_root(p, &default_root))
    .transpose()?
    .unwrap_or(default_root);

[BLOCK 2] fetcher.rs:272pinned_milestone() is a dead public function with a misleading doc

The ensure_chromium doc comment claims the download is "pinned at pinned_milestone()". The function's own doc says: "The Chromium milestone Plumb pins for auto-fetch." Neither is true. ensure_chromium uses BrowserVersion::Channel(Channel::Stable) and never calls pinned_milestone(). The only caller is the test at fetcher.rs:587.

This creates two problems: a documentation lie in a security-critical path (milestone pinning vs. stable-channel), and a dead public symbol that violates the no-legacy-code rule.

Fix: either remove pinned_milestone() and correct the ensure_chromium doc to describe the stable-channel approach accurately, or keep it and actually use it once chromiumoxide exposes Milestone (but then it must not be pub const fn — it should be private until it's wired up).

The module-level doc at fetcher.rs:14 also says "pinned at the supported milestone (PRD §16)" — this must be updated to match the stable-channel reality.


5. Warnings (non-blocking but should be fixed)

fetcher.rs:253expected_root.canonicalize().unwrap_or_else(|_| expected_root.to_path_buf()) silently swallows a canonicalization failure on the root. If the root itself is a broken symlink, canonical.starts_with(&root_canonical) will compare against the uncanonicalized root, and a legitimate path may fail the check (false reject). Low probability but the silent swallow merits a comment or a logged warning.

lib.rs:1206options.auto_fetch_cache_dir.clone() clones an Option<PathBuf> unnecessarily. as_deref() and to_path_buf() in the branch would avoid the full clone, though this is minor.

docs/src/install-chromium.md:87 — The trust-model section accurately describes the download but doesn't mention that a stable-channel drift outside the supported range is caught by validate_browser_version. Worth one sentence so readers understand they're not silently running an unchecked version.


6. Documentation

All public functions have # Errors sections. ChromiumOptions new fields are documented. CLI --help string is clear and includes the trust-acknowledgement language.


Verdict: BLOCK

@aram-devdocs
aram-devdocs merged commit b8c6f3b into main May 6, 2026
16 checks passed
aram-devdocs added a commit that referenced this pull request May 6, 2026
* fix(cli): wire auto_fetch_chromium through plumb watch

Add the `--auto-fetch-chromium` flag to the `Watch` clap command and
forward it to the inner `LintArgs` so `plumb watch` builds again. Without
this, the workspace failed to compile because `LintArgs` declares
`auto_fetch_chromium` as a required field after #220 landed.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(e2e): add plumb-e2e harness crate

Dev-only crate that drives the locally built `plumb` binary against
the framework fixtures under `e2e-sites/`. Builds each fixture, serves
its `dist/` over loopback HTTP via tiny_http, runs `plumb lint
http://127.0.0.1:<port>/ --config e2e-sites/plumb.toml --format json`
three times, asserts byte-equality across runs, and diffs the
target-rule violation breakdown against `expected.json`.

`plumb-e2e` is excluded from `default-members`, never linked from
upstream crates (it shells out to the binary), and depends only on
external crates plus the workspace lints. Fixtures register
themselves in `sites::SITES`; the matrix CI workflow iterates the
same list. Harness invariants:

- The static server refuses path-traversal and never follows symlinks.
- A 25 ms recv polling cadence plus a worker-thread fan-out keeps
  Chromium's many-parallel-chunks pattern responsive.
- The harness shell-execs `just build` per fixture, so each fixture
  owns its own toolchain pinning (npm lockfile, etc.).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* feat(e2e): real-world fixture matrix under e2e-sites/

Six framework fixtures rendering the same intentional violations:

- html-css      — vanilla HTML + hand-rolled CSS tokens.
- tailwind-html — static HTML built with Tailwind v4 CLI.
- react-vite    — React 18 + Vite + Tailwind v4.
- vue           — Vue 3 + Vite + Tailwind v4.
- angular       — Angular 17 standalone components + Tailwind v4
                  (compiled standalone — Angular's bundler peer-pins
                  Tailwind 2/3).
- nextjs        — Next.js 14 App Router exported as static HTML.

Every fixture targets `color/palette-conformance`,
`spacing/grid-conformance`, and `spacing/scale-conformance` via a
`p-[13px]` hero (4 + 4 violations across the longhand sides) and a
`text-[#2e7d2e]` alert (1 palette violation). The `border-[#0b0b0b]`
on each alert pins border-color to a palette token so the four
`border-*-color` longhands stay clean.

Each fixture has:

- A `Justfile` with `build` / `clean` recipes.
- An `expected.json` declaring `target_rules`, `by_rule_id`, and
  `total_target_violations`. The harness asserts equality on the
  target subset; non-target rules are tolerated to stay robust
  against incidental Chromium-side rendering differences (Next.js's
  hidden `<next-route-announcer>` + 4 sides of `margin: -1px` is
  documented in nextjs/README.md).

Lockfiles are committed for every fixture that has node deps so CI
builds are reproducible.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* ci(e2e): publish test-sites + matrix workflow + dogfood diff

Three pieces wire the new e2e-sites infrastructure into CI:

1. `.github/workflows/e2e-sites.yml` — 18-leg matrix
   ({ubuntu, macos, windows} × six fixtures). Each leg sets up Node
   20 (actions/setup-node@v4 with npm cache keyed on the per-fixture
   lockfile) and Chrome stable, builds the fixture via `just build`,
   compiles `plumb-cli` in release, and runs the harness against
   that single site. The harness asserts byte-equality across three
   lint runs and the target-rule breakdown against `expected.json`.

2. `.github/workflows/pages.yml` — extends the docs deploy to also
   build every fixture's `dist/` and copy it under
   `book/test-sites/<framework>/`. Pages serves the docs and
   fixtures from the same artifact; deployed URLs are
   `https://plumb.aramhammoudeh.com/test-sites/<framework>/`.

3. `.github/workflows/dogfood.yml` — adds a `lint-deployed-test-sites`
   matrix that lints each deployed fixture URL daily and asserts the
   live target-rule counts still match the fixture's
   `expected.json`. The local copy is also re-linted via the harness
   so a remote drift surfaces before a real regression ships.

Plus the `just test-e2e` recipe (local mirror of the CI matrix) and a
CHANGELOG entry under `[Unreleased] / Added`.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

* fix(e2e): Windows binary path + flag Next.js leg as advisory

- Resolve `--plumb-bin` paths on Windows by appending `EXE_SUFFIX`
  when the bare path doesn't exist; canonicalize to drop mixed
  `\` / `/` separators in error messages and child-process spawns.
- Mark the Next.js matrix leg as `continue-on-error` while we sort
  out the Chromium hydration timeout in a follow-up.

---------

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@aram-devdocs aram-devdocs mentioned this pull request May 6, 2026
9 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cdp): Chromium auto-fetch via chromiumoxide fetcher

1 participant