From 90beed6eb100590e0d12ee309ac980a88962e9d3 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:28:40 +0700 Subject: [PATCH 01/22] fix(qa): a named descriptor that is not there is an error `--descriptor ` for a file that does not exist fell through to the same discovery a bare invocation uses, which attaches to the newest descriptor in the temporary directory. On a machine running several suites at once that is another site's host: the connection succeeds, the tree comes back, and the run confidently reports a page nobody asked about. One agent spent a round trip reading a different site's document before noticing. Naming a descriptor is a statement about which host to attach to, so a typo is a failure rather than an invitation to pick one. --- crates/ps-qa/src/inspector.rs | 16 ++++++++++++-- crates/ps-qa/tests/cli.rs | 41 +++++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 2 deletions(-) diff --git a/crates/ps-qa/src/inspector.rs b/crates/ps-qa/src/inspector.rs index 410db42..db19bfc 100644 --- a/crates/ps-qa/src/inspector.rs +++ b/crates/ps-qa/src/inspector.rs @@ -71,12 +71,24 @@ impl Descriptor { /// `--descriptor ` wins. Otherwise the build's own pinned path is tried, /// then the temporary directory is scanned, which is the fallback for a /// hand-launched build and the one that can find a stale instance. +/// +/// A named descriptor that is not there is an error, not an invitation to +/// scan. Falling through to discovery attached to the newest *other* host on +/// the machine, which on a machine running several suites at once is another +/// site's document: the tree came back, it was plausible, and it described a +/// page nobody had asked about. A typo in a path is not consent to inspect +/// somebody else's application. pub fn discover(explicit: Option<&str>) -> Result { if let Some(path) = explicit { let path = PathBuf::from(path); - if path.exists() { - return read_descriptor(&path); + if !path.exists() { + bail!( + "descriptor {} does not exist. ps-qa attaches to the descriptor you \ + name and to no other; omit --descriptor to discover a running host.", + path.display() + ); } + return read_descriptor(&path); } // The delivery script pins this path into the bundle's `Info.plist`, so a diff --git a/crates/ps-qa/tests/cli.rs b/crates/ps-qa/tests/cli.rs index 9622fc5..0dcf46b 100644 --- a/crates/ps-qa/tests/cli.rs +++ b/crates/ps-qa/tests/cli.rs @@ -32,3 +32,44 @@ fn component_sweep_without_profile_returns_a_clear_error() { assert!(stderr.contains("no application profile"), "{stderr}"); assert!(!stderr.contains("panicked at"), "{stderr}"); } + +/// A named descriptor that is not there must not attach to somebody else's host. +/// +/// The failure this pins is silent: discovery would fall through to the newest +/// descriptor in the temporary directory, which on a machine running several +/// suites at once is another site's application. The run then succeeds and +/// reports a tree that is real, plausible, and about the wrong page. +#[test] +fn a_named_descriptor_that_does_not_exist_is_an_error() { + let missing = std::env::temp_dir().join(format!( + "ps-qa-absent-descriptor-{}-{}.json", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock is after the epoch") + .as_nanos() + )); + assert!(!missing.exists(), "the test needs a path that is not there"); + + let output = Command::new(env!("CARGO_BIN_EXE_ps-qa")) + .args([ + "--descriptor", + missing.to_str().expect("a UTF-8 temporary path"), + "dom", + "Save", + ]) + .output() + .expect("run ps-qa"); + + let stderr = String::from_utf8(output.stderr).expect("stderr is UTF-8"); + assert!(!output.status.success(), "{stderr}"); + assert!( + stderr.contains(missing.to_str().expect("a UTF-8 temporary path")), + "the error must name the descriptor that is missing: {stderr}" + ); + assert!( + !stderr.contains("no reachable inspector descriptor found"), + "a named descriptor must not fall through to discovery: {stderr}" + ); + assert!(!stderr.contains("panicked at"), "{stderr}"); +} From 1e9adfa9a8aff866176f69196219669d582c43a1 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:30:35 +0700 Subject: [PATCH 02/22] fix(qa): the selector grammar decides what Contrast and dom look at `Expect::Contrast` and `ps-qa dom` matched with `name.contains(want) || role.contains(want)`, which is not the grammar the rest of the harness reads. A subject spelled `link:crates.vip` therefore matched nothing: no accessible name contains that literal text and no role does either. The audit then reported "no visible painted node matched", which reads as a page whose text does not paint rather than as a selector nobody had implemented, and the same spelling in `dom` came back with zero rows while a check addressed the node perfectly well. Both now go through `selector_matches_node`, so `role:name`, `#id` and `@slot` mean the same thing everywhere. A grammar selector the document cannot answer at all -- a role no node carries, a DOM id or slot nobody declares -- is rejected by name instead of quietly selecting an empty set. A bare word still searches names, roles and values in `dom`, which is the other half of that command's job. --- crates/ps-qa/src/diagnostics.rs | 24 +++++++ crates/ps-qa/src/paint_audit.rs | 19 +++++- crates/ps-qa/src/target.rs | 115 ++++++++++++++++++++++++++++++++ 3 files changed, 157 insertions(+), 1 deletion(-) diff --git a/crates/ps-qa/src/diagnostics.rs b/crates/ps-qa/src/diagnostics.rs index fc60ddc..f1ebe17 100644 --- a/crates/ps-qa/src/diagnostics.rs +++ b/crates/ps-qa/src/diagnostics.rs @@ -8,6 +8,7 @@ use blitz_control_protocol::{ use eyre::{Result, bail}; use crate::inspector::{Client, inspect}; +use crate::target::{selector_matches_node, unmatched_selector_reason}; use crate::{reach, report}; pub(crate) async fn metrics(client: &mut Client) -> Result { @@ -477,10 +478,33 @@ pub(crate) async fn dom(client: &mut Client, want: &str, depth: usize) -> Result ) }; + /* + * The selector grammar first, free text second. + * + * `dom` is the tool an author reaches for when a check cannot find its + * subject, so it has to answer the same question the check asked. Matching + * only `name.contains || role.contains` meant a subject spelled + * `role:name`, `#id` or `@slot` reported zero matches here while the same + * spelling addressed a node perfectly well in a check, which sends the + * reader looking for a missing element instead of at their selector. + * + * Free-text substring search over name, role and value is still what a + * bare word does: exploring a document you do not know yet is the other + * half of this command's job, and a selector is not always what you have. + */ + if let Some(reason) = unmatched_selector_reason(&snapshot.nodes, want) { + bail!("{want:?} selects nothing in this document: {reason}"); + } + let grammar = want.starts_with('#') + || want.starts_with('@') + || crate::target::selector_role(want).is_some(); let matched: Vec<&SemanticNode> = snapshot .nodes .iter() .filter(|node| { + if grammar { + return selector_matches_node(node, want); + } node.name.contains(want) || node.role.contains(want) || node.value.as_deref().is_some_and(|v| v.contains(want)) diff --git a/crates/ps-qa/src/paint_audit.rs b/crates/ps-qa/src/paint_audit.rs index b86550b..b2662fd 100644 --- a/crates/ps-qa/src/paint_audit.rs +++ b/crates/ps-qa/src/paint_audit.rs @@ -10,6 +10,7 @@ use eyre::{Result, bail}; use crate::inspector::{Client, inspect}; use crate::paint_color::{Rgba, composite, contrast_ratio, luminance, parse}; +use crate::target::{selector_matches_node, unmatched_selector_reason}; /// Print the resolved foreground, background, opacity, and visibility of /// matching painted boxes, largest first. @@ -125,6 +126,12 @@ pub(crate) async fn contrast( control_ratio: f64, ) -> Result<()> { let (semantic, elapsed) = inspect(client).await?; + // Reject a selector this tree cannot answer before measuring anything. + // Silently auditing nothing and calling it a clean page is the worst of the + // three available outcomes. + if let Some(reason) = unmatched_selector_reason(&semantic.nodes, want) { + bail!("{want:?} selects nothing in this document: {reason}"); + } let answer = client .diagnostics(&DiagnosticsRequest::Snapshot(SnapshotRequest { include_dom: false, @@ -287,10 +294,20 @@ fn computed_styles(value: Option<&serde_json::Value>) -> HashMap bool { !node.name.is_empty() && (node.enabled || !is_interactive(&node.role)) - && (want.is_empty() || node.name.contains(want) || node.role.contains(want)) + && (want.is_empty() || selector_matches_node(node, want)) && node.visible && node .bounds diff --git a/crates/ps-qa/src/target.rs b/crates/ps-qa/src/target.rs index ee81c4e..f229de0 100644 --- a/crates/ps-qa/src/target.rs +++ b/crates/ps-qa/src/target.rs @@ -361,6 +361,64 @@ pub(crate) fn selector_matches_node(node: &SemanticNode, selector: &str) -> bool name_matches(&node.name, selector) } +/// The role half of a `role:name` selector, when the selector has one. +/// +/// A role is a single token, which is what separates the selector grammar from +/// an accessible name that happens to contain a colon: `link:crates.vip` names +/// a role, `Default permission: Auto` does not. +pub(crate) fn selector_role(selector: &str) -> Option<&str> { + if selector.starts_with('#') || selector.starts_with('@') { + return None; + } + let (role, name) = selector.split_once(':')?; + (!role.is_empty() && !name.is_empty() && !role.contains(char::is_whitespace)).then_some(role) +} + +/// Why a selector this tree cannot answer was not understood. +/// +/// Only for the grammar. A plain name that matches nothing is an ordinary +/// negative result and the caller says so in its own words; a `role:name`, +/// `#id` or `@slot` whose grammatical half does not exist in the tree at all is +/// a selector the tool did not understand, and reporting that as "nothing +/// matched" is how `Contrast` on `link:crates.vip` came back as a page with no +/// painted text on it rather than as a selector nobody had implemented. +pub(crate) fn unmatched_selector_reason(nodes: &[SemanticNode], selector: &str) -> Option { + if selector.is_empty() + || nodes + .iter() + .any(|node| selector_matches_node(node, selector)) + { + return None; + } + if let Some(dom_id) = selector_dom_id(selector) { + return Some(format!( + "no node carries the DOM id {dom_id:?}; drop the leading '#' to match an \ + accessible name instead" + )); + } + if let Some(slot) = selector_slot(selector) { + return Some(format!( + "no node carries the component slot {slot:?}; drop the leading '@' to match an \ + accessible name instead" + )); + } + let role = selector_role(selector)?; + if nodes + .iter() + .any(|node| node.role.eq_ignore_ascii_case(role)) + { + return None; + } + let mut present: Vec<&str> = nodes.iter().map(|node| node.role.as_str()).collect(); + present.sort_unstable(); + present.dedup(); + present.truncate(12); + Some(format!( + "no node has role {role:?}, so {selector:?} can never match; roles present: {}", + present.join(", ") + )) +} + pub(crate) fn exact_selector_matches_node(node: &SemanticNode, selector: &str) -> bool { if let Some(dom_id) = selector_dom_id(selector) { return node.dom_id.as_deref() == Some(dom_id); @@ -440,6 +498,63 @@ mod tests { assert!(selector_matches_node(&save, "save")); } + /// The audit's old predicate is written out here because the point is that + /// it *passes* the document it should have rejected. + fn contains_either(node: &SemanticNode, want: &str) -> bool { + node.name.contains(want) || node.role.contains(want) + } + + #[test] + fn a_role_selector_matched_nothing_under_a_substring_predicate() { + let link = SemanticNode { + role: "link".into(), + ..node(None, "crates.vip") + }; + let heading = SemanticNode { + role: "heading".into(), + ..node(None, "A private registry") + }; + let page = [link.clone(), heading.clone()]; + + // The false clean bill of health: nothing is audited, so nothing fails. + assert!( + !page + .iter() + .any(|node| contains_either(node, "link:crates.vip")), + "the substring predicate silently selects an empty page" + ); + // The grammar every check is written in selects the one link. + assert!(selector_matches_node(&link, "link:crates.vip")); + assert!(!selector_matches_node(&heading, "link:crates.vip")); + assert!(unmatched_selector_reason(&page, "link:crates.vip").is_none()); + } + + #[test] + fn a_grammar_selector_the_tree_cannot_answer_is_reported_as_such() { + let page = [node(Some("save"), "Save settings")]; + + let role = unmatched_selector_reason(&page, "link:crates.vip") + .expect("this document has no links at all"); + assert!(role.contains("role \"link\""), "{role}"); + assert!( + role.contains("button"), + "the roles present are named: {role}" + ); + + let dom_id = + unmatched_selector_reason(&page, "#missing").expect("no node carries that DOM id"); + assert!(dom_id.contains("DOM id"), "{dom_id}"); + + let slot = unmatched_selector_reason(&page, "@listbox").expect("no node carries that slot"); + assert!(slot.contains("slot"), "{slot}"); + + // A plain name that matches nothing is an ordinary negative result, and + // so is a role selector whose role exists but whose name does not. + assert!(unmatched_selector_reason(&page, "Nothing here").is_none()); + assert!(unmatched_selector_reason(&page, "button:Nothing here").is_none()); + assert!(unmatched_selector_reason(&page, "Default permission: Auto").is_none()); + } + #[test] fn a_colon_in_a_bare_name_is_not_misread_as_a_role() { let permission = node(None, "Default permission: Auto"); From 7737f721ad6bb5f302d3b6b481c56f8d4fe38baf Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:32:30 +0700 Subject: [PATCH 03/22] fix(qa): a control parked outside the window is not on screen `Vanishes` filtered its matches on `visible && paints`, and a box is not a place on the screen. The responsive pattern of one desktop control plus a narrow duplicate parked outside the window therefore defeated it completely: the duplicate reports visible, keeps a full-size box, and sits at a negative x, so every dialog with a hidden mirror copy was reported as "still on screen; it did not close" no matter how correctly it closed. The expectation was unusable on any site built that way. A match now also has to be somewhere `offscreen` says a press could land, which is the predicate the driving side already uses to decide the same question. The failure message carries the position, so the next reader can see which copy answered. --- crates/ps-qa/src/qa.rs | 84 ++++++++++++++++++++++++++++++++++++-- crates/ps-qa/src/target.rs | 19 +++++++-- 2 files changed, 96 insertions(+), 7 deletions(-) diff --git a/crates/ps-qa/src/qa.rs b/crates/ps-qa/src/qa.rs index 9219829..71dafd2 100644 --- a/crates/ps-qa/src/qa.rs +++ b/crates/ps-qa/src/qa.rs @@ -716,16 +716,40 @@ pub fn verdict( * would reintroduce the false negative. Only this arm, which asks * whether something went away, needs to hear that it did. */ + /* + * And a position, because a box is not a place on the screen. + * + * A responsive composition routinely carries two copies of the same + * control: the desktop one, and a narrow duplicate the layout parks + * outside the window rather than removing. The parked copy is + * `visible`, has a full-size box, and is at x=-1180. Judging it by + * geometry alone reported every dialog with such a mirror as "still + * on screen; it did not close" no matter how correctly it closed, + * which made this expectation unusable for the whole pattern. + * + * `offscreen` is the predicate the driving side already uses to + * decide that pressing a control would land on nothing. A control + * nothing can be aimed at is not on screen for this verdict either. + */ let on_screen: Vec<&SemanticNode> = found .iter() .copied() - .filter(|node| node.visible && paints(node)) + .filter(|node| { + node.visible + && paints(node) + && !node.bounds.is_some_and(|bounds| { + crate::target::offscreen( + bounds, + crate::target::viewport_for_node_in(after, node.id), + ) + }) + }) .collect(); if let Some(node) = on_screen.first() { let b = node.bounds.unwrap_or([0.0; 4]); return Err(format!( - "{:?} is still on screen at {:.0}x{:.0}; it did not close", - check.subject, b[2], b[3] + "{:?} is still on screen at {:.0}x{:.0} at {:.0},{:.0}; it did not close", + check.subject, b[2], b[3], b[0], b[1] )); } } @@ -1460,6 +1484,60 @@ mod tests { } } + /// A hidden mirror copy must not keep a closed control "on screen". + /// + /// The responsive pattern is one desktop control plus a narrow duplicate + /// the layout parks outside the window. The duplicate is `visible`, keeps a + /// full-size box, and sits at a negative x. Judging `Vanishes` on geometry + /// and the flag alone reported the dialog as never closing, whatever the + /// component did, so the expectation could not be used at all on any site + /// built that way. + #[test] + fn a_parked_duplicate_is_not_a_control_that_stayed_open() { + let mut check = parse(""); + check.click = None; + check.expect = Expect::Vanishes; + check.subject = "Delete project".into(); + + let main = SemanticNode { + role: "main".into(), + bounds: Some([0.0, 58.0, 1280.0, 842.0]), + ..painted_node(1, "", 1280.0, 842.0) + }; + let closed = SemanticNode { + parent: Some(1), + visible: false, + bounds: Some([200.0, 200.0, 420.0, 180.0]), + ..painted_node(2, "Delete project", 420.0, 180.0) + }; + // The narrow duplicate: on the wire it is indistinguishable from the + // desktop dialog except for where the layout put it. + let parked = SemanticNode { + parent: Some(1), + bounds: Some([-1180.0, 200.0, 420.0, 180.0]), + ..painted_node(3, "Delete project", 420.0, 180.0) + }; + + assert!( + verdict(&check, &[], &[main.clone(), closed.clone(), parked.clone()]).is_ok(), + "a control nothing can be aimed at is not on screen" + ); + + // The expectation still has teeth: the same dialog inside the window is + // a dialog that did not close. + let open = SemanticNode { + bounds: Some([200.0, 200.0, 420.0, 180.0]), + ..parked + }; + let error = verdict(&check, &[], &[main, closed, open]) + .expect_err("a dialog in the window did not close"); + assert!(error.contains("did not close"), "{error}"); + assert!( + error.contains("at 200,200"), + "the position is reported: {error}" + ); + } + #[test] fn measures_rejects_any_matching_node_that_breaks_the_contract() { let mut check = parse(""); diff --git a/crates/ps-qa/src/target.rs b/crates/ps-qa/src/target.rs index f229de0..433031c 100644 --- a/crates/ps-qa/src/target.rs +++ b/crates/ps-qa/src/target.rs @@ -82,6 +82,14 @@ pub(crate) fn name_matches(name: &str, pattern: &str) -> bool { /// `cover` already read it this way; `open_named` and `press_named` did not, /// which is the bug the two helpers below exist to close. pub(crate) fn viewport_of(snapshot: &AgentSnapshot) -> (f64, f64) { + viewport_of_nodes(&snapshot.nodes) +} + +/// The same viewport, for callers that hold a tree rather than a snapshot. +/// +/// A verdict is judged from two node lists, so the geometry rules a verdict +/// needs cannot be reachable only through the transport type. +pub(crate) fn viewport_of_nodes(nodes: &[SemanticNode]) -> (f64, f64) { /* * The window, not `main`. * @@ -92,8 +100,7 @@ pub(crate) fn viewport_of(snapshot: &AgentSnapshot) -> (f64, f64) { * Taking the top of the window keeps the below-the-fold case, which is what * this bound is actually for, without swallowing the header. */ - let bottom = snapshot - .nodes + let bottom = nodes .iter() .filter(|node| node.role == "main") .filter_map(|node| node.bounds) @@ -109,10 +116,14 @@ pub(crate) fn viewport_of(snapshot: &AgentSnapshot) -> (f64, f64) { /// scroll coordinates as window-visible sends pointer events behind the tab /// strip instead of revealing the row inside its panel. pub(crate) fn viewport_for_node(snapshot: &AgentSnapshot, node_id: u64) -> (f64, f64) { + viewport_for_node_in(&snapshot.nodes, node_id) +} + +pub(crate) fn viewport_for_node_in(nodes: &[SemanticNode], node_id: u64) -> (f64, f64) { let mut cursor = Some(node_id); for _ in 0..32 { let Some(id) = cursor else { break }; - let Some(node) = snapshot.nodes.iter().find(|node| node.id == id) else { + let Some(node) = nodes.iter().find(|node| node.id == id) else { break; }; if node.role == "main" @@ -122,7 +133,7 @@ pub(crate) fn viewport_for_node(snapshot: &AgentSnapshot, node_id: u64) -> (f64, } cursor = node.parent; } - viewport_of(snapshot) + viewport_of_nodes(nodes) } /// Whether a node's box lies outside the window, so pressing it would land on From 335db253e2c15d2728cb006c205de529424447a8 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:35:04 +0700 Subject: [PATCH 04/22] feat(qa): a check can require the visibility flag as well as a box `Paints`, `PaintsNamed`, `PaintsMore` and `Count` judge on geometry, and for good reason: the semantic tree's `visible` walks ancestors for `display:none` and `aria-hidden`, disagrees with what the renderer drew, and trusting it once reported a screen full of icons as painting nothing. The cost of that is a whole class of component nothing could assert. An Accordion, a Collapsible or a Tabs panel hides by flipping `hidden` while keeping its box, so every geometry assertion is satisfied by the closed panel exactly as it is by the open one. `Vanishes` reads the flag and can prove such a thing closed; nothing could prove it open, which is why disclosure coverage stopped where it did on every site that has one. `require_visible` is that question, opt-in per check so the default keeps the geometry semantics every existing check was written against. The flag is already on the wire in `SemanticNode`, so this needs nothing from the runtime. --- crates/ps-qa/src/qa.rs | 127 +++++++++++++++++++++++++++++++++++-- crates/ps-qa/src/runner.rs | 1 + 2 files changed, 122 insertions(+), 6 deletions(-) diff --git a/crates/ps-qa/src/qa.rs b/crates/ps-qa/src/qa.rs index 71dafd2..1921663 100644 --- a/crates/ps-qa/src/qa.rs +++ b/crates/ps-qa/src/qa.rs @@ -522,6 +522,32 @@ pub struct Check { /// be reported as a fast completed render. #[serde(default)] pub stable_for_ms: u64, + /// Also require the semantic tree's visibility flag, not geometry alone. + /// + /// [`Paints`](Expect::Paints), [`PaintsNamed`](Expect::PaintsNamed), + /// [`PaintsMore`](Expect::PaintsMore) and [`Count`](Expect::Count) judge on + /// boxes, for the reason written on the `paints` predicate: the flag and + /// the renderer disagree, and trusting the flag once reported a screen full + /// of icons as painting nothing. + /// + /// That leaves a whole class of component unprovable in one direction. An + /// Accordion, a Collapsible, a Tabs panel and anything else that hides by + /// flipping `hidden` while keeping its box satisfies every geometry + /// assertion whether it is open or closed. [`Vanishes`](Expect::Vanishes) + /// reads the flag and so can prove such a thing closed; nothing could prove + /// it open, which caps what a suite can assert about disclosure on every + /// site that has one. + /// + /// So this is opt-in and per check. A check that declares it is saying "for + /// this subject the flag is the honest signal", which is true exactly where + /// the box does not move. Leave it off for anything with a box of its own: + /// there the geometry question is stronger and it does not depend on a flag + /// that walks ancestors. + /// + /// With `Count`, only members that are shown are counted, which is what + /// makes an exact count of open panels expressible at all. + #[serde(default)] + pub require_visible: bool, /// Run this check only after every ordinary shared-instance outcome. /// /// A destructive sequence may deliberately remove fixture state that @@ -693,6 +719,15 @@ fn paints(node: &SemanticNode) -> bool { node.bounds.is_some_and(|b| b[2] > 0.0 && b[3] > 0.0) } +/// Whether a node counts as shown, under this check's declared strictness. +/// +/// Geometry is the default and stays the default for the reason written on +/// [`paints`]. [`Check::require_visible`] is how a check that needs the flag as +/// well says so, for the components geometry alone cannot judge. +fn shows(check: &Check, node: &SemanticNode) -> bool { + paints(node) && (!check.require_visible || node.visible) +} + /// The verdict for one check, given the tree before and after its action. pub fn verdict( check: &Check, @@ -757,7 +792,11 @@ pub fn verdict( if found.is_empty() { return Err(format!("no node matching {:?} exists", check.subject)); } - let broken: Vec<_> = found.iter().copied().filter(|node| !paints(node)).collect(); + let broken: Vec<_> = found + .iter() + .copied() + .filter(|node| !shows(check, node)) + .collect(); if !broken.is_empty() { /* * Say which half of "paints" failed. @@ -887,7 +926,7 @@ pub fn verdict( } } Expect::PaintsNamed => { - if !found.iter().any(|node| paints(node)) { + if !found.iter().any(|node| shows(check, node)) { let state = found .iter() .map(|node| { @@ -1178,9 +1217,9 @@ pub fn verdict( Expect::PaintsMore => { let was = matching(before, &check.subject) .into_iter() - .filter(|node| paints(node)) + .filter(|node| shows(check, node)) .count(); - let now = found.iter().filter(|node| paints(node)).count(); + let now = found.iter().filter(|node| shows(check, node)).count(); if now <= was { return Err(format!( "{:?} on screen went {was} -> {now}, expected one more", @@ -1236,11 +1275,19 @@ pub fn verdict( let want = check .expect_count .ok_or_else(|| "Count requires expect_count".to_owned())?; - if found.len() != want { + // Tree membership by default, so an exact family size keeps + // counting the members a retained pane legitimately holds. A check + // that declared `require_visible` is asking about the ones a person + // can see, and counts only those. + let counted: Vec<&&SemanticNode> = found + .iter() + .filter(|node| !check.require_visible || shows(check, node)) + .collect(); + if counted.len() != want { return Err(format!( "{:?} has {} member(s), expected {want}", check.subject, - found.len() + counted.len() )); } } @@ -1484,6 +1531,73 @@ mod tests { } } + /// A disclosure that keeps its box can be proven closed but never open. + /// + /// This is the false pass: the geometry assertion is satisfied by the + /// *closed* panel, so it says nothing at all about the control it names. No + /// spelling available before `require_visible` could tell the two states + /// apart, which is why disclosure coverage stopped at "it closes". + #[test] + fn a_panel_that_hides_by_flag_alone_needs_the_flag_to_be_read() { + let panel = |visible: bool| SemanticNode { + visible, + bounds: Some([0.0, 120.0, 400.0, 200.0]), + ..painted_node(2, "Advanced options", 400.0, 200.0) + }; + let closed = [ + painted_node(1, "Advanced options", 180.0, 30.0), + panel(false), + ]; + let open = [ + painted_node(1, "Advanced options", 180.0, 30.0), + panel(true), + ]; + + let mut geometry = parse(""); + geometry.click = Some("Advanced options".into()); + geometry.subject = "Advanced options".into(); + geometry.expect = Expect::Paints; + assert!( + verdict(&geometry, &closed, &closed).is_ok(), + "the closed panel already satisfies a geometry-only assertion" + ); + + let mut strict = geometry.clone(); + strict.require_visible = true; + assert!( + verdict(&strict, &closed, &closed).is_err(), + "reading the flag distinguishes closed from open" + ); + assert!(verdict(&strict, &closed, &open).is_ok()); + + // The same is true of the family assertions: nothing about the boxes + // moves when the panel opens. + let mut more = strict.clone(); + more.expect = Expect::PaintsMore; + more.require_visible = false; + assert!( + verdict(&more, &closed, &open).is_err(), + "counting boxes cannot see a disclosure open" + ); + more.require_visible = true; + assert!(verdict(&more, &closed, &open).is_ok()); + + let mut count = strict.clone(); + count.expect = Expect::Count; + count.expect_count = Some(2); + count.require_visible = false; + assert!( + verdict(&count, &closed, &closed).is_ok(), + "counting tree membership cannot see a disclosure closed either" + ); + count.require_visible = true; + assert!( + verdict(&count, &closed, &closed).is_err(), + "only the trigger is on screen while the panel is closed" + ); + assert!(verdict(&count, &closed, &open).is_ok()); + } + /// A hidden mirror copy must not keep a closed control "on screen". /// /// The responsive pattern is one desktop control plus a narrow duplicate @@ -1811,6 +1925,7 @@ mod tests { settle_after_ms: 0, outcome_timeout_ms: 0, stable_for_ms: 0, + require_visible: false, destructive: false, subject: "Output level".into(), expect: Expect::ValueChanges, diff --git a/crates/ps-qa/src/runner.rs b/crates/ps-qa/src/runner.rs index fd53efe..60f8c21 100644 --- a/crates/ps-qa/src/runner.rs +++ b/crates/ps-qa/src/runner.rs @@ -6345,6 +6345,7 @@ mod tests { settle_after_ms: 0, outcome_timeout_ms: 0, stable_for_ms: 0, + require_visible: false, destructive: false, subject: subject.into(), expect: Expect::Paints, From f8759cbad7ddd245c225ce6d506fc96ac5cb6a91 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:36:50 +0700 Subject: [PATCH 05/22] fix(qa): a setup value must reach the tree before the baseline does `setup_type_into` sends `SetValue` and moves on. The acknowledgement says the runtime applied it; the semantic tree the baseline reads is a separate observation, and a baseline taken in between records the pre-setup value. The difference between that baseline and the tree after the action is then the harness's own typing, so a `ValueChanges` on the setup field passes whether or not the measured action did anything. The test writes that false pass out: a click that changes nothing, and an `Ok` verdict. The setup step now waits for the value to appear on the exact node `SetValue` addressed, and says so plainly if it never does. The degenerate form of the same check is rejected at load: a change expectation on the field its own setup typed into, with no action declared at all, has nothing between its two observations but the setup and cannot report anything about the application. --- crates/ps-qa/src/qa.rs | 51 ++++++++++++++++++ crates/ps-qa/src/runner.rs | 103 ++++++++++++++++++++++++++++++++++--- 2 files changed, 148 insertions(+), 6 deletions(-) diff --git a/crates/ps-qa/src/qa.rs b/crates/ps-qa/src/qa.rs index 1921663..9def3b2 100644 --- a/crates/ps-qa/src/qa.rs +++ b/crates/ps-qa/src/qa.rs @@ -670,6 +670,39 @@ fn validate_check( )); } + /* + * A setup value cannot be the thing a check proves. + * + * `setup_type_into` establishes a precondition before the baseline, so the + * only honest reading of a change expectation on that same field is "the + * measured action changed what setup put there". With no measured action + * there is nothing between the two observations but the harness's own + * typing: the check either compares the setup value with itself, or, if + * the baseline wins the race with the runtime, reports the setup's own + * effect as the outcome. Neither says anything about the application. + */ + if matches!( + check.expect, + Expect::ValueChanges | Expect::NameChanges | Expect::SelectionChanges + ) && check.setup_type_into.as_deref() == Some(check.subject.as_str()) + && check.click.is_none() + && check.text.is_none() + && check.key.is_none() + && check.scroll_over.is_none() + { + return Err(format!( + concat!( + "{}: check {:?} asserts {:?} on {:?}, which is the field its own setup value ", + "was typed into, and drives no action; declare the action that is supposed to ", + "change it, or assert the value with a different subject" + ), + file.display(), + check.id, + check.expect, + check.subject, + )); + } + if check.scroll_over.is_some() && (check.scroll_ticks == 0 || check.scroll_delta == 0.0) { return Err(format!( "{}: check {:?} must declare non-zero scroll_ticks and scroll_delta with scroll_over", @@ -1879,6 +1912,24 @@ mod tests { ); } + #[test] + fn a_change_expectation_on_the_setup_field_needs_an_action() { + let mut check = parse("setup_type_into:Some(\"Filter\"),setup_text:Some(\"acme\"),"); + check.click = None; + check.subject = "Filter".into(); + check.expect = Expect::ValueChanges; + + let error = validate_check(&check, Path::new("filter.ron"), &mut HashMap::new()) + .expect_err("nothing but the harness's own typing happens between the observations"); + assert!(error.contains("setup value"), "{error}"); + + // The ordinary shape is still valid: setup establishes the value and a + // declared action is what has to change it. + check.click = Some("Clear".into()); + validate_check(&check, Path::new("filter.ron"), &mut HashMap::new()) + .expect("an action between the two observations is the point"); + } + #[test] fn count_checks_require_an_exact_count() { let mut check = parse(""); diff --git a/crates/ps-qa/src/runner.rs b/crates/ps-qa/src/runner.rs index 60f8c21..edc6322 100644 --- a/crates/ps-qa/src/runner.rs +++ b/crates/ps-qa/src/runner.rs @@ -242,6 +242,17 @@ async fn wait_for_semantic_condition( } } +/// Whether the value a setup step sent has reached the semantic tree. +/// +/// By the id `SetValue` addressed, and by exact equality: `SetValue` replaces +/// the field's contents with this string, so anything else is either a +/// different node with the same name or a value that has not landed yet. +fn setup_value_landed(nodes: &[SemanticNode], node_id: u64, value: &str) -> bool { + nodes + .iter() + .any(|node| node.id == node_id && node.value.as_deref() == Some(value)) +} + async fn settle_sweep_case( client: &mut Client, case: &sweep::Case, @@ -1220,11 +1231,45 @@ async fn run_qa( check.setup_type_into.as_deref(), check.setup_text.as_deref(), ) - && let Err(error) = type_text(client, field, value).await { - open_error = Some(format!( - "could not establish setup value in {field:?}: {error}" - )); + match type_text(client, field, value).await { + Err(error) => { + open_error = Some(format!( + "could not establish setup value in {field:?}: {error}" + )); + } + /* + * Wait for the value to reach the tree, not merely for the + * runtime to acknowledge being told. + * + * `SetValue` is acknowledged when it is applied, and the + * semantic tree the baseline reads is a separate observation. + * A baseline taken in between records the *pre-setup* value, + * and then the setup's own effect is a change between before + * and after: a `ValueChanges` on that field passes without the + * measured action having done anything at all. That is a check + * reporting a feature as working because the harness typed + * into it. + * + * The value is compared against the id `SetValue` addressed, + * so a same-named neighbour cannot answer for it. + */ + Ok(node_id) => { + let settled = + wait_for_semantic_condition(client, check_timeout(900), |nodes| { + setup_value_landed(nodes, node_id, value) + }) + .await?; + if !setup_value_landed(&settled.nodes, node_id, value) { + open_error = Some(format!( + "the setup value {value:?} had not reached {field:?} within {}ms; \ + a baseline taken before it lands lets the setup satisfy the \ + check's own outcome", + check_timeout(900).as_millis() + )); + } + } + } } /* @@ -5764,8 +5809,8 @@ mod tests { named_document_is_active_with_permanent, named_document_opener_for, outcome_check_ids, outcome_verdict, pagination_advanced, painted_bounds, painted_named, pixels_change, pixels_hold, require_transparent_window_tint, resolved_action_target, rgb_pixels_hold, - saved_control_node, saved_controls, selector_matches_node, stable_arrival, - subject_belongs_to_scope, validate_surface_filter_against, + saved_control_node, saved_controls, selector_matches_node, setup_value_landed, + stable_arrival, subject_belongs_to_scope, validate_surface_filter_against, }; use crate::app::{AppProfile, SurfaceSpec}; use crate::interaction::parse_key_chord; @@ -6313,6 +6358,52 @@ mod tests { )); } + /// A baseline that predates the setup value lets the setup pass the check. + /// + /// This is the shape the harness has to make impossible: the `before` + /// snapshot still holds the pre-setup value because the tree had not caught + /// up with `SetValue`, so the difference between the two observations is + /// the harness's own typing and the measured action is never consulted. + /// The verdict below is `Ok` with a click that did nothing. + #[test] + fn a_baseline_that_predates_the_setup_makes_its_outcome_vacuous() { + let field = |value: &str| SemanticNode { + dom_id: None, + id: 7, + parent: None, + role: "textbox".into(), + name: "Filter".into(), + value: Some(value.into()), + enabled: true, + visible: true, + selected: false, + bounds: Some([0.0, 0.0, 240.0, 28.0]), + slot: None, + }; + let mut check = check("filter-value", Some("Apply"), "Filter"); + check.setup_type_into = Some("Filter".into()); + check.setup_text = Some("acme".into()); + check.expect = Expect::ValueChanges; + + let raced = [field("")]; + let settled = [field("acme")]; + assert!( + crate::qa::verdict(&check, &raced, &settled).is_ok(), + "the setup's own effect satisfies the outcome when the baseline predates it" + ); + + // The gate: that baseline is not one the harness may take, and the + // settled tree is. + assert!(!setup_value_landed(&raced, 7, "acme")); + assert!(setup_value_landed(&settled, 7, "acme")); + // By id, so a same-named neighbour cannot answer for the field. + assert!(!setup_value_landed(&settled, 8, "acme")); + + // Once the baseline is honest the check measures the action, and a + // click that changed nothing fails. + assert!(crate::qa::verdict(&check, &settled, &settled).is_err()); + } + fn check(id: &str, click: Option<&str>, subject: &str) -> Check { Check { id: id.into(), From 8d38f04576db880bec9cc9e87efd040cc1c82829 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:40:39 +0700 Subject: [PATCH 06/22] fix(qa): stop reporting a declared budget as a measured latency Between the action and judging it, the runner waited for a committed frame for the whole of the check's declared outcome budget, described as the cheapest happens-after boundary. A host with no compositor never commits one, and neither does a valid action that changes no pixels, so that wait ran to its deadline and the check's reported duration became its own budget: 211ms against a declared 200, 3004ms against 3000. Read as latency those numbers say a control is on the edge of its deadline. They are a constant, and the outcome had usually arrived in a few milliseconds. `settle_for_outcome` already inspects before it waits and waits on the same paint stream against the same deadline, so the boundary is not lost by removing the blocking one in front of it. The arming stays, and is handed on so a frame committed between the action and the settle loop is not discarded by a second arm. The test drives a host that serves the paint stream and never commits: the old boundary spends the entire budget against it, the path the runner now takes returns as soon as the tree answers. --- crates/ps-qa/src/runner.rs | 165 +++++++++++++++++++++++++++++++++++-- 1 file changed, 156 insertions(+), 9 deletions(-) diff --git a/crates/ps-qa/src/runner.rs b/crates/ps-qa/src/runner.rs index edc6322..de1d980 100644 --- a/crates/ps-qa/src/runner.rs +++ b/crates/ps-qa/src/runner.rs @@ -1938,12 +1938,26 @@ async fn run_qa( } } } - if action_error.is_none() && action_paint_armed { - // A real committed frame is the cheapest happens-after boundary. - // Some valid actions change no pixels, so absence is a bounded - // compatibility fallback rather than a verdict by itself. - let _ = client.wait_for_paint(declared_outcome_timeout(check)).await; - } + /* + * No blocking wait for a frame here. + * + * This used to wait the whole declared budget for a committed paint as + * a "cheapest happens-after boundary". A host with no compositor never + * commits one, and neither does a valid action that changes no pixels, + * so the wait ran to the deadline and the check's reported duration + * became its own budget: 211ms against a declared 200, 3004ms against + * 3000. Read as latency those numbers say a control is on the edge of + * its deadline. They are a constant, and the outcome had usually + * arrived within a few milliseconds. + * + * Nothing is lost by dropping it. `settle_for_outcome` inspects before + * it waits and then waits on the same paint stream against the same + * deadline, so a frame that matters is still waited for and one that + * never comes no longer decides what the harness reports as a + * measurement. The arming above stays, and is handed on below so a + * frame committed between the action and the settle loop is not + * discarded by a second arm. + */ let transport_timed_out = action_error .as_deref() .is_some_and(|error| error.contains("inspector did not answer within")); @@ -1999,6 +2013,7 @@ async fn run_qa( &before.nodes, action_target.as_deref(), action_node_id, + action_paint_armed, ) .await { @@ -2344,6 +2359,7 @@ async fn settle_for_outcome( before: &[SemanticNode], action_target: Option<&str>, action_node_id: Option, + already_armed: bool, ) -> Result<(AgentSnapshot, Option, u32)> { let outcome_timeout = declared_outcome_timeout(check); let deadline = tokio::time::Instant::now() + outcome_timeout; @@ -2354,7 +2370,14 @@ async fn settle_for_outcome( // When the whole tree was last serialised, so a scoped poll cannot go // permanently blind to the rest of the page. See the interval below. let mut last_full_probe: Option = None; - let event_driven = client.arm_paint_events().await.unwrap_or(false); + // Arming discards the frames already queued, which is right before an + // action and wrong after one: the commit this loop is waiting for may + // already have arrived. A caller that armed before driving says so. + let event_driven = if already_armed { + true + } else { + client.arm_paint_events().await.unwrap_or(false) + }; loop { let mut after = if let Some(scoped) = scope.as_ref() { match inspect_subtree(client, scoped.root).await { @@ -5809,8 +5832,9 @@ mod tests { named_document_is_active_with_permanent, named_document_opener_for, outcome_check_ids, outcome_verdict, pagination_advanced, painted_bounds, painted_named, pixels_change, pixels_hold, require_transparent_window_tint, resolved_action_target, rgb_pixels_hold, - saved_control_node, saved_controls, selector_matches_node, setup_value_landed, - stable_arrival, subject_belongs_to_scope, validate_surface_filter_against, + saved_control_node, saved_controls, selector_matches_node, settle_for_outcome, + setup_value_landed, stable_arrival, subject_belongs_to_scope, + validate_surface_filter_against, }; use crate::app::{AppProfile, SurfaceSpec}; use crate::interaction::parse_key_chord; @@ -6358,6 +6382,129 @@ mod tests { )); } + /// A host with a document and no compositor, which never commits a frame. + /// + /// That is not a contrived shape: it is exactly what the headless host is, + /// and every wait for a paint event against it runs to its deadline. + async fn serve_silent_host(socket: std::path::PathBuf, nodes: Vec) { + use blitz_control_protocol::{ + DebugResponse, IncomingRequest, MessageStream, TransportStream, decode_incoming, + encode_initialize_response, encode_response, framed_json, + }; + use tokio::net::UnixListener; + + let listener = UnixListener::bind(&socket).expect("bind test socket"); + let (stream, _) = listener.accept().await.expect("accept test client"); + let mut stream = TransportStream::new(framed_json(stream)); + while let Some(Ok(message)) = stream.recv().await { + let answer = match decode_incoming(message) { + Ok(IncomingRequest::Initialize { id }) => { + encode_initialize_response(id, "silent-host-fixture") + .expect("encode initialize response") + } + Ok(IncomingRequest::Agent { id, .. }) => encode_response( + id, + &DebugResponse::AgentSnapshot(AgentSnapshot { + nodes: nodes.clone(), + ..AgentSnapshot::default() + }), + ) + .expect("encode snapshot"), + // Streaming is available. Frames are not, because nothing here + // draws. + Ok(IncomingRequest::Diagnostics { id, .. }) => { + encode_response(id, &DebugResponse::Ack).expect("encode ack") + } + _ => continue, + }; + if stream.send(answer).await.is_err() { + return; + } + } + } + + /// The post-action wait for a frame reported the budget as a latency. + /// + /// The runner used to block on `wait_for_paint(declared_outcome_timeout)` + /// between the action and judging it. Against a host that commits no frame + /// that call always costs the whole declared budget, and the check's + /// reported duration became its own deadline: 211ms against a declared 200, + /// 3004ms against 3000. The first half of this test is that constant. The + /// second is the path the runner takes now, against the same silent host + /// and the same budget. + #[tokio::test(flavor = "current_thread")] + async fn the_outcome_path_does_not_spend_the_declared_budget_waiting_for_a_frame() { + let socket = std::env::temp_dir().join(format!( + "ps-qa-silent-host-{}-{}.sock", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock is after the epoch") + .as_nanos() + )); + let saved = SemanticNode { + dom_id: None, + id: 3, + parent: None, + role: "button".into(), + name: "Saved".into(), + value: None, + enabled: true, + visible: true, + selected: false, + bounds: Some([0.0, 0.0, 90.0, 24.0]), + slot: None, + }; + let host = serve_silent_host(socket.clone(), vec![saved.clone()]); + + let client_socket = socket.clone(); + let driver = async move { + let mut check = check("saved-paints", Some("Save"), "Saved"); + check.outcome_timeout_ms = 200; + let budget = declared_outcome_timeout(&check); + + let mut client = crate::inspector::Client::connect(&client_socket) + .await + .expect("connect to the silent host"); + client.initialize().await.expect("initialize completes"); + assert!( + client + .arm_paint_events() + .await + .expect("streaming is served"), + "the host serves the paint stream; it just never commits a frame" + ); + + let waited = std::time::Instant::now(); + assert!( + !client + .wait_for_paint(budget) + .await + .expect("waiting for a frame is not itself an error"), + "no frame is ever committed" + ); + assert!( + waited.elapsed() >= budget, + "the removed boundary spent the whole declared budget: {:?}", + waited.elapsed() + ); + + let settled = std::time::Instant::now(); + let (_, error, _) = settle_for_outcome(&mut client, &check, &[], None, None, true) + .await + .expect("the outcome is judged from the tree"); + assert!(error.is_none(), "{error:?}"); + assert!( + settled.elapsed() < budget / 2, + "the outcome is judged when it arrives, not when the budget expires: {:?}", + settled.elapsed() + ); + }; + + tokio::join!(host, driver); + let _ = std::fs::remove_file(socket); + } + /// A baseline that predates the setup value lets the setup pass the check. /// /// This is the shape the harness has to make impossible: the `before` From a485ca733566457ca16bb6fe6de7af3e9b9cc01d Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:42:56 +0700 Subject: [PATCH 07/22] feat(qa): the open step takes its own arrival budget A check's navigation had a fixed 900ms with no way to say otherwise, and `click` has taken an explicit deadline for a while. Any route change that costs a live network round trip lands on either side of that number, so whether a check passes depends on the day. The message it produced was also about the wrong thing: `could not open "Crates"` reads as a missing tab, and the reader goes looking for a control that is there. `open_timeout_ms` is that budget, independent of `outcome_timeout_ms` so that covering a slow route does not weaken the interaction the check exists to measure. The arrival failure now says the control was activated and the destination did not paint in time, and names the budget it missed. --- crates/ps-qa/src/qa.rs | 15 ++++++ crates/ps-qa/src/runner.rs | 107 ++++++++++++++++++++++++++++--------- 2 files changed, 98 insertions(+), 24 deletions(-) diff --git a/crates/ps-qa/src/qa.rs b/crates/ps-qa/src/qa.rs index 9def3b2..192f5c0 100644 --- a/crates/ps-qa/src/qa.rs +++ b/crates/ps-qa/src/qa.rs @@ -505,6 +505,20 @@ pub struct Check { /// rendered action still has to pass the one-second budget. #[serde(default)] pub settle_after_ms: u64, + /// Deadline for this check's [`open`](Self::open) step. + /// + /// Navigation is not the interaction contract. `open` may be a route change + /// that fetches before it can paint, and a live network round trip lands on + /// either side of the 900ms every other step gets. The failure that + /// produces is also the wrong sentence: `could not open "Crates"` reads as + /// a missing tab rather than as a deadline, and the reader goes looking for + /// a control that is there. + /// + /// So a route that is known to fetch declares what it costs here, and every + /// other navigation in the suite keeps the strict default rather than being + /// weakened to cover the slow one. + #[serde(default)] + pub open_timeout_ms: u64, /// Deadline for this check's rendered outcome. /// /// Ordinary interactions keep the 900ms contract. A declared backend or @@ -1974,6 +1988,7 @@ mod tests { covers: Vec::new(), press: false, settle_after_ms: 0, + open_timeout_ms: 0, outcome_timeout_ms: 0, stable_for_ms: 0, require_visible: false, diff --git a/crates/ps-qa/src/runner.rs b/crates/ps-qa/src/runner.rs index de1d980..579325c 100644 --- a/crates/ps-qa/src/runner.rs +++ b/crates/ps-qa/src/runner.rs @@ -51,14 +51,19 @@ use crate::{app, audit, cli, inspector, paint_audit, qa, reach, report, sweep}; /// /// Background counters and provider refreshes can keep the whole semantic tree /// changing indefinitely. They are irrelevant to whether the requested panel -/// arrived, so navigation gets the same sub-second budget as every other QA -/// action and polls its exact marker. +/// arrived, so it polls its exact marker rather than a settled tree. +/// +/// `within` is the caller's budget, already scaled. Ordinary reveals keep the +/// sub-second interaction contract; a check's `open` step passes whatever that +/// check declared, because a route change that costs a live network round trip +/// is not an interaction and cannot honestly be held to one. async fn wait_for_arrival( client: &mut Client, destination: Option<&reach::Surface>, want_here: &str, + within: Duration, ) -> Result { - let deadline = tokio::time::Instant::now() + check_timeout(900); + let deadline = tokio::time::Instant::now() + within; let mut painted_streak = 0; let mut root = None; loop { @@ -148,11 +153,12 @@ async fn wait_for_navigation_arrival( want_here: &str, named_document: bool, document_name: &str, + within: Duration, ) -> Result { if !named_document { - return wait_for_arrival(client, destination, want_here).await; + return wait_for_arrival(client, destination, want_here, within).await; } - let deadline = tokio::time::Instant::now() + check_timeout(900); + let deadline = tokio::time::Instant::now() + within; let mut painted_streak = 0; let mut selected_tab = None; loop { @@ -1102,6 +1108,7 @@ async fn run_qa( */ let mut open_error = None; let mut pixel_outcome: Option> = None; + let open_budget = declared_open_timeout(check); if let Some(want) = check.open.as_deref() { /* * A permanent surface marker can answer "already there". A @@ -1171,6 +1178,7 @@ async fn run_qa( want_here, named_document, want, + open_budget, ) .await?; // A surface transition can briefly remove the opener before @@ -1185,6 +1193,7 @@ async fn run_qa( want_here, named_document, want, + open_budget, ) .await?; } @@ -1195,6 +1204,7 @@ async fn run_qa( want_here, named_document, want, + open_budget, ) .await?; } @@ -1210,12 +1220,15 @@ async fn run_qa( want_here, named_document, want, + open_budget, ) .await? { open_error = Some(format!( - "could not open {want:?}: destination did not paint within {}ms", - check_timeout(900).as_millis() + "could not open {want:?}: the control was activated but the \ + destination did not paint within {}ms. Raise open_timeout_ms \ + if this route fetches.", + open_budget.as_millis() )); } } @@ -1334,7 +1347,7 @@ async fn run_qa( if open_error.is_none() && let Some(reveal) = check.reveal_before_capture.as_deref() { - let arrived = wait_for_arrival(client, None, reveal).await?; + let arrived = wait_for_arrival(client, None, reveal, check_timeout(900)).await?; if !arrived { open_error = Some(format!( "could not reveal {reveal:?}: it did not paint within {}ms", @@ -1389,7 +1402,7 @@ async fn run_qa( if let Err(error) = hovered { open_error = Some(error); } else if let Some(next) = check.prepare.as_deref().or(check.click.as_deref()) - && !wait_for_arrival(client, None, next).await? + && !wait_for_arrival(client, None, next, check_timeout(900)).await? { // A virtualized row can reconcile after ScrollIntoView // and lose the hover that was sent to its prior node. @@ -1397,7 +1410,7 @@ async fn run_qa( // race into a misleading "could not click" failure. if let Err(error) = repeat_hover(client, hover, false).await { open_error = Some(error); - } else if !wait_for_arrival(client, None, next).await? { + } else if !wait_for_arrival(client, None, next, check_timeout(900)).await? { open_error = Some(format!( "hovering {:?} did not reveal {next:?}", hover.target() @@ -1506,7 +1519,7 @@ async fn run_qa( .or(check.type_into.as_deref()) .or(check.key_on.as_deref()) { - let _ = wait_for_arrival(client, None, next).await?; + let _ = wait_for_arrival(client, None, next, check_timeout(900)).await?; } } @@ -1546,7 +1559,7 @@ async fn run_qa( pixel_outcome = Some(measured); } else if check.expect == qa::Expect::PixelsHoldAfterHover { let measured = async { - match wait_for_arrival(client, None, hover.target()).await { + match wait_for_arrival(client, None, hover.target(), check_timeout(900)).await { Ok(true) => {} Ok(false) => { return Err(format!( @@ -2463,6 +2476,24 @@ async fn settle_for_outcome( /// Capture settling, semantic settling and transport waits must all derive /// from this value. Independent literals let one layer give up while another /// still claims the outcome has time remaining. +/// The budget a check's navigation step is allowed. +/// +/// Separate from the outcome budget on purpose. Opening a surface and measuring +/// a control are different questions with different costs: the outcome contract +/// is about how fast the interface answers a person, while `open` may be a +/// route change that fetches. A route that takes a live network round trip +/// lands on either side of a fixed 900ms with no way to say so, and the failure +/// it produced blamed the control -- `could not open "Crates"` reads as a +/// missing tab, not as a deadline. Raising the deadline for every check to +/// cover the slow one would have weakened every other navigation in the suite. +fn declared_open_timeout(check: &qa::Check) -> Duration { + check_timeout(if check.open_timeout_ms == 0 { + 900 + } else { + check.open_timeout_ms + }) +} + fn declared_outcome_timeout(check: &qa::Check) -> Duration { check_timeout(if check.outcome_timeout_ms == 0 { 900 @@ -3385,7 +3416,7 @@ async fn materialize_deferred_content( }; let query = want.split_once(':').map_or(want, |(_, name)| name); type_text(client, field, query).await?; - if wait_for_arrival(client, None, want).await? { + if wait_for_arrival(client, None, want, check_timeout(900)).await? { // The reveal field is a discovery mechanism, not part of the check's // authored state. Leaving it filled hides outcomes whose accessible // name changes (and poisons every later check on the surface). Lazy @@ -3394,9 +3425,9 @@ async fn materialize_deferred_content( // A virtualized catalogue that truly requires its query gets it put // back rather than losing the control before the action. type_text(client, field, "").await?; - if !wait_for_arrival(client, None, want).await? { + if !wait_for_arrival(client, None, want, check_timeout(900)).await? { type_text(client, field, query).await?; - let _ = wait_for_arrival(client, None, want).await?; + let _ = wait_for_arrival(client, None, want, check_timeout(900)).await?; } } else { // Absence is itself a valid authored outcome (for example, a setup @@ -5826,15 +5857,15 @@ pub async fn run() -> Result<()> { mod tests { use super::{ InventoryClass, OutcomeStability, accumulated_hover_signatures, arrival_sample_matches, - arrived_without_navigation, assess_pixel_change, capture_node_id, declared_outcome_timeout, - duplicate_dom_ids, generated_dom_id, hover_signature_counts, inventory_class, - is_pagination_control, measure_ink, name_matches, named_document_is_active, - named_document_is_active_with_permanent, named_document_opener_for, outcome_check_ids, - outcome_verdict, pagination_advanced, painted_bounds, painted_named, pixels_change, - pixels_hold, require_transparent_window_tint, resolved_action_target, rgb_pixels_hold, - saved_control_node, saved_controls, selector_matches_node, settle_for_outcome, - setup_value_landed, stable_arrival, subject_belongs_to_scope, - validate_surface_filter_against, + arrived_without_navigation, assess_pixel_change, capture_node_id, declared_open_timeout, + declared_outcome_timeout, duplicate_dom_ids, generated_dom_id, hover_signature_counts, + inventory_class, is_pagination_control, measure_ink, name_matches, + named_document_is_active, named_document_is_active_with_permanent, + named_document_opener_for, outcome_check_ids, outcome_verdict, pagination_advanced, + painted_bounds, painted_named, pixels_change, pixels_hold, require_transparent_window_tint, + resolved_action_target, rgb_pixels_hold, saved_control_node, saved_controls, + selector_matches_node, settle_for_outcome, setup_value_landed, stable_arrival, + subject_belongs_to_scope, validate_surface_filter_against, }; use crate::app::{AppProfile, SurfaceSpec}; use crate::interaction::parse_key_chord; @@ -6581,6 +6612,7 @@ mod tests { covers: Vec::new(), press: false, settle_after_ms: 0, + open_timeout_ms: 0, outcome_timeout_ms: 0, stable_for_ms: 0, require_visible: false, @@ -6601,6 +6633,33 @@ mod tests { ); } + /// Navigation has its own budget, and it is not the outcome budget. + /// + /// A route change that fetches lands on either side of a fixed 900ms, and + /// the only way to cover it before this was to raise `outcome_timeout_ms`, + /// which weakens the measured interaction the check exists to time. The two + /// deadlines move independently. + #[test] + fn the_open_step_takes_its_own_declared_deadline() { + let mut check = check("crates-tab", Some("Publish"), "Published"); + assert_eq!(declared_open_timeout(&check), Duration::from_millis(900)); + + check.open_timeout_ms = 4_000; + assert_eq!(declared_open_timeout(&check), Duration::from_millis(4_000)); + assert_eq!( + declared_outcome_timeout(&check), + Duration::from_millis(900), + "a slow route must not weaken the interaction contract" + ); + + check.outcome_timeout_ms = 1_500; + assert_eq!(declared_open_timeout(&check), Duration::from_millis(4_000)); + assert_eq!( + declared_outcome_timeout(&check), + Duration::from_millis(1_500) + ); + } + #[test] fn a_surface_filter_is_case_insensitive_and_never_succeeds_empty() { let surfaces = [SurfaceSpec { From 427c73bef3702780b56b1e49c91ae7de89d45450 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:45:21 +0700 Subject: [PATCH 08/22] fix(qa): checks run in the order their files declare them The runner bucketed every check by the surface its `open` named, stable-sorted by that bucket, and let a check with no `open` inherit the previous check's bucket across every file at once. Four sites paid for it: groups written as ordered sequences came back interleaved, so steps ran between steps; one site's checks inherited a surface from a file they have no relationship with and measured a page they had never navigated to, and passed; and another could not declare a second surface at all without its existing sequences coming apart. What the bucketing bought was mount amortization. What it cost is the one property an ordered suite needs, which is that the order is the one somebody wrote down. Files in name order, checks in declaration order, and nothing moves but the destructive tail, which is a correctness requirement rather than an optimization. Inheriting the previous check's screen inside one file is how a sequence is written and stays. Across files it is now an error naming both files and the surface that would have been inherited: a reader of the second file has no way to know what the first one left in front of them. --- crates/ps-qa/README.md | 13 ++- crates/ps-qa/src/qa.rs | 102 ++++++++++++++++++++++- crates/ps-qa/src/runner.rs | 165 ++++++++++++++++++++++++++++--------- 3 files changed, 235 insertions(+), 45 deletions(-) diff --git a/crates/ps-qa/README.md b/crates/ps-qa/README.md index 1b7674d..a8fa8c3 100644 --- a/crates/ps-qa/README.md +++ b/crates/ps-qa/README.md @@ -278,10 +278,15 @@ file, line and column rather than degrading to empty in silence. **`tests/ps-qa/*.ron`** — the checks. A check is a precondition, an action and an assertion with no behaviour of its own, so it is data: editing a selector is an edit and a re-run, not a recompile. Found by `--checks`, or -`tests/ps-qa/`. Files are read in name order for manifests and focused runs. -A full execution groups non-destructive checks by surface to avoid repeated -navigation, then runs destructive checks last. The report retains each check's -stable id, so optimized run order cannot be mistaken for source order. +`tests/ps-qa/`. Files are read in name order, and checks run in the order they +are declared: a sequence written as steps runs as steps. The only thing that +moves is the destructive tail, which runs last so a check that deletes fixture +state cannot pull it out from under a later one. + +A check with no `open` runs on whatever the previous check left in front of it, +which is how a sequence inside one file is written. Across files that is a +dependency nobody declared, so the first check of a file must say which surface +it starts on whenever an earlier file navigated somewhere. `reconcile` decodes the emitted TOON directly, including nested control rows with per-control check arrays. Do not flatten or scrape that report before diff --git a/crates/ps-qa/src/qa.rs b/crates/ps-qa/src/qa.rs index 192f5c0..a5a4b8f 100644 --- a/crates/ps-qa/src/qa.rs +++ b/crates/ps-qa/src/qa.rs @@ -630,13 +630,50 @@ pub fn checks(dir: Option<&std::path::Path>) -> Result, String> { let mut all = Vec::new(); let mut ids = HashMap::new(); + // The last surface any file asked for, and which file asked. A check with + // no `open` runs on whatever the previous check left in front of it, which + // is a fine and deliberate way to write a sequence *inside* one file. See + // below for why it stops there. + let mut established: Option<(std::path::PathBuf, String)> = None; for file in files { let text = std::fs::read_to_string(&file) .map_err(|error| format!("could not read {}: {error}", file.display()))?; let group: Vec = ron::from_str(&text) .map_err(|error| format!("could not parse {}: {error}", file.display()))?; - for check in &group { + for (index, check) in group.iter().enumerate() { validate_check(check, &file, &mut ids)?; + /* + * A file may not inherit another file's surface in silence. + * + * Files are separate documents, read in name order, and a reader of + * one has no reason to know which surface the file before it left + * open. A first check with no `open` therefore runs against a + * screen nobody in this file chose: one site's checks measured a + * page they had never navigated to and passed, which is the worst + * possible outcome for a check. + * + * Inside a file the same inheritance is how a sequence is written + * and is left alone. + */ + if index == 0 + && check.open.is_none() + && let Some((previous_file, opener)) = &established + { + return Err(format!( + concat!( + "{}: check {:?} is the first in its file and declares no `open`, so it ", + "would run on {:?}, which {} navigated to. Declare the surface this ", + "file starts on." + ), + file.display(), + check.id, + opener, + previous_file.display(), + )); + } + if let Some(opener) = check.open.as_deref() { + established = Some((file.to_path_buf(), opener.to_owned())); + } } all.extend(group); } @@ -1547,7 +1584,7 @@ pub fn tally<'a>(results: &[(&'a Check, Result<(), String>)]) -> HashMap<&'a str #[cfg(test)] mod tests { use super::{ - Check, Expect, action_description, name_changed, selection_changed, validate_check, + Check, Expect, action_description, checks, name_changed, selection_changed, validate_check, value_changed, verdict, }; use blitz_control_protocol::SemanticNode; @@ -1926,6 +1963,67 @@ mod tests { ); } + /// A file may not start on a surface another file navigated to. + /// + /// Inheriting the previous check's screen is how a sequence is written + /// inside one file. Across files it is a dependency nobody declared and + /// nobody reading either file can see, and it is how one site's checks came + /// to measure a page they had never navigated to and pass. + #[test] + fn a_file_may_not_inherit_the_surface_another_file_opened() { + let directory = std::env::temp_dir().join(format!( + "ps-qa-check-files-{}-{}", + std::process::id(), + std::time::SystemTime::now() + .duration_since(std::time::UNIX_EPOCH) + .expect("system clock is after the epoch") + .as_nanos() + )); + std::fs::create_dir(&directory).expect("create check directory"); + let check = |id: &str, open: &str| { + format!( + "(id:\"{id}\",group:\"g\",what:\"w\",open:{open},hover:None,\ + click:Some(\"Act\"),subject:\"Result\",expect:Paints)" + ) + }; + std::fs::write( + directory.join("a-settings.ron"), + format!( + "[{},{}]", + check("settings-open", "Some(\"Settings\")"), + check("settings-edit", "None") + ), + ) + .expect("write the first file"); + std::fs::write( + directory.join("b-registry.ron"), + format!("[{}]", check("registry-publish", "None")), + ) + .expect("write the second file"); + + let error = checks(Some(&directory)).expect_err("the second file declares no surface"); + assert!(error.contains("registry-publish"), "{error}"); + assert!( + error.contains("Settings"), + "the inherited surface is named: {error}" + ); + assert!( + error.contains("a-settings.ron"), + "so is the file that set it: {error}" + ); + + // Declaring the surface is the fix, and the whole directory then loads. + std::fs::write( + directory.join("b-registry.ron"), + format!("[{}]", check("registry-publish", "Some(\"Registry\")")), + ) + .expect("rewrite the second file"); + let loaded = checks(Some(&directory)).expect("both files declare where they start"); + assert_eq!(loaded.len(), 3); + + std::fs::remove_dir_all(&directory).expect("remove check directory"); + } + #[test] fn a_change_expectation_on_the_setup_field_needs_an_action() { let mut check = parse("setup_type_into:Some(\"Filter\"),setup_text:Some(\"acme\"),"); diff --git a/crates/ps-qa/src/runner.rs b/crates/ps-qa/src/runner.rs index 579325c..97fedc8 100644 --- a/crates/ps-qa/src/runner.rs +++ b/crates/ps-qa/src/runner.rs @@ -1024,46 +1024,49 @@ async fn run_component( run_qa(&mut client, selector, checks_dir).await } +/// The order a run executes in: the order the files declare. +/// +/// This used to bucket every check by the surface its `open` names, stable-sort +/// by that bucket, and let a check with no `open` inherit the previous check's +/// bucket -- across every file at once. Three things came of that, on four +/// sites: +/// +/// - a group written as a sequence was interleaved with another group that +/// happened to name a different surface, so steps ran between steps; +/// - a check with no `open`, first in its file, inherited a surface set in a +/// file it has no relationship with, and measured a page it had never +/// navigated to. It passed; +/// - a suite could not declare a second surface at all without its existing +/// sequences coming apart. +/// +/// The bucketing bought mount amortization. It cost the one property a suite of +/// ordered checks needs, which is that the order is the one somebody wrote +/// down, and a check that reads correctly next to its neighbours ran somewhere +/// else. Files in name order, checks in declaration order, and nothing moves +/// but the destructive tail. +/// +/// Destructive checks still go last, and that is not an optimization: a +/// sequence that deletes fixture state a later surface needs makes an ordered +/// shared run conflict with itself. `sort_by_key` is stable, so their own +/// relative order is the declared one too. +fn ordered_checks<'a>(all: &'a [qa::Check], group: Option<&str>) -> Vec<&'a qa::Check> { + // A group *or* one check's id, so chasing a single failure does not mean + // re-running its neighbours against the real app every time. + let mut selected: Vec<&qa::Check> = all + .iter() + .filter(|check| group.is_none_or(|want| check.group == want || check.id == want)) + .collect(); + selected.sort_by_key(|check| check.destructive); + selected +} + async fn run_qa( client: &mut Client, group: Option<&str>, checks_dir: Option<&std::path::Path>, ) -> Result { let all = qa::checks(checks_dir).map_err(|error| eyre!(error))?; - // A group *or* one check's id, so chasing a single failure does not mean - // re-running its neighbours against the real app every time. - // Stable surface buckets: keep dependent checks in manifest order while - // avoiding repeated remounts of the same large application pane. The - // application profile owns the surface openers; an unknown plain opener is - // the configured dynamic document, while role-qualified openers stay in - // the current bucket because they open a dialog within that surface. - let surfaces = reach::surfaces(); - let dynamic = surfaces - .iter() - .position(|surface| surface.opener == reach::DYNAMIC_DOCUMENT) - .unwrap_or(0); - let mut affinity = dynamic; - let mut selected: Vec<(usize, &qa::Check)> = Vec::new(); - for check in &all { - if let Some(opener) = check.open.as_deref() { - if let Some(index) = surfaces - .iter() - .position(|surface| surface.opener.eq_ignore_ascii_case(opener)) - { - affinity = index; - } else if !opener.contains(':') { - affinity = dynamic; - } - } - if group.is_none_or(|want| check.group == want || check.id == want) { - selected.push((affinity, check)); - } - } - // Surface affinity amortizes large retained-pane mounts. Destructive - // sequences outrank that optimization: deleting fixture state before a - // later surface uses it makes an ordered shared sweep conflict with itself. - selected.sort_by_key(|(surface, check)| (check.destructive, *surface)); - let selected: Vec<&qa::Check> = selected.into_iter().map(|(_, check)| check).collect(); + let selected = ordered_checks(&all, group); if selected.is_empty() { let mut names: Vec = all .iter() @@ -5861,11 +5864,12 @@ mod tests { declared_outcome_timeout, duplicate_dom_ids, generated_dom_id, hover_signature_counts, inventory_class, is_pagination_control, measure_ink, name_matches, named_document_is_active, named_document_is_active_with_permanent, - named_document_opener_for, outcome_check_ids, outcome_verdict, pagination_advanced, - painted_bounds, painted_named, pixels_change, pixels_hold, require_transparent_window_tint, - resolved_action_target, rgb_pixels_hold, saved_control_node, saved_controls, - selector_matches_node, settle_for_outcome, setup_value_landed, stable_arrival, - subject_belongs_to_scope, validate_surface_filter_against, + named_document_opener_for, ordered_checks, outcome_check_ids, outcome_verdict, + pagination_advanced, painted_bounds, painted_named, pixels_change, pixels_hold, + require_transparent_window_tint, resolved_action_target, rgb_pixels_hold, + saved_control_node, saved_controls, selector_matches_node, settle_for_outcome, + setup_value_landed, stable_arrival, subject_belongs_to_scope, + validate_surface_filter_against, }; use crate::app::{AppProfile, SurfaceSpec}; use crate::interaction::parse_key_chord; @@ -6633,6 +6637,89 @@ mod tests { ); } + /// Checks run in the order the files declare them. + /// + /// `old_order` below is the surface bucketing this replaced, written out + /// because the point is what it did to a sequence: two groups, each written + /// as ordered steps against its own surface, come back interleaved. Every + /// step still runs, and every one of them runs somewhere else. + #[test] + fn a_declared_sequence_is_not_reordered_by_the_surface_it_names() { + fn old_order<'a>(all: &'a [Check], surfaces: &[&str]) -> Vec<&'a str> { + let mut affinity = 0; + let mut selected: Vec<(usize, &Check)> = Vec::new(); + for check in all { + if let Some(opener) = check.open.as_deref() + && let Some(index) = surfaces.iter().position(|surface| *surface == opener) + { + affinity = index; + } + selected.push((affinity, check)); + } + selected.sort_by_key(|(surface, check)| (check.destructive, *surface)); + selected + .into_iter() + .map(|(_, check)| check.id.as_str()) + .collect() + } + + let step = |id: &str, open: Option<&str>| { + let mut check = check(id, Some("Act"), "Result"); + check.open = open.map(str::to_owned); + check + }; + // Two sequences, each written as steps: open the surface, then act on + // what the previous step left behind. + let mut teardown = step("reset-fixtures", Some("Settings")); + teardown.destructive = true; + let all = [ + step("settings-open", Some("Settings")), + step("settings-edit", None), + step("registry-open", Some("Registry")), + step("registry-publish", None), + step("settings-save", Some("Settings")), + teardown, + ]; + + assert_eq!( + old_order(&all, &["Settings", "Registry"]), + [ + "settings-open", + "settings-edit", + "settings-save", + "registry-open", + "registry-publish", + "reset-fixtures", + ], + "the bucketing ran a Settings step between the two Registry steps" + ); + + assert_eq!( + ordered_checks(&all, None) + .into_iter() + .map(|check| check.id.as_str()) + .collect::>(), + [ + "settings-open", + "settings-edit", + "registry-open", + "registry-publish", + "settings-save", + "reset-fixtures", + ], + "declaration order, with the destructive tail last" + ); + + assert_eq!( + ordered_checks(&all, Some("registry-publish")) + .into_iter() + .map(|check| check.id.as_str()) + .collect::>(), + ["registry-publish"], + "one check by id still runs alone" + ); + } + /// Navigation has its own budget, and it is not the outcome budget. /// /// A route change that fetches lands on either side of a fixed 900ms, and From e8ad3074db934184cf476f988254084e077eb087 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:47:08 +0700 Subject: [PATCH 09/22] docs(ps-qa): the new check fields, and what dom now accepts The README described neither `require_visible` nor `open_timeout_ms`, and still said a bare substring is all `dom` matches. --- crates/ps-qa/README.md | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/crates/ps-qa/README.md b/crates/ps-qa/README.md index a8fa8c3..648a071 100644 --- a/crates/ps-qa/README.md +++ b/crates/ps-qa/README.md @@ -95,6 +95,12 @@ ps-qa click "" # activate the first matching semantic node ps-qa nodes # tree size and a role histogram ``` +`dom` takes either a selector or free text: `role:name`, `#id` and `@slot` mean +what they mean in a check, and a bare word searches names, roles and values. A +selector the document cannot answer at all, such as a role no node carries or +an id nobody declares, is reported as such rather than as zero matches, which +is the difference between "your selector" and "your page". + `press` remains a generic, explicit pointer-path diagnostic. Application suites use semantic activation by default: resolve a name with `find`, retain the node id, and act on that id. When repeated rows intentionally share an accessible @@ -174,6 +180,20 @@ tabs, then activates the exact semantic node id. | `SelectionChanges` | the same semantic node changes selected/pressed state | | `NameChanges` | the same semantic node exposes a different accessible name after the action | +`Paints`, `PaintsNamed`, `PaintsMore` and `Count` judge on boxes, because the +tree's visibility flag and the renderer disagree and trusting the flag once +reported a screen full of icons as painting nothing. That leaves a disclosure +unprovable in one direction: an Accordion, a Collapsible or a Tabs panel keeps +its box and flips `hidden`, so every geometry assertion is satisfied whether it +is open or closed, and only `Vanishes` could tell. Add `require_visible: true` +to ask for the flag as well, for a subject whose box does not move. + +Navigation has its own deadline. `open_timeout_ms` is the arrival budget for +the `open` step, separate from `outcome_timeout_ms` so that a route which +fetches can declare what it costs without weakening the interaction the check +exists to measure. Without it a live network round trip lands on either side of +900ms and fails as `could not open …`, which reads as a missing control. + Outcome checks can continue past activation with literal semantic input: `type_into: Some("New item"), text: Some("qa audit newest"), key: Some("Enter")`. Text is focused, selected, and exactly replaced by node id; no coordinate pointer is involved. From 6624820184ee06509246e85be1a2c32a14a8f70d Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 18:47:08 +0700 Subject: [PATCH 10/22] release: ps-qa 0.7.0 Two new check fields, `require_visible` and `open_timeout_ms`, both defaulted so existing manifests parse unchanged. Two behaviour changes a suite will notice: checks now run in declaration order rather than bucketed by surface, and a file whose first check declares no `open` while an earlier file navigated somewhere is rejected instead of inheriting that surface in silence. --- crates/ps-qa/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/ps-qa/Cargo.toml b/crates/ps-qa/Cargo.toml index 55b5248..3342566 100644 --- a/crates/ps-qa/Cargo.toml +++ b/crates/ps-qa/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "ps-qa" description = "Drive a running Blitz app through its MCP control socket and assert what the renderer did" -version = "0.6.3" +version = "0.7.0" edition = "2024" rust-version = "1.88" license = "MIT OR Apache-2.0" From a9f87528f347aa19f2cb745db804283b5611d580 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 20:39:26 +0700 Subject: [PATCH 11/22] feat(qa): a control sized by its text can still be judged enabled `Enabled` and `Disabled` ask two questions at once: the tree says this control accepts input, and it has a box a person could aim at. The second half is the font question `Present` already exists for, and CI runs a host with no font catalogue. A `w-full` primary call to action collapses to its label and lays out `0x44` there, carrying `enabled=true` in the very tree the check reads, and both verdicts refuse it. Two sites hit this within a day. One rewrote two checks to assert what the button gates rather than the button; the other dropped to `Present` and lost the state assertion entirely. `text_sized` trades the box for the weaker thing that survives a fontless host: the node was laid out. Opt-in and per check, on the shape `require_visible` already set, so no existing check changes meaning and no suite has to migrate. Only the box is given up: an absent subject, one that never reached layout, and one carrying the wrong flag all still fail, and the field is refused on any expectation that would not consult it. The failure message names the field when the flag was already right and only the box was empty, since that is a one-word fix in the check file. --- crates/ps-qa/src/qa.rs | 222 +++++++++++++++++++++++++++++++++---- crates/ps-qa/src/runner.rs | 1 + 2 files changed, 201 insertions(+), 22 deletions(-) diff --git a/crates/ps-qa/src/qa.rs b/crates/ps-qa/src/qa.rs index a5a4b8f..b0a242d 100644 --- a/crates/ps-qa/src/qa.rs +++ b/crates/ps-qa/src/qa.rs @@ -120,12 +120,20 @@ pub enum Expect { /// /// Use this for validation flows where entering a valid value unlocks a /// Save or Send control without changing its geometry. + /// + /// A control whose box comes from its own label has no box on a host with + /// no fonts, and this refuses it however right the flag is. Declare + /// [`text_sized`](Check::text_sized) on such a subject to judge the flag + /// alone. Enabled, /// A painted named node refuses input after the action. /// /// This is the observable completion state for actions such as clearing a /// saved value: the control remains visible, but cannot be invoked again /// until there is something new to act on. + /// + /// Takes [`text_sized`](Check::text_sized) on the same terms as + /// [`Enabled`](Expect::Enabled). Disabled, /// No node matching the name exists. /// @@ -562,6 +570,36 @@ pub struct Check { /// makes an exact count of open panels expressible at all. #[serde(default)] pub require_visible: bool, + /// The subject's box comes from its text, so judge its input state on the + /// flag and layout alone. + /// + /// [`Enabled`](Expect::Enabled) and [`Disabled`](Expect::Disabled) ask two + /// questions at once: the tree says this control accepts input, *and* it has + /// a box a person could aim at. That second half is the same font question + /// [`Present`](Expect::Present) exists for, and it is unanswerable on the + /// host that matters most. CI runs without a font catalogue; text lays out + /// at zero there, so a control sized by its own label measures nothing. + /// Measured on a `w-full` primary call to action: `0x44`, `enabled=true` + /// in the very tree the check reads, and both verdicts refused it. Two + /// sites hit this within a day of each other, one rewriting two checks to + /// assert what the button *gates* instead of the button, the other + /// dropping to [`Present`](Expect::Present) and losing the state assertion + /// entirely. + /// + /// So this is opt-in and per check, exactly like + /// [`require_visible`](Self::require_visible), and for the same reason: the + /// default meaning of an existing check must not change under it. A check + /// that declares this is saying "this subject is sized by its glyphs", and + /// gives up only the box, never the state. The node must still exist and + /// must still have been laid out, which is the floor + /// [`Present`](Expect::Present) already holds, so a control the document + /// never produced still fails. + /// + /// It is rejected on any other expectation. For the geometry axis the + /// vocabulary already has an answer, and it is + /// [`Present`](Expect::Present). + #[serde(default)] + pub text_sized: bool, /// Run this check only after every ordinary shared-instance outcome. /// /// A destructive sequence may deliberately remove fixture state that @@ -762,6 +800,28 @@ fn validate_check( )); } + /* + * A weakening that does nothing must not look like it did something. + * + * `text_sized` trades a box for a layout, and only the two input-state + * verdicts ask for a box on the subject's own behalf. Accepted silently + * anywhere else it would read as "this check tolerates a fontless host" + * while the geometry it actually depends on was never relaxed, which is + * the sort of thing a suite discovers on the CI run it was written for. + */ + if check.text_sized && !matches!(check.expect, Expect::Enabled | Expect::Disabled) { + return Err(format!( + concat!( + "{}: check {:?} declares text_sized with {:?}, which does not judge the ", + "subject's box on its own. text_sized applies to Enabled and Disabled; for a ", + "geometry assertion on a fontless host the expectation is Present" + ), + file.display(), + check.id, + check.expect, + )); + } + if check.expect == Expect::Count && check.expect_count.is_none() { return Err(format!( "{}: check {:?} must declare expect_count with Count", @@ -812,6 +872,21 @@ fn shows(check: &Check, node: &SemanticNode) -> bool { paints(node) && (!check.require_visible || node.visible) } +/// Whether this check may read the subject's input state off this node. +/// +/// A box is the default evidence that the control is on a screen, and stays +/// the default. [`Check::text_sized`] trades it for the weaker thing that +/// survives a fontless host: the node was laid out. That is the same floor +/// [`Expect::Present`] holds, and it is still falsifiable - a node the document +/// never created is not here, and one that never reached layout has no bounds. +fn state_readable(check: &Check, node: &SemanticNode) -> bool { + if check.text_sized { + node.bounds.is_some() + } else { + paints(node) + } +} + /// The verdict for one check, given the tree before and after its action. pub fn verdict( check: &Check, @@ -919,30 +994,28 @@ pub fn verdict( )); } } - Expect::Enabled => { - if found.iter().any(|node| paints(node) && node.enabled) { - return Ok(()); - } - let states = found + Expect::Enabled | Expect::Disabled => { + let wanted = check.expect == Expect::Enabled; + let word = if wanted { "enabled" } else { "disabled" }; + if found .iter() - .take(3) - .map(|node| { - format!( - "id={} role={:?} name={:?} enabled={} bounds={:?}", - node.id, node.role, node.name, node.enabled, node.bounds - ) - }) - .collect::>(); - return Err(format!( - "no painted, enabled node matching {:?} ({})", - check.subject, - states.join(", ") - )); - } - Expect::Disabled => { - if found.iter().any(|node| paints(node) && !node.enabled) { + .any(|node| state_readable(check, node) && node.enabled == wanted) + { return Ok(()); } + /* + * Say when the state was right and only the box was empty. + * + * "no painted, enabled node" sends the reader looking for a + * missing control, and on a fontless host the control is there + * with the flag the check asked for. That is a one-word fix in the + * check file, so name the word rather than making the next person + * measure the tree to find it. + */ + let text_sized_would_pass = !check.text_sized + && found + .iter() + .any(|node| node.bounds.is_some() && node.enabled == wanted); let states = found .iter() .take(3) @@ -953,8 +1026,22 @@ pub fn verdict( ) }) .collect::>(); + let hint = if text_sized_would_pass { + format!( + "; a laid-out node is already {word} with an empty box, which is what a \ + control sized by its own label measures on a host with no fonts. Declare \ + text_sized: true to judge the flag there" + ) + } else { + String::new() + }; return Err(format!( - "no painted, disabled node matching {:?} ({})", + "no {}, {word} node matching {:?} ({}){hint}", + if check.text_sized { + "laid out" + } else { + "painted" + }, check.subject, states.join(", ") )); @@ -2090,6 +2177,7 @@ mod tests { outcome_timeout_ms: 0, stable_for_ms: 0, require_visible: false, + text_sized: false, destructive: false, subject: "Output level".into(), expect: Expect::ValueChanges, @@ -2427,6 +2515,96 @@ mod tests { assert!(verdict(&check, &[], &[node(false, Some([0.0, 0.0, 0.0, 0.0]))]).is_err()); } + /// A primary call to action is unassertable on the host CI actually runs. + /// + /// This is the shape both sites measured: a `w-full` button whose width + /// collapses to its label, so with no font catalogue it lays out `0x44` + /// while the very tree the check reads carries `enabled=true`. `Enabled` + /// and `Disabled` both refuse it on the box, so the state nobody disputes + /// cannot be asserted at all. One suite rewrote two checks to assert what + /// the button gates; another dropped to `Present` and lost the state. + #[test] + fn a_control_sized_by_its_label_can_still_be_judged_on_a_fontless_host() { + let cta = |enabled: bool, bounds| SemanticNode { + dom_id: None, + id: 41, + parent: None, + role: "button".into(), + name: "Continue".into(), + value: None, + enabled, + visible: true, + selected: false, + bounds, + slot: None, + }; + // What a host with no font catalogue reports for that button. + let fontless = [cta(true, Some([24.0, 612.0, 0.0, 44.0]))]; + + let mut check = parse(""); + check.subject = "button:Continue".into(); + check.expect = Expect::Enabled; + let error = verdict(&check, &[], &fontless) + .expect_err("this is the gap: the flag is right and the box is empty"); + assert!( + error.contains("text_sized"), + "the failure has to name the one-word fix, got {error:?}" + ); + + check.text_sized = true; + verdict(&check, &[], &fontless).expect("the tree carries the state the check asks about"); + + // And it still fails everywhere it should. A subject the document + // never produced is the case that matters most: an expectation that + // passes for a node which does not exist is worse than no expectation. + assert!( + verdict(&check, &[], &[]).is_err(), + "an absent control must still fail" + ); + assert!( + verdict(&check, &[], &[cta(true, None)]).is_err(), + "a node that never reached layout must still fail" + ); + assert!( + verdict(&check, &[], &[cta(false, Some([24.0, 612.0, 0.0, 44.0]))]).is_err(), + "only the box is given up, never the state" + ); + + // The same subject on a fonted host is unaffected either way. + let fonted = [cta(true, Some([24.0, 612.0, 328.0, 44.0]))]; + verdict(&check, &[], &fonted).expect("a real box satisfies the weaker box test too"); + check.text_sized = false; + verdict(&check, &[], &fonted).expect("the default meaning of Enabled has not moved"); + + // Disabled reads the flag on the same terms. + check.expect = Expect::Disabled; + check.text_sized = true; + verdict(&check, &[], &[cta(false, Some([24.0, 612.0, 0.0, 44.0]))]) + .expect("a disabled text-sized control is assertable too"); + assert!(verdict(&check, &[], &fontless).is_err()); + } + + /// The weakening is scoped to the verdicts it weakens. + /// + /// Declared from a check file, so this also pins the spelling a suite + /// writes. + #[test] + fn text_sized_is_refused_where_it_would_relax_nothing() { + let mut check = parse("text_sized:true,"); + assert!(check.text_sized, "the field is read from the check file"); + check.expect = Expect::Paints; + let error = validate_check(&check, Path::new("cta.ron"), &mut HashMap::new()) + .expect_err("Paints does not consult text_sized"); + assert!( + error.contains("Present"), + "point at the answer, got {error:?}" + ); + + check.expect = Expect::Enabled; + validate_check(&check, Path::new("cta.ron"), &mut HashMap::new()) + .expect("Enabled is one of the two verdicts it applies to"); + } + #[test] fn verdict_subjects_honor_role_qualified_names() { let mut check = parse(""); diff --git a/crates/ps-qa/src/runner.rs b/crates/ps-qa/src/runner.rs index 97fedc8..b1a3a03 100644 --- a/crates/ps-qa/src/runner.rs +++ b/crates/ps-qa/src/runner.rs @@ -6620,6 +6620,7 @@ mod tests { outcome_timeout_ms: 0, stable_for_ms: 0, require_visible: false, + text_sized: false, destructive: false, subject: subject.into(), expect: Expect::Paints, From 54b5bc3d02816255f8ff7f6c792668e0a7dbdf80 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 20:39:50 +0700 Subject: [PATCH 12/22] docs(ps-qa): the fontless enabled-state flag, and the Present row it needs The expectation table never listed `Present`, which is the geometry-axis answer to a fontless host and the thing `text_sized` has to be distinguished from. Add both, with the measurement that produced the field. --- crates/ps-qa/README.md | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/crates/ps-qa/README.md b/crates/ps-qa/README.md index 648a071..d2e8c90 100644 --- a/crates/ps-qa/README.md +++ b/crates/ps-qa/README.md @@ -148,6 +148,7 @@ tabs, then activates the exact semantic node id. | Expectation | Passes when | | --- | --- | | `Paints` | the subject exists and has a **non-zero rendered box** | +| `Present` | the subject exists in the tree and was laid out, whatever it measures | | `Enabled` | a painted subject accepts input after the action | | `Disabled` | a painted subject refuses input after the action | | `Vanishes` | nothing matching is on screen (it may remain in the tree) | @@ -188,6 +189,24 @@ its box and flips `hidden`, so every geometry assertion is satisfied whether it is open or closed, and only `Vanishes` could tell. Add `require_visible: true` to ask for the flag as well, for a subject whose box does not move. +A box is also a font question, and CI runs a host with no font catalogue. Text +lays out at height zero there, so a control sized by its own label has no box +at all: a `w-full` primary call to action measures `0x44` while the same tree +carries `enabled=true`. `Present` is the answer on the geometry axis, but it +says nothing about input state, so `Enabled` and `Disabled` were unassertable +for exactly the controls a suite most wants to gate on. Add `text_sized: true` +to judge the flag on a subject that was laid out, whatever it measures: + +```ron +subject: "button:Continue", +expect: Enabled, +text_sized: true, +``` + +It gives up the box and nothing else. A subject the document never produced, one +that never reached layout, and one carrying the wrong flag all still fail, and +declaring it on any other expectation is an error rather than a silent no-op. + Navigation has its own deadline. `open_timeout_ms` is the arrival budget for the `open` step, separate from `outcome_timeout_ms` so that a route which fetches can declare what it costs without weakening the interaction the check From cd72b4d303f81d027a8911f844ab7c2ea901d8b8 Mon Sep 17 00:00:00 2001 From: meh Date: Wed, 9 Sep 2026 22:53:12 +0700 Subject: [PATCH 13/22] refactor(protocol): the control core moves in from the runtime The semantic tree, the capture surface and input injection lived in `tauri-runtime-blitz`, beside the Tauri runtime. Nothing in them needs a window: they read and drive a `blitz-dom` document. Depending on them meant compiling Tauri anyway, and on Linux that means GTK, so a headless host pulled in system libraries to build a binary that opens nothing. A runtime bridges Tauri to Blitz and owns a native window; it is not where an inspection service lives. So this is the whole of `agent.rs`, taken from `fix/semantic-tree-tables-and-text` rather than from master, because master silently lacks the eight naming fixes on that branch. All 27 of its tests come with it. It is behind two features, cut on the blitz boundary rather than on what any consumer happened to want. `engine` is the tree and input injection; `capture` adds the offscreen paint, the diagnostic snapshots and the renderer metrics, and it is the only thing that pulls in `blitz-shell`. With neither enabled this crate is what it has always been: the vocabulary, at serde plus endpoint-libs. `cargo tree -p ps-qa` is the check and CI fails on it. The role table goes with the move rather than after it, because keeping it would have made three copies where there were two. `blitz-dom` owns the rules now, in `accessibility::implicit_role`, and this consumes them. What is left here is a projection: blitz-dom answers in AccessKit's vocabulary, and this wire answers in ARIA role names. That projection is deliberately lossy, and the loss is the compatibility guarantee. Eleven fleet sites and roughly 1,700 checks are written against the role strings this surface has always reported, so every arm reproduces one of them and `role_projection_tests` asserts it element by element. `

`, `

`, `` and the rest carry a distinct AccessKit role and have always arrived here as `generic`; making any of them more precise is a decision to take with the checks that read it, not a consequence of the tables becoming one. The one rule that changes: `` is a `columnheader` or a `rowheader`, as blitz-dom has always said and this surface did not. blitz-control-protocol 0.5.0, since the crate's shape is public. --- Cargo.toml | 2 +- crates/blitz-control-protocol/Cargo.toml | 85 +- crates/blitz-control-protocol/src/document.rs | 2669 +++++++++++++++++ crates/blitz-control-protocol/src/lib.rs | 34 +- 4 files changed, 2780 insertions(+), 10 deletions(-) create mode 100644 crates/blitz-control-protocol/src/document.rs diff --git a/Cargo.toml b/Cargo.toml index 4693840..3925e09 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -13,7 +13,7 @@ license = "MIT OR Apache-2.0" repository = "https://github.com/pathscale/ps-observability" [workspace.dependencies] -blitz-control-protocol = { version = "^0.4", path = "crates/blitz-control-protocol" } +blitz-control-protocol = { version = "^0.5", path = "crates/blitz-control-protocol" } endpoint-libs = { version = "^3", default-features = false, features = ["agent-control"] } serde = { version = "^1", features = ["derive"] } serde_json = "^1" diff --git a/crates/blitz-control-protocol/Cargo.toml b/crates/blitz-control-protocol/Cargo.toml index c158bbe..6e5af0a 100644 --- a/crates/blitz-control-protocol/Cargo.toml +++ b/crates/blitz-control-protocol/Cargo.toml @@ -1,7 +1,7 @@ [package] name = "blitz-control-protocol" -description = "Wire types for the Blitz agent-control and diagnostics protocol" -version = "0.4.0" +description = "The Blitz agent-control and diagnostics surface: one vocabulary, one core, two transports" +version = "0.5.0" edition.workspace = true rust-version.workspace = true license.workspace = true @@ -13,19 +13,88 @@ keywords = ["observability", "testing", "blitz", "mcp", "ui"] categories = ["development-tools::testing", "development-tools::debugging"] publish = true -# Deliberately tiny. This crate exists so a client can speak the protocol -# without building the runtime that serves it: adding anything here that pulls -# in a window, a renderer or a font stack defeats the whole point. The check -# is `cargo tree -p blitz-control-protocol`, which should stay in the dozens -# of crates rather than the hundreds. +# The vocabulary is what this crate is by default, and it stays deliberately +# tiny. A client speaks the protocol without building the renderer that serves +# it: adding anything to the *default* dependencies that pulls in a window, a +# renderer or a font stack defeats the whole point. The check is +# `cargo tree -p blitz-control-protocol`, which should stay in the dozens of +# crates rather than the hundreds, and `cargo tree -p ps-qa`, which CI fails if +# a renderer or a window runtime appears in it. +# +# Everything that needs blitz is behind a feature, and the features are cut on +# that boundary rather than on what each consumer happened to want: +# +# engine the core. Reads and drives a `blitz-dom` document. +# capture the core's expensive half: offscreen paint, layout and +# computed-style snapshots, renderer metrics. +[features] +default = [] +engine = [ + "dep:blitz-dom", + "dep:blitz-script", + "dep:blitz-traits", + "dep:keyboard-types", + "dep:style", + # The role rules live in blitz-dom, which owns them for the AccessKit tree + # as well. Naming the feature rather than relying on it being a default: + # a consumer that turns blitz-dom's defaults off must still get the roles. + "blitz-dom/accessibility", +] +capture = [ + "engine", + "dep:anyrender", + "dep:anyrender_vello_cpu", + "dep:base64", + "dep:blitz-paint", + # Frame timings and the deep-profiling session, which blitz-shell owns + # because it is what presents frames. This is the only reason the shell is + # in the graph, so a build that reads the semantic tree and drives input + # gets no window stack at all. + "dep:blitz-shell", + # Where the frame timings and the sampling guard are: `blitz-shell` keeps + # both behind this feature, and it forwards to `blitz-script/debug-control` + # for the script half of the same capture. + "blitz-shell/debug-control", +] + [dependencies] endpoint-libs.workspace = true serde.workspace = true serde_json.workspace = true schemars.workspace = true +# The core, and nothing else, reaches for these. +blitz-dom = { package = "ps-blitz-dom", version = "^0.4.8", optional = true } +# Naming stylo directly: the computed-style reader and the visibility check read +# `style::values::computed`, and blitz-dom does not re-export it. The version is +# the one blitz-dom resolves, so cargo unifies them instead of putting two +# incompatible copies of every computed value in the graph. +style = { version = "0.20.0", package = "stylo", optional = true } +# No `system-fonts`. It reaches `fontique/system` and, on Linux, the +# `yeslogic-fontconfig-sys` system library, so enabling it here would decide +# that every consumer needs one installed, including a headless one that reads +# no font catalogue. An application that wants the machine's fonts asks itself. +blitz-script = { package = "ps-blitz-script", version = "^0.4.8", optional = true } +blitz-traits = { package = "ps-blitz-traits", version = "^0.4.8", optional = true } +keyboard-types = { version = "0.7", optional = true } + +# Capture. The CPU renderer on purpose: a capture must not need a GPU, a +# surface or a display server, or it stops working in the places it is most +# needed. It draws the same scene through the same `blitz-paint` entry point +# the window uses, so what it returns is the real frame rather than a second +# opinion about it. +anyrender = { package = "ps-anyrender", version = "^0.13.0", optional = true } +anyrender_vello_cpu = { package = "ps-anyrender-vello-cpu", version = "^0.17.0", optional = true } +blitz-paint = { package = "ps-blitz-paint", version = "^0.4.8", optional = true } +blitz-shell = { package = "ps-blitz-shell", version = "^0.4.8", default-features = false, optional = true } +base64 = { version = "0.22", optional = true } + + [dev-dependencies] # The framing round-trip test drives a real duplex pipe rather than asserting # on a string, because the bug this protocol keeps hitting is at the seam # between the typed value and the bytes, not inside serde. -tokio = { version = "1", features = ["io-util", "macros", "rt"] } +tokio = { version = "1", features = ["io-util", "macros", "net", "rt", "time"] } + +[package.metadata.docs.rs] +all-features = true diff --git a/crates/blitz-control-protocol/src/document.rs b/crates/blitz-control-protocol/src/document.rs new file mode 100644 index 0000000..efceae2 --- /dev/null +++ b/crates/blitz-control-protocol/src/document.rs @@ -0,0 +1,2669 @@ +//! Inspecting, capturing and driving a document, with no window involved. +//! +//! This is the core the two transports sit on. It answers a request against a +//! `blitz-dom` document and returns a response, and it knows nothing about +//! sockets, listeners, event loops or windows: [`crate::in_process`] calls it +//! directly and [`crate::server`] calls it across a socket, and neither is +//! visible from here. +//! +//! It used to live in `tauri-runtime-blitz`, beside the Tauri runtime, which +//! meant that depending on it meant compiling Tauri. On Linux that means GTK: +//! system libraries pulled in to build a binary that never creates a window, +//! and a crate that would not compile there at all. The dependency edge was +//! wrong, not the platform. A runtime bridges Tauri to Blitz and owns a native +//! window; it is not where an inspection service lives. + +use std::collections::HashMap; + +use crate::{ + AgentSnapshot, DebugError, DebugResponse, KeyPhase, Modifiers as ControlModifiers, SemanticNode, +}; +#[cfg(feature = "capture")] +use crate::{ + DebugSnapshot, FrameMetrics, FrameWindowMetrics, LayoutBounds, LayoutDiagnosticRow, + LayoutEdges, LayoutOffset, LayoutSize, RendererMetrics, RevisionSet, ScriptMetrics, + ScriptSource, SnapshotCost, SnapshotRequest, TimingStats, +}; +use blitz_dom::Document; +use blitz_script::ScriptDocument; +use blitz_traits::events::{ + BlitzKeyEvent, BlitzPointerEvent, BlitzPointerId, DomEvent, DomEventData, KeyState, + MouseEventButton, MouseEventButtons, Point, PointerCoords, PointerDetails, UiEvent, +}; +#[cfg(feature = "capture")] +use blitz_traits::node_id::NodeId; +use keyboard_types::{Code, Key, Location, Modifiers as KeyboardModifiers}; + +/// The live inspector's reusable offscreen surface. +/// +/// A capture used to construct this whole renderer for every frame. Besides +/// reallocating the viewport-sized RGBA buffer, that threw away the CPU text +/// renderer's glyph resources, so a stability assertion shaped and rasterised +/// every label four times. The surface belongs to one runtime and is resized +/// only when the window or requested scale changes. +#[cfg(feature = "capture")] +pub(crate) struct CaptureSurface { + pub(crate) width: u32, + pub(crate) height: u32, + pub(crate) renderer: anyrender_vello_cpu::VelloCpuImageRenderer, + pub(crate) rgba: Vec, +} + +/// Reusable offscreen renderer for captures of one document. +/// +/// A headless inspection host asks for several adjacent frames when it checks +/// visual stability. Reusing this object preserves the CPU renderer's glyph +/// resources and pixel allocation between those requests instead of rebuilding +/// an entire renderer for every sample. +#[cfg(feature = "capture")] +pub struct DocumentCapture { + surface: Option, +} + +#[cfg(feature = "capture")] +impl DocumentCapture { + pub fn new() -> Self { + Self { surface: None } + } + + pub fn capture( + &mut self, + document: &mut ScriptDocument, + request: crate::CaptureRequest, + ) -> Result { + capture_document_with_surface(document, request, &mut self.surface) + } +} + +#[cfg(feature = "capture")] +impl Default for DocumentCapture { + fn default() -> Self { + Self::new() + } +} + +#[cfg(feature = "capture")] +impl CaptureSurface { + pub(crate) fn new(width: u32, height: u32) -> Self { + use anyrender::ImageRenderer as _; + + Self { + width, + height, + renderer: anyrender_vello_cpu::VelloCpuImageRenderer::new(width, height), + rgba: Vec::with_capacity((width as usize) * (height as usize) * 4), + } + } + + pub(crate) fn size_to(&mut self, width: u32, height: u32) { + use anyrender::ImageRenderer as _; + + if self.width == width && self.height == height { + return; + } + self.renderer.resize(width, height); + self.width = width; + self.height = height; + } +} + +/// Draw a standalone script document through the same CPU paint path used by +/// runtime diagnostics. +/// +/// Headless QA hosts intentionally have no `RuntimeApplication`, but they must +/// not substitute a second renderer for native visual checks. Keeping the +/// capture implementation here makes a host capture and a live-app capture +/// byte-for-byte comparable. +#[cfg(feature = "capture")] +pub fn capture_document( + script_document: &mut ScriptDocument, + request: crate::CaptureRequest, +) -> Result { + DocumentCapture::new().capture(script_document, request) +} + +#[cfg(feature = "capture")] +pub(crate) fn capture_document_with_surface( + script_document: &mut ScriptDocument, + request: crate::CaptureRequest, + surface: &mut Option, +) -> Result { + use anyrender::ImageRenderer; + use base64::Engine as _; + + // Clamped rather than trusted. A scale of zero produces a zero-sized + // buffer and a negative one panics inside the rasteriser, and neither + // should be reachable from a debug socket. + let scale = if request.scale.is_finite() && request.scale > 0.0 { + request.scale.clamp(0.1, 8.0) + } else { + 1.0 + }; + + let node_id = request.node_id; + + // Style and layout first, so the capture reflects pending mutations + // rather than the frame before them. Same call `collect_diagnostics` + // makes, for the same reason. + script_document.inner_mut().resolve(0.0); + + // Copied out rather than held: the guard is a `Ref` and the borrow has + // to end before the mutable one the paint below needs. + let (full_width, full_height) = { + let inner = script_document.inner(); + let viewport = inner.viewport(); + (viewport.window_size.0, viewport.window_size.1) + }; + if full_width == 0 || full_height == 0 { + return Err(debug_error( + "captureUnavailable", + "the document has no viewport to draw", + )); + } + + // The region to keep, in unscaled document pixels. + let (crop_x, crop_y, crop_width, crop_height) = match node_id { + None => ( + 0.0_f64, + 0.0_f64, + f64::from(full_width), + f64::from(full_height), + ), + Some(id) => { + let inner = script_document.inner(); + let node = inner + .get_node(NodeId::from_u64(id)) + .ok_or_else(|| debug_error("unknownNode", &format!("no node {id}")))?; + let layout = node.final_layout(); + let position = node.absolute_position(0.0, 0.0); + if layout.size.width <= 0.0 || layout.size.height <= 0.0 { + return Err(debug_error( + "captureEmpty", + &format!("node {id} has a zero-sized box, so there is nothing to capture"), + )); + } + let box_ = ( + f64::from(position.x), + f64::from(position.y), + f64::from(layout.size.width), + f64::from(layout.size.height), + ); + drop(inner); + box_ + } + }; + + let full_pixel_width = ((f64::from(full_width) * f64::from(scale)).round() as u32).max(1); + let full_pixel_height = ((f64::from(full_height) * f64::from(scale)).round() as u32).max(1); + // Clamp before painting: a node partly offscreen yields the visible part, + // and the regional renderer never allocates pixels that will be discarded. + let left = ((crop_x * f64::from(scale)).round().max(0.0) as u32).min(full_pixel_width); + let top = ((crop_y * f64::from(scale)).round().max(0.0) as u32).min(full_pixel_height); + let width = ((crop_width * f64::from(scale)).round() as u32) + .min(full_pixel_width.saturating_sub(left)) + .max(1); + let height = ((crop_height * f64::from(scale)).round() as u32) + .min(full_pixel_height.saturating_sub(top)) + .max(1); + // Leave room for the JSON-RPC and MCP envelopes inside the transport's + // fixed frame ceiling. The old 64-million-pixel limit allowed a 256 MiB + // raster and a 341 MiB base64 string, only for protocol encoding to reject + // the result against its 16 MiB frame limit after all that work was done. + const FRAME_ENVELOPE_RESERVE: usize = 64 * 1024; + const MAX_BASE64_BYTES: usize = crate::MAX_DEBUG_FRAME_BYTES - FRAME_ENVELOPE_RESERVE; + const MAX_RAW_BYTES: usize = (MAX_BASE64_BYTES / 4) * 3; + const MAX_PIXELS: u64 = (MAX_RAW_BYTES / 4) as u64; + if u64::from(width) * u64::from(height) > MAX_PIXELS { + return Err(debug_error( + "captureTooLarge", + &format!( + "{width}x{height} cannot fit in one diagnostic frame; capture a node or lower the scale" + ), + )); + } + + let surface = surface.get_or_insert_with(|| CaptureSurface::new(width, height)); + surface.size_to(width, height); + // `ImageRenderer` retains its scene between calls. A capture is a complete + // frame, not an incremental paint, so carrying the previous command list + // forward duplicates every shape and makes each sample slower than the + // last. Keep reusable renderer resources, but always begin with an empty + // scene. + surface.renderer.reset(); + let mut document = script_document.inner_mut(); + surface.renderer.render_to_vec( + |scene| { + if node_id.is_some() { + blitz_paint::paint_scene_region( + scene, + &mut document, + blitz_paint::PaintRegion::crop( + f64::from(scale), + f64::from(left) / f64::from(scale), + f64::from(top) / f64::from(scale), + width, + height, + ), + ); + } else { + blitz_paint::paint_scene( + scene, + &mut document, + f64::from(scale), + width, + height, + 0, + 0, + ); + } + }, + &mut surface.rgba, + ); + + Ok(crate::CapturedImage { + width, + height, + rgba_base64: base64::engine::general_purpose::STANDARD.encode(&surface.rgba), + node_id, + }) +} + +/// Collect the same typed diagnostic snapshot from a standalone Blitz document +/// that the windowed runtime exposes over its control socket. +/// +/// Headless component hosts own a `ScriptDocument` without a Tauri event loop. +/// Keeping snapshot collection here gives those hosts the renderer's real DOM, +/// layout and computed paint data instead of a partial or reimplemented view. +#[cfg(feature = "capture")] +pub fn snapshot_document( + document: &mut ScriptDocument, + request: SnapshotRequest, + revision: u64, +) -> Result { + let started = std::time::Instant::now(); + let poll_started = std::time::Instant::now(); + let mut polls = 0u64; + for _ in 0..100 { + polls += 1; + if !document.poll(None) { + break; + } + } + let poll_ms = poll_started.elapsed().as_secs_f64() * 1_000.0; + // This forces a style and layout pass so the snapshot reports current + // geometry. It is work the observer caused, so it is reported as snapshot + // cost, never as the cost of a frame the application drew. + let resolve_started = std::time::Instant::now(); + document.inner_mut().resolve(0.0); + let snapshot_resolve_ms = resolve_started.elapsed().as_secs_f64() * 1_000.0; + let inner = document.inner(); + let layout_node_limit = inner.tree().iter().count(); + let active_element = inner.get_focussed_node_id().map(|id| id.as_u64()); + // Once for the whole snapshot: the question a control asks is "which label + // points at me", and answering it from the control costs a document scan + // each time. + let labels = LabelIndex::build(&inner); + let nodes: Vec = inner + .tree() + .iter() + .filter_map(|(id, node)| { + if !request.node_ids.is_empty() && !request.node_ids.contains(&id.as_u64()) { + return None; + } + let element = node.element_data()?; + if !dom_chain_is_attached(&inner, id, layout_node_limit) + || !layout_chain_is_valid(&inner, id, layout_node_limit) + { + return None; + } + let rect = inner.get_client_bounding_rect(id); + let visible = node_is_visible(&inner, id) + && rect + .as_ref() + .is_some_and(|rect| rect.width > 0.0 && rect.height > 0.0); + let role = semantic_role(element); + let value = if role == "generic" { + Some( + element + .attrs() + .iter() + .map(|attribute| format!("{}={}", attribute.name.local, attribute.value)) + .collect::>() + .join(" "), + ) + } else { + semantic_value(element) + }; + Some(SemanticNode { + dom_id: element_attr(element, "id").map(str::to_owned), + id: id.as_u64(), + parent: semantic_parent(&inner, id, None).map(|id| id.as_u64()), + name: semantic_name(element, node, &role, &inner, id, &labels), + role, + value, + enabled: element_attr(element, "disabled").is_none() + && element_attr(element, "aria-disabled") != Some("true"), + visible, + selected: semantic_selected(element), + bounds: rect.and_then(|rect| { + let bounds = [rect.x, rect.y, rect.width, rect.height]; + bounds + .iter() + .all(|value| value.is_finite()) + .then_some(bounds) + }), + slot: element_attr(element, "data-slot").map(str::to_owned), + }) + }) + .collect(); + let total_ms = started.elapsed().as_secs_f64() * 1_000.0; + // The runtime keeps one counter and stamps it onto all four revision + // fields. Style, layout and paint are not versioned independently + // anywhere in blitz, so four copies of one number would claim a + // resolution that does not exist. Report the counter once, as the + // document revision, and leave the rest at zero. + let revisions = RevisionSet { + document: revision, + style: 0, + layout: 0, + paint: 0, + }; + // Real per-frame timings, published by blitz-shell from `View::redraw`. + // These describe frames the application actually presented. Everything + // measured inside this function describes the snapshot collection instead, + // and is reported under `snapshot` so the two never get mixed up again. + let frame_stats = blitz_shell::latest_frame_stats(); + let metrics = RendererMetrics { + revisions: revisions.clone(), + queue_depth: None, + invalidations_coalesced: polls.saturating_sub(1), + frame: frame_stats.as_ref().map(|stats| FrameMetrics { + input_to_present_ms: None, + style_ms: None, + layout_ms: None, + resolve_ms: stats.latest.resolve_ms, + scene_ms: stats.latest.paint_ms, + submit_ms: None, + present_ms: None, + renderer_ms: stats.latest.renderer_ms, + total_ms: stats.latest.total_ms, + age_ms: stats.latest.age_ms, + }), + frame_window: frame_stats.as_ref().map(|stats| FrameWindowMetrics { + frames_total: stats.frames_total, + window_frames: stats.window_frames, + resolve: timing_stats(stats.resolve), + scene: timing_stats(stats.paint), + renderer: timing_stats(stats.renderer), + total: timing_stats(stats.frame_total), + interval: timing_stats(stats.interval), + active_fps: stats.active_fps, + missed_refreshes: stats.missed_refreshes, + display_refresh_hz: stats.display_refresh_hz, + }), + snapshot: Some(SnapshotCost { + poll_ms, + resolve_ms: snapshot_resolve_ms, + total_ms, + }), + // The other half of a frame. Everything above this line is the + // engine; this is the language runtime the application actually + // spends its time in. + script: blitz_script::script_stats::latest_script_stats().map(|stats| ScriptMetrics { + mean_ms: stats.mean_ms, + p95_ms: stats.p95_ms, + max_ms: stats.max_ms, + window_polls: stats.window_polls, + total_polls: stats.total_polls, + productive_polls: stats.productive_polls, + spent_ms: stats.spent_ms, + breakdown: blitz_script::script_stats::work_breakdown() + .into_iter() + .take(12) + .map(|(label, calls, total_ms, worst_ms)| ScriptSource { + label, + calls, + total_ms, + worst_ms, + }) + .collect(), + }), + resident_bytes: resident_bytes(), + }; + let dom = request + .include_dom + .then(|| serde_json::to_value(&nodes).unwrap_or(serde_json::Value::Null)); + let layout = request.include_layout.then(|| { + nodes + .iter() + .filter_map(|node| diagnostic_layout_row(&inner, node)) + .collect() + }); + /* + * Resolved colours, folded into the layout rows. + * + * This used to answer `computedStyleUnavailable`, which left one class + * of bug unanswerable from outside: an element whose *declared* colour + * is correct and whose *painted* colour is not. Reading the stylesheet + * cannot settle that - the cascade, the custom-property chain and the + * `@supports` gating all sit between the two - and neither can a DOM + * test environment, which has no cascade at all. + * + * Only the four that decide legibility, rather than a full style dump: + * a snapshot of every longhand for 4,500 nodes is megabytes of JSON + * nobody reads, and these are what a "why is this text invisible" + * question actually needs. + */ + let computed_style = request.include_computed_style.then(|| { + serde_json::Value::Array( + nodes + .iter() + .filter_map(|node| diagnostic_style_row(&inner, node)) + .collect(), + ) + }); + Ok(DebugSnapshot { + revisions, + active_window: Some("blitz-main".into()), + active_element, + dom, + layout, + computed_style, + metrics, + }) +} + +pub(crate) fn element_attr<'a>(element: &'a blitz_dom::ElementData, name: &str) -> Option<&'a str> { + element + .attrs() + .iter() + .find(|attribute| attribute.name.local.as_ref() == name) + // `as_ref`, not `as_str`. Attribute values are an interned atom as of + // ps-blitz-dom 0.3.0-beta.11, and `str::as_str` is still unstable, so + // `as_str` here resolved to the nightly-only inherent method and + // failed to build on stable. `as_ref` borrows the atom as a `&str`, + // which is what this signature returns. + .map(|attribute| attribute.value.as_ref()) +} + +pub(crate) fn semantic_role(element: &blitz_dom::ElementData) -> String { + semantic_role_ref(element).to_owned() +} + +/// The same answer without owning it. +/// +/// A text node asks every element above it whether that element is already +/// named by the words in question, and the owned form allocated a `String` per +/// ancestor per text node to be compared against a fixed list and dropped. The +/// role is either a `&'static str` or the `role` attribute's own text, so +/// nothing here needs a copy. +pub(crate) fn semantic_role_ref(element: &blitz_dom::ElementData) -> &str { + // An author's explicit role wins, and travels verbatim. ARIA is a + // vocabulary the page may extend with roles no HTML element implies, and a + // harness addressing `role="switch"` needs the word the page used. + if let Some(role) = element_attr(element, "role") { + return role; + } + wire_role(blitz_dom::accessibility::implicit_role(element)) +} + +/// What this surface calls one of blitz-dom's roles. +/// +/// # Why there is a projection at all +/// +/// The rules are blitz-dom's, in `accessibility::implicit_role`, and there is +/// one copy of them. What is here is only the naming: blitz-dom answers in +/// AccessKit's vocabulary, which is what a platform screen-reader adapter +/// consumes, and this wire answers in ARIA role names, which is what a check +/// is written against. +/// +/// The mapping is deliberately lossy, and the loss is the compatibility +/// guarantee. Eleven fleet sites and roughly 1,700 checks are written against +/// the role strings this surface has always reported, so every arm below +/// reproduces one of them. `_ => "generic"` is not a fallback for roles nobody +/// thought about: it is the answer this surface has always given for +/// `
`, `

`, `