feat(cdp,cli): driver ergonomics — wait/cookies/storage/animations

[aram-devdocs]

Closes #74, #75, #76, #77.

Summary

Bundles four CDP-side V1 driver-ergonomics features that all extend the same Target and ChromiumOptions types in plumb-cdp. Splitting would have forced three rebases on the same struct.

• feat(cli): --wait-for <selector> and --wait-ms <ms> flags #74 --wait-for <selector> and --wait-ms <ms> — wait for an element to render or sleep an extra interval before the snapshot.
• feat(cli): --cookie / --header / --auth-script flags #75 --cookie name=value, --header 'name: value', --auth-script <path> — pre-inject session state and run a .js bootstrap before navigation. Cookies and headers are control-character-validated at parse time; auth-script paths are canonicalized and refused when they resolve outside CWD.
• feat(cli,cdp): Playwright storage-state loading #76 --storage-state <path> — load a Playwright storage-state.json. Cookies install before navigation; localStorage writes via page.evaluate after navigation when the origin matches. Parser uses serde(deny_unknown_fields).
• feat(cdp): disable animations + hide scrollbars + pin DPR #77 --disable-animations [bool], --hide-scrollbars [bool], --dpr <factor> — three per-target capture knobs. The first two default to true and inject CSS via Page.addScriptToEvaluateOnNewDocument. --dpr overrides the viewport's device_pixel_ratio through Emulation.setDeviceMetricsOverride.

A new pre_navigate helper centralizes the "set up the page before goto" stages — animation killer, scrollbar killer, auth script, headers, cookies, storage-state cookies — so ChromiumDriver and PersistentBrowser share one implementation.

CLI flags wire through the new LintArgs struct in plumb_cli::commands::lint.

Determinism guarantees

• Cookie iteration is sorted by (name, value) before CDP traffic.
• Header iteration is sorted by name before CDP traffic.
• Storage-state cookies are sorted by (domain, name) and origins by origin.
• The selector wait uses tokio::time::timeout (no Instant::now).
• All three runs of just determinism-check produce byte-identical JSON.

Security guarantees

• --cookie and --header reject CR / LF / NUL bytes (header injection).
• Header names additionally reject whitespace and :.
• --auth-script and --storage-state paths are canonicalized; resolution outside CWD is refused.
• Auth-script paths must end in .js.
• Storage-state JSON refuses unknown fields.

Test plan

• cargo fmt --all -- --check
• cargo clippy --workspace --all-targets --all-features -- -D warnings
• cargo test --workspace --all-features (all suites pass; 12 new lib unit tests in plumb-cdp, 8 new integration tests in plumb-cli)
• just determinism-check (3 runs byte-identical)
• cargo deny check (advisories ok, bans ok, licenses ok, sources ok)
• just validate (full local mirror of CI passes)

Files changed

