diff --git a/.github/workflows/claude-code-review.yml b/.github/workflows/claude-code-review.yml index a7ae9ab..f309770 100644 --- a/.github/workflows/claude-code-review.yml +++ b/.github/workflows/claude-code-review.yml @@ -39,7 +39,7 @@ jobs: github.actor != 'dependabot[bot]' && github.actor != 'renovate[bot]' runs-on: ubuntu-latest - timeout-minutes: 20 + timeout-minutes: 30 steps: - uses: actions/checkout@v6 with: @@ -76,7 +76,7 @@ jobs: references. Skip nits unless they stack; focus on substantive issues. claude_args: | - --max-turns 20 + --max-turns 40 --model claude-sonnet-4-6 --allowed-tools "Bash(cargo *),Bash(git diff:*),Bash(git log:*),Read,Glob,Grep" --disallowed-tools "Bash(git push:*),Bash(git commit:*)" diff --git a/.github/workflows/e2e-sites.yml b/.github/workflows/e2e-sites.yml index 365faa7..a008734 100644 --- a/.github/workflows/e2e-sites.yml +++ b/.github/workflows/e2e-sites.yml @@ -26,6 +26,7 @@ jobs: timeout-minutes: 25 strategy: fail-fast: false + max-parallel: 3 matrix: os: [ubuntu-latest, macos-latest, windows-latest] framework: diff --git a/Cargo.lock b/Cargo.lock index b078976..769ff1b 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2116,7 +2116,7 @@ dependencies = [ [[package]] name = "plumb-cdp" -version = "0.0.12" +version = "0.0.13" dependencies = [ "chromiumoxide", "criterion", @@ -2136,7 +2136,7 @@ dependencies = [ [[package]] name = "plumb-cli" -version = "0.0.12" +version = "0.0.13" dependencies = [ "anyhow", "assert_cmd", @@ -2166,7 +2166,7 @@ dependencies = [ [[package]] name = "plumb-codegen" -version = "0.0.12" +version = "0.0.13" dependencies = [ "indexmap", "insta", @@ -2179,7 +2179,7 @@ dependencies = [ [[package]] name = "plumb-config" -version = "0.0.12" +version = "0.0.13" dependencies = [ "dunce", "figment", @@ -2200,7 +2200,7 @@ dependencies = [ [[package]] name = "plumb-core" -version = "0.0.12" +version = "0.0.13" dependencies = [ "indexmap", "insta", @@ -2214,7 +2214,7 @@ dependencies = [ [[package]] name = "plumb-e2e" -version = "0.0.12" +version = "0.0.13" dependencies = [ "anyhow", "assert_cmd", @@ -2231,7 +2231,7 @@ dependencies = [ [[package]] name = "plumb-format" -version = "0.0.12" +version = "0.0.13" dependencies = [ "indexmap", "insta", @@ -2243,7 +2243,7 @@ dependencies = [ [[package]] name = "plumb-mcp" -version = "0.0.12" +version = "0.0.13" dependencies = [ "axum", "indexmap", @@ -4430,7 +4430,7 @@ checksum = "1ffae5123b2d3fc086436f8834ae3ab053a283cfac8fe0a0b8eaae044768a4c4" [[package]] name = "xtask" -version = "0.0.12" +version = "0.0.13" dependencies = [ "anyhow", "clap", diff --git a/crates/plumb-cdp/Cargo.toml b/crates/plumb-cdp/Cargo.toml index 109948f..dcde80d 100644 --- a/crates/plumb-cdp/Cargo.toml +++ b/crates/plumb-cdp/Cargo.toml @@ -52,12 +52,12 @@ serde_json = { workspace = true } url = { workspace = true } # SHA-256 verification of auto-fetched Chromium binaries (issue #78). sha2 = { workspace = true } +tempfile = { workspace = true } [dev-dependencies] plumb-core = { workspace = true, features = ["test-fake"] } tokio = { workspace = true, features = ["test-util"] } serde_json = { workspace = true } -tempfile = { workspace = true } tracing-subscriber = { workspace = true } criterion = { workspace = true } diff --git a/crates/plumb-cdp/src/lib.rs b/crates/plumb-cdp/src/lib.rs index 0fd4ce9..83855fa 100644 --- a/crates/plumb-cdp/src/lib.rs +++ b/crates/plumb-cdp/src/lib.rs @@ -23,10 +23,11 @@ //! //! [`ChromiumDriver::snapshot_all`] launches Chromium exactly once, //! validates [`Browser::version`](chromiumoxide::Browser::version), -//! and then loops over the requested targets — for each it opens a -//! fresh page, applies the per-target viewport via CDP -//! `Emulation.setDeviceMetricsOverride`, navigates to the URL, and -//! calls `DOMSnapshot.captureSnapshot` with the +//! and then loops over the requested targets — the first target's +//! viewport is pinned at launch through Chromium's window size and DPR +//! flags, later targets and explicit DPR pins are applied via CDP +//! `Emulation.setDeviceMetricsOverride`, then Plumb navigates to the +//! URL and calls `DOMSnapshot.captureSnapshot` with the //! [`COMPUTED_STYLE_WHITELIST`] from PRD §10.3. Each CDP response is //! flattened into a [`PlumbSnapshot`] with deterministic ordering //! (nodes sorted by `dom_order`, computed styles inserted in @@ -57,12 +58,18 @@ use indexmap::IndexMap; use plumb_core::report::Rect; use plumb_core::snapshot::{SnapshotNode, TextBox}; use plumb_core::{PlumbSnapshot, ViewportKey}; +use std::future::Future; use std::io; use std::path::{Path, PathBuf}; use std::sync::{Arc, Mutex}; +use std::time::Duration; +use tempfile::TempDir; use chromiumoxide::Page; -use chromiumoxide::cdp::browser_protocol::browser::CloseParams as BrowserCloseParams; +use chromiumoxide::browser::BrowserConfigBuilder; +use chromiumoxide::cdp::browser_protocol::browser::{ + CloseParams as BrowserCloseParams, GetVersionParams, +}; use chromiumoxide::cdp::browser_protocol::dom_snapshot::{ CaptureSnapshotParams, CaptureSnapshotReturns, DocumentSnapshot, }; @@ -70,12 +77,17 @@ use chromiumoxide::cdp::browser_protocol::emulation::SetDeviceMetricsOverridePar use chromiumoxide::cdp::browser_protocol::network::{ CookieParam, Headers, SetCookiesParams, SetExtraHttpHeadersParams, }; -use chromiumoxide::cdp::browser_protocol::page::AddScriptToEvaluateOnNewDocumentParams; +use chromiumoxide::cdp::browser_protocol::page::{ + AddScriptToEvaluateOnNewDocumentParams, GetFrameTreeParams, NavigateParams, +}; use chromiumoxide::cdp::browser_protocol::target::{ - CreateBrowserContextParams, CreateTargetParams, + AttachToTargetParams, CreateBrowserContextParams, CreateTargetParams, SessionId, }; +use chromiumoxide::cdp::js_protocol::runtime::EvaluateParams; +use chromiumoxide::cdp::{CdpEvent, CdpEventMessage}; use chromiumoxide::detection::DetectionOptions; -use chromiumoxide::{Browser, BrowserConfig, Handler}; +use chromiumoxide::types::{CallId, Command, Message, MethodId, Response}; +use chromiumoxide::{Browser, BrowserConfig, Connection, Handler}; use futures_util::StreamExt; use serde::Deserialize; use tokio::task::JoinHandle; @@ -89,6 +101,26 @@ pub const MIN_SUPPORTED_CHROMIUM_MAJOR: u32 = 131; /// constant after running the e2e suite against the new major. pub const MAX_SUPPORTED_CHROMIUM_MAJOR: u32 = 150; +const BROWSER_LAUNCH_TIMEOUT: Duration = Duration::from_secs(30); +const BROWSER_CLOSE_TIMEOUT: Duration = Duration::from_secs(5); +const BROWSER_WAIT_TIMEOUT: Duration = Duration::from_secs(5); +const BROWSER_KILL_TIMEOUT: Duration = Duration::from_secs(5); +const CHROMIUMOXIDE_REQUEST_TIMEOUT: Duration = Duration::from_mins(1); +const CDP_CONTROL_TIMEOUT: Duration = Duration::from_secs(10); +const TARGET_CREATE_TIMEOUT: Duration = Duration::from_secs(10); +const TARGET_ATTACH_TIMEOUT: Duration = Duration::from_secs(75); +const PAGE_COMMAND_TIMEOUT: Duration = Duration::from_secs(25); +const NAVIGATION_ASSIGNMENT_TIMEOUT: Duration = Duration::from_secs(2); +const DOCUMENT_READY_TIMEOUT: Duration = Duration::from_secs(30); +const INITIAL_DOCUMENT_SETTLE_DELAY: Duration = Duration::from_millis(100); +const POST_READY_SETTLE_DELAY: Duration = Duration::from_millis(100); +const NAVIGATION_STATE_READ_TIMEOUT: Duration = Duration::from_secs(10); +const SNAPSHOT_CAPTURE_TIMEOUT_SECS: u64 = 60; +const SNAPSHOT_CAPTURE_TIMEOUT: Duration = Duration::from_secs(SNAPSHOT_CAPTURE_TIMEOUT_SECS); +const TRANSIENT_CAPTURE_RETRIES: usize = 1; +const INITIAL_PAGE_URL: &str = "about:blank"; +const RAW_DEFAULT_READY_SELECTOR: &str = "body"; + /// CSS property whitelist passed to `DOMSnapshot.captureSnapshot` as the /// `computedStyles` argument. /// @@ -166,8 +198,8 @@ pub struct Target { /// Optional additional milliseconds to sleep before capturing the /// snapshot, after navigation (and after [`Self::wait_for_selector`]). pub wait_ms: Option, - /// Inject CSS that disables animations and transitions before the - /// page renders. Defaults to `true` — the historical Plumb behavior + /// Inject CSS that disables animations and transitions before + /// capture. Defaults to `true` — the historical Plumb behavior /// (PRD §16) — and the CLI exposes a flag that flips this value. pub disable_animations: bool, /// Inject CSS that hides page-level scrollbars. Defaults to `true` @@ -777,11 +809,9 @@ pub struct ChromiumOptions { /// Explicit Chrome or Chromium executable path. When unset, Plumb asks /// `chromiumoxide` to detect stable Chrome/Chromium installations. pub executable_path: Option, - /// Override the Chromium profile directory. When unset, `chromiumoxide` - /// reuses a single temp directory across all launches — which is fine - /// for sequential CLI invocations but causes profile-singleton lock - /// contention when multiple drivers run concurrently (e.g. the e2e - /// test suite). Tests pass per-thread tempdirs here. + /// Override the Chromium profile directory. When unset, Plumb creates an + /// isolated temporary profile per browser launch so concurrent or + /// back-to-back drivers never contend on Chromium's SingletonLock. /// /// Profile contents do not flow into [`PlumbSnapshot`] output, so /// varying this path does not violate the determinism invariant. @@ -821,6 +851,11 @@ pub struct ChromiumDriver { options: ChromiumOptions, } +struct ChromiumLaunch { + config: BrowserConfig, + profile_dir: Option, +} + impl ChromiumDriver { /// Build a driver with explicit options. #[must_use] @@ -832,16 +867,20 @@ impl ChromiumDriver { &self, target: &Target, resolved_executable: Option<&Path>, - ) -> Result { + ) -> Result { // PRD §16: pinning launch args removes a class of nondeterminism // (scrollbar overlay differences across DPRs, OS-level scaling). let scale_factor_arg = format!("--force-device-scale-factor={}", target.device_pixel_ratio); let builder = BrowserConfig::builder() + .new_headless_mode() .chrome_detection(DetectionOptions { msedge: false, unstable: false, }) + .request_timeout(CHROMIUMOXIDE_REQUEST_TIMEOUT) + .launch_timeout(BROWSER_LAUNCH_TIMEOUT) .window_size(target.width, target.height) + .viewport(None) .arg("--hide-scrollbars") .arg(scale_factor_arg); @@ -864,13 +903,14 @@ impl ChromiumDriver { builder }; - let builder = if let Some(profile) = &self.options.user_data_dir { - builder.user_data_dir(profile) - } else { - builder - }; + let (builder, profile_dir) = + apply_user_data_dir(builder, self.options.user_data_dir.as_deref())?; - builder.build().map_err(|_| chromium_not_found()) + let config = builder.build().map_err(|_| chromium_not_found())?; + Ok(ChromiumLaunch { + config, + profile_dir, + }) } } @@ -892,22 +932,66 @@ impl BrowserDriver for ChromiumDriver { return Ok(Vec::new()); } + let mut attempts = 0; + let mut use_raw_capture = should_use_raw_capture_path(&targets); + loop { + let result = if use_raw_capture { + self.snapshot_all_once_raw(&targets).await + } else { + self.snapshot_all_once_page(&targets).await + }; + if attempts < TRANSIENT_CAPTURE_RETRIES + && result + .as_ref() + .err() + .is_some_and(is_retryable_capture_timeout) + { + if let Err(err) = &result { + tracing::debug!(attempt = attempts + 1, error = %err, "retrying Chromium capture after transient timeout"); + if should_fallback_to_raw_capture(err) { + tracing::debug!( + attempt = attempts + 1, + "retrying Chromium capture through raw CDP page path" + ); + use_raw_capture = true; + } + } + attempts += 1; + continue; + } + return result; + } + } +} + +impl ChromiumDriver { + async fn snapshot_all_once_page( + &self, + targets: &[Target], + ) -> Result, CdpError> { // Use the first target's dimensions and DPR for the initial - // launch (the `--force-device-scale-factor` arg is fixed at - // launch time). Per-target viewport / DPR is then applied via - // CDP `Emulation.setDeviceMetricsOverride` inside - // `capture_target`, which overrides the launch-time scale - // factor for every page after the first. + // launch. Chromiumoxide's built-in viewport emulation is + // disabled in `browser_config`; otherwise it sends its own + // unlabelled page-level Emulation commands during target + // initialization. The first unpinned target can reuse the + // launch-pinned window/DPR, while later targets and explicit + // `--dpr` pins still use Plumb's bounded/labeled override. let first = &targets[0]; let resolved_executable = resolve_auto_fetch(&self.options).await?; - let config = self.browser_config(first, resolved_executable.as_deref())?; - let mut session = ChromiumSession::launch(config).await?; + let launch = self.browser_config(first, resolved_executable.as_deref())?; + let mut session = ChromiumSession::launch(launch).await?; let result: Result, CdpError> = async { validate_browser_version(&session.browser).await?; let mut snapshots = Vec::with_capacity(targets.len()); - for target in &targets { - let snap = capture_target(&session.browser, target, &self.options).await?; + for (target_index, target) in targets.iter().enumerate() { + let snap = capture_target( + &session.browser, + target, + &self.options, + should_apply_viewport_override(target_index, target), + ) + .await?; snapshots.push(snap); } Ok(snapshots) @@ -923,36 +1007,540 @@ impl BrowserDriver for ChromiumDriver { result } + + async fn snapshot_all_once_raw( + &self, + targets: &[Target], + ) -> Result, CdpError> { + let first = &targets[0]; + let resolved_executable = resolve_auto_fetch(&self.options).await?; + let launch = self.browser_config(first, resolved_executable.as_deref())?; + let mut session = RawChromiumSession::launch(launch).await?; + let mut raw = RawCdpClient::connect(session.websocket_address()).await?; + + let result: Result, CdpError> = async { + validate_browser_version_raw(&mut raw).await?; + let mut snapshots = Vec::with_capacity(targets.len()); + for (target_index, target) in targets.iter().enumerate() { + let snap = capture_target_raw( + &mut raw, + target, + &self.options, + should_apply_viewport_override(target_index, target), + ) + .await?; + snapshots.push(snap); + } + Ok(snapshots) + } + .await; + + if let Err(cleanup_err) = session.shutdown(&mut raw).await { + tracing::debug!(error = %cleanup_err, "failed to clean up raw Chromium session"); + if result.is_ok() { + return Err(cleanup_err); + } + } + + result + } +} + +fn should_use_raw_capture_path(targets: &[Target]) -> bool { + targets.iter().any(|target| { + target.wait_for_selector.is_some() + || matches!( + navigation_method_for_url(target.url.as_str()), + NavigationMethod::CdpNavigate + ) + }) +} + +fn should_fallback_to_raw_capture(err: &CdpError) -> bool { + let CdpError::Driver(source) = err else { + return false; + }; + + source.downcast_ref::().is_some_and(|err| { + let message = err.to_string(); + (err.kind() == io::ErrorKind::TimedOut && message.contains("Browser.new_page")) + || is_page_navigation_init_timeout(&message) + }) +} + +fn is_page_navigation_init_timeout(message: &str) -> bool { + message.contains("exhausted 30s ready-state budget") + && message.contains("Page.navigate exceeded 30s budget") } async fn capture_target( browser: &Browser, target: &Target, options: &ChromiumOptions, + apply_viewport_override: bool, +) -> Result { + let page = create_page_without_load_wait( + browser, + CreateTargetParams { + width: Some(i64::from(target.width)), + height: Some(i64::from(target.height)), + new_window: Some(true), + ..CreateTargetParams::new(INITIAL_PAGE_URL) + }, + ) + .await?; + + settle_initial_document().await; + capture_on_page(&page, target, options, apply_viewport_override).await +} + +async fn capture_target_raw( + cdp: &mut RawCdpClient, + target: &Target, + options: &ChromiumOptions, + apply_viewport_override: bool, ) -> Result { - let page = browser - .new_page("about:blank") + let page = RawPage::create( + cdp, + CreateTargetParams { + width: Some(i64::from(target.width)), + height: Some(i64::from(target.height)), + new_window: Some(true), + ..CreateTargetParams::new(INITIAL_PAGE_URL) + }, + ) + .await?; + + settle_initial_document().await; + capture_on_raw_page(cdp, &page, target, options, apply_viewport_override).await +} + +struct RawCdpClient { + conn: Connection, +} + +impl RawCdpClient { + async fn connect(websocket_address: &str) -> Result { + let conn = with_timeout("CDP websocket connect", CDP_CONTROL_TIMEOUT, async { + Connection::::connect(websocket_address) + .await + .map_err(driver_error) + }) + .await?; + Ok(Self { conn }) + } + + async fn execute( + &mut self, + session_id: Option<&SessionId>, + cmd: T, + ) -> Result { + let method = cmd.identifier(); + let call_id = self.submit(session_id, cmd)?; + self.wait_for_response::(call_id, method).await + } + + fn submit( + &mut self, + session_id: Option<&SessionId>, + cmd: T, + ) -> Result { + let method = cmd.identifier(); + let params = serde_json::to_value(cmd).map_err(serde_driver_error)?; + self.conn + .submit_command(method, session_id.cloned(), params) + .map_err(serde_driver_error) + } + + async fn execute_collecting_page_events( + &mut self, + session_id: &SessionId, + cmd: T, + events: &mut RawNavigationEvents, + ) -> Result { + let method = cmd.identifier(); + let params = serde_json::to_value(cmd).map_err(serde_driver_error)?; + let call_id = self + .conn + .submit_command(method.clone(), Some(session_id.clone()), params) + .map_err(serde_driver_error)?; + self.wait_for_response_collecting_page_events::(call_id, method, session_id, events) + .await + } + + async fn wait_for_response( + &mut self, + call_id: CallId, + method: MethodId, + ) -> Result { + loop { + match self.conn.next().await { + Some(Ok(Message::Response(response))) if response.id == call_id => { + return raw_command_response::(response, &method); + } + Some(Ok(Message::Response(_) | Message::Event(_))) => {} + Some(Err(err)) => return Err(driver_error(err)), + None => { + return Err(CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::UnexpectedEof, + format!("{method} received no response from Chromium"), + )))); + } + } + } + } + + async fn wait_for_response_collecting_page_events( + &mut self, + call_id: CallId, + method: MethodId, + session_id: &SessionId, + events: &mut RawNavigationEvents, + ) -> Result { + loop { + match self.conn.next().await { + Some(Ok(Message::Response(response))) if response.id == call_id => { + return raw_command_response::(response, &method); + } + Some(Ok(Message::Event(event))) => { + events.observe_message(&event, session_id); + } + Some(Ok(Message::Response(_))) => {} + Some(Err(err)) => return Err(driver_error(err)), + None => { + return Err(CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::UnexpectedEof, + format!("{method} received no response from Chromium"), + )))); + } + } + } + } + + async fn collect_next_page_event( + &mut self, + session_id: &SessionId, + events: &mut RawNavigationEvents, + ) -> Result<(), CdpError> { + loop { + match self.conn.next().await { + Some(Ok(Message::Event(event))) => { + if events.observe_message(&event, session_id) { + return Ok(()); + } + } + Some(Ok(Message::Response(_))) => {} + Some(Err(err)) => return Err(driver_error(err)), + None => { + return Err(CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::UnexpectedEof, + "raw CDP event stream ended before navigation completed", + )))); + } + } + } + } +} + +fn raw_command_response( + response: Response, + method: &MethodId, +) -> Result { + if let Some(result) = response.result { + return T::response_from_value(result).map_err(serde_driver_error); + } + if let Some(err) = response.error { + return Err(CdpError::Driver(Box::new(io::Error::other(format!( + "{method} failed: {err}" + ))))); + } + Err(CdpError::Driver(Box::new(io::Error::other(format!( + "{method} returned neither result nor error" + ))))) +} + +struct RawPage { + session_id: SessionId, +} + +#[derive(Default)] +struct RawNavigationEvents { + main_frame_url: Option, + dom_content_event: bool, + load_event: bool, +} + +impl RawNavigationEvents { + fn observe_message(&mut self, event: &CdpEventMessage, session_id: &SessionId) -> bool { + if event.session_id.as_deref() != Some(session_id.as_ref()) { + return false; + } + match &event.params { + CdpEvent::PageFrameNavigated(frame) if frame.frame.parent_id.is_none() => { + self.observe_main_frame_url(&frame.frame.url); + true + } + CdpEvent::PageDomContentEventFired(_) => { + self.observe_dom_content_event(); + true + } + CdpEvent::PageLoadEventFired(_) => { + self.observe_load_event(); + true + } + _ => false, + } + } + + fn observe_main_frame_url(&mut self, url: &str) { + if url_has_navigated(url) { + self.main_frame_url = Some(url.to_owned()); + self.dom_content_event = false; + self.load_event = false; + } + } + + fn observe_dom_content_event(&mut self) { + self.dom_content_event = true; + } + + fn observe_load_event(&mut self) { + self.load_event = true; + } + + fn is_ready_for_capture(&self, allow_interactive: bool) -> bool { + self.main_frame_url.is_some() + && (self.load_event || (allow_interactive && self.dom_content_event)) + } + + fn has_navigated(&self) -> bool { + self.main_frame_url.is_some() + } + + fn is_chrome_error_page(&self) -> bool { + self.main_frame_url + .as_deref() + .is_some_and(|url| url.starts_with("chrome-error:")) + } + + fn main_frame_url(&self) -> Option<&str> { + self.main_frame_url.as_deref() + } +} + +impl RawPage { + async fn create(cdp: &mut RawCdpClient, params: CreateTargetParams) -> Result { + let target_id = with_timeout("Target.createTarget", TARGET_CREATE_TIMEOUT, async { + cdp.execute(None, params) + .await + .map(|response| response.target_id) + }) .await - .map_err(driver_error)?; + .map_err(|err| target_lifecycle_error("Target.createTarget", &err))?; + + let attach = AttachToTargetParams::builder() + .target_id(target_id) + .flatten(true) + .build() + .map_err(driver_message)?; + let session_id = with_timeout("Target.attachToTarget", TARGET_ATTACH_TIMEOUT, async { + cdp.execute(None, attach) + .await + .map(|response| response.session_id) + }) + .await + .map_err(|err| target_lifecycle_error("Target.attachToTarget", &err))?; + + Ok(Self { session_id }) + } + + async fn execute( + &self, + cdp: &mut RawCdpClient, + operation: &str, + timeout: Duration, + cmd: T, + ) -> Result { + with_timeout(operation, timeout, async { + cdp.execute(Some(&self.session_id), cmd).await + }) + .await + } + + async fn execute_collecting_page_events( + &self, + cdp: &mut RawCdpClient, + operation: &str, + timeout: Duration, + cmd: T, + events: &mut RawNavigationEvents, + ) -> Result { + with_timeout(operation, timeout, async { + cdp.execute_collecting_page_events(&self.session_id, cmd, events) + .await + }) + .await + } + + async fn evaluate_value( + &self, + cdp: &mut RawCdpClient, + operation: &str, + timeout: Duration, + expression: &str, + ) -> Result { + let params = EvaluateParams::builder() + .expression(expression) + .await_promise(true) + .return_by_value(true) + .build() + .map_err(driver_message)?; + let result = self.execute(cdp, operation, timeout, params).await?; + if let Some(exception) = result.exception_details { + return Err(driver_error( + chromiumoxide::error::CdpError::JavascriptException(Box::new(exception)), + )); + } + let value = result.result.value.ok_or_else(|| { + CdpError::Driver(Box::new(io::Error::other(format!( + "{operation} returned no value" + )))) + })?; + serde_json::from_value(value).map_err(serde_driver_error) + } + + async fn evaluate_unit( + &self, + cdp: &mut RawCdpClient, + operation: &str, + timeout: Duration, + expression: &str, + ) -> Result<(), CdpError> { + let params = EvaluateParams::builder() + .expression(expression) + .await_promise(true) + .return_by_value(true) + .build() + .map_err(driver_message)?; + let result = self.execute(cdp, operation, timeout, params).await?; + if let Some(exception) = result.exception_details { + return Err(driver_error( + chromiumoxide::error::CdpError::JavascriptException(Box::new(exception)), + )); + } + Ok(()) + } +} - capture_on_page(&page, target, options).await +async fn capture_on_raw_page( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, + options: &ChromiumOptions, + apply_viewport_override: bool, +) -> Result { + if apply_viewport_override { + apply_viewport_raw(cdp, page, target).await?; + } + let storage_state = pre_navigate_raw(cdp, page, target, options).await?; + let deterministic_styles_installed = false; + let page_events_enabled = false; + + navigate_raw(cdp, page, target, page_events_enabled).await?; + settle_ready_document().await; + + apply_post_navigate_waits_raw(cdp, page, target).await?; + apply_storage_state_local_storage_raw(cdp, page, target, storage_state.as_ref()).await?; + if should_apply_raw_post_navigation_deterministic_styles( + deterministic_styles_installed, + target, + page_events_enabled, + ) { + apply_deterministic_styles_raw_best_effort(cdp, page, target).await?; + } + + let params = CaptureSnapshotParams { + computed_styles: COMPUTED_STYLE_WHITELIST + .iter() + .map(|s| (*s).to_string()) + .collect(), + include_paint_order: Some(true), + include_dom_rects: Some(true), + include_blended_background_colors: Some(true), + include_text_color_opacities: None, + }; + + let response = page + .execute( + cdp, + "DOMSnapshot.captureSnapshot", + SNAPSHOT_CAPTURE_TIMEOUT, + params, + ) + .await?; + flatten_snapshot(target, &response) +} + +async fn create_page_without_load_wait( + browser: &Browser, + params: CreateTargetParams, +) -> Result { + let target_id = with_timeout("Target.createTarget", TARGET_CREATE_TIMEOUT, async { + browser + .execute(params) + .await + .map(|response| response.result.target_id) + .map_err(driver_error) + }) + .await + .map_err(|err| target_lifecycle_error("Target.createTarget", &err))?; + + with_timeout("Target.attachToTarget", TARGET_ATTACH_TIMEOUT, async { + loop { + match browser.get_page(target_id.clone()).await { + Ok(page) => return Ok(page), + Err(chromiumoxide::error::CdpError::NotFound) => { + tokio::time::sleep(Duration::from_millis(50)).await; + } + Err(err) => return Err(driver_error(err)), + } + } + }) + .await + .map_err(|err| target_lifecycle_error("Target.attachToTarget", &err)) +} + +fn target_lifecycle_error(stage: &str, err: &CdpError) -> CdpError { + let kind = if is_retryable_capture_timeout(err) { + io::ErrorKind::TimedOut + } else { + io::ErrorKind::Other + }; + CdpError::Driver(Box::new(io::Error::new( + kind, + format!("{stage} failed before navigation: {err}"), + ))) } -/// Apply viewport / animation hooks, install cookies and headers, -/// navigate, capture a DOM snapshot. +/// Apply viewport, install pre-navigation state, navigate, wait for +/// final page state, apply deterministic styling, then capture a DOM +/// snapshot. /// /// Shared between `ChromiumDriver::capture_target` and /// [`PersistentBrowser::snapshot`] so that the per-target work is /// expressed in exactly one place. The function is split into discrete /// stages — `apply_viewport` (DPR + dimensions), `pre_navigate` -/// (cookies, headers, auth-script, storage-state, animation killer, -/// scrollbar killer), `goto` + waits, then capture. +/// (cookies, headers, auth-script, storage-state cookies), `goto`, +/// waits, deterministic style injection, then capture. async fn capture_on_page( page: &Page, target: &Target, options: &ChromiumOptions, + apply_viewport_override: bool, ) -> Result { - apply_viewport(page, target).await?; + if apply_viewport_override { + apply_viewport(page, target).await?; + } // `pre_navigate` returns the parsed `StorageState` (when one is // configured) so the post-navigate localStorage step reuses the // same parsed value. Loading the file twice would open a @@ -960,11 +1548,11 @@ async fn capture_on_page( // cookie installation and localStorage replay. let storage_state = pre_navigate(page, target, options).await?; - page.goto(target.url.as_str()).await.map_err(driver_error)?; - page.wait_for_navigation().await.map_err(driver_error)?; + navigate_page(page, target.url.as_str()).await?; apply_post_navigate_waits(page, target).await?; apply_storage_state_local_storage(page, target, storage_state.as_ref()).await?; + apply_deterministic_styles(page, target).await?; let params = CaptureSnapshotParams { computed_styles: COMPUTED_STYLE_WHITELIST @@ -977,7 +1565,12 @@ async fn capture_on_page( include_text_color_opacities: None, }; - let response = page.execute(params).await.map_err(driver_error)?; + let response = with_timeout( + "DOMSnapshot.captureSnapshot", + SNAPSHOT_CAPTURE_TIMEOUT, + async { page.execute(params).await.map_err(driver_error) }, + ) + .await?; flatten_snapshot(target, &response.result) } @@ -1002,16 +1595,17 @@ pub struct PersistentBrowser { struct PersistentBrowserInner { browser: Browser, handler_task: Mutex>>, + _profile_dir: Option, options: ChromiumOptions, } impl PersistentBrowser { /// Launch Chromium and validate its version. /// - /// Per-call viewport and DPR are applied via - /// `Emulation.setDeviceMetricsOverride` inside [`Self::snapshot`], - /// so the launch-time defaults here are placeholders sized to a - /// 1280×800 desktop window. + /// Each snapshot creates a fresh target at the requested viewport + /// size. DPR overrides are still applied via + /// `Emulation.setDeviceMetricsOverride`, so the launch-time defaults + /// here are placeholders sized to a 1280×800 desktop window. /// /// # Errors /// @@ -1021,16 +1615,31 @@ impl PersistentBrowser { /// range, or [`CdpError::Driver`] for any other launch failure. pub async fn launch(options: ChromiumOptions) -> Result { let resolved_executable = resolve_auto_fetch(&options).await?; - let config = persistent_browser_config(&options, resolved_executable.as_deref())?; - let (browser, handler) = Browser::launch(config).await.map_err(map_launch_error)?; + let launch = persistent_browser_config(&options, resolved_executable.as_deref())?; + let ChromiumLaunch { + config, + profile_dir, + } = launch; + let (mut browser, handler) = + with_timeout("Chromium launch", BROWSER_LAUNCH_TIMEOUT, async { + Browser::launch(config).await.map_err(map_launch_error) + }) + .await?; let handler_task = poll_handler(handler); // Validate the version before stashing the browser in `Arc` — - // on failure, dropping the browser here causes - // `Browser::drop` to reap the child synchronously. + // on failure, explicitly close/wait before the isolated + // profile is dropped so Windows does not retain locked files. if let Err(err) = validate_browser_version(&browser).await { - handler_task.abort(); - drop(browser); + if let Err(cleanup_err) = + cleanup_failed_persistent_launch(&mut browser, handler_task).await + { + tracing::debug!( + error = %cleanup_err, + "failed to clean up Chromium after version validation failure" + ); + } + let _profile_dir = profile_dir; return Err(err); } @@ -1038,6 +1647,7 @@ impl PersistentBrowser { inner: Arc::new(PersistentBrowserInner { browser, handler_task: Mutex::new(Some(handler_task)), + _profile_dir: profile_dir, options, }), }) @@ -1052,47 +1662,73 @@ impl PersistentBrowser { /// [`CdpError::MalformedSnapshot`] when the response cannot be /// flattened. pub async fn snapshot(&self, target: Target) -> Result { - let ctx_id = self - .inner - .browser - .create_browser_context(CreateBrowserContextParams::default()) - .await - .map_err(driver_error)?; + let mut attempts = 0; + loop { + let result = self.snapshot_once(&target).await; + if attempts < TRANSIENT_CAPTURE_RETRIES + && result + .as_ref() + .err() + .is_some_and(is_retryable_capture_timeout) + { + if let Err(err) = &result { + tracing::debug!(attempt = attempts + 1, error = %err, "retrying persistent Chromium capture after transient timeout"); + } + attempts += 1; + continue; + } + return result; + } + } + + async fn snapshot_once(&self, target: &Target) -> Result { + let ctx_id = with_timeout("Target.createBrowserContext", CDP_CONTROL_TIMEOUT, async { + self.inner + .browser + .create_browser_context(CreateBrowserContextParams::default()) + .await + .map_err(driver_error) + }) + .await?; let result: Result = async { let create_params = CreateTargetParams { - url: "about:blank".to_string(), + url: INITIAL_PAGE_URL.to_string(), left: None, top: None, - width: None, - height: None, + width: Some(i64::from(target.width)), + height: Some(i64::from(target.height)), window_state: None, browser_context_id: Some(ctx_id.clone()), enable_begin_frame_control: None, - new_window: None, + new_window: Some(true), background: None, for_tab: None, hidden: None, }; - let page = self - .inner - .browser - .new_page(create_params) - .await - .map_err(driver_error)?; - capture_on_page(&page, &target, &self.inner.options).await + let page = create_page_without_load_wait(&self.inner.browser, create_params).await?; + settle_initial_document().await; + capture_on_page( + &page, + target, + &self.inner.options, + should_apply_persistent_viewport_override(target), + ) + .await } .await; // Always dispose the incognito context, even on failure. Mirror // the swallow-and-log pattern from `ChromiumSession::shutdown` // so cleanup errors never mask the underlying snapshot result. - if let Err(err) = self - .inner - .browser - .dispose_browser_context(ctx_id) - .await - .map_err(driver_error) + if let Err(err) = with_timeout("Target.disposeBrowserContext", CDP_CONTROL_TIMEOUT, async { + self.inner + .browser + .dispose_browser_context(ctx_id) + .await + .map_err(driver_error) + }) + .await { tracing::debug!(error = %err, "failed to dispose incognito browser context"); } @@ -1163,21 +1799,45 @@ impl BrowserDriver for PersistentBrowser { } } +fn apply_user_data_dir( + builder: BrowserConfigBuilder, + user_data_dir: Option<&Path>, +) -> Result<(BrowserConfigBuilder, Option), CdpError> { + if let Some(profile) = user_data_dir { + return Ok((builder.user_data_dir(profile), None)); + } + + let profile = tempfile::Builder::new() + .prefix("plumb-chromium-") + .tempdir() + .map_err(|err| { + CdpError::Driver(Box::new(io::Error::other(format!( + "create isolated Chromium profile: {err}" + )))) + })?; + let builder = builder.user_data_dir(profile.path()); + Ok((builder, Some(profile))) +} + fn persistent_browser_config( options: &ChromiumOptions, resolved_executable: Option<&Path>, -) -> Result { +) -> Result { // PRD §16: pinning launch args removes a class of nondeterminism // (scrollbar overlay differences across DPRs, OS-level scaling). - // `PersistentBrowser` does not fix a launch-time DPR — every - // snapshot calls `Emulation.setDeviceMetricsOverride` to drive - // both viewport and DPR per-call. + // `PersistentBrowser` creates every target at the requested + // viewport size. It does not fix a launch-time DPR — snapshots + // that request a non-default DPR use `Emulation.setDeviceMetricsOverride`. let builder = BrowserConfig::builder() + .new_headless_mode() .chrome_detection(DetectionOptions { msedge: false, unstable: false, }) + .request_timeout(CHROMIUMOXIDE_REQUEST_TIMEOUT) + .launch_timeout(BROWSER_LAUNCH_TIMEOUT) .window_size(1280, 800) + .viewport(None) .arg("--hide-scrollbars"); // Same precedence rule as `ChromiumDriver::browser_config`. @@ -1193,13 +1853,13 @@ fn persistent_browser_config( builder }; - let builder = if let Some(profile) = &options.user_data_dir { - builder.user_data_dir(profile) - } else { - builder - }; + let (builder, profile_dir) = apply_user_data_dir(builder, options.user_data_dir.as_deref())?; - builder.build().map_err(|_| chromium_not_found()) + let config = builder.build().map_err(|_| chromium_not_found())?; + Ok(ChromiumLaunch { + config, + profile_dir, + }) } /// When auto-fetch is enabled and the user didn't pin an @@ -1224,6 +1884,14 @@ async fn resolve_auto_fetch(options: &ChromiumOptions) -> Result Ok(Some(path)) } +fn should_apply_viewport_override(target_index: usize, target: &Target) -> bool { + target_index != 0 || target.pin_dpr.is_some() +} + +fn should_apply_persistent_viewport_override(target: &Target) -> bool { + target.pin_dpr.is_some() || (target.effective_dpr() - 1.0).abs() > f64::EPSILON +} + async fn apply_viewport(page: &Page, target: &Target) -> Result<(), CdpError> { // `pin_dpr` (PRD §15 — `--dpr`) wins over `device_pixel_ratio` so // that callers can stress determinism by pinning a hidpi factor @@ -1242,19 +1910,52 @@ async fn apply_viewport(page: &Page, target: &Target) -> Result<(), CdpError> { screen_orientation: None, viewport: None, }; - page.execute(params).await.map_err(driver_error)?; + with_timeout( + "Emulation.setDeviceMetricsOverride", + PAGE_COMMAND_TIMEOUT, + async { page.execute(params).await.map(|_| ()).map_err(driver_error) }, + ) + .await?; + Ok(()) +} + +async fn apply_viewport_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, +) -> Result<(), CdpError> { + let params = SetDeviceMetricsOverrideParams { + width: i64::from(target.width), + height: i64::from(target.height), + device_scale_factor: target.effective_dpr(), + mobile: false, + scale: None, + screen_width: None, + screen_height: None, + position_x: None, + position_y: None, + dont_set_visible_size: None, + screen_orientation: None, + viewport: None, + }; + page.execute( + cdp, + "Emulation.setDeviceMetricsOverride", + PAGE_COMMAND_TIMEOUT, + params, + ) + .await?; Ok(()) } /// All work that must happen on a fresh page before navigation. /// /// Runs in this fixed order so behavior matches what users expect: -/// 1. Animation/scrollbar CSS killers — PRD §16 determinism. -/// 2. Auth script — runs before any page script, so the page-side +/// 1. Auth script — runs before any page script, so the page-side /// bootstrap can set window globals before the SPA boots. -/// 3. Cookies and HTTP headers — set on the network layer before the +/// 2. Cookies and HTTP headers — set on the network layer before the /// very first request leaves Chromium. -/// 4. Storage-state cookies — same network layer; localStorage entries +/// 3. Storage-state cookies — same network layer; localStorage entries /// in the storage-state are deferred to [`apply_storage_state_local_storage`] /// after the origin loads, since localStorage is origin-scoped. /// @@ -1269,12 +1970,6 @@ async fn pre_navigate( target: &Target, options: &ChromiumOptions, ) -> Result, CdpError> { - if target.disable_animations { - inject_animation_killer(page).await?; - } - if target.hide_scrollbars { - inject_scrollbar_killer(page).await?; - } if let Some(script_path) = options.auth_script.as_deref() { inject_auth_script(page, script_path).await?; } @@ -1294,6 +1989,558 @@ async fn pre_navigate( Ok(storage_state) } +async fn pre_navigate_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, + options: &ChromiumOptions, +) -> Result, CdpError> { + if let Some(script_path) = options.auth_script.as_deref() { + inject_auth_script_raw(cdp, page, script_path).await?; + } + if !options.headers.is_empty() { + install_extra_headers_raw(cdp, page, &options.headers).await?; + } + if !options.cookies.is_empty() { + install_cookies_raw(cdp, page, &options.cookies, target.url.as_str()).await?; + } + let storage_state = if let Some(state_path) = options.storage_state.as_deref() { + let state = StorageState::load_from_path(state_path)?; + install_storage_state_cookies_raw(cdp, page, &state).await?; + Some(state) + } else { + None + }; + Ok(storage_state) +} + +#[derive(Debug, Deserialize)] +struct NavigationState { + href: String, + #[serde(rename = "readyState")] + ready_state: String, + #[serde(rename = "isChromeErrorPage", default)] + is_chrome_error_page: bool, +} + +async fn navigate_page(page: &Page, url: &str) -> Result<(), CdpError> { + let initial_result = match page_navigation_method_for_url(url) { + NavigationMethod::ChromiumoxideGoto => { + with_timeout("Page.navigate", DOCUMENT_READY_TIMEOUT, async { + page.goto(url).await.map(|_| ()).map_err(driver_error) + }) + .await + } + NavigationMethod::CdpNavigate => { + with_timeout("Page.navigate", PAGE_COMMAND_TIMEOUT, async { + page.execute(NavigateParams::new(url)) + .await + .map_err(driver_error) + .and_then(|response| { + if let Some(error_text) = &response.error_text { + Err(CdpError::Driver(Box::new(io::Error::other(format!( + "Page.navigate failed: {error_text}" + ))))) + } else { + Ok(()) + } + }) + }) + .await + } + NavigationMethod::LocationAssign => { + let script = navigation_assignment_script(url)?; + with_timeout( + "navigation location assignment", + NAVIGATION_ASSIGNMENT_TIMEOUT, + async { + page.evaluate(script.as_str()) + .await + .map(|_| ()) + .map_err(driver_error) + }, + ) + .await + } + }; + + wait_for_document_ready(page, navigation_display_url(url), initial_result.err()).await +} + +fn page_navigation_method_for_url(url: &str) -> NavigationMethod { + if url.starts_with("data:") { + NavigationMethod::CdpNavigate + } else { + NavigationMethod::ChromiumoxideGoto + } +} + +async fn navigate_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, + page_events_enabled: bool, +) -> Result<(), CdpError> { + let mut events = RawNavigationEvents::default(); + let initial_result = navigate_raw_by_page_navigate( + cdp, + page, + target.url.as_str(), + page_events_enabled, + &mut events, + uses_raw_tolerant_page_navigate(target.url.as_str()), + ) + .await; + + wait_for_document_ready_raw( + cdp, + page, + navigation_display_url(target.url.as_str()), + initial_result.err(), + target.wait_for_selector.is_some(), + raw_navigation_events_for_wait(page_events_enabled, events), + ) + .await +} + +async fn navigate_raw_by_page_navigate( + cdp: &mut RawCdpClient, + page: &RawPage, + url: &str, + page_events_enabled: bool, + events: &mut RawNavigationEvents, + tolerate_navigation_abort: bool, +) -> Result<(), CdpError> { + if page_events_enabled { + page.execute_collecting_page_events( + cdp, + "Page.navigate", + PAGE_COMMAND_TIMEOUT, + NavigateParams::new(url), + events, + ) + .await + } else { + page.execute( + cdp, + "Page.navigate", + PAGE_COMMAND_TIMEOUT, + NavigateParams::new(url), + ) + .await + } + .and_then(|response| { + if let Some(error_text) = response.error_text { + if tolerate_navigation_abort && raw_page_navigate_error_is_tolerated(&error_text) { + events.observe_main_frame_url(url); + return Ok(()); + } + Err(CdpError::Driver(Box::new(io::Error::other(format!( + "Page.navigate failed: {error_text}" + ))))) + } else { + events.observe_main_frame_url(url); + Ok(()) + } + }) +} + +fn raw_page_navigate_error_is_tolerated(error_text: &str) -> bool { + error_text == "net::ERR_ABORTED" +} + +fn raw_navigation_events_for_wait( + page_events_enabled: bool, + events: RawNavigationEvents, +) -> Option { + (page_events_enabled || events.has_navigated()).then_some(events) +} + +fn uses_chromiumoxide_goto(url: &str) -> bool { + url.starts_with("file://") +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +enum NavigationMethod { + ChromiumoxideGoto, + CdpNavigate, + LocationAssign, +} + +fn navigation_method_for_url(url: &str) -> NavigationMethod { + if uses_chromiumoxide_goto(url) { + NavigationMethod::ChromiumoxideGoto + } else if url.starts_with("data:") { + NavigationMethod::CdpNavigate + } else { + NavigationMethod::LocationAssign + } +} + +fn uses_raw_tolerant_page_navigate(url: &str) -> bool { + matches!( + navigation_method_for_url(url), + NavigationMethod::LocationAssign + ) +} + +fn navigation_assignment_script(url: &str) -> Result { + let quoted_url = serde_json::to_string(url).map_err(|err| { + CdpError::Driver(Box::new(io::Error::other(format!( + "serialize navigation URL: {err}" + )))) + })?; + Ok(format!("window.location.assign({quoted_url});")) +} + +async fn wait_for_document_ready( + page: &Page, + display_url: &str, + initial_error: Option, +) -> Result<(), CdpError> { + let mut last_state_error = None; + let attempt = async { + loop { + tokio::time::sleep(Duration::from_millis(50)).await; + if poll_document_ready(page, display_url, &mut last_state_error).await? { + return Ok(()); + } + } + }; + + if let Ok(result) = tokio::time::timeout(DOCUMENT_READY_TIMEOUT, attempt).await { + return result; + } + + let reason = navigation_ready_timeout_reason( + display_url, + initial_error.as_ref().map(ToString::to_string).as_deref(), + last_state_error.as_deref(), + ); + Err(CdpError::Driver(Box::new(io::Error::other(reason)))) +} + +async fn settle_initial_document() { + // Avoid probing the bootstrap page before the real navigation. On + // macOS CFT 150, pre-navigation probes and interrupted data: loads + // can make the subsequent Page.navigate unreliable. + tokio::time::sleep(INITIAL_DOCUMENT_SETTLE_DELAY).await; +} + +async fn settle_ready_document() { + tokio::time::sleep(POST_READY_SETTLE_DELAY).await; +} + +async fn wait_for_document_ready_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + display_url: &str, + initial_error: Option, + allow_interactive: bool, + events: Option, +) -> Result<(), CdpError> { + let Some(mut events) = events else { + return wait_for_document_ready_raw_by_polling( + cdp, + page, + display_url, + initial_error, + allow_interactive, + ) + .await; + }; + + let mut last_state_error = None; + if !events.is_ready_for_capture(allow_interactive) { + match wait_for_raw_navigation_events(cdp, page, &mut events, allow_interactive).await { + Ok(()) => {} + Err(err) => { + last_state_error = Some(err.to_string()); + } + } + } + + if events.is_chrome_error_page() { + return Err(chrome_error_page_error( + display_url, + events + .main_frame_url() + .unwrap_or("chrome-error://chromewebdata/"), + )); + } + + if raw_navigation_can_skip_state_read(&events, allow_interactive) { + return Ok(()); + } + + match tokio::time::timeout( + NAVIGATION_STATE_READ_TIMEOUT, + read_navigation_state_raw(cdp, page), + ) + .await + { + Ok(Ok(state)) if state.is_chrome_error_page => { + return Err(chrome_error_page_error(display_url, &state.href)); + } + Ok(Ok(state)) if document_is_ready_for_capture(&state, allow_interactive) => return Ok(()), + Ok(Ok(state)) + if events.is_ready_for_capture(allow_interactive) && document_has_navigated(&state) => + { + return Ok(()); + } + Ok(Ok(_)) if events.is_ready_for_capture(allow_interactive) => return Ok(()), + Ok(Ok(_)) => {} + Ok(Err(err)) if events.is_ready_for_capture(allow_interactive) => { + tracing::debug!(error = %err, "raw navigation state check failed after page event readiness"); + return Ok(()); + } + Ok(Err(err)) => last_state_error = Some(err.to_string()), + Err(_) if events.is_ready_for_capture(allow_interactive) => { + tracing::debug!("raw navigation state check timed out after page event readiness"); + return Ok(()); + } + Err(_) => { + last_state_error = Some(timeout_reason( + "navigation state read", + NAVIGATION_STATE_READ_TIMEOUT, + )); + } + } + + let reason = navigation_ready_timeout_reason( + display_url, + initial_error.as_ref().map(ToString::to_string).as_deref(), + last_state_error.as_deref(), + ); + Err(CdpError::Driver(Box::new(io::Error::other(reason)))) +} + +fn raw_navigation_can_skip_state_read( + events: &RawNavigationEvents, + allow_interactive: bool, +) -> bool { + events.is_ready_for_capture(allow_interactive) || events.has_navigated() +} + +async fn wait_for_document_ready_raw_by_polling( + cdp: &mut RawCdpClient, + page: &RawPage, + display_url: &str, + initial_error: Option, + allow_interactive: bool, +) -> Result<(), CdpError> { + let mut last_state_error = None; + let attempt = async { + loop { + tokio::time::sleep(Duration::from_millis(50)).await; + if poll_document_ready_raw( + cdp, + page, + display_url, + &mut last_state_error, + allow_interactive, + ) + .await? + { + return Ok(()); + } + } + }; + + if let Ok(result) = tokio::time::timeout(DOCUMENT_READY_TIMEOUT, attempt).await { + return result; + } + + let reason = navigation_ready_timeout_reason( + display_url, + initial_error.as_ref().map(ToString::to_string).as_deref(), + last_state_error.as_deref(), + ); + Err(CdpError::Driver(Box::new(io::Error::other(reason)))) +} + +async fn wait_for_raw_navigation_events( + cdp: &mut RawCdpClient, + page: &RawPage, + events: &mut RawNavigationEvents, + allow_interactive: bool, +) -> Result<(), CdpError> { + let attempt = async { + loop { + if events.is_ready_for_capture(allow_interactive) { + return Ok(()); + } + cdp.collect_next_page_event(&page.session_id, events) + .await?; + } + }; + + match tokio::time::timeout(DOCUMENT_READY_TIMEOUT, attempt).await { + Ok(result) => result, + Err(_) => Err(CdpError::Driver(Box::new(io::Error::other( + timeout_reason("raw navigation page event", DOCUMENT_READY_TIMEOUT), + )))), + } +} + +async fn poll_document_ready_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + display_url: &str, + last_state_error: &mut Option, + allow_interactive: bool, +) -> Result { + match tokio::time::timeout( + NAVIGATION_STATE_READ_TIMEOUT, + read_navigation_state_raw(cdp, page), + ) + .await + { + Ok(Ok(state)) if state.is_chrome_error_page => { + Err(chrome_error_page_error(display_url, &state.href)) + } + Ok(Ok(state)) if document_is_ready_for_capture(&state, allow_interactive) => Ok(true), + Ok(Ok(_)) => Ok(false), + Ok(Err(err)) => { + *last_state_error = Some(err.to_string()); + Ok(false) + } + Err(_) => { + *last_state_error = Some(timeout_reason( + "navigation state read", + NAVIGATION_STATE_READ_TIMEOUT, + )); + Ok(false) + } + } +} + +async fn poll_document_ready( + page: &Page, + display_url: &str, + last_state_error: &mut Option, +) -> Result { + match tokio::time::timeout(NAVIGATION_STATE_READ_TIMEOUT, read_navigation_state(page)).await { + Ok(Ok(state)) if state.is_chrome_error_page => { + Err(chrome_error_page_error(display_url, &state.href)) + } + Ok(Ok(state)) if document_is_loaded(&state) => Ok(true), + Ok(Ok(_)) => Ok(false), + Ok(Err(err)) => { + *last_state_error = Some(err.to_string()); + Ok(false) + } + Err(_) => { + *last_state_error = Some(timeout_reason( + "navigation state read", + NAVIGATION_STATE_READ_TIMEOUT, + )); + Ok(false) + } + } +} + +fn chrome_error_page_error(display_url: &str, error_href: &str) -> CdpError { + CdpError::Driver(Box::new(io::Error::other(format!( + "navigation to `{display_url}` failed: Chrome rendered error page `{error_href}`" + )))) +} + +fn document_is_loaded(state: &NavigationState) -> bool { + document_has_navigated(state) && state.ready_state == "complete" +} + +fn document_is_ready_for_capture(state: &NavigationState, allow_interactive: bool) -> bool { + document_has_navigated(state) + && (state.ready_state == "complete" + || (allow_interactive && state.ready_state == "interactive")) +} + +fn document_has_navigated(state: &NavigationState) -> bool { + url_has_navigated(&state.href) && !state.is_chrome_error_page +} + +fn url_has_navigated(url: &str) -> bool { + url != INITIAL_PAGE_URL +} + +async fn read_navigation_state(page: &Page) -> Result { + let result = page + .evaluate( + "JSON.stringify({ + href: window.location.href, + readyState: document.readyState, + isChromeErrorPage: window.location.protocol === 'chrome-error:' + || document.getElementById('main-frame-error') !== null + })", + ) + .await + .map_err(driver_error)?; + let raw: String = result.into_value().map_err(|err| { + CdpError::Driver(Box::new(io::Error::other(format!( + "read navigation state: {err}" + )))) + })?; + parse_navigation_state(&raw) +} + +async fn read_navigation_state_raw( + cdp: &mut RawCdpClient, + page: &RawPage, +) -> Result { + let frame_tree = page + .execute( + cdp, + "Page.getFrameTree navigation state", + PAGE_COMMAND_TIMEOUT, + GetFrameTreeParams::default(), + ) + .await? + .frame_tree; + let href = frame_tree.frame.url; + Ok(NavigationState { + ready_state: "complete".to_string(), + is_chrome_error_page: href.starts_with("chrome-error:"), + href, + }) +} + +fn parse_navigation_state(raw: &str) -> Result { + serde_json::from_str(raw).map_err(|err| { + CdpError::Driver(Box::new(io::Error::other(format!( + "parse navigation state `{raw}`: {err}" + )))) + }) +} + +fn navigation_ready_timeout_reason( + display_url: &str, + initial_error: Option<&str>, + last_state_error: Option<&str>, +) -> String { + let mut reason = format!( + "navigation to `{display_url}` exhausted {} ready-state budget", + timeout_budget_label(DOCUMENT_READY_TIMEOUT) + ); + if let Some(err) = initial_error { + reason.push_str(" after initial location assignment failed: "); + reason.push_str(err); + } + if let Some(err) = last_state_error { + reason.push_str("; last navigation state read failed: "); + reason.push_str(err); + } + reason +} + +fn navigation_display_url(url: &str) -> &str { + if url.starts_with("data:") { + "data:" + } else { + url + } +} + /// Wait stages that must run *after* navigation. PRD §15 — `--wait-for` /// and `--wait-ms`. /// @@ -1311,6 +2558,25 @@ async fn apply_post_navigate_waits(page: &Page, target: &Target) -> Result<(), C Ok(()) } +async fn apply_post_navigate_waits_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, +) -> Result<(), CdpError> { + wait_for_selector_raw(cdp, page, raw_ready_selector(target)).await?; + if let Some(ms) = target.wait_ms { + tokio::time::sleep(std::time::Duration::from_millis(ms)).await; + } + Ok(()) +} + +fn raw_ready_selector(target: &Target) -> &str { + target + .wait_for_selector + .as_deref() + .unwrap_or(RAW_DEFAULT_READY_SELECTOR) +} + /// Install localStorage entries from an already-parsed Playwright /// storage-state. /// @@ -1351,7 +2617,57 @@ async fn apply_storage_state_local_storage( } })?; let script = format!("window.localStorage.setItem({key}, {value});"); - page.evaluate(script.as_str()).await.map_err(driver_error)?; + with_timeout( + "Runtime.evaluate localStorage", + PAGE_COMMAND_TIMEOUT, + async { + page.evaluate(script.as_str()) + .await + .map(|_| ()) + .map_err(driver_error) + }, + ) + .await?; + } + } + Ok(()) +} + +async fn apply_storage_state_local_storage_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, + state: Option<&StorageState>, +) -> Result<(), CdpError> { + let Some(state) = state else { + return Ok(()); + }; + let target_origin = origin_of(target.url.as_str()).unwrap_or_default(); + for origin_entry in &state.origins { + if origin_entry.origin != target_origin { + continue; + } + for entry in &origin_entry.local_storage { + let key = serde_json::to_string(&entry.name).map_err(|err| { + CdpError::MalformedStorageState { + path: PathBuf::new(), + reason: format!("could not serialize key: {err}"), + } + })?; + let value = serde_json::to_string(&entry.value).map_err(|err| { + CdpError::MalformedStorageState { + path: PathBuf::new(), + reason: format!("could not serialize value: {err}"), + } + })?; + let script = format!("window.localStorage.setItem({key}, {value});"); + page.evaluate_unit( + cdp, + "Runtime.evaluate localStorage", + PAGE_COMMAND_TIMEOUT, + script.as_str(), + ) + .await?; } } Ok(()) @@ -1376,49 +2692,122 @@ fn origin_of(input: &str) -> Option { } } -async fn inject_animation_killer(page: &Page) -> Result<(), CdpError> { - // PRD §16 determinism mitigation: install a CSS-injection script that - // runs before any page script, so transitions/animations don't race - // with `captureSnapshot` and produce different bounds across runs. - let source = "(() => { \ - const style = document.createElement('style'); \ - style.textContent = '*, *::before, *::after { \ - animation-duration: 0s !important; \ - animation-delay: 0s !important; \ - transition-duration: 0s !important; \ - transition-delay: 0s !important; \ - caret-color: transparent !important; \ - }'; \ - (document.head || document.documentElement).appendChild(style); \ - })();"; - add_script_to_evaluate_on_new_document(page, source).await -} - -async fn inject_scrollbar_killer(page: &Page) -> Result<(), CdpError> { - // PRD §16 determinism mitigation: scrollbar overlay differs across - // platforms / DPRs. The `--hide-scrollbars` Chromium launch arg is a - // first line of defense; this CSS injection covers the cases where - // the launch arg alone is not honored (Linux non-overlay scrollbars, - // CSS-painted scrollbars in some apps). - let source = "(() => { \ - const style = document.createElement('style'); \ - style.textContent = 'html { overflow: hidden !important; } \ - ::-webkit-scrollbar { display: none !important; }'; \ - (document.head || document.documentElement).appendChild(style); \ - })();"; - add_script_to_evaluate_on_new_document(page, source).await +async fn apply_deterministic_styles(page: &Page, target: &Target) -> Result<(), CdpError> { + let Some(source) = deterministic_style_source(target) else { + return Ok(()); + }; + + with_timeout( + "Runtime.evaluate deterministic styles", + PAGE_COMMAND_TIMEOUT, + async { + page.evaluate(source.as_str()) + .await + .map(|_| ()) + .map_err(driver_error) + }, + ) + .await?; + Ok(()) } -/// Read `path` (validated as a `.js` file under the CWD) and register -/// it as `Page.addScriptToEvaluateOnNewDocument` so it runs before any -/// page script. -/// -/// # Security boundary -/// -/// The safe-path check via `canonicalize_safe_path` is best-effort -/// only — see that function's docs. Treat the resulting file content -/// as user-trusted: the CLI hands us a path supplied either by the -/// invoking user or by an `auth-script` already in the project, never +async fn apply_deterministic_styles_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, +) -> Result<(), CdpError> { + let Some(source) = deterministic_style_source(target) else { + return Ok(()); + }; + + page.evaluate_unit( + cdp, + "Runtime.evaluate deterministic styles", + PAGE_COMMAND_TIMEOUT, + source.as_str(), + ) + .await?; + Ok(()) +} + +async fn apply_deterministic_styles_raw_best_effort( + cdp: &mut RawCdpClient, + page: &RawPage, + target: &Target, +) -> Result<(), CdpError> { + match apply_deterministic_styles_raw(cdp, page, target).await { + Ok(()) => Ok(()), + Err(err) if is_deterministic_styles_timeout(&err) => { + tracing::debug!(error = %err, "skipping raw deterministic styles after CDP timeout"); + Ok(()) + } + Err(err) => Err(err), + } +} + +fn should_apply_post_navigation_deterministic_styles(preinstalled: bool, target: &Target) -> bool { + !preinstalled && deterministic_style_source(target).is_some() +} + +fn should_apply_raw_post_navigation_deterministic_styles( + preinstalled: bool, + target: &Target, + page_events_enabled: bool, +) -> bool { + page_events_enabled && should_apply_post_navigation_deterministic_styles(preinstalled, target) +} + +fn deterministic_style_source(target: &Target) -> Option { + if !target.disable_animations && !target.hide_scrollbars { + return None; + } + + let mut css = String::new(); + if target.disable_animations { + // PRD §16 determinism mitigation: transitions/animations should + // not race with `captureSnapshot` and produce different bounds + // across runs. + css.push_str( + "*, *::before, *::after { \ + animation-duration: 0s !important; \ + animation-delay: 0s !important; \ + transition-duration: 0s !important; \ + transition-delay: 0s !important; \ + caret-color: transparent !important; \ + }", + ); + } + if target.hide_scrollbars { + // The `--hide-scrollbars` Chromium launch arg is the first line + // of defense; this CSS covers cases where the launch arg alone + // is not honored or the page paints custom scrollbars. + css.push_str( + "html { overflow: hidden !important; } \ + ::-webkit-scrollbar { display: none !important; }", + ); + } + + let css_literal = serde_json::to_string(&css).ok()?; + Some(format!( + "(() => {{ \ + const style = document.createElement('style'); \ + style.setAttribute('data-plumb-deterministic-style', 'true'); \ + style.textContent = {css_literal}; \ + (document.head || document.documentElement).appendChild(style); \ + }})();" + )) +} + +/// Read `path` (validated as a `.js` file under the CWD) and register +/// it as `Page.addScriptToEvaluateOnNewDocument` so it runs before any +/// page script. +/// +/// # Security boundary +/// +/// The safe-path check via `canonicalize_safe_path` is best-effort +/// only — see that function's docs. Treat the resulting file content +/// as user-trusted: the CLI hands us a path supplied either by the +/// invoking user or by an `auth-script` already in the project, never /// by a remote source. The TOCTOU window between canonicalization and /// `std::fs::read_to_string` is acknowledged but not yet closed; the /// full fix requires `cap_std`. @@ -1437,15 +2826,59 @@ async fn inject_auth_script(page: &Page, path: &Path) -> Result<(), CdpError> { add_script_to_evaluate_on_new_document(page, &source).await } +async fn inject_auth_script_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + path: &Path, +) -> Result<(), CdpError> { + let canonical = canonicalize_safe_path(path)?; + if canonical.extension().and_then(|s| s.to_str()) != Some("js") { + return Err(CdpError::InvalidPath { + path: path.to_path_buf(), + reason: "auth script must have a `.js` extension".to_owned(), + }); + } + let source = std::fs::read_to_string(&canonical).map_err(|err| CdpError::InvalidPath { + path: canonical.clone(), + reason: format!("could not read: {err}"), + })?; + add_script_to_evaluate_on_new_document_raw(cdp, page, &source).await +} + async fn add_script_to_evaluate_on_new_document(page: &Page, source: &str) -> Result<(), CdpError> { - let params = AddScriptToEvaluateOnNewDocumentParams { + let params = add_script_to_evaluate_params(source); + with_timeout( + "Page.addScriptToEvaluateOnNewDocument", + PAGE_COMMAND_TIMEOUT, + async { page.execute(params).await.map(|_| ()).map_err(driver_error) }, + ) + .await?; + Ok(()) +} + +async fn add_script_to_evaluate_on_new_document_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + source: &str, +) -> Result<(), CdpError> { + let params = add_script_to_evaluate_params(source); + page.execute( + cdp, + "Page.addScriptToEvaluateOnNewDocument", + PAGE_COMMAND_TIMEOUT, + params, + ) + .await?; + Ok(()) +} + +fn add_script_to_evaluate_params(source: &str) -> AddScriptToEvaluateOnNewDocumentParams { + AddScriptToEvaluateOnNewDocumentParams { source: source.to_owned(), world_name: None, include_command_line_api: None, - run_immediately: Some(true), - }; - page.execute(params).await.map_err(driver_error)?; - Ok(()) + run_immediately: None, + } } async fn install_extra_headers(page: &Page, headers: &[(String, String)]) -> Result<(), CdpError> { @@ -1466,7 +2899,34 @@ async fn install_extra_headers(page: &Page, headers: &[(String, String)]) -> Res object.insert(name, serde_json::Value::String(value)); } let params = SetExtraHttpHeadersParams::new(Headers::new(serde_json::Value::Object(object))); - page.execute(params).await.map_err(driver_error)?; + with_timeout("Network.setExtraHTTPHeaders", PAGE_COMMAND_TIMEOUT, async { + page.execute(params).await.map(|_| ()).map_err(driver_error) + }) + .await?; + Ok(()) +} + +async fn install_extra_headers_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + headers: &[(String, String)], +) -> Result<(), CdpError> { + let mut entries: Vec<(String, String)> = headers.to_vec(); + entries.sort_by(|a, b| a.0.cmp(&b.0)); + let mut object = serde_json::Map::with_capacity(entries.len()); + for (name, value) in entries { + validate_header_name(&name)?; + validate_no_ctl(&value, "value", "header")?; + object.insert(name, serde_json::Value::String(value)); + } + let params = SetExtraHttpHeadersParams::new(Headers::new(serde_json::Value::Object(object))); + page.execute( + cdp, + "Network.setExtraHTTPHeaders", + PAGE_COMMAND_TIMEOUT, + params, + ) + .await?; Ok(()) } @@ -1507,7 +2967,46 @@ async fn install_cookies( .map(|c| c.into_cdp_param(url_for_cookies)) .collect(), ); - page.execute(params).await.map_err(driver_error)?; + with_timeout("Network.setCookies", PAGE_COMMAND_TIMEOUT, async { + page.execute(params).await.map(|_| ()).map_err(driver_error) + }) + .await?; + Ok(()) +} + +async fn install_cookies_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + cookies: &[Cookie], + default_url: &str, +) -> Result<(), CdpError> { + let mut sorted: Vec = cookies.to_vec(); + sorted.sort_by(|a, b| { + (a.name.as_str(), a.value.as_str()).cmp(&(b.name.as_str(), b.value.as_str())) + }); + for cookie in &sorted { + validate_cookie_name(&cookie.name)?; + validate_cookie_value(&cookie.value)?; + if let Some(domain) = cookie.domain.as_deref() { + validate_no_ctl(domain, "domain", "cookie")?; + } + if let Some(path) = cookie.path.as_deref() { + validate_no_ctl(path, "path", "cookie")?; + } + } + let url_for_cookies = if default_url.starts_with("http") { + Some(default_url) + } else { + None + }; + let params = SetCookiesParams::new( + sorted + .into_iter() + .map(|c| c.into_cdp_param(url_for_cookies)) + .collect(), + ); + page.execute(cdp, "Network.setCookies", PAGE_COMMAND_TIMEOUT, params) + .await?; Ok(()) } @@ -1524,9 +3023,44 @@ async fn install_storage_state_cookies(page: &Page, state: &StorageState) -> Res p.http_only = Some(cookie.http_only); params.push(p); } - page.execute(SetCookiesParams::new(params)) - .await - .map_err(driver_error)?; + with_timeout( + "Network.setCookies storageState", + PAGE_COMMAND_TIMEOUT, + async { + page.execute(SetCookiesParams::new(params)) + .await + .map(|_| ()) + .map_err(driver_error) + }, + ) + .await?; + Ok(()) +} + +async fn install_storage_state_cookies_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + state: &StorageState, +) -> Result<(), CdpError> { + if state.cookies.is_empty() { + return Ok(()); + } + let mut params: Vec = Vec::with_capacity(state.cookies.len()); + for cookie in &state.cookies { + let mut p = CookieParam::new(cookie.name.clone(), cookie.value.clone()); + p.domain = Some(cookie.domain.clone()); + p.path = Some(cookie.path.clone()); + p.secure = Some(cookie.secure); + p.http_only = Some(cookie.http_only); + params.push(p); + } + page.execute( + cdp, + "Network.setCookies storageState", + PAGE_COMMAND_TIMEOUT, + SetCookiesParams::new(params), + ) + .await?; Ok(()) } @@ -1561,6 +3095,39 @@ async fn wait_for_selector(page: &Page, selector: &str) -> Result<(), CdpError> } } +async fn wait_for_selector_raw( + cdp: &mut RawCdpClient, + page: &RawPage, + selector: &str, +) -> Result<(), CdpError> { + let selector = serde_json::to_string(selector).map_err(serde_driver_error)?; + let script = format!("document.querySelector({selector}) !== null"); + let attempt = async { + loop { + match page + .evaluate_value::( + cdp, + "Runtime.evaluate wait_for_selector", + PAGE_COMMAND_TIMEOUT, + script.as_str(), + ) + .await + { + Ok(true) => return Ok::<(), CdpError>(()), + Ok(false) | Err(_) => { + tokio::time::sleep(std::time::Duration::from_millis(50)).await; + } + } + } + }; + match tokio::time::timeout(std::time::Duration::from_secs(10), attempt).await { + Ok(result) => result, + Err(_) => Err(CdpError::Driver(Box::new(io::Error::other(format!( + "wait_for_selector `{selector}` exhausted 10s budget" + ))))), + } +} + /// Deterministic fake driver. Recognizes `plumb-fake://hello` and returns /// [`PlumbSnapshot::canned`]. Used by the walking-skeleton CLI and by /// downstream tests. @@ -1644,49 +3211,68 @@ fn chromium_install_hint() -> String { struct ChromiumSession { browser: Browser, - handler_task: JoinHandle<()>, + handler_task: Option>, + profile_dir: Option, } impl ChromiumSession { - async fn launch(config: BrowserConfig) -> Result { - let (browser, handler) = Browser::launch(config).await.map_err(map_launch_error)?; + async fn launch(launch: ChromiumLaunch) -> Result { + let (browser, handler) = with_timeout("Chromium launch", BROWSER_LAUNCH_TIMEOUT, async { + Browser::launch(launch.config) + .await + .map_err(map_launch_error) + }) + .await?; let handler_task = poll_handler(handler); Ok(Self { browser, - handler_task, + handler_task: Some(handler_task), + profile_dir: launch.profile_dir, }) } async fn shutdown(&mut self) -> Result<(), CdpError> { - let close_result = self.browser.close().await.map_err(driver_error); - if let Err(close_err) = close_result { - if let Err(kill_err) = kill_browser(&mut self.browser).await { - tracing::debug!(error = %kill_err, "failed to kill Chromium after close error"); + let close_result = close_browser_best_effort(&mut self.browser).await; + if let Some(task) = self.handler_task.take() { + task.abort(); + if let Err(join_err) = task.await + && !join_err.is_cancelled() + { + tracing::debug!(error = %join_err, "Chromium handler task failed"); } - self.abort_handler().await; - return Err(close_err); } + let _profile_dir = self.profile_dir.take(); + close_result + } +} - if let Err(wait_err) = self.browser.wait().await { - let cleanup_err = io_error(wait_err); - if let Err(kill_err) = kill_browser(&mut self.browser).await { - tracing::debug!(error = %kill_err, "failed to kill Chromium after wait error"); - } - self.abort_handler().await; - return Err(cleanup_err); - } +struct RawChromiumSession { + browser: Browser, + profile_dir: Option, +} - self.abort_handler().await; - Ok(()) +impl RawChromiumSession { + async fn launch(launch: ChromiumLaunch) -> Result { + let (browser, _handler) = with_timeout("Chromium launch", BROWSER_LAUNCH_TIMEOUT, async { + Browser::launch(launch.config) + .await + .map_err(map_launch_error) + }) + .await?; + Ok(Self { + browser, + profile_dir: launch.profile_dir, + }) } - async fn abort_handler(&mut self) { - self.handler_task.abort(); - if let Err(join_err) = (&mut self.handler_task).await - && !join_err.is_cancelled() - { - tracing::debug!(error = %join_err, "Chromium handler task failed"); - } + fn websocket_address(&self) -> &str { + self.browser.websocket_address() + } + + async fn shutdown(&mut self, cdp: &mut RawCdpClient) -> Result<(), CdpError> { + let cleanup_result = close_raw_browser_best_effort(cdp, &mut self.browser).await; + let _profile_dir = self.profile_dir.take(); + cleanup_result } } @@ -1700,15 +3286,215 @@ fn poll_handler(mut handler: Handler) -> JoinHandle<()> { }) } +async fn with_timeout(operation: &str, timeout: Duration, future: F) -> Result +where + F: Future>, +{ + match tokio::time::timeout(timeout, future).await { + Ok(result) => result.map_err(|err| contextualize_request_timeout(operation, err)), + Err(_) => Err(timeout_error(operation, timeout)), + } +} + +fn contextualize_request_timeout(operation: &str, err: CdpError) -> CdpError { + let CdpError::Driver(source) = &err else { + return err; + }; + + if matches!( + source.downcast_ref::(), + Some(chromiumoxide::error::CdpError::Timeout) + ) { + return CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + format!( + "{operation} hit Chromiumoxide request budget ({})", + timeout_budget_label(CHROMIUMOXIDE_REQUEST_TIMEOUT) + ), + ))); + } + + err +} + +fn is_retryable_capture_timeout(err: &CdpError) -> bool { + let CdpError::Driver(source) = err else { + return false; + }; + + if matches!( + source.downcast_ref::(), + Some(chromiumoxide::error::CdpError::Timeout) + ) { + return true; + } + + source.downcast_ref::().is_some_and(|err| { + (err.kind() == io::ErrorKind::TimedOut && !is_snapshot_capture_timeout(err)) + || is_startup_navigation_abort(err) + || is_ready_state_read_timeout(err) + }) +} + +fn is_snapshot_capture_timeout(err: &io::Error) -> bool { + err.kind() == io::ErrorKind::TimedOut + && err + .to_string() + .contains("DOMSnapshot.captureSnapshot exceeded") +} + +fn is_startup_navigation_abort(err: &io::Error) -> bool { + if err.kind() != io::ErrorKind::Other { + return false; + } + + let message = err.to_string(); + message.contains("exhausted 30s ready-state budget") + && message.contains("after initial location assignment failed:") + && message.contains("Page.navigate failed: net::ERR_ABORTED") + && message.contains("last navigation state read failed: navigation state read exceeded") +} + +fn is_ready_state_read_timeout(err: &io::Error) -> bool { + if err.kind() != io::ErrorKind::Other { + return false; + } + + let message = err.to_string(); + message.contains("exhausted 30s ready-state budget") + && message.contains("last navigation state read failed: navigation state read exceeded") +} + +fn is_deterministic_styles_timeout(err: &CdpError) -> bool { + let CdpError::Driver(source) = err else { + return false; + }; + + source.downcast_ref::().is_some_and(|err| { + err.kind() == io::ErrorKind::TimedOut + && err + .to_string() + .contains("Runtime.evaluate deterministic styles") + }) +} + +fn timeout_error(operation: &str, timeout: Duration) -> CdpError { + CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + timeout_reason(operation, timeout), + ))) +} + +fn timeout_reason(operation: &str, timeout: Duration) -> String { + format!( + "{operation} exceeded {} budget", + timeout_budget_label(timeout) + ) +} + +fn timeout_budget_label(timeout: Duration) -> String { + if timeout.as_millis().is_multiple_of(1_000) { + format!("{}s", timeout.as_secs()) + } else { + format!("{}ms", timeout.as_millis()) + } +} + +async fn cleanup_failed_persistent_launch( + browser: &mut Browser, + handler_task: JoinHandle<()>, +) -> Result<(), CdpError> { + let close_result = close_browser_best_effort(browser).await; + handler_task.abort(); + if let Err(join_err) = handler_task.await + && !join_err.is_cancelled() + { + tracing::debug!(error = %join_err, "Chromium handler task failed"); + } + + close_result +} + +async fn close_browser_best_effort(browser: &mut Browser) -> Result<(), CdpError> { + if let Err(err) = with_timeout("Browser.close", BROWSER_CLOSE_TIMEOUT, async { + browser.close().await.map_err(driver_error) + }) + .await + { + tracing::debug!(error = %err, "failed to close Chromium"); + } + + if let Err(wait_err) = with_timeout("Chromium process wait", BROWSER_WAIT_TIMEOUT, async { + browser.wait().await.map_err(io_error) + }) + .await + { + tracing::debug!(error = %wait_err, "failed to wait for Chromium process"); + kill_browser(browser).await?; + with_timeout( + "Chromium process wait after kill", + BROWSER_WAIT_TIMEOUT, + async { browser.wait().await.map_err(io_error) }, + ) + .await?; + } + Ok(()) +} + +async fn close_raw_browser_best_effort( + cdp: &mut RawCdpClient, + browser: &mut Browser, +) -> Result<(), CdpError> { + if let Err(err) = with_timeout("Browser.close", BROWSER_CLOSE_TIMEOUT, async { + cdp.execute(None, BrowserCloseParams::default()) + .await + .map(|_: chromiumoxide::cdp::browser_protocol::browser::CloseReturns| ()) + }) + .await + { + tracing::debug!(error = %err, "failed to close Chromium over raw CDP"); + } + + if let Err(wait_err) = with_timeout("Chromium process wait", BROWSER_WAIT_TIMEOUT, async { + browser.wait().await.map_err(io_error) + }) + .await + { + tracing::debug!(error = %wait_err, "failed to wait for Chromium process"); + kill_browser(browser).await?; + with_timeout( + "Chromium process wait after kill", + BROWSER_WAIT_TIMEOUT, + async { browser.wait().await.map_err(io_error) }, + ) + .await?; + } + Ok(()) +} + async fn kill_browser(browser: &mut Browser) -> Result<(), CdpError> { - if let Some(result) = browser.kill().await { + if let Some(result) = tokio::time::timeout(BROWSER_KILL_TIMEOUT, browser.kill()) + .await + .map_err(|_| timeout_error("Chromium kill", BROWSER_KILL_TIMEOUT))? + { result.map_err(io_error)?; } Ok(()) } async fn validate_browser_version(browser: &Browser) -> Result<(), CdpError> { - let version = browser.version().await.map_err(driver_error)?; + let version = with_timeout("Browser.version", CDP_CONTROL_TIMEOUT, async { + browser.version().await.map_err(driver_error) + }) + .await?; + validate_chromium_product_major(&version.product) +} + +async fn validate_browser_version_raw(cdp: &mut RawCdpClient) -> Result<(), CdpError> { + let version = with_timeout("Browser.version", CDP_CONTROL_TIMEOUT, async { + cdp.execute(None, GetVersionParams::default()).await + }) + .await?; validate_chromium_product_major(&version.product) } @@ -1765,6 +3551,14 @@ fn driver_error(err: chromiumoxide::error::CdpError) -> CdpError { CdpError::Driver(Box::new(err)) } +fn serde_driver_error(err: serde_json::Error) -> CdpError { + driver_error(chromiumoxide::error::CdpError::Serde(err)) +} + +fn driver_message(message: impl Into) -> CdpError { + CdpError::Driver(Box::new(io::Error::other(message.into()))) +} + fn io_error(err: io::Error) -> CdpError { CdpError::Driver(Box::new(err)) } @@ -2359,6 +4153,9 @@ fn rect_from_bounds(inner: &[f64]) -> Rect { #[cfg(test)] mod tests { + use std::io; + use std::path::PathBuf; + use super::{ COMPUTED_STYLE_WHITELIST, CdpError, MAX_SUPPORTED_CHROMIUM_MAJOR, MIN_SUPPORTED_CHROMIUM_MAJOR, @@ -2752,6 +4549,71 @@ mod tests { assert!(t.pin_dpr.is_none()); } + #[test] + fn deterministic_style_source_uses_default_capture_knobs() { + let Some(source) = super::deterministic_style_source(&super::Target::default()) else { + panic!("default target should inject deterministic CSS"); + }; + + assert!(source.contains("data-plumb-deterministic-style")); + assert!(source.contains("animation-duration")); + assert!(source.contains("transition-duration")); + assert!(source.contains("overflow: hidden")); + assert!(source.contains("::-webkit-scrollbar")); + } + + #[test] + fn deterministic_style_source_skips_when_knobs_disabled() { + let target = super::Target { + disable_animations: false, + hide_scrollbars: false, + ..super::Target::default() + }; + + assert!(super::deterministic_style_source(&target).is_none()); + } + + #[test] + fn raw_deterministic_styles_skip_post_navigation_when_preinstalled() { + let target = super::Target::default(); + + assert!(!super::should_apply_post_navigation_deterministic_styles( + true, &target + )); + assert!(super::should_apply_post_navigation_deterministic_styles( + false, &target + )); + } + + #[test] + fn raw_deterministic_styles_skip_post_navigation_without_page_events() { + let target = super::Target::default(); + + assert!( + !super::should_apply_raw_post_navigation_deterministic_styles(false, &target, false) + ); + } + + #[test] + fn raw_deterministic_styles_apply_post_navigation_with_page_events() { + let target = super::Target::default(); + + assert!(super::should_apply_raw_post_navigation_deterministic_styles(false, &target, true)); + } + + #[test] + fn raw_deterministic_styles_skip_post_navigation_without_source() { + let target = super::Target { + disable_animations: false, + hide_scrollbars: false, + ..super::Target::default() + }; + + assert!( + !super::should_apply_raw_post_navigation_deterministic_styles(false, &target, true) + ); + } + #[test] fn target_effective_dpr_prefers_pin_over_default() { let mut t = super::Target { @@ -2763,6 +4625,586 @@ mod tests { assert!((t.effective_dpr() - 3.0).abs() < f64::EPSILON); } + #[test] + fn viewport_override_skips_first_unpinned_target() { + let target = super::Target::default(); + + assert!(!super::should_apply_viewport_override(0, &target)); + } + + #[test] + fn viewport_override_applies_to_first_pinned_target() { + let target = super::Target { + pin_dpr: Some(2.0), + ..super::Target::default() + }; + + assert!(super::should_apply_viewport_override(0, &target)); + } + + #[test] + fn viewport_override_applies_to_later_targets() { + let target = super::Target::default(); + + assert!(super::should_apply_viewport_override(1, &target)); + } + + #[test] + fn persistent_viewport_override_skips_default_dpr_target() { + let target = super::Target::default(); + + assert!(!super::should_apply_persistent_viewport_override(&target)); + } + + #[test] + fn persistent_viewport_override_applies_to_pinned_dpr() { + let target = super::Target { + pin_dpr: Some(2.0), + ..super::Target::default() + }; + + assert!(super::should_apply_persistent_viewport_override(&target)); + } + + #[test] + fn persistent_viewport_override_applies_to_non_default_dpr() { + let target = super::Target { + device_pixel_ratio: 2.0, + ..super::Target::default() + }; + + assert!(super::should_apply_persistent_viewport_override(&target)); + } + + #[test] + fn initial_page_url_uses_blank_bootstrap_document() { + assert_eq!(super::INITIAL_PAGE_URL, "about:blank"); + } + + #[test] + fn add_script_params_registers_for_future_documents_only() { + let params = super::add_script_to_evaluate_params("window.__plumb = true;"); + + assert_eq!(params.source, "window.__plumb = true;"); + assert!(params.world_name.is_none()); + assert!(params.include_command_line_api.is_none()); + assert!(params.run_immediately.is_none()); + } + + #[test] + fn browser_config_creates_isolated_profile_by_default() { + let driver = super::ChromiumDriver::new(super::ChromiumOptions { + executable_path: Some(test_executable_path()), + ..super::ChromiumOptions::default() + }); + let launch = match driver.browser_config(&super::Target::default(), None) { + Ok(launch) => launch, + Err(err) => panic!("browser config failed: {err}"), + }; + + assert!(launch.profile_dir.is_some()); + let Some(configured) = launch.config.user_data_dir.as_deref() else { + panic!("expected generated user data dir"); + }; + assert!(configured.exists()); + } + + #[test] + fn browser_config_preserves_explicit_profile() { + let profile = match tempfile::tempdir() { + Ok(profile) => profile, + Err(err) => panic!("tempdir failed: {err}"), + }; + let driver = super::ChromiumDriver::new(super::ChromiumOptions { + executable_path: Some(test_executable_path()), + user_data_dir: Some(profile.path().to_path_buf()), + ..super::ChromiumOptions::default() + }); + let launch = match driver.browser_config(&super::Target::default(), None) { + Ok(launch) => launch, + Err(err) => panic!("browser config failed: {err}"), + }; + + assert!(launch.profile_dir.is_none()); + assert_eq!(launch.config.user_data_dir.as_deref(), Some(profile.path())); + } + + #[test] + fn navigation_assignment_script_json_escapes_url() { + let script = match super::navigation_assignment_script("https://example.com/a\"b\nc") { + Ok(script) => script, + Err(err) => panic!("script generation failed: {err}"), + }; + assert_eq!( + script, + "window.location.assign(\"https://example.com/a\\\"b\\nc\");" + ); + } + + #[test] + fn file_urls_keep_chromiumoxide_goto_path() { + assert!(super::uses_chromiumoxide_goto("file:///tmp/static.html")); + assert_eq!( + super::navigation_method_for_url("file:///tmp/static.html"), + super::NavigationMethod::ChromiumoxideGoto + ); + } + + #[test] + fn navigation_method_avoids_script_assignment_for_data_urls() { + assert_eq!( + super::navigation_method_for_url("data:text/html;base64,PHNjcmlwdD4="), + super::NavigationMethod::CdpNavigate + ); + assert_eq!( + super::navigation_method_for_url("http://127.0.0.1:49197/"), + super::NavigationMethod::LocationAssign + ); + assert_eq!( + super::navigation_method_for_url("https://example.com/"), + super::NavigationMethod::LocationAssign + ); + } + + #[test] + fn page_navigation_uses_chromiumoxide_goto_for_web_urls() { + assert_eq!( + super::page_navigation_method_for_url("http://127.0.0.1:49197/"), + super::NavigationMethod::ChromiumoxideGoto + ); + assert_eq!( + super::page_navigation_method_for_url("https://example.com/"), + super::NavigationMethod::ChromiumoxideGoto + ); + assert_eq!( + super::page_navigation_method_for_url("data:text/html;base64,PHNjcmlwdD4="), + super::NavigationMethod::CdpNavigate + ); + } + + #[test] + fn raw_capture_path_is_reserved_for_data_urls_and_selector_gated_pages() { + let web_target = super::Target { + url: "https://example.com/".to_owned(), + ..super::Target::default() + }; + assert!(!super::should_use_raw_capture_path(&[web_target])); + + let app_target = super::Target { + url: "http://127.0.0.1:49197/".to_owned(), + wait_for_selector: Some("html[data-plumb-ready=\"true\"]".to_owned()), + ..super::Target::default() + }; + assert!(super::should_use_raw_capture_path(&[app_target])); + + let data_target = super::Target { + url: "data:text/html;base64,PHNjcmlwdD4=".to_owned(), + ..super::Target::default() + }; + assert!(super::should_use_raw_capture_path(&[data_target])); + } + + #[test] + fn raw_capture_fallback_accepts_browser_new_page_timeout() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "Browser.new_page exceeded 75s budget", + ))); + + assert!(super::should_fallback_to_raw_capture(&err)); + } + + #[test] + fn raw_capture_fallback_accepts_browser_new_page_request_budget() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "Browser.new_page hit Chromiumoxide request budget (60s)", + ))); + + assert!(super::should_fallback_to_raw_capture(&err)); + } + + #[test] + fn raw_capture_fallback_accepts_page_navigation_init_timeout() { + let err = CdpError::Driver(Box::new(io::Error::other( + "navigation to `http://127.0.0.1:49196/` exhausted 30s ready-state \ + budget after initial location assignment failed: driver failure: \ + Page.navigate exceeded 30s budget; last navigation state read failed: \ + navigation state read exceeded 10s budget", + ))); + + assert!(super::should_fallback_to_raw_capture(&err)); + } + + #[test] + fn raw_capture_fallback_rejects_unrelated_timeouts() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "DOMSnapshot.captureSnapshot exceeded 60s budget", + ))); + + assert!(!super::should_fallback_to_raw_capture(&err)); + } + + #[test] + fn raw_ready_selector_defaults_to_body() { + let target = super::Target::default(); + + assert_eq!(super::raw_ready_selector(&target), "body"); + } + + #[test] + fn raw_ready_selector_preserves_user_selector() { + let target = super::Target { + wait_for_selector: Some("#app-ready".to_owned()), + ..super::Target::default() + }; + + assert_eq!(super::raw_ready_selector(&target), "#app-ready"); + } + + #[test] + fn raw_navigation_tolerates_page_navigate_abort_for_web_urls() { + assert!(super::uses_raw_tolerant_page_navigate( + "http://127.0.0.1:49197/" + )); + assert!(super::uses_raw_tolerant_page_navigate( + "https://example.com/" + )); + assert!(!super::uses_raw_tolerant_page_navigate( + "data:text/html;base64,PHNjcmlwdD4=" + )); + assert!(!super::uses_raw_tolerant_page_navigate( + "file:///tmp/static.html" + )); + } + + #[test] + fn raw_page_navigate_tolerates_only_abort_errors() { + assert!(super::raw_page_navigate_error_is_tolerated( + "net::ERR_ABORTED" + )); + assert!(!super::raw_page_navigate_error_is_tolerated( + "net::ERR_CONNECTION_REFUSED" + )); + } + + #[test] + fn document_load_wait_accepts_redirected_complete_document_only() { + assert!(super::document_is_loaded(&super::NavigationState { + href: "https://example.com/login".to_string(), + ready_state: "complete".to_string(), + is_chrome_error_page: false, + })); + assert!(!super::document_is_loaded(&super::NavigationState { + href: "https://example.com/login".to_string(), + ready_state: "interactive".to_string(), + is_chrome_error_page: false, + })); + assert!(!super::document_is_loaded(&super::NavigationState { + href: super::INITIAL_PAGE_URL.to_string(), + ready_state: "complete".to_string(), + is_chrome_error_page: false, + })); + assert!(!super::document_is_loaded(&super::NavigationState { + href: "chrome-error://chromewebdata/".to_string(), + ready_state: "complete".to_string(), + is_chrome_error_page: true, + })); + } + + #[test] + fn selector_gated_raw_navigation_accepts_interactive_document() { + let state = super::NavigationState { + href: "https://example.com/app".to_string(), + ready_state: "interactive".to_string(), + is_chrome_error_page: false, + }; + + assert!(super::document_is_ready_for_capture(&state, true)); + assert!(!super::document_is_ready_for_capture(&state, false)); + } + + #[test] + fn selector_gated_raw_navigation_still_rejects_initial_documents() { + assert!(!super::document_is_ready_for_capture( + &super::NavigationState { + href: super::INITIAL_PAGE_URL.to_string(), + ready_state: "interactive".to_string(), + is_chrome_error_page: false, + }, + true, + )); + assert!(!super::document_is_ready_for_capture( + &super::NavigationState { + href: "chrome-error://chromewebdata/".to_string(), + ready_state: "interactive".to_string(), + is_chrome_error_page: true, + }, + true, + )); + } + + #[test] + fn raw_navigation_events_require_navigated_main_frame() { + let mut events = super::RawNavigationEvents::default(); + events.observe_load_event(); + + assert!(!events.is_ready_for_capture(false)); + + events.observe_main_frame_url(super::INITIAL_PAGE_URL); + events.observe_load_event(); + + assert!(!events.is_ready_for_capture(false)); + + events.observe_main_frame_url("https://example.com/app"); + + assert!(events.has_navigated()); + assert!(!events.is_ready_for_capture(false)); + + events.observe_load_event(); + + assert!(events.is_ready_for_capture(false)); + } + + #[test] + fn selector_gated_raw_events_accept_dom_content_after_navigation() { + let mut events = super::RawNavigationEvents::default(); + events.observe_main_frame_url("https://example.com/app"); + events.observe_dom_content_event(); + + assert!(events.is_ready_for_capture(true)); + assert!(!events.is_ready_for_capture(false)); + } + + #[test] + fn raw_navigation_wait_keeps_accepted_navigation_without_page_events() { + let mut events = super::RawNavigationEvents::default(); + events.observe_main_frame_url("https://example.com/app"); + + let events = super::raw_navigation_events_for_wait(false, events); + + assert!(events.is_some_and(|events| events.has_navigated())); + } + + #[test] + fn raw_navigation_wait_keeps_events_when_page_events_enabled() { + let mut events = super::RawNavigationEvents::default(); + events.observe_main_frame_url("https://example.com/app"); + + let events = super::raw_navigation_events_for_wait(true, events); + + assert!(events.is_some_and(|events| events.has_navigated())); + } + + #[test] + fn raw_navigation_skips_state_read_after_event_timeout_with_navigation() { + let mut events = super::RawNavigationEvents::default(); + events.observe_main_frame_url("https://example.com/app"); + + assert!(super::raw_navigation_can_skip_state_read(&events, false)); + } + + #[test] + fn parse_navigation_state_reads_href_and_ready_state() { + let state = match super::parse_navigation_state( + r#"{"href":"http://127.0.0.1:49197/","readyState":"complete"}"#, + ) { + Ok(state) => state, + Err(err) => panic!("navigation state parse failed: {err}"), + }; + assert_eq!(state.href, "http://127.0.0.1:49197/"); + assert_eq!(state.ready_state, "complete"); + assert!(!state.is_chrome_error_page); + } + + #[test] + fn parse_navigation_state_reads_chrome_error_page_marker() { + let state = match super::parse_navigation_state( + r#"{"href":"chrome-error://chromewebdata/","readyState":"complete","isChromeErrorPage":true}"#, + ) { + Ok(state) => state, + Err(err) => panic!("navigation state parse failed: {err}"), + }; + assert!(state.is_chrome_error_page); + } + + #[test] + fn parse_navigation_state_rejects_malformed_json() { + let err = super::parse_navigation_state("not json"); + assert!(matches!(err, Err(CdpError::Driver(_)))); + } + + #[test] + fn navigation_ready_timeout_reason_preserves_stage_errors() { + let reason = super::navigation_ready_timeout_reason( + "http://127.0.0.1:49197/", + Some("navigation location assignment exceeded 2s budget"), + Some("navigation state read exceeded 2s budget"), + ); + + assert!(reason.contains("exhausted 30s ready-state budget")); + assert!(reason.contains("after initial location assignment failed")); + assert!(reason.contains("last navigation state read failed")); + } + + #[test] + fn navigation_display_url_redacts_data_urls() { + assert_eq!( + super::navigation_display_url("data:text/html;base64,PHNjcmlwdD4="), + "data:" + ); + assert_eq!( + super::navigation_display_url("http://127.0.0.1:49197/"), + "http://127.0.0.1:49197/" + ); + } + + #[test] + fn contextualize_request_timeout_labels_operation() { + let err = super::contextualize_request_timeout( + "Target.attachToTarget", + CdpError::Driver(Box::new(chromiumoxide::error::CdpError::Timeout)), + ); + + let message = err.to_string(); + assert!(message.contains("Target.attachToTarget")); + assert!(message.contains("Chromiumoxide request budget")); + } + + #[test] + fn target_lifecycle_error_labels_pre_navigation_stage() { + let err = super::target_lifecycle_error( + "Target.createTarget", + &CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "Target.createTarget exceeded 10s budget", + ))), + ); + + let message = err.to_string(); + assert!(message.contains("Target.createTarget failed before navigation")); + assert!(message.contains("Target.createTarget exceeded 10s budget")); + assert!(super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn target_lifecycle_error_keeps_non_timeout_errors_non_retryable() { + let err = super::target_lifecycle_error( + "Target.attachToTarget", + &CdpError::Driver(Box::new(io::Error::other("target disappeared"))), + ); + + let message = err.to_string(); + assert!(message.contains("Target.attachToTarget failed before navigation")); + assert!(message.contains("target disappeared")); + assert!(!super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_accepts_chromiumoxide_timeout() { + let err = CdpError::Driver(Box::new(chromiumoxide::error::CdpError::Timeout)); + + assert!(super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_accepts_plumb_timed_out_io() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "Emulation.setDeviceMetricsOverride exceeded 25s budget", + ))); + + assert!(super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_rejects_snapshot_capture_timeout() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "DOMSnapshot.captureSnapshot exceeded 60s budget", + ))); + + assert!(!super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_accepts_startup_navigation_abort() { + let err = CdpError::Driver(Box::new(io::Error::other( + "navigation to `http://127.0.0.1:49216/` exhausted 30s ready-state budget \ + after initial location assignment failed: driver failure: Page.navigate failed: \ + net::ERR_ABORTED; last navigation state read failed: navigation state read \ + exceeded 2s budget", + ))); + + assert!(super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_accepts_ready_state_read_timeout() { + let err = CdpError::Driver(Box::new(io::Error::other( + "navigation to `http://127.0.0.1:49216/` exhausted 30s ready-state \ + budget; last navigation state read failed: navigation state read exceeded \ + 2s budget", + ))); + + assert!(super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn deterministic_styles_timeout_is_skippable() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "Runtime.evaluate deterministic styles exceeded 25s budget", + ))); + + assert!(super::is_deterministic_styles_timeout(&err)); + } + + #[test] + fn deterministic_styles_timeout_rejects_unrelated_timeouts() { + let err = CdpError::Driver(Box::new(io::Error::new( + io::ErrorKind::TimedOut, + "Runtime.evaluate wait_for_selector exceeded 25s budget", + ))); + + assert!(!super::is_deterministic_styles_timeout(&err)); + } + + #[test] + fn deterministic_styles_timeout_rejects_non_timeout_errors() { + let err = CdpError::Driver(Box::new(io::Error::other( + "Runtime.evaluate deterministic styles failed: detached target", + ))); + + assert!(!super::is_deterministic_styles_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_rejects_bare_navigation_abort() { + let err = CdpError::Driver(Box::new(io::Error::other( + "Page.navigate failed: net::ERR_ABORTED", + ))); + + assert!(!super::is_retryable_capture_timeout(&err)); + } + + #[test] + fn retryable_capture_timeout_rejects_non_timeout_errors() { + let err = CdpError::MalformedSnapshot { + reason: "missing document".to_owned(), + }; + + assert!(!super::is_retryable_capture_timeout(&err)); + } + + fn test_executable_path() -> PathBuf { + match std::env::current_exe() { + Ok(path) => path, + Err(err) => panic!("current executable path unavailable: {err}"), + } + } + #[test] fn origin_of_handles_https_url() { assert_eq!( diff --git a/crates/plumb-cli/src/commands/init.rs b/crates/plumb-cli/src/commands/init.rs index dbd3c11..efe8f30 100644 --- a/crates/plumb-cli/src/commands/init.rs +++ b/crates/plumb-cli/src/commands/init.rs @@ -22,7 +22,8 @@ use std::path::Path; use std::process::ExitCode; use anyhow::{Context, Result, bail}; -use plumb_codegen::{InferredConfig, infer_config, render_toml}; +use plumb_codegen::{InferredConfig, TokenSourceKind, infer_config, render_toml}; +use plumb_config::{ConfigError, TailwindOptions, merge_tailwind}; const GENERIC_TEMPLATE: &str = include_str!("../../templates/plumb.toml"); const TAILWIND_TEMPLATE: &str = include_str!("../../templates/plumb-tailwind.toml"); @@ -93,14 +94,53 @@ pub fn run(force: bool, from: Option<&Path>) -> Result { /// Walk `source_dir` and render an inferred starter TOML. fn render_from_source(source_dir: &Path) -> Result<(String, String)> { - let inferred = + let mut inferred = infer_config(source_dir).with_context(|| format!("walk {}", source_dir.display()))?; + merge_tailwind_sources(&mut inferred, source_dir)?; let content = render_toml(&inferred) .with_context(|| format!("render TOML from {}", source_dir.display()))?; let summary = summary_for_inferred(&inferred, source_dir); Ok((content, summary)) } +fn merge_tailwind_sources(inferred: &mut InferredConfig, source_dir: &Path) -> Result<()> { + let options = TailwindOptions { + cwd_root: Some(source_dir.to_path_buf()), + ..TailwindOptions::default() + }; + let mut config = std::mem::take(&mut inferred.config); + + for source in &inferred.sources { + if source.kind != TokenSourceKind::TailwindConfig { + continue; + } + + let tailwind_path = source_dir.join(&source.relative_path); + let before = config.clone(); + match merge_tailwind(config, &tailwind_path, &options) { + Ok(merged) => config = merged, + Err(ConfigError::TailwindUnavailable { .. }) => { + config = before; + break; + } + Err(ConfigError::TailwindEval { reason, .. }) + if reason.contains("TS_LOADER_MISSING") => + { + config = before; + break; + } + Err(err) => { + inferred.config = before; + return Err(err) + .with_context(|| format!("merge Tailwind config {}", tailwind_path.display())); + } + } + } + + inferred.config = config; + Ok(()) +} + fn summary_for_inferred(inferred: &InferredConfig, source_dir: &Path) -> String { if inferred.sources.is_empty() { return format!( diff --git a/crates/plumb-cli/src/commands/lint.rs b/crates/plumb-cli/src/commands/lint.rs index fe9b5b7..6ae0fda 100644 --- a/crates/plumb-cli/src/commands/lint.rs +++ b/crates/plumb-cli/src/commands/lint.rs @@ -4,9 +4,9 @@ //! engine → formatter → stdout. //! //! The orchestrator builds one [`Target`] per requested viewport and -//! calls [`BrowserDriver::snapshot_all`] exactly once, so a real -//! Chromium driver launches the browser only once per CLI invocation -//! (PRD §10.3). +//! calls [`BrowserDriver::snapshot_all`] exactly once, so real +//! Chromium-backed linting launches the browser only once per CLI +//! invocation (PRD §10.3). use std::path::{Path, PathBuf}; use std::process::ExitCode; diff --git a/crates/plumb-cli/src/main.rs b/crates/plumb-cli/src/main.rs index 707afef..671138e 100644 --- a/crates/plumb-cli/src/main.rs +++ b/crates/plumb-cli/src/main.rs @@ -140,7 +140,7 @@ enum Command { /// navigation. #[arg(long, value_name = "PATH")] storage_state: Option, - /// Disable CSS animations and transitions before navigation + /// Disable CSS animations and transitions before capture /// for byte-stable snapshots. The driver already does this by /// default; pass `--disable-animations false` to opt out. #[arg( @@ -151,7 +151,7 @@ enum Command { default_missing_value = "true" )] disable_animations: bool, - /// Inject CSS that hides scrollbars before navigation. The + /// Inject CSS that hides scrollbars before capture. The /// driver already does this by default; pass /// `--hide-scrollbars false` to opt out. #[arg( @@ -273,7 +273,7 @@ enum Command { /// Path to a Playwright `storage-state.json`. #[arg(long, value_name = "PATH")] storage_state: Option, - /// Disable CSS animations and transitions before navigation. + /// Disable CSS animations and transitions before capture. #[arg( long = "disable-animations", default_value_t = true, @@ -282,7 +282,7 @@ enum Command { default_missing_value = "true" )] disable_animations: bool, - /// Inject CSS that hides scrollbars before navigation. + /// Inject CSS that hides scrollbars before capture. #[arg( long = "hide-scrollbars", default_value_t = true, diff --git a/crates/plumb-cli/tests/init_from.rs b/crates/plumb-cli/tests/init_from.rs index 5026d74..44f44cf 100644 --- a/crates/plumb-cli/tests/init_from.rs +++ b/crates/plumb-cli/tests/init_from.rs @@ -16,12 +16,12 @@ fn init_from_infers_starter_config_from_real_project_tree() -> Result<(), Box Result<(), Box Result<(), Box Result<(), Box Result<(), Box") .replace(&project_path, ""); - insta::assert_snapshot!("init_from_real_project", redacted); + if node_available { + insta::assert_snapshot!("init_from_real_project", redacted); + } Ok(()) } +fn node_on_path() -> bool { + std::process::Command::new("node") + .arg("--version") + .output() + .is_ok_and(|out| out.status.success()) +} + #[test] fn init_from_missing_directory_errors() -> Result<(), Box> { let outdir = TempDir::new()?; diff --git a/crates/plumb-cli/tests/snapshots/init_from__init_from_real_project.snap b/crates/plumb-cli/tests/snapshots/init_from__init_from_real_project.snap index b2d6c01..cb49b1a 100644 --- a/crates/plumb-cli/tests/snapshots/init_from__init_from_real_project.snap +++ b/crates/plumb-cli/tests/snapshots/init_from__init_from_real_project.snap @@ -1,45 +1,23 @@ --- source: crates/plumb-cli/tests/init_from.rs -assertion_line: 97 +assertion_line: 124 expression: redacted --- # Plumb configuration — bootstrapped by `plumb init --from ` from the sources below. # -# - tailwind: tailwind.config.ts +# - tailwind: tailwind.config.js # - css: src/styles/tokens.css # -# Tailwind config detected. Plumb merges Tailwind theme tokens at lint time — -# run `plumb lint` from the same directory and the adapter will resolve Tailwind's theme. - -[viewports] - -[spacing] -base_unit = 4 -scale = [ - 4, - 8, - 16, -] - -[spacing.tokens] -space-xs = 4 -space-sm = 8 -space-md = 16 - -[type] -families = [] -weights = [] -scale = [] - -[type.tokens] +# Tailwind config detected. Resolved theme tokens are included when available — +# edit the inferred values below if your project overrides them elsewhere. [color] delta_e_tolerance = 2.0 [color.tokens] +color-accent = "#0b7285" color-bg = "#ffffff" color-fg = "#0b0b0b" -color-accent = "#0b7285" [radius] scale = [ @@ -47,25 +25,19 @@ scale = [ 8, ] -[alignment] -tolerance_px = 3 - -[shadow] -scale = [] - -[z_index] -scale = [] - -[opacity] -scale = [] - -[rhythm] -base_line_px = 0 -tolerance_px = 2 -cap_height_fallback_px = 0 - -[a11y.touch_target] -min_width_px = 24 -min_height_px = 24 +[spacing] +base_unit = 4 +scale = [ + 2, + 4, + 6, + 8, + 16, +] -[rules] +[spacing.tokens] +"0.5" = 2 +"1.5" = 6 +space-md = 16 +space-sm = 8 +space-xs = 4 diff --git a/crates/plumb-codegen/src/lib.rs b/crates/plumb-codegen/src/lib.rs index fc8521a..4789315 100644 --- a/crates/plumb-codegen/src/lib.rs +++ b/crates/plumb-codegen/src/lib.rs @@ -127,8 +127,8 @@ pub struct TokenSource { #[non_exhaustive] pub enum TokenSourceKind { /// `tailwind.config.{js,ts,mjs,cjs,mts,cts}` at the project root. - /// V0 records the presence in the header comment; full theme - /// resolution is on the linter side. + /// The codegen crate records the presence in the header comment; + /// CLI callers may resolve the theme before rendering. TailwindConfig, /// CSS file containing one or more `:root` blocks. CssCustomProperties, diff --git a/crates/plumb-codegen/src/render.rs b/crates/plumb-codegen/src/render.rs index e65bd91..4675ebf 100644 --- a/crates/plumb-codegen/src/render.rs +++ b/crates/plumb-codegen/src/render.rs @@ -1,9 +1,10 @@ //! Render an [`InferredConfig`] to a `plumb.toml` string. //! //! The output starts with a generated header comment that records, in -//! sorted order, every source the inference pass consumed. The body is -//! the [`plumb_core::Config`] serialized via `toml::to_string_pretty`, -//! preserving `IndexMap` insertion order for tokens. +//! sorted order, every source the inference pass consumed. The body +//! contains only sections whose inferred values differ from +//! [`plumb_core::Config::default`], preserving runtime defaults for +//! viewports and rules. use crate::{CodegenError, InferredConfig, TokenSource, TokenSourceKind}; @@ -13,17 +14,15 @@ use crate::{CodegenError, InferredConfig, TokenSource, TokenSourceKind}; const HEADER: &str = "# Plumb configuration — bootstrapped by `plumb init --from ` from the sources below."; -/// Note appended when a Tailwind config was discovered. Plumb's -/// `extends = "./tailwind.config.*"` directive is still in flight; this -/// wording is shared with `examples/plumb-tailwind.toml`. +/// Note appended when a Tailwind config was discovered. const TAILWIND_HINT: &str = - "# Tailwind config detected. Plumb merges Tailwind theme tokens at lint time —"; + "# Tailwind config detected. Resolved theme tokens are included when available —"; /// Render `inferred` to a TOML string. /// /// The output is byte-identical given the same [`InferredConfig`]. The -/// header lists discovered source files in stable order; the body is -/// `toml::to_string_pretty` over the [`plumb_core::Config`]. +/// header lists discovered source files in stable order; the body is a +/// minimal TOML table that omits default-empty sections. /// /// # Errors /// @@ -45,12 +44,14 @@ pub fn render_toml(inferred: &InferredConfig) -> Result { out.push_str("#\n"); out.push_str(TAILWIND_HINT); out.push('\n'); - out.push_str("# run `plumb lint` from the same directory and the adapter will resolve Tailwind's theme.\n"); + out.push_str( + "# edit the inferred values below if your project overrides them elsewhere.\n", + ); } out.push('\n'); - let body = toml::to_string_pretty(&inferred.config)?; + let body = render_config_body(&inferred.config)?; out.push_str(&body); // Always end on a single newline. `toml::to_string_pretty` emits @@ -62,6 +63,75 @@ pub fn render_toml(inferred: &InferredConfig) -> Result { Ok(out) } +fn render_config_body(config: &plumb_core::Config) -> Result { + let default = plumb_core::Config::default(); + let mut table = toml::Table::new(); + + if config.viewports != default.viewports { + table.insert( + "viewports".to_owned(), + toml::Value::try_from(&config.viewports)?, + ); + } + if config.spacing != default.spacing { + table.insert( + "spacing".to_owned(), + toml::Value::try_from(&config.spacing)?, + ); + } + if config.type_scale != default.type_scale { + table.insert( + "type".to_owned(), + toml::Value::try_from(&config.type_scale)?, + ); + } + if config.color != default.color { + table.insert("color".to_owned(), toml::Value::try_from(&config.color)?); + } + if config.radius != default.radius { + table.insert("radius".to_owned(), toml::Value::try_from(&config.radius)?); + } + if config.alignment != default.alignment { + table.insert( + "alignment".to_owned(), + toml::Value::try_from(&config.alignment)?, + ); + } + if config.shadow != default.shadow { + table.insert("shadow".to_owned(), toml::Value::try_from(&config.shadow)?); + } + if config.z_index != default.z_index { + table.insert( + "z_index".to_owned(), + toml::Value::try_from(&config.z_index)?, + ); + } + if config.opacity != default.opacity { + table.insert( + "opacity".to_owned(), + toml::Value::try_from(&config.opacity)?, + ); + } + if config.rhythm != default.rhythm { + table.insert("rhythm".to_owned(), toml::Value::try_from(&config.rhythm)?); + } + if config.a11y != default.a11y { + table.insert("a11y".to_owned(), toml::Value::try_from(&config.a11y)?); + } + if config.rules != default.rules { + table.insert("rules".to_owned(), toml::Value::try_from(&config.rules)?); + } + if config.ignore != default.ignore { + table.insert("ignore".to_owned(), toml::Value::try_from(&config.ignore)?); + } + + if table.is_empty() { + Ok(String::new()) + } else { + toml::to_string_pretty(&toml::Value::Table(table)) + } +} + fn write_source_list(out: &mut String, sources: &[TokenSource]) { use std::fmt::Write as _; let mut sorted: Vec<&TokenSource> = sources.iter().collect(); diff --git a/crates/plumb-config/src/css_props.rs b/crates/plumb-config/src/css_props.rs index c90b0d6..5224685 100644 --- a/crates/plumb-config/src/css_props.rs +++ b/crates/plumb-config/src/css_props.rs @@ -1,8 +1,8 @@ //! CSS custom-properties scraper for token discovery (e.g. `plumb init`). //! //! Scans each input file for `:root { ... }` blocks at the top level or -//! wrapped inside a single `@media` / `@supports` at-rule, then extracts -//! every `--foo: ;` declaration. +//! wrapped inside a single supported at-rule (`@media`, `@supports`, or +//! `@layer`), then extracts every `--foo: ;` declaration. //! //! Values are lightly typed: //! @@ -38,9 +38,10 @@ use crate::ConfigError; pub struct CssPropertyScrape { /// Source path the declaration came from. pub source: PathBuf, - /// `None` for top-level `:root`. `Some("@media (...)")` (or - /// `"@supports (...)"`) when the `:root` block was wrapped in a - /// single at-rule. Preserves the at-rule prelude verbatim. + /// `None` for top-level `:root`. `Some("@media (...)")`, + /// `Some("@supports (...)")`, or `Some("@layer ...")` when the + /// `:root` block was wrapped in a single supported at-rule. + /// Preserves the at-rule prelude verbatim. pub at_rule: Option, /// Custom-property name, e.g. `--bg-canvas`. pub name: String, @@ -127,6 +128,9 @@ fn scrape_one( } if !parser.consume_byte_eq(b'{') { + if parser.consume_byte_eq(b';') { + continue; + } return Err(parse_error( path, contents, @@ -138,10 +142,10 @@ fn scrape_one( if is_root_selector(prelude_trimmed) { collect_root_block(&mut parser, path, contents, None, out)?; } else if let Some(at_rule) = parse_at_rule_prelude(prelude_trimmed) { - // We allow a single level of @media / @supports wrapping a - // :root block. Any other at-rule (or nested rules inside - // this one beyond a single :root) gets skipped without - // erroring — that's tolerant by design. + // We allow a single supported at-rule wrapping a :root + // block. Any nested rules inside this one beyond a direct + // :root get skipped without erroring — that's tolerant by + // design. scan_at_rule_body(&mut parser, path, contents, &at_rule, out)?; } else { // Plain selector that isn't :root — skip its block. @@ -161,8 +165,8 @@ fn is_root_selector(prelude: &str) -> bool { prelude.split_whitespace().collect::() == ":root" } -/// Recognize `@media (...)` / `@supports (...)` and return the -/// trimmed prelude (e.g. `@media (prefers-color-scheme: dark)`). +/// Recognize supported wrapper at-rules and return the trimmed prelude +/// (e.g. `@media (prefers-color-scheme: dark)`). fn parse_at_rule_prelude(prelude: &str) -> Option { let trimmed = prelude.trim(); if !trimmed.starts_with('@') { @@ -171,7 +175,7 @@ fn parse_at_rule_prelude(prelude: &str) -> Option { let (kw, _) = trimmed .split_once(|c: char| c.is_ascii_whitespace() || c == '(') .unwrap_or((trimmed, "")); - if kw == "@media" || kw == "@supports" { + if kw == "@media" || kw == "@supports" || kw == "@layer" { Some(trimmed.to_owned()) } else { None @@ -223,9 +227,9 @@ fn collect_root_block( } } -/// We're sitting at the open brace of an `@media (...)` / -/// `@supports (...)` block. Look inside for a single `:root { ... }` -/// rule and collect from it. Anything else is tolerantly skipped. +/// We're sitting at the open brace of a supported wrapper at-rule. Look +/// inside for a direct `:root { ... }` rule and collect from it. +/// Anything else is tolerantly skipped. fn scan_at_rule_body( parser: &mut Parser<'_>, path: &Path, @@ -256,6 +260,9 @@ fn scan_at_rule_body( .map_err(|fault| fault.into_error(path, contents))?; let prelude_trimmed = prelude.trim(); if !parser.consume_byte_eq(b'{') { + if parser.consume_byte_eq(b';') { + continue; + } return Err(parse_error( path, contents, diff --git a/crates/plumb-config/src/tailwind/mod.rs b/crates/plumb-config/src/tailwind/mod.rs index 71859e9..2bae345 100644 --- a/crates/plumb-config/src/tailwind/mod.rs +++ b/crates/plumb-config/src/tailwind/mod.rs @@ -642,31 +642,36 @@ fn merge_font_weight(spec: &mut TypeScaleSpec, font_weight: &serde_json::Map) { let mut seen: IndexMap = IndexMap::new(); for family in &spec.families { seen.insert(family.clone(), ()); } for value in font_family.values() { - let primary = match value { - Value::String(s) => Some(s.trim().trim_matches(['\'', '"']).to_owned()), - Value::Array(arr) => arr - .first() - .and_then(Value::as_str) - .map(|s| s.trim().trim_matches(['\'', '"']).to_owned()), - _ => None, - }; - if let Some(family) = primary - && !family.is_empty() - { - seen.insert(family, ()); + match value { + Value::String(s) => insert_font_family(&mut seen, s), + Value::Array(arr) => { + for item in arr { + if let Some(s) = item.as_str() { + insert_font_family(&mut seen, s); + } + } + } + _ => {} } } spec.families = seen.into_keys().collect(); } +fn insert_font_family(seen: &mut IndexMap, raw: &str) { + let family = raw.trim().trim_matches(['\'', '"']).to_owned(); + if !family.is_empty() { + seen.insert(family, ()); + } +} + /// Merge `theme.borderRadius` into [`RadiusSpec`]. fn merge_radius(spec: &mut RadiusSpec, radius: &serde_json::Map) { let mut values: Vec = spec.scale.clone(); @@ -849,7 +854,7 @@ mod tests { } #[test] - fn merge_font_family_keeps_primary() { + fn merge_font_family_keeps_stack_entries() { let mut spec = TypeScaleSpec::default(); let theme = serde_json::json!({ "sans": ["Inter", "ui-sans-serif", "system-ui"], @@ -857,7 +862,16 @@ mod tests { }); let map = theme.as_object().expect("object"); merge_font_family(&mut spec, map); - assert_eq!(spec.families, vec!["Inter", "JetBrains Mono"]); + assert_eq!( + spec.families, + vec![ + "Inter", + "ui-sans-serif", + "system-ui", + "JetBrains Mono", + "monospace" + ] + ); } #[test] diff --git a/crates/plumb-config/tests/css_props_scraper.rs b/crates/plumb-config/tests/css_props_scraper.rs index 21a06c3..38406d9 100644 --- a/crates/plumb-config/tests/css_props_scraper.rs +++ b/crates/plumb-config/tests/css_props_scraper.rs @@ -79,6 +79,47 @@ fn scrapes_root_inside_supports_block() { assert!(matches!(grid_gap.value, ScrapedValue::Px(16))); } +#[test] +fn scrapes_root_inside_tailwind_layer_with_directives() { + let dir = tempfile::tempdir().expect("tempdir"); + let path = dir.path().join("tailwind.css"); + std::fs::write( + &path, + "@tailwind base;\n\ + @tailwind components;\n\ + @tailwind utilities;\n\ + \n\ + @layer base {\n\ + :root {\n\ + --bg-canvas: #ffffff;\n\ + --space-2: 8px;\n\ + }\n\ + }\n\ + \n\ + @layer utilities {\n\ + .focus-ring {\n\ + @apply focus-visible:outline-none focus-visible:ring-2;\n\ + }\n\ + }\n", + ) + .expect("write css"); + + let scrapes = scrape_css_properties(&[path]).expect("scrape"); + + let bg = scrapes + .iter() + .find(|s| s.name == "--bg-canvas") + .expect("--bg-canvas in @layer base"); + assert_eq!(bg.at_rule.as_deref(), Some("@layer base")); + assert!(matches!(&bg.value, ScrapedValue::Color(c) if c == "#ffffff")); + + let space = scrapes + .iter() + .find(|s| s.name == "--space-2") + .expect("--space-2 in @layer base"); + assert!(matches!(space.value, ScrapedValue::Px(8))); +} + #[test] fn handles_comments_and_quoted_strings_in_values() { let dir = tempfile::tempdir().expect("tempdir"); diff --git a/crates/plumb-core/src/rules/spacing/grid_conformance.rs b/crates/plumb-core/src/rules/spacing/grid_conformance.rs index 4844b50..c379508 100644 --- a/crates/plumb-core/src/rules/spacing/grid_conformance.rs +++ b/crates/plumb-core/src/rules/spacing/grid_conformance.rs @@ -20,7 +20,7 @@ use indexmap::IndexMap; use crate::config::Config; use crate::report::{Confidence, Fix, FixKind, Severity, Violation, ViolationSink}; use crate::rules::Rule; -use crate::rules::spacing::SPACING_PROPERTIES; +use crate::rules::spacing::{SPACING_PROPERTIES, is_framework_hidden_spacing_node}; use crate::rules::util::{nearest_in_scale, nearest_multiple, parse_px}; use crate::snapshot::SnapshotCtx; @@ -63,7 +63,13 @@ impl Rule for GridConformance { } for node in ctx.nodes() { + if is_framework_hidden_spacing_node(node) { + continue; + } for prop in SPACING_PROPERTIES { + if is_root_body_margin(&node.tag, &node.selector, prop) { + continue; + } let Some(raw) = node.computed_styles.get(*prop) else { continue; }; @@ -122,3 +128,7 @@ impl Rule for GridConformance { } } } + +fn is_root_body_margin(tag: &str, selector: &str, prop: &str) -> bool { + tag == "body" && selector == "html > body" && prop.starts_with("margin-") +} diff --git a/crates/plumb-core/src/rules/spacing/mod.rs b/crates/plumb-core/src/rules/spacing/mod.rs index ec8170e..222c6c3 100644 --- a/crates/plumb-core/src/rules/spacing/mod.rs +++ b/crates/plumb-core/src/rules/spacing/mod.rs @@ -12,6 +12,8 @@ pub mod grid_conformance; pub mod scale_conformance; +use crate::snapshot::SnapshotNode; + /// Physical-longhand spacing properties the rules in this category /// inspect. /// @@ -32,3 +34,7 @@ pub(crate) const SPACING_PROPERTIES: &[&str] = &[ "row-gap", "column-gap", ]; + +pub(crate) fn is_framework_hidden_spacing_node(node: &SnapshotNode) -> bool { + node.selector == "html > body > next-route-announcer > div" +} diff --git a/crates/plumb-core/src/rules/spacing/scale_conformance.rs b/crates/plumb-core/src/rules/spacing/scale_conformance.rs index 425b695..4e69d41 100644 --- a/crates/plumb-core/src/rules/spacing/scale_conformance.rs +++ b/crates/plumb-core/src/rules/spacing/scale_conformance.rs @@ -11,7 +11,7 @@ use indexmap::IndexMap; use crate::config::Config; use crate::report::{Confidence, Fix, FixKind, Severity, Violation, ViolationSink}; use crate::rules::Rule; -use crate::rules::spacing::SPACING_PROPERTIES; +use crate::rules::spacing::{SPACING_PROPERTIES, is_framework_hidden_spacing_node}; use crate::rules::util::{nearest_in_scale, parse_px}; use crate::snapshot::SnapshotCtx; @@ -47,6 +47,9 @@ impl Rule for ScaleConformance { } for node in ctx.nodes() { + if is_framework_hidden_spacing_node(node) { + continue; + } for prop in SPACING_PROPERTIES { let Some(raw) = node.computed_styles.get(*prop) else { continue; diff --git a/crates/plumb-core/tests/golden_spacing_grid.rs b/crates/plumb-core/tests/golden_spacing_grid.rs index 76baa0c..b6c02f9 100644 --- a/crates/plumb-core/tests/golden_spacing_grid.rs +++ b/crates/plumb-core/tests/golden_spacing_grid.rs @@ -242,6 +242,68 @@ fn spacing_grid_conformance_tolerance_band() { ); } +#[test] +fn spacing_grid_conformance_skips_root_body_margins() { + let mut body = body_node(); + body.computed_styles + .insert("margin-top".to_owned(), "98.55px".to_owned()); + body.computed_styles + .insert("margin-bottom".to_owned(), "98.55px".to_owned()); + body.computed_styles + .insert("padding-top".to_owned(), "13px".to_owned()); + + let snapshot = PlumbSnapshot { + url: "plumb-fake://spacing-grid-root-body".into(), + viewport: ViewportKey::new("desktop"), + viewport_width: 1280, + viewport_height: 800, + nodes: vec![root_html_with_body(), body], + text_boxes: Vec::new(), + }; + let violations: Vec = run(&snapshot, &fixture_config()) + .into_iter() + .filter(|v| v.rule_id == "spacing/grid-conformance") + .collect(); + + assert_eq!(violations.len(), 1); + assert_eq!(violations[0].selector, "html > body"); + assert!(violations[0].message.contains("padding-top 13px")); +} + +#[test] +fn spacing_grid_conformance_skips_next_route_announcer() { + let announcer = node( + 2, + "html > body > next-route-announcer > div", + &[ + ("margin-top", "-1px"), + ("margin-right", "-1px"), + ("margin-bottom", "-1px"), + ("margin-left", "-1px"), + ], + Some(Rect { + x: -1, + y: 333, + width: 1, + height: 1, + }), + ); + + let snapshot = PlumbSnapshot { + url: "plumb-fake://spacing-grid-next-route-announcer".into(), + viewport: ViewportKey::new("desktop"), + viewport_width: 1280, + viewport_height: 800, + nodes: vec![root_html_with_body(), body_node(), announcer], + text_boxes: Vec::new(), + }; + let has_violation = run(&snapshot, &fixture_config()) + .into_iter() + .any(|v| v.rule_id == "spacing/grid-conformance"); + + assert!(!has_violation); +} + /// Build a one-node snapshot carrying a single spacing property and /// count `spacing/grid-conformance` violations under `config`. Lets the /// scale-deferral tests vary both the property value and the configured diff --git a/crates/plumb-core/tests/golden_spacing_scale.rs b/crates/plumb-core/tests/golden_spacing_scale.rs index 8b482d0..7482010 100644 --- a/crates/plumb-core/tests/golden_spacing_scale.rs +++ b/crates/plumb-core/tests/golden_spacing_scale.rs @@ -178,6 +178,40 @@ fn spacing_scale_conformance_golden() -> Result<(), serde_json::Error> { Ok(()) } +#[test] +fn spacing_scale_conformance_skips_next_route_announcer() { + let announcer = node( + 2, + "html > body > next-route-announcer > div", + &[ + ("margin-top", "-1px"), + ("margin-right", "-1px"), + ("margin-bottom", "-1px"), + ("margin-left", "-1px"), + ], + Some(Rect { + x: -1, + y: 333, + width: 1, + height: 1, + }), + ); + + let snapshot = PlumbSnapshot { + url: "plumb-fake://spacing-scale-next-route-announcer".into(), + viewport: ViewportKey::new("desktop"), + viewport_width: 1280, + viewport_height: 800, + nodes: vec![root_html(), body_node(), announcer], + text_boxes: Vec::new(), + }; + let has_violation = run(&snapshot, &fixture_config()) + .into_iter() + .any(|v| v.rule_id == "spacing/scale-conformance"); + + assert!(!has_violation); +} + #[test] fn spacing_scale_conformance_run_is_deterministic() -> Result<(), serde_json::Error> { let snapshot = fixture_snapshot(); diff --git a/crates/plumb-e2e/README.md b/crates/plumb-e2e/README.md index aed6d04..33ac67c 100644 --- a/crates/plumb-e2e/README.md +++ b/crates/plumb-e2e/README.md @@ -43,6 +43,9 @@ cargo run -p plumb-e2e -- --all --chrome-path /usr/bin/google-chrome-stable # Override the plumb binary path. cargo run -p plumb-e2e -- --all --plumb-bin /tmp/plumb + +# Fail a stuck child `plumb lint` run faster. +cargo run -p plumb-e2e -- --site html-css --lint-timeout-secs 30 ``` The simpler entry point is `just test-e2e`, which builds the binary diff --git a/crates/plumb-e2e/src/main.rs b/crates/plumb-e2e/src/main.rs index ef9cf15..6010e17 100644 --- a/crates/plumb-e2e/src/main.rs +++ b/crates/plumb-e2e/src/main.rs @@ -7,6 +7,7 @@ use std::path::PathBuf; use std::process::ExitCode; +use std::time::Duration; use anyhow::Context as _; use clap::Parser; @@ -45,6 +46,10 @@ struct Cli { /// Number of lint runs to compare for byte-equality. Defaults to 3. #[arg(long, default_value_t = 3)] determinism_runs: usize, + + /// Timeout, in seconds, for each child `plumb lint` invocation. + #[arg(long, default_value_t = 120, value_parser = clap::value_parser!(u64).range(1..))] + lint_timeout_secs: u64, } fn main() -> ExitCode { @@ -74,6 +79,7 @@ fn real_main() -> Result<(), anyhow::Error> { config.chrome_path = cli.chrome_path; config.build_first = !cli.no_build; config.determinism_runs = cli.determinism_runs; + config.lint_timeout = Duration::from_secs(cli.lint_timeout_secs); let sites: Vec = if cli.all || cli.site.is_empty() { SITES.iter().map(|s| s.name.to_owned()).collect() diff --git a/crates/plumb-e2e/src/runner.rs b/crates/plumb-e2e/src/runner.rs index 7aa9401..02cc389 100644 --- a/crates/plumb-e2e/src/runner.rs +++ b/crates/plumb-e2e/src/runner.rs @@ -1,7 +1,9 @@ //! Per-site execution. Build → serve → lint × 3 → assert. use std::path::{Path, PathBuf}; -use std::process::Command; +use std::process::{Command, ExitStatus, Stdio}; +use std::thread; +use std::time::Duration; use indexmap::IndexMap; @@ -9,6 +11,11 @@ use crate::HarnessError; use crate::expected::{Expected, WaitFor}; use crate::server::StaticServer; +const DEFAULT_LINT_TIMEOUT_SECS: u64 = 120; +const POLL_INTERVAL: Duration = Duration::from_millis(50); +const CLEANUP_RETRIES: usize = 100; +const DIAGNOSTIC_LIMIT: usize = 4096; + /// Runtime configuration for a harness invocation. #[derive(Debug, Clone)] pub struct HarnessConfig { @@ -26,6 +33,8 @@ pub struct HarnessConfig { /// How many lint runs to compare for byte-equality. The default /// is 3, matching `just determinism-check`. pub determinism_runs: usize, + /// Timeout for each child `plumb lint` invocation. + pub lint_timeout: Duration, } impl HarnessConfig { @@ -40,6 +49,7 @@ impl HarnessConfig { chrome_path: None, build_first: true, determinism_runs: 3, + lint_timeout: Duration::from_secs(DEFAULT_LINT_TIMEOUT_SECS), } } } @@ -95,7 +105,7 @@ pub fn run_site(name: &str, config: &HarnessConfig) -> Result, + run_idx: usize, ) -> Result, HarnessError> { let plumb_config = config.workspace_root.join("e2e-sites").join("plumb.toml"); let mut cmd = Command::new(&config.plumb_bin); @@ -187,27 +198,344 @@ fn run_lint( cmd.arg("--wait-for").arg(&gate.selector); cmd.arg("--wait-ms").arg(gate.timeout_ms.to_string()); } - let output = cmd.output().map_err(|err| HarnessError::Lint { + let tmp_dir = isolated_tmp_dir(name, run_idx, std::process::id()); + std::fs::create_dir_all(&tmp_dir).map_err(|err| HarnessError::Lint { site: name.to_owned(), - reason: format!("spawn plumb binary `{}`: {err}", config.plumb_bin.display()), + reason: format!("create isolated TMPDIR `{}`: {err}", tmp_dir.display()), })?; + set_child_temp_env(&mut cmd, &tmp_dir); + let command_preview = format_command(&cmd); + tracing::info!( + site = %name, + run = run_idx, + timeout_secs = config.lint_timeout.as_secs(), + command = %command_preview, + tmp_dir = %tmp_dir.display(), + "harness — running plumb lint", + ); + + let output = run_command_with_timeout(&mut cmd, config.lint_timeout).map_err(|err| { + cleanup_tmp_dir(&tmp_dir); + HarnessError::Lint { + site: name.to_owned(), + reason: format!( + "spawn or wait for plumb binary `{}`: {err}; command={command_preview}", + config.plumb_bin.display() + ), + } + })?; + cleanup_tmp_dir(&tmp_dir); + + let (status, stdout, stderr, pid) = match output { + ChildOutput::Exited { + status, + stdout, + stderr, + pid, + } => (status, stdout, stderr, pid), + ChildOutput::TimedOut { + stdout, + stderr, + pid, + } => { + return Err(HarnessError::Lint { + site: name.to_owned(), + reason: format!( + "plumb lint timed out after {:?}; pid={pid}; run={run_idx}; command={command_preview}; stdout_bytes={}; stderr=\n{}", + config.lint_timeout, + stdout.len(), + diagnostic_text(&stderr), + ), + }); + } + }; + // PRD §13.3: 0 = clean, 1 = one or more violations at/above the // default `--min-severity warn` threshold, 2 = CLI / infrastructure // failure. The fixtures intentionally produce warnings, so exit 1 is // the steady state; anything outside {0, 1} is an infrastructure // failure. - let code = output.status.code(); + let code = status.code(); let allowed = matches!(code, Some(0 | 1)); if !allowed { return Err(HarnessError::Lint { site: name.to_owned(), reason: format!( - "plumb exited with code {code:?}; stderr=\n{}", - String::from_utf8_lossy(&output.stderr), + "plumb exited with code {code:?}; pid={pid}; run={run_idx}; command={command_preview}; stdout_bytes={}; stderr=\n{}", + stdout.len(), + diagnostic_text(&stderr), ), }); } - Ok(output.stdout) + Ok(stdout) +} + +enum ChildOutput { + Exited { + status: ExitStatus, + stdout: Vec, + stderr: Vec, + pid: u32, + }, + TimedOut { + stdout: Vec, + stderr: Vec, + pid: u32, + }, +} + +fn run_command_with_timeout( + cmd: &mut Command, + timeout: Duration, +) -> Result { + let mut child = cmd + .stdin(Stdio::null()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn()?; + let pid = child.id(); + + // Reader threads must start before waiting so large JSON/stdout or + // stderr output cannot fill an OS pipe and deadlock the child. + let stdout_handle = child.stdout.take().map(spawn_reader); + let stderr_handle = child.stderr.take().map(spawn_reader); + + match wait_with_timeout(&mut child, timeout) { + WaitOutcome::Exited(status) => Ok(ChildOutput::Exited { + status, + stdout: drain_reader(stdout_handle), + stderr: drain_reader(stderr_handle), + pid, + }), + WaitOutcome::TimedOut => { + kill_child_process_tree(&mut child, pid); + Ok(ChildOutput::TimedOut { + stdout: drain_reader(stdout_handle), + stderr: drain_reader(stderr_handle), + pid, + }) + } + WaitOutcome::Errored(err) => { + kill_child_process_tree(&mut child, pid); + let _ = drain_reader(stdout_handle); + let _ = drain_reader(stderr_handle); + Err(err) + } + } +} + +#[cfg(unix)] +fn kill_child_process_tree(child: &mut std::process::Child, pid: u32) { + let mut pids = child_process_tree(pid); + pids.push(pid); + signal_pids(&pids, "TERM"); + wait_for_child_exit(child, Duration::from_millis(500)); + + pids.extend(child_process_tree(pid)); + pids.sort_unstable(); + pids.dedup(); + signal_pids(&pids, "KILL"); + let _ = child.kill(); + let _ = child.wait(); +} + +#[cfg(not(unix))] +fn kill_child_process_tree(child: &mut std::process::Child, _pid: u32) { + let _ = child.kill(); + let _ = child.wait(); +} + +#[cfg(unix)] +fn child_process_tree(root: u32) -> Vec { + let mut seen = std::collections::BTreeSet::new(); + collect_child_processes(root, &mut seen); + seen.into_iter().collect() +} + +#[cfg(unix)] +fn collect_child_processes(parent: u32, seen: &mut std::collections::BTreeSet) { + for child in direct_child_pids(parent) { + if seen.insert(child) { + collect_child_processes(child, seen); + } + } +} + +#[cfg(unix)] +fn direct_child_pids(parent: u32) -> Vec { + let Ok(output) = Command::new("pgrep") + .arg("-P") + .arg(parent.to_string()) + .stdin(Stdio::null()) + .stderr(Stdio::null()) + .output() + else { + return Vec::new(); + }; + if !output.status.success() { + return Vec::new(); + } + String::from_utf8_lossy(&output.stdout) + .lines() + .filter_map(|line| line.trim().parse::().ok()) + .collect() +} + +#[cfg(unix)] +fn signal_pids(pids: &[u32], signal: &str) { + for pid in pids.iter().rev() { + signal_pid(*pid, signal); + } +} + +#[cfg(unix)] +fn signal_pid(pid: u32, signal: &str) { + let _ = Command::new("kill") + .arg(format!("-{signal}")) + .arg(pid.to_string()) + .stdin(Stdio::null()) + .stdout(Stdio::null()) + .stderr(Stdio::null()) + .status(); +} + +#[cfg(unix)] +fn wait_for_child_exit(child: &mut std::process::Child, timeout: Duration) { + let max_ticks = (timeout.as_millis() / POLL_INTERVAL.as_millis()).max(1); + for _ in 0..max_ticks { + match child.try_wait() { + Ok(None) => thread::sleep(POLL_INTERVAL), + Ok(Some(_)) | Err(_) => return, + } + } +} + +fn spawn_reader(mut reader: R) -> thread::JoinHandle>> +where + R: std::io::Read + Send + 'static, +{ + thread::spawn(move || { + let mut buf = Vec::new(); + reader.read_to_end(&mut buf)?; + Ok(buf) + }) +} + +fn drain_reader(handle: Option>>>) -> Vec { + match handle { + None => Vec::new(), + Some(h) => match h.join() { + Ok(Ok(buf)) => buf, + Ok(Err(_)) | Err(_) => Vec::new(), + }, + } +} + +enum WaitOutcome { + Exited(ExitStatus), + Errored(std::io::Error), + TimedOut, +} + +fn wait_with_timeout(child: &mut std::process::Child, timeout: Duration) -> WaitOutcome { + let max_ticks = (timeout.as_millis() / POLL_INTERVAL.as_millis()).max(1); + for _ in 0..max_ticks { + match child.try_wait() { + Ok(Some(status)) => return WaitOutcome::Exited(status), + Ok(None) => thread::sleep(POLL_INTERVAL), + Err(err) => return WaitOutcome::Errored(err), + } + } + WaitOutcome::TimedOut +} + +fn isolated_tmp_dir(site: &str, run_idx: usize, process_id: u32) -> PathBuf { + let safe_site = site + .chars() + .map(|ch| { + if ch.is_ascii_alphanumeric() || ch == '-' || ch == '_' { + ch + } else { + '-' + } + }) + .collect::(); + short_temp_root().join(format!("pe2e-{safe_site}-{process_id}-{run_idx}")) +} + +fn short_temp_root() -> PathBuf { + #[cfg(unix)] + { + PathBuf::from("/tmp") + } + #[cfg(windows)] + { + for key in ["TMP", "TEMP"] { + if let Some(path) = std::env::var_os(key) { + return PathBuf::from(path); + } + } + PathBuf::from(r"C:\Temp") + } + #[cfg(not(any(unix, windows)))] + { + PathBuf::from("tmp") + } +} + +fn set_child_temp_env(cmd: &mut Command, tmp_dir: &Path) { + cmd.env("TMPDIR", tmp_dir); + cmd.env("TMP", tmp_dir); + cmd.env("TEMP", tmp_dir); +} + +fn cleanup_tmp_dir(path: &Path) { + for _ in 0..CLEANUP_RETRIES { + match std::fs::remove_dir_all(path) { + Ok(()) => return, + Err(err) if err.kind() == std::io::ErrorKind::NotFound => return, + Err(_) => thread::sleep(POLL_INTERVAL), + } + } + let _ = std::fs::remove_dir_all(path); +} + +fn format_command(cmd: &Command) -> String { + std::iter::once(cmd.get_program()) + .chain(cmd.get_args()) + .map(shell_quote) + .collect::>() + .join(" ") +} + +fn shell_quote(arg: &std::ffi::OsStr) -> String { + let s = arg.to_string_lossy(); + if s.is_empty() { + return String::from("''"); + } + if s.chars().all(|ch| { + ch.is_ascii_alphanumeric() || matches!(ch, '/' | '.' | '_' | '-' | ':' | '=' | ',' | '@') + }) { + return s.into_owned(); + } + format!("'{}'", s.replace('\'', "'\\''")) +} + +fn diagnostic_text(bytes: &[u8]) -> String { + let prefix = if bytes.len() > DIAGNOSTIC_LIMIT { + &bytes[..DIAGNOSTIC_LIMIT] + } else { + bytes + }; + let text = String::from_utf8_lossy(prefix); + if bytes.len() > DIAGNOSTIC_LIMIT { + format!( + "{text}\n", + bytes.len() + ) + } else { + text.into_owned() + } } #[derive(Debug)] @@ -249,7 +577,12 @@ fn parse_counts(stdout: &[u8], target_rules: &[String]) -> Result", DIAGNOSTIC_LIMIT + 2))); + } + + #[test] + fn set_child_temp_env_sets_unix_and_windows_vars() { + let mut cmd = Command::new("plumb"); + let tmp = std::path::Path::new("/workspace/target/plumb-e2e-tmp/run"); + let expected = tmp.as_os_str().to_owned(); + + set_child_temp_env(&mut cmd, tmp); + + let envs = cmd + .get_envs() + .filter_map(|(key, value)| value.map(|v| (key.to_owned(), v.to_owned()))) + .collect::>(); + assert_eq!(envs.get(std::ffi::OsStr::new("TMPDIR")), Some(&expected)); + assert_eq!(envs.get(std::ffi::OsStr::new("TMP")), Some(&expected)); + assert_eq!(envs.get(std::ffi::OsStr::new("TEMP")), Some(&expected)); + } + + #[test] + #[cfg(unix)] + fn command_timeout_kills_stuck_child() { + use std::time::Duration; + + use super::{ChildOutput, run_command_with_timeout}; + + let mut cmd = Command::new("sh"); + cmd.arg("-c").arg("printf ready; sleep 5; printf never >&2"); + + let output = + run_command_with_timeout(&mut cmd, Duration::from_millis(50)).expect("run command"); + + let ChildOutput::TimedOut { stdout, stderr, .. } = output else { + panic!("child should time out"); + }; + assert_eq!(stdout, b"ready"); + assert!(stderr.is_empty()); + } + + #[test] + #[cfg(unix)] + fn command_timeout_kills_descendant_processes() { + use std::time::Duration; + + use super::{ChildOutput, run_command_with_timeout}; + + let mut cmd = Command::new("sh"); + cmd.arg("-c") + .arg("(trap '' TERM; printf child-ready; while :; do sleep 10; done) & wait"); + + let output = + run_command_with_timeout(&mut cmd, Duration::from_millis(50)).expect("run command"); + + let ChildOutput::TimedOut { stdout, .. } = output else { + panic!("child should time out"); + }; + assert_eq!(stdout, b"child-ready"); + } + + #[test] + #[cfg(unix)] + fn command_output_drains_large_stdout() { + use std::time::Duration; + + use super::{ChildOutput, run_command_with_timeout}; + + let mut cmd = Command::new("sh"); + cmd.arg("-c").arg("yes x | head -c 131072"); + + let output = + run_command_with_timeout(&mut cmd, Duration::from_secs(5)).expect("run command"); + + let ChildOutput::Exited { status, stdout, .. } = output else { + panic!("child should exit"); + }; + assert!(status.success()); + assert_eq!(stdout.len(), 131_072); + } } diff --git a/crates/plumb-mcp/src/lib.rs b/crates/plumb-mcp/src/lib.rs index 9b47f4a..cac9f80 100644 --- a/crates/plumb-mcp/src/lib.rs +++ b/crates/plumb-mcp/src/lib.rs @@ -404,9 +404,10 @@ impl PlumbServer { let config = self.resolve_config_object(args.working_dir.as_deref())?; + let guarded_html = lint_page_html_guarded_document(&args.html); let data_url = format!( "data:text/html;base64,{}", - base64_encode(args.html.as_bytes()) + base64_encode(guarded_html.as_bytes()) ); let target = Target { url: data_url, @@ -822,6 +823,74 @@ const LINT_PAGE_HTML_INPUT_BYTE_CAP: usize = 1024 * 1024; /// (10 000), estimated cheaply from the raw HTML before rendering. const LINT_PAGE_HTML_ELEMENT_CAP: usize = 10_000; +const LINT_PAGE_HTML_CSP_META: &str = concat!( + r#""# +); + +fn lint_page_html_guarded_document(html: &str) -> String { + let mut guarded = String::with_capacity(LINT_PAGE_HTML_CSP_META.len() + html.len()); + if let Some(index) = lint_page_html_csp_insert_index(html) { + guarded.push_str(&html[..index]); + guarded.push_str(LINT_PAGE_HTML_CSP_META); + guarded.push_str(&html[index..]); + } else { + guarded.push_str(LINT_PAGE_HTML_CSP_META); + guarded.push_str(html); + } + guarded +} + +fn lint_page_html_csp_insert_index(html: &str) -> Option { + find_head_tag_end(html).or_else(|| find_doctype_end(html)) +} + +fn find_head_tag_end(html: &str) -> Option { + let bytes = html.as_bytes(); + let needle = b"').map(|offset| after + offset + 1); + } + None +} + +fn find_doctype_end(html: &str) -> Option { + let start = html + .char_indices() + .find(|(_, ch)| !ch.is_ascii_whitespace()) + .map_or(html.len(), |(index, _)| index); + let rest = &html[start..]; + let needle = b"').map(|offset| start + offset + 1) +} + +fn is_html_tag_boundary(byte: u8) -> bool { + matches!(byte, b'>' | b'/' | b' ' | b'\t' | b'\n' | b'\r' | 0x0c) +} + /// Conservative pre-render estimate of an HTML document's element count: /// the number of `<` bytes immediately followed by an ASCII letter (an /// opening tag). Closing tags (` Result<(), McpError> { let handler = PlumbServer::new(cwd); - let service = handler - .clone() - .serve(stdio()) + let service = Box::pin(handler.clone().serve(stdio())) .await .map_err(|e| McpError::Service(e.to_string()))?; let service_result = service @@ -1382,4 +1449,48 @@ mod tests { "unexpected error: {err:?}" ); } + + #[test] + fn lint_page_html_guarded_document_prepends_fetch_blocking_csp() { + let html = r#"

x

"#; + let guarded = lint_page_html_guarded_document(html); + + assert!( + guarded.starts_with(r#"x

"#)); + } + + #[test] + fn lint_page_html_guarded_document_inserts_csp_before_head_resources() { + let html = r#"x"#; + let guarded = lint_page_html_guarded_document(html); + + assert!(guarded.starts_with("")); + let csp_index = guarded + .find(r#"x

"#; + let guarded = lint_page_html_guarded_document(html); + + assert!( + guarded.starts_with(r#" PlumbServer { PlumbServer::new(PathBuf::from("/")) } +fn tool_text(result: &CallToolResult) -> Option { + result + .content + .iter() + .find_map(|content| content.as_text().map(|text| text.text.clone())) +} + +#[cfg(feature = "e2e-chromium")] +fn chromium_unavailable_result(result: &CallToolResult) -> bool { + if !result.is_error.unwrap_or(false) { + return false; + } + + tool_text(result).is_some_and(|text| { + text.contains("Chromium executable not found") + || text.contains("Chromium major version") + || text.contains("Chromium auto-fetch failed") + }) +} + +#[cfg(feature = "e2e-chromium")] +fn assert_rendered_or_skip_unavailable(result: &CallToolResult, context: &str) -> bool { + if chromium_unavailable_result(result) { + return false; + } + + assert!( + !result.is_error.unwrap_or(false), + "{context}: {}", + tool_text(result).unwrap_or_else(|| "".to_owned()) + ); + true +} + #[test] fn server_info_declares_plumb() { let server = server(); @@ -351,11 +395,7 @@ async fn lint_page_html_defect_html_never_returns_false_clean() { if result.is_error.unwrap_or(false) { // Chromium unavailable (or another driver failure): the response // must be a clear error, not a misleading clean. - let text = result - .content - .iter() - .find_map(|content| content.as_text().map(|text| text.text.clone())) - .expect("error response must include a text content block"); + let text = tool_text(&result).expect("error response must include a text content block"); assert!( text.contains("lint_page_html failed"), "driver failure must be explicit, got: {text}" @@ -395,8 +435,10 @@ async fn lint_page_html_renders_embedded_style_into_grid_finding() { .await .expect("lint_page_html must not raise a JSON-RPC error for valid input"); - if result.is_error.unwrap_or(false) { - // No usable Chromium on this host — skip, same as the cdp e2e suite. + if !assert_rendered_or_skip_unavailable( + &result, + "lint_page_html must render embedded styles when Chromium is available", + ) { return; } @@ -415,6 +457,94 @@ async fn lint_page_html_renders_embedded_style_into_grid_finding() { server.shutdown().await.expect("shutdown must succeed"); } +#[cfg(feature = "e2e-chromium")] +#[tokio::test] +async fn lint_page_html_blocks_external_resource_fetches() { + let listener = TcpListener::bind(("127.0.0.1", 0)).expect("bind local canary server"); + listener + .set_nonblocking(true) + .expect("listener must be nonblocking"); + let port = listener.local_addr().expect("local addr").port(); + let hits = Arc::new(AtomicUsize::new(0)); + let stop = Arc::new(AtomicBool::new(false)); + let thread_hits = Arc::clone(&hits); + let thread_stop = Arc::clone(&stop); + let handle = std::thread::spawn(move || { + while !thread_stop.load(Ordering::SeqCst) { + match listener.accept() { + Ok((mut stream, _addr)) => { + thread_hits.fetch_add(1, Ordering::SeqCst); + let mut buf = [0; 512]; + let _ = stream.read(&mut buf); + let _ = + stream.write_all(b"HTTP/1.1 204 No Content\r\nContent-Length: 0\r\n\r\n"); + } + Err(err) if err.kind() == std::io::ErrorKind::WouldBlock => { + std::thread::sleep(Duration::from_millis(20)); + } + Err(_) => break, + } + } + }); + + let external = format!("http://127.0.0.1:{port}/blocked"); + let html = format!( + r#" + + + + + + + + +
x
+ +"# + ); + + let server = server(); + let result = server + .lint_page_html(LintPageHtmlArgs { + html, + base_url: "https://example.com/".to_owned(), + working_dir: None, + }) + .await + .expect("lint_page_html must not raise a JSON-RPC error for valid input"); + + tokio::time::sleep(Duration::from_millis(250)).await; + stop.store(true, Ordering::SeqCst); + handle.join().expect("canary thread must join"); + + assert_eq!( + hits.load(Ordering::SeqCst), + 0, + "lint_page_html must not fetch external resources" + ); + + if !assert_rendered_or_skip_unavailable( + &result, + "lint_page_html must block external resources without breaking data-url rendering", + ) { + return; + } + + let structured = result + .structured_content + .expect("successful response must include structured_content"); + let by_rule = structured + .get("by_rule") + .and_then(serde_json::Value::as_object) + .expect("structuredContent.by_rule must be an object"); + assert!( + by_rule.contains_key("spacing/grid-conformance"), + "inline style must still render after external fetches are blocked: {structured}", + ); + + server.shutdown().await.expect("shutdown must succeed"); +} + /// `lint_page_html` rejects empty `html` with a JSON-RPC `invalid_params` /// error so the agent gets a clear contract failure rather than a /// silently-empty snapshot. Validated before any rendering, so it runs diff --git a/e2e-sites/nextjs/README.md b/e2e-sites/nextjs/README.md index 11362e0..c4ac37b 100644 --- a/e2e-sites/nextjs/README.md +++ b/e2e-sites/nextjs/README.md @@ -13,16 +13,15 @@ post-hydration DOM via Chromium DevTools. Next.js injects a hidden `` element with `margin: -1px` (the standard "visually hidden" trick used to stay accessible -without contributing visible layout). The four longhand margin values -are off-grid against `spacing.base_unit = 4` and off-scale against the -configured `spacing.scale`, so they emit 4 grid + 4 scale violations -on top of the hero's intended 4 + 4. `expected.json` documents the -total target counts as `8` + `8` accordingly. +without contributing visible layout). Plumb ignores that framework +infrastructure for spacing rules because the node is hidden, +nondeterministic across Next versions, and not actionable for an agent +fix loop. The target counts below cover the app-owned hero violation. | Rule | Source | Count | | --------------------------- | ------------------------------------------------------- | ----- | -| `spacing/grid-conformance` | `
` + route announcer | 8 | -| `spacing/scale-conformance` | `
` + route announcer | 8 | +| `spacing/grid-conformance` | `
` | 4 | +| `spacing/scale-conformance` | `
` | 4 | | `color/palette-conformance` | `

` | 1 | ## Build diff --git a/e2e-sites/nextjs/expected.json b/e2e-sites/nextjs/expected.json index 8f01738..1a64506 100644 --- a/e2e-sites/nextjs/expected.json +++ b/e2e-sites/nextjs/expected.json @@ -1,5 +1,5 @@ { - "$comment": "Next.js 14 statically exported. The framework injects a hidden `` that uses the visually-hidden `margin: -1px` trick — that contributes 4 grid + 4 scale violations on top of the intentional `p-[13px]` hero. Expected counts are doubled accordingly. The non-target rules `sibling/height-consistency` and `sibling/padding-consistency` also fire on the announcer; they're not asserted on. The `wait_for` block points at a `data-plumb-ready` sentinel server-rendered onto `` in `app/layout.tsx`, which makes the captured DOM byte-stable across Linux/macOS/Windows runs.", + "$comment": "Next.js 14 statically exported. The framework may inject a hidden `` that uses the visually-hidden `margin: -1px` trick; spacing rules ignore that framework infrastructure because it is nondeterministic and unactionable. The `wait_for` block points at a `data-plumb-ready` sentinel server-rendered onto `` in `app/layout.tsx`, which makes the app-owned DOM byte-stable across Linux/macOS/Windows runs.", "target_rules": [ "color/palette-conformance", "spacing/grid-conformance", @@ -7,10 +7,10 @@ ], "by_rule_id": { "color/palette-conformance": 1, - "spacing/grid-conformance": 8, - "spacing/scale-conformance": 8 + "spacing/grid-conformance": 4, + "spacing/scale-conformance": 4 }, - "total_target_violations": 17, + "total_target_violations": 9, "wait_for": { "selector": "html[data-plumb-ready=\"true\"]", "timeout_ms": 30000 diff --git a/scripts/noise-scoreboard.sh b/scripts/noise-scoreboard.sh index 2b55077..5ac729c 100755 --- a/scripts/noise-scoreboard.sh +++ b/scripts/noise-scoreboard.sh @@ -30,26 +30,43 @@ CHROME="${PLUMB_CHROME:-/Applications/Google Chrome.app/Contents/MacOS/Google Ch KITCHEN_SINK="file://$ROOT/e2e-sites/noise-kitchen-sink/dist/index.html" bucket() { # reads JSON on stdin, prints "total" + per-rule counts - python3 - <<'PY' + python3 -c ' import json,sys,collections try: d=json.load(sys.stdin) except Exception: - print(" (no JSON — lint failed)"); sys.exit(0) + print(" (no JSON — lint failed)"); sys.exit(1) s=d.get("summary",{}) -print(f" total={s.get('total','?')} (error={s.get('error',0)} warning={s.get('warning',0)} info={s.get('info',0)})") +print(" total={} (error={} warning={} info={})".format( + s.get("total", "?"), + s.get("error", 0), + s.get("warning", 0), + s.get("info", 0), +)) c=collections.Counter(v["rule_id"] for v in d.get("violations",[])) for k,v in sorted(c.items()): print(f" {v:5d} {k}") -PY +' } lint() { # $1=label $2=url [$3=config-flag...] local label="$1"; shift local url="$1"; shift + local status=0 + local output + output="$(mktemp "${TMPDIR:-/tmp}/plumb-scoreboard.XXXXXX")" rm -rf "${TMPDIR:-/tmp}/chromiumoxide-runner" 2>/dev/null || true echo "#### $label" - "$BIN" lint "$url" --executable-path "$CHROME" --format json "$@" 2>/dev/null | bucket || echo " (lint errored)" + if "$BIN" lint "$url" --executable-path "$CHROME" --format json "$@" >"$output" 2>/dev/null; then + status=0 + else + status=$? + fi + bucket <"$output" || true + rm -f "$output" + if [ "$status" -gt 1 ]; then + echo " (lint errored; exit $status)" + fi echo }