Fix false-green network failures + robustness gaps (review round 2)

docs/profile-schema.md

@@ -42,7 +42,7 @@ Required selectors for chatbot-style profiles:
 
 `answerDoneText` is optional. When set, `question` scenarios wait for an extra occurrence of this text to confirm the answer finished; when omitted, they wait only for the submitted text to appear.
 
-Optional consent selectors:
+Consent selectors:
 
 ```json
 {
@@ -51,6 +51,19 @@ Optional consent selectors:
 }
 ```
 
+`consentAgreeButton` is **required** when any `consent` scenario is present (validation fails otherwise). `consentLabelInput` is optional and only used when a consent scenario carries a `label`.
+
+> A selector must include at least one positive matcher (`role` or `nameIncludes`); an exclude-only selector matches nothing.
+
+## Common scenario fields
+
+These apply to every scenario type:
+
+- `type` (required): one of the scenario types below.
+- `name`: label used for artifacts and report entries (defaults to `type`).
+- `viewport`: emulate a device viewport before the scenario runs. Format `"<width>x<height>x<devicePixelRatio>[,mobile][,touch][,landscape]"`, e.g. `"390x844x2,mobile,touch"`. Validated at load time; a malformed value is rejected.
+- `timeoutMs`: per-scenario override for the wait budget (falls back to the global `--timeout`; `consent` uses a fixed 10s).
+
 ## Scenarios
 
 ### `consent`
@@ -80,9 +93,9 @@ Fills `selectors.chatInput`, presses Enter, then waits for the submitted text to
 
 ### `empty-input`
 
-Attempts to submit empty/blank input and asserts the accessibility tree does not materially change (i.e. no new answer was produced).
+Attempts to submit empty/blank input and asserts no new answer was produced. Two oracles run: if `selectors.answerDoneText` is set, the count of that marker must not increase (catches an app that echoes blank input by reusing nodes); and the accessibility tree must not grow by more than `maxNodeDelta` nodes.
 