• crates/plumb-cdp/src/lib.rs — Target + ChromiumOptions extension, Cookie, parse_header_kv, StorageState{,Cookie,Origin,LocalStorageEntry}, pre_navigate, apply_post_navigate_waits, apply_storage_state_local_storage, inject_scrollbar_killer, inject_auth_script, install_extra_headers, install_cookies, install_storage_state_cookies, wait_for_selector, canonicalize_safe_path. New variants on CdpError. 12 new unit tests.
• crates/plumb-cdp/Cargo.toml — adds serde + serde_json to runtime deps.
• crates/plumb-cli/src/main.rs — 8 new Lint flags.
• crates/plumb-cli/src/commands/lint.rs — new LintArgs struct, capture-knob threading, cookie/header parsing, ChromiumOptions wiring.
• crates/plumb-cli/tests/cli_integration.rs — 8 new integration tests.
• crates/plumb-mcp/src/lib.rs, crates/plumb-cdp/tests/*.rs, crates/plumb-cdp/benches/cdp_benchmarks.rs — call-site updates for ..Target::default() / ..ChromiumOptions::default() after the field expansion.
• docs/src/cli.md — updated flag table.

🤖 Generated with Claude Code

[github-actions]

I now have enough context to write the full review.

---

Review: PR #215 — CDP Driver Ergonomics + Security Wave (H1-H3, M1-M4)

Files reviewed: crates/plumb-cdp/src/lib.rs, crates/plumb-cli/src/commands/lint.rs, crates/plumb-cli/src/main.rs, crates/plumb-cli/tests/cli_integration.rs, crates/plumb-mcp/src/lib.rs, Cargo.toml, crates/plumb-cdp/Cargo.toml, docs/src/cli.md

Bucket: plumb-cdp (primary), plumb-cli, plumb-mcp (one-liner), docs, deps

---

1. Determinism invariants

[REQUEST CHANGES] std::env::current_dir in plumb-cdp
crates/plumb-cdp/src/lib.rs:543-545 — canonicalize_safe_path calls std::env::current_dir() inside plumb-cdp. The determinism rule in .agents/rules/determinism.md bans std::env::current_dir in all library crates except plumb-cli. The function doesn't affect snapshot content, but it violates the letter of the written rule. The clippy.toml disallowed-methods list does not include current_dir (only SystemTime::now, Instant::now, temp_dir), so CI won't catch this.

Fix options — pick one:

• Move canonicalize_safe_path / validate_safe_path to a path_guard module inside plumb-cli, pass the validated canonical path into ChromiumOptions::auth_script_canonical/storage_state_canonical.
• Or update determinism.md with an explicit exception: "path-validation guards in plumb-cdp may use current_dir provided the result does not flow into snapshot content."

The second option is acceptable given the documented security rationale, but it must be recorded or the rule and the code are in contradiction.

No other determinism violations found. serde_json::Map (BTreeMap-backed, ordered iteration) is used in install_extra_headers — correct choice. All sorting in StorageState::parse_str uses tuple-string keys — correct. tokio::time::sleep in wait_for_selector is timing-only and doesn't leak into snapshot output — fine.

---

2. Workspace layering

No violations. plumb-cdp only gains serde, serde_json, and url — all external crates already in the workspace. plumb-mcp change is a one-liner (..Target::default()). plumb-core is untouched. No new unsafe blocks introduced — the existing #![deny(unsafe_op_in_unsafe_fn)] override is not changed.

---

3. Error handling

No unwrap/expect in production code paths. All unwrap_or / unwrap_or_default / unwrap_or_else usages are safe fallbacks:

• crates/plumb-cdp/src/lib.rs:547 — cwd.canonicalize().unwrap_or(cwd): silent fallback is conservative (may over-reject but never under-reject). Acceptable for a best-effort guard.
• crates/plumb-cdp/src/lib.rs:778 — origin_of(...).unwrap_or_default(): returns empty string if URL parsing fails, which will never match a stored origin. Safe.

[WARNING] validate_no_ctl uses untyped string dispatch
crates/plumb-cdp/src/lib.rs (private function) — dispatches on kind: &'static str with match kind { "cookie" => ..., _ => ... }. The _ arm silently maps any typo (e.g., "COOKIE") to InvalidHeader. A enum ValidationKind { Cookie, Header } would be safer. This is a private function so the risk is contained, but it's a pattern to avoid in any future callers.

[WARNING] install_storage_state_cookies silently drops expires and sameSite
crates/plumb-cdp/src/lib.rs (the install_storage_state_cookies async fn) — builds CookieParam from StorageStateCookie but does not set expires or same_site. Cookies with explicit expiry are installed as session-only cookies in Chromium. For Plumb's single-navigation use case this is fine, but the function has no doc comment. A single-line doc explaining this is intentional would prevent a future reader from "fixing" it and breaking expiry semantics.

---

4. Test coverage

Good overall. The new CLI tests in crates/plumb-cli/tests/cli_integration.rs cover:

• Happy path for every new flag against FakeDriver.
• Malformed-cookie rejection (no =, CRLF injection).
• Header injection rejection (LF).
• --auth-script and --storage-state outside-CWD rejection — the two M4 tests are exactly the right shape: separate script dir, separate CWD, absolute path passed.

The unit tests in plumb-cdp/src/lib.rs #[cfg(test)] cover the validators exhaustively including all 32 C0 bytes.

[WARNING] wait_for_selector has no unit test
The 50ms-poll + 10s-timeout loop in wait_for_selector is exercisable only against a real Chromium page. The tests correctly note this is deferred to the e2e-chromium feature. This is documented in the test comment block — no action required, but worth acknowledging.

[WARNING] deny_unknown_fields will break on any Playwright storage-state format change
crates/plumb-cdp/src/lib.rs:344,356,383,394 — every StorageState* type uses #[serde(deny_unknown_fields)]. The comment says this is intentional ("a future Playwright addition fails loudly"). The serde error message (unknown field 'X', expected one of [...]) is actually readable. However, docs/src/cli.md describes --storage-state without any mention of format strictness. A user who upgrades Playwright and gets malformed storage-state file: unknown field 'partitionKey' has no indication from the CLI reference that this is a known behavior. Add a note to the --storage-state table entry: "Parsed strictly; unknown Playwright fields cause a parse error — file an issue if your Playwright version adds fields."

---

5. Documentation

docs/src/cli.md adds the new flags table — good. Two items:

[REQUEST CHANGES] Missing strict-parsing note for --storage-state
As above: the table row should note that deny_unknown_fields is in effect. One sentence.

[WARNING] --header value_name inconsistency
crates/plumb-cli/src/main.rs — value_name = "NAME:VALUE" (no space) but the Clap doc says "in name: value form" (with space). parse_header_kv handles both via trim(). Minor documentation inconsistency; value_name = "NAME: VALUE" or "NAME:VALUE" should match the doc example.

---

6. Dependencies

[WARNING] criterion downgrade in PR branch
Cargo.toml:108 (PR branch tip 326b33a) — downgrades criterion from 0.8 to 0.5 with no commit message or comment explaining why. The only bench change is ..Target::default(), which compiles with either version. The pull/215/merge conflict resolution keeps 0.8 from main, so the final artifact is fine — but the PR branch should have the downgrade reverted or justified so the merge commit doesn't mask an unexplained change.

---

Punch list

#	File:line	Severity	Issue
1	crates/plumb-cdp/src/lib.rs:543	REQUEST CHANGES	std::env::current_dir in plumb-cdp violates written determinism rule; update determinism.md or move to plumb-cli
2	docs/src/cli.md (storage-state row)	REQUEST CHANGES	No mention of deny_unknown_fields strictness; add one sentence to table
3	Cargo.toml:108	WARNING	criterion 0.8→0.5 in PR branch is unexplained; revert or document
4	private validate_no_ctl	WARNING	Untyped kind: &'static str dispatch with _ default; use an enum
5	install_storage_state_cookies	WARNING	Silently drops expires/sameSite; add a one-line doc saying this is intentional
6	crates/plumb-cli/src/commands/lint.rs:107-112	WARNING	validate_safe_path return value discarded; let _ = would signal intent
7	main.rs --header clap arg	nit	value_name = "NAME:VALUE" doesn't match the doc's name: value example

Items 1 and 2 require code or doc changes before merge. Items 3–7 are improvements that don't block the security fix.

---

Verdict: REQUEST_CHANGES

[aram-devdocs]

Addresses 06-security-auditor REQUEST_CHANGES findings on PR #215.

Finding	Status	Commit
H1 — TOCTOU on storage-state double-load	fixed	dd46908
H2 — safe-path is TOCTOU (canonicalize-then-open)	doc-only	6f76579
H3 — install_* re-validate inputs	fixed	7ffb119
M1 — validators reject only CR/LF/NUL	fixed	7ffb119
M2 — origin_of lacks WHATWG normalization	fixed	34ce6e2
M3 — storage-state domain/path unvalidated	fixed	22031b1
M4 — CLI safe-path integration tests	fixed	c087a3e

Notes

• H1 — pre_navigate now loads StorageState::load_from_path exactly once and threads the parsed value into both the cookie installer and the post-navigate localStorage replay; the file is never re-opened.
• H2 — added doc comments on canonicalize_safe_path, inject_auth_script, and StorageState::load_from_path explicitly stating that the safe-path check is best-effort, not a sandbox against a co-located attacker. Full cap_std::Dir::open mitigation called out as out of scope.
• H3 — factored out validate_header_name, validate_cookie_name, validate_cookie_value and applied them inside install_extra_headers/install_cookies so the library boundary is checked even when callers bypass the parsers. Cookie domain/path also flow through validate_no_ctl at the install site.
• M1 — validate_no_ctl now rejects every C0 control byte plus DEL (< 0x20 || == 0x7F). Tab is rejected; HTTP whitespace folding is deprecated (RFC 7230 §3.2.4).
• M2 — origin_of now uses url::Url::parse(s).origin().ascii_serialization(). url v2 was already a transitive dep; promoted it to a direct workspace dependency.
• M3 — moved storage-state validation into parse_str (testable without disk I/O); domain and path now go through validate_no_ctl.
• M4 — exposed plumb_cdp::validate_safe_path and called it from the CLI lint orchestrator before driver dispatch, so the safe-path check fires regardless of URL scheme. Added lint_rejects_auth_script_outside_cwd and lint_rejects_storage_state_outside_cwd integration tests.

Local validation

• cargo fmt --all -- --check — clean
• cargo clippy --workspace --all-targets --all-features -- -D warnings — clean
• cargo nextest run --workspace --all-features — 352 tests, 352 passed
• just determinism-check — three runs byte-identical
• cargo deny check — advisories ok, bans ok, licenses ok, sources ok