-`maxNodeDelta` (default `2`) is the number of additional accessibility nodes tolerated after submitting blank input — raise it if the UI legitimately shows an aria-live validation hint on empty submit.
+`maxNodeDelta` (default `2`, must be an integer) is the number of additional accessibility nodes tolerated after submitting blank input — raise it if the UI legitimately shows an aria-live validation hint on empty submit.
 
 ```json
 {
@@ -96,21 +109,22 @@ Attempts to submit empty/blank input and asserts the accessibility tree does not
 
 ### `fill`
 
-Takes a snapshot, finds `target`, fills it with `value`, and optionally presses `submitKey`.
+Takes a snapshot, finds `target`, fills it with `value`, and optionally presses `submitKey`. Set `screenshot: true` to capture an after-screenshot (default: off for `fill`).
 
 ```json
 {
   "type": "fill",
   "name": "fill-search",
   "target": { "role": "textbox", "nameIncludes": "Search" },
   "value": "hello world",
-  "submitKey": "Enter"
+  "submitKey": "Enter",
+  "screenshot": true
 }
 ```
 
 ### `click`
 
-Takes a snapshot, finds `target`, and clicks it.
+Takes a snapshot, finds `target`, and clicks it. Captures an after-screenshot unless `screenshot: false`.
 
 ```json
 {
@@ -147,15 +161,18 @@ Polls snapshots until text appears.
 
 ### `screenshot`
 
-Captures a screenshot artifact.
+Captures a screenshot artifact. `fileName` overrides the artifact base name (defaults to the scenario `name`).
 
 ```json
 {
   "type": "screenshot",
-  "name": "after-submit"
+  "name": "after-submit",
+  "fileName": "home-hero"
 }
 ```
 
+> Screenshot defaults differ by scenario: `click`, `wait-for-text`, and the chatbot scenarios capture an after-screenshot unless `screenshot: false`; `fill` captures one only when `screenshot: true`.
+
 ### `assert-no-console-errors`
 
 Fails the scenario if DevTools reports console messages of type `error`. Use `ignoreTextIncludes` for known noisy messages.
@@ -170,7 +187,7 @@ Fails the scenario if DevTools reports console messages of type `error`. Use `ig
 
 ### `assert-no-http-errors`
 
-Fails the scenario if DevTools reports HTTP 4xx/5xx responses. Use `ignoreFavicon404`, `ignoreUrlIncludes`, or `failOn4xx: false` to tune strictness.
+Fails the scenario if DevTools reports HTTP 4xx/5xx responses **or transport-layer failures** (`net::ERR_*` such as connection-refused / DNS failure / blocked). Use `ignoreFavicon404`, `ignoreUrlIncludes`, or `failOn4xx: false` to tune strictness. `failOn4xx: false` relaxes only numeric 4xx — transport failures are always reported.
 
 ```json
 {
@@ -195,7 +212,8 @@ Fails the scenario if DevTools reports HTTP 4xx/5xx responses. Use `ignoreFavico
 
 Current quality checks:
 
-- Console errors become warnings.
-- HTTP 5xx responses become failures.
-- HTTP 4xx responses become warnings unless ignored favicon 404.
+- Console `error` and `assert` messages become warnings.
+- HTTP 5xx responses and transport-layer failures (`net::ERR_*`) become failures.
+- HTTP 4xx responses become warnings unless an ignored favicon 404 (matched by URL pathname, so query strings/cache-busters are still ignored).
+- In-flight (`pending`) requests are not treated as failures.
 - Lighthouse scores below thresholds become warnings/failures.

src/cli.mjs

@@ -3,13 +3,35 @@ import path from 'node:path';
 import { parseArgs } from './core/args.mjs';
 import { loadProfile } from './core/profile.mjs';
 import { runQa } from './core/runner.mjs';
+import { safeName } from './core/utils.mjs';
 
 const args = parseArgs(process.argv.slice(2));
 const url = args.url || 'http://localhost:8080';
-const timeoutMs = Number(args.timeout || 120000);
+
+// A missing/invalid --timeout would otherwise become 1 (Number(true), when the
+// flag is passed with no value) or NaN, making every wait scenario fail instantly
+// with a misleading "timeout" error. Require an explicit numeric value.
+const timeoutMs = args.timeout === undefined ? 120000 : Number(args.timeout);
+if (args.timeout === true || !Number.isFinite(timeoutMs) || timeoutMs <= 0) {
+  console.error(`Invalid --timeout: ${JSON.stringify(args.timeout)} (expected a positive number of milliseconds)`);
+  process.exit(2);
+}
+
 const profileArg = args.profile || 'qa/devtools-qa-runner/profiles/lms-chatbot.json';
 const { profile, profilePath } = await loadProfile(profileArg);
-const outDir = path.resolve(args.out || `reports/devtools-qa-runner/${profile.name || 'default'}/latest`);
+
+// outDir is wiped (fs.rm recursive) at the start of a run. profile.name flows
+// into the default path, so sanitize it; and refuse any outDir that would delete
+// the working directory itself, an ancestor of it, or the filesystem root.
+const defaultOut = path.join('reports', 'devtools-qa-runner', safeName(profile.name || 'default'), 'latest');
+const outDir = path.resolve(typeof args.out === 'string' ? args.out : defaultOut);
+const cwd = process.cwd();
+const cwdRelativeToOut = path.relative(outDir, cwd); // '' if equal; non-'..' & non-absolute if cwd is inside outDir
+const isAncestorOfCwd = cwdRelativeToOut === '' || (!cwdRelativeToOut.startsWith('..') && !path.isAbsolute(cwdRelativeToOut));
+if (isAncestorOfCwd || outDir === path.parse(outDir).root) {
+  console.error(`Refusing to use outDir that would wipe the working directory or an ancestor: ${outDir}`);
+  process.exit(2);
+}
 
 try {
   const report = await runQa({ url, profile, profilePath, outDir, timeoutMs });

src/core/devtools-client.mjs

@@ -4,6 +4,13 @@ import { parseJsonOutput } from './utils.mjs';
 
 const execFileAsync = promisify(execFile);
 
+// chrome-devtools returns each result as a single JSON.stringify(structuredContent)
+// line on stdout. A take_snapshot of a large a11y tree, or list_network_requests /
+// list_console_messages with preserved history on a real app, easily exceeds Node's
+// default 1 MiB stdout ceiling and would otherwise crash the run with
+// ERR_CHILD_PROCESS_STDIO_MAXBUFFER. Give it generous headroom.
+const MAX_OUTPUT_BYTES = 64 * 1024 * 1024;
+
 export class DevToolsClient {
   constructor({ timeoutMs, cwd = process.cwd(), report }) {
     this.timeoutMs = timeoutMs;
@@ -17,6 +24,7 @@ export class DevToolsClient {
       const { stdout, stderr } = await execFileAsync('npx', ['chrome-devtools', ...cliArgs, '--output-format=json'], {
         cwd: this.cwd,
         timeout: Math.max(this.timeoutMs, 60000),
+        maxBuffer: MAX_OUTPUT_BYTES,
         env: {
           ...process.env,
           CI: '1',

src/core/network.mjs

@@ -0,0 +1,38 @@
+// Shared classification of chrome-devtools network request `status` values.
+//
+// NetworkFormatter.toJSON() (chrome-devtools-mcp) emits `status` as a STRING that
+// is one of three shapes (see formatters/NetworkFormatter.js #getStatusFromRequest):
+//   - a numeric HTTP code as a string ("200", "404", "502") when a response arrived
+//   - the transport failure text ("net::ERR_CONNECTION_REFUSED", "net::ERR_NAME_NOT_RESOLVED", ...)
+//     when the request failed before/without an HTTP response
+//   - the literal "pending" when the request is still in flight
+//
+// Numeric-only comparisons (Number(status) >= 500) silently miss the transport
+// failures because Number("net::ERR_...") is NaN and NaN >= N is always false.
+// Both the quality gate and the assert-no-http-errors scenario must treat a
+// transport failure as an error, not a pass.
+
+export function classifyNetworkStatus(rawStatus) {
+  const text = String(rawStatus ?? '').trim();
+  // Check empty/pending first: Number('') is 0 (finite), which would otherwise
+  // be misread as a successful response.
+  if (text === '' || text.toLowerCase() === 'pending') {
+    return { kind: 'pending', code: null, text: text || 'pending' };
+  }
+  const code = Number(text);
+  if (Number.isFinite(code)) {
+    if (code >= 500) return { kind: 'server-error', code, text };
+    if (code >= 400) return { kind: 'client-error', code, text };
+    return { kind: 'ok', code, text };
+  }
+  // Anything else is a transport-layer failure surfaced as failure.errorText.
+  return { kind: 'transport-error', code: null, text };
+}
+
+export function isFaviconUrl(url) {
+  try {
+    return new URL(String(url), 'http://_').pathname.endsWith('/favicon.ico');
+  } catch {
+    return String(url).endsWith('/favicon.ico');
+  }
+}

src/core/profile.mjs

@@ -33,6 +33,11 @@ export function validateProfile(profile, source = '<profile>') {
     throw new Error(`${source}: selectors.chatInput is required for chatbot scenarios`);
   }
 
+  const hasConsent = profile.scenarios.some((scenario) => scenario.type === 'consent');
+  if (hasConsent && !profile.selectors?.consentAgreeButton) {
+    throw new Error(`${source}: selectors.consentAgreeButton is required for consent scenarios`);
+  }
+
   for (const [index, scenario] of profile.scenarios.entries()) {
     if (!scenario.type) throw new Error(`${source}: scenarios[${index}].type is required`);
     if (!supportedScenarioTypes.includes(scenario.type)) {
@@ -42,6 +47,10 @@ export function validateProfile(profile, source = '<profile>') {
   }
 }
 
+// Matches the dep's emulate viewport grammar:
+//   '<width>x<height>x<devicePixelRatio>[,mobile][,touch][,landscape]'
+const VIEWPORT_RE = /^\d+x\d+x\d+(?:,(?:mobile|touch|landscape))*$/;
+
 function validateScenarioFields(scenario, index, source) {
   const prefix = `${source}: scenarios[${index}]`;
   if (['click', 'fill'].includes(scenario.type) && !scenario.target) {
@@ -53,4 +62,10 @@ function validateScenarioFields(scenario, index, source) {
   if (scenario.type === 'wait-for-text' && !scenario.text) {
     throw new Error(`${prefix}.wait-for-text requires text`);
   }
+  if (scenario.viewport !== undefined && !VIEWPORT_RE.test(String(scenario.viewport))) {
+    throw new Error(`${prefix}.viewport "${scenario.viewport}" is invalid (expected <w>x<h>x<dpr>[,mobile][,touch][,landscape])`);
+  }
+  if (scenario.maxNodeDelta !== undefined && !Number.isInteger(scenario.maxNodeDelta)) {
+    throw new Error(`${prefix}.maxNodeDelta must be an integer`);
+  }
 }

src/core/quality.mjs

@@ -1,16 +1,29 @@
+import { classifyNetworkStatus, isFaviconUrl } from './network.mjs';
+
+// Console message types that indicate a real problem worth surfacing. The dep's
+// ConsoleFormatter emits `type` from the underlying CDP console message; uncaught
+// errors are normalized to 'error'. 'assert' (console.assert failures) is treated
+// as an error-equivalent; everything else is left to lighthouse/explicit scenarios.
+const CONSOLE_ERROR_TYPES = new Set(['error', 'assert']);
+
 export function analyzeQuality(r, quality = {}) {
   const failures = [];
   const warnings = [];
 
   for (const msg of r.consoleMessages?.consoleMessages || []) {
-    if (msg.type === 'error') warnings.push(`Console error: ${msg.text}`);
+    if (CONSOLE_ERROR_TYPES.has(msg.type)) warnings.push(`Console ${msg.type}: ${msg.text}`);
   }
 
   for (const req of r.networkRequests?.networkRequests || []) {
-    const status = Number(req.status);
-    const isFavicon = String(req.url).endsWith('/favicon.ico');
-    if (status >= 500) failures.push(`${req.method} ${req.url} -> ${req.status}`);
-    else if (status >= 400 && !(quality.ignoreFavicon404 && isFavicon)) warnings.push(`${req.method} ${req.url} -> ${req.status}`);
+    const cls = classifyNetworkStatus(req.status);
+    const label = `${req.method} ${req.url} -> ${req.status}`;
+    if (cls.kind === 'server-error' || cls.kind === 'transport-error') {
+      failures.push(label);
+    } else if (cls.kind === 'client-error') {
+      if (quality.ignoreFavicon404 && cls.code === 404 && isFaviconUrl(req.url)) continue;
+      warnings.push(label);
+    }
+    // 'ok' and 'pending' requests are not quality failures here.
   }
 
   for (const score of r.lighthouse?.lighthouseResult?.summary?.scores || []) {

src/core/runner.mjs

@@ -35,26 +35,48 @@ export async function runQa({ url, profile, profilePath, outDir, timeoutMs }) {
         return runScenarioSpec({ spec, item, profile, client, artifacts, timeoutMs });
       });
     }
-    report.consoleMessages = await client.run('list_console_messages', ['list_console_messages', '--includePreservedMessages']);
-    report.networkRequests = await client.run('list_network_requests', ['list_network_requests', '--includePreservedRequests']);
-    report.lighthouse = await client.run('lighthouse_audit', [
-      'lighthouse_audit',
-      '--mode',
-      'snapshot',
-      '--device',
-      'desktop',
-      '--outputDirPath',
-      path.join(outDir, 'lighthouse'),
-    ]);
+    // Post-scenario collection. Each call is isolated so one failure (e.g. a
+    // Lighthouse timeout, or an oversized response) records its error and lets
+    // the rest — and the persisted report — still complete.
+    report.consoleMessages = await collect(report, 'list_console_messages', () =>
+      client.run('list_console_messages', ['list_console_messages', '--includePreservedMessages']));
+    report.networkRequests = await collect(report, 'list_network_requests', () =>
+      client.run('list_network_requests', ['list_network_requests', '--includePreservedRequests']));
+    report.lighthouse = await collect(report, 'lighthouse_audit', () =>
+      client.run('lighthouse_audit', [
+        'lighthouse_audit',
+        '--mode',
+        'snapshot',
+        '--device',
+        'desktop',
+        '--outputDirPath',
+        path.join(outDir, 'lighthouse'),
+      ]));
     report.quality = analyzeQuality(report, profile.quality || {});
   } finally {
     await client.stop();
+    // Always persist the report best-effort, even if a step above threw.
+    await writeReport({ outDir, report, profilePath, timeoutMs }).catch((err) => {
+      report.reportWriteError = err?.message || String(err);
+    });
   }
 
-  await writeReport({ outDir, report, profilePath, timeoutMs });
   return report;
 }
 
+// Run a post-scenario collection step, capturing any failure onto the report
+// instead of aborting the whole run. Returns null on failure so downstream
+// analysis treats the data as absent rather than crashing.
+async function collect(report, name, fn) {
+  try {
+    return await fn();
+  } catch (err) {
+    report.collectionErrors = report.collectionErrors || {};
+    report.collectionErrors[name] = err?.message || String(err);
+    return null;
+  }
+}
+
 async function scenario(report, name, fn) {
   const item = { name, status: 'pass', durationMs: 0, screenshots: [], snapshots: [], error: null };
   const started = Date.now();

src/core/snapshot.mjs

@@ -9,7 +9,12 @@ export function flatten(root) {
   return nodes;
 }
 
-export function findBySpec(root, spec = {}) {
+export function findBySpec(root, spec) {
+  // A selector needs at least one POSITIVE matcher (role or nameIncludes).
+  // Without one — a missing selector, {}, or an exclude-only spec — findBySpec
+  // would otherwise match the first non-excluded node (usually the root) and the
+  // scenario would act on the wrong element.
+  if (!spec || (!spec.role && !spec.nameIncludes)) return undefined;
   return flatten(root).find((node) => {
     if (spec.role && node.role !== spec.role) return false;
     if (spec.nameIncludes && !String(node.name || '').includes(spec.nameIncludes)) return false;

src/scenarios/chatbot.mjs

@@ -43,12 +43,27 @@ async function runQuestionScenario({ spec, item, profile, client, artifacts, tim
 
 async function runEmptyInputScenario({ spec, item, profile, client, artifacts }) {
   const before = await artifacts.snapshot(`${item.name}-before`, item);
+  const answerDoneText = profile.selectors?.answerDoneText || '';
   const beforeTextCount = flatten(before).length;
+  const beforeAnswers = countTextOccurrences(before, answerDoneText);
   await askFromSnapshot({ snap: before, text: spec.text || '   ', profile, client });
   await sleep(spec.waitMs || 500);
   const after = await artifacts.snapshot(`${item.name}-after`, item);
-  // Empty input should not produce a new answer. A few extra nodes (e.g. an
-  // aria-live validation hint) are tolerated; tune via spec.maxNodeDelta.
+
+  // Primary oracle: if an answerDoneText marker exists, a real answer would add
+  // a new occurrence of it. This catches the case where the app echoes the blank
+  // input by reusing/transforming nodes (so the node-count delta stays small).
+  if (answerDoneText) {
+    const afterAnswers = countTextOccurrences(after, answerDoneText);
+    assert(
+      afterAnswers <= beforeAnswers,
+      `empty input should not produce a new answer (answer markers ${beforeAnswers} -> ${afterAnswers})`,
+    );
+  }
+
+  // Secondary oracle: the accessibility tree should not grow materially. A few
+  // extra nodes (e.g. an aria-live validation hint) are tolerated; tune via
+  // spec.maxNodeDelta.
   const maxNodeDelta = Number.isInteger(spec.maxNodeDelta) ? spec.maxNodeDelta : 2;
   assert(
     flatten(after).length <= beforeTextCount + maxNodeDelta,