diff --git a/docs/follow-ups/missing-state-change-executor-gaps.md b/docs/follow-ups/missing-state-change-executor-gaps.md new file mode 100644 index 0000000..6fd1aa6 --- /dev/null +++ b/docs/follow-ups/missing-state-change-executor-gaps.md @@ -0,0 +1,64 @@ +# Follow-up: `missing_state_change` executor-signal gaps (B2) + +**Status:** open, deliberately deferred (2026-06-02) +**Severity:** false-positive source, low volume +**Related:** the v0.54 new-window/download fix (`openedNewWindowOrDownload`) closes +the third FP in this cluster; the two below are left open on purpose. + +## Context + +The honest full-surface spoonworks re-validation (run `trucp69ve6r6j9vk0i4ttkfc`) +produced 3 `missing_state_change` clusters, all false positives. Adversarial +triage confirmed all three controls are genuinely wired and functional: + +1. **"Download label"** (`/admin/orders/[id]`) — `onClick` → `window.open(url, '_blank')`. + Opens a new tab; the current page legitimately shows no change. + **→ FIXED** in v0.54 via the `openedNewWindowOrDownload` signal. +2. **"Create return label"** (`/admin/orders/[id]`) — `onClick` → `setLoading(true)` + (button text → "Creating…", `disabled` set) → `await fetch(POST ...)`. +3. **"Increase quantity"** (`/products/[slug]`) — `useState` increment, async React + re-render of the quantity display. + +## Root cause for #2 and #3 + +Two real gaps in how `execute.ts` assembles `PostState` for the UI click path: + +- **`networkRequests` is hardcoded `[]`.** `execute.ts` builds the click `PostState` + with `networkRequests: []` (and calls `classifyNetworkRequests([], ...)`). So + `classifyMissingStateChange`'s `networkCompleted = postState.networkRequests.length > 0` + is **always false** on this path — a button that does nothing but fire a POST + (no URL change, no DOM topology change) is indistinguishable from a dead button. + This is why "Create return label" (a real POST) is flagged. + +- **`domMutationCount` counts only `childList` mutations.** Per the v0.53 design, + "meaningful" mutations are `addedNodes + removedNodes > 0`. A loading-state flip + (`disabled` attribute + text "Creating…") is an `attributes`/`characterData` + mutation, not `childList`, so it is not counted. Same for a text-only quantity + re-render that swaps a text node's content without adding/removing nodes. + +## Why deferred rather than fixed now + +Both fixes touch the **core detector's true-positive surface**, and this detector's +entire job is to catch genuinely-dead controls: + +- Threading real `networkRequests` into the click `PostState` requires capturing + per-action network activity in the UI executor (it is captured elsewhere but not + wired here). Worth doing, but it is an executor change, not a classifier change, + and needs its own test fixtures. +- Counting `attributes`/`characterData` mutations as "state change" would suppress + these FPs but also risks suppressing real TPs (a `disabled` flip with no user- + visible effect is arguably still "nothing happened"). Needs calibration. + +Given (a) this is a low-volume FP category (2 clusters), (b) the target app has been +swept repeatedly so real bugs here are already depleted, and (c) the project's +history of timing/observer tweaks regressing TPs, the disciplined call is to record +the root cause and defer rather than risk a core-detector regression for 2 FPs. + +## Suggested approach when picked up + +1. Wire real per-action `networkRequests` into the UI click `PostState` (the + highest-value, lowest-ambiguity fix — a POST is an unambiguous state change). +2. Only then consider an `attributes`-mutation signal, gated so a pure `disabled` + flip with no other change does **not** count (preserve the dead-button TP). +3. Add fixtures mirroring "Create return label" (POST+loading) and a text-only + stepper before changing the classifier. diff --git a/packages/cli/src/classify/seo.test.ts b/packages/cli/src/classify/seo.test.ts new file mode 100644 index 0000000..b48dea7 --- /dev/null +++ b/packages/cli/src/classify/seo.test.ts @@ -0,0 +1,105 @@ +// Unit tests for SEO indexability gating — closes the spoonworks FP where +// seo_h1_missing_or_multiple (and other hygiene rules) fired on auth-gated +// /admin/* pages that search engines never index. + +import { describe, it, expect } from 'vitest'; +import { classifySeoCorpus, isSeoIndexable } from './seo.js'; +import type { SeoPageInput } from './seo.js'; + +function makePage(overrides: Partial = {}): SeoPageInput { + return { + pageRoute: '/', + title: 'Home', + metaDescription: 'desc', + canonicalHref: null, + h1Count: 1, + metaRobots: null, + ...overrides, + }; +} + +const ORIGIN = 'http://localhost:3456'; + +describe('classifySeoCorpus — indexability gating', () => { + it('does not emit seo_h1_missing_or_multiple for a non-indexable (auth-gated) page', () => { + const dets = classifySeoCorpus({ + pages: [makePage({ pageRoute: '/admin/accounting/pl', h1Count: 2, indexable: false })], + robotsTxt: null, + origin: ORIGIN, + }); + expect(dets.some(d => d.kind === 'seo_h1_missing_or_multiple')).toBe(false); + }); + + it('still emits seo_h1_missing_or_multiple for an indexable public page', () => { + const dets = classifySeoCorpus({ + pages: [makePage({ pageRoute: '/products/x', h1Count: 2, indexable: true })], + robotsTxt: null, + origin: ORIGIN, + }); + expect(dets.some(d => d.kind === 'seo_h1_missing_or_multiple')).toBe(true); + }); + + it('treats indexable:undefined as indexable (backward compatibility)', () => { + const dets = classifySeoCorpus({ + pages: [makePage({ pageRoute: '/p', h1Count: 0 })], + robotsTxt: null, + origin: ORIGIN, + }); + expect(dets.some(d => d.kind === 'seo_h1_missing_or_multiple')).toBe(true); + }); + + it('suppresses all per-page hygiene findings for a non-indexable page', () => { + const dets = classifySeoCorpus({ + pages: [makePage({ pageRoute: '/admin/x', title: null, metaDescription: null, h1Count: 2, indexable: false })], + robotsTxt: null, + origin: ORIGIN, + }); + expect(dets.length).toBe(0); + }); + + it('excludes a non-indexable page from the duplicate-title cross-check', () => { + const dets = classifySeoCorpus({ + pages: [ + makePage({ pageRoute: '/', title: 'Healthy Spoon', indexable: true }), + makePage({ pageRoute: '/admin/orders', title: 'Healthy Spoon', indexable: false }), + ], + robotsTxt: null, + origin: ORIGIN, + }); + expect(dets.some(d => d.kind === 'seo_title_duplicate_across_routes')).toBe(false); + }); + + it('still flags duplicate titles across two indexable public pages', () => { + const dets = classifySeoCorpus({ + pages: [ + makePage({ pageRoute: '/', title: 'Healthy Spoon', indexable: true }), + makePage({ pageRoute: '/contact', title: 'Healthy Spoon', indexable: true }), + ], + robotsTxt: null, + origin: ORIGIN, + }); + expect(dets.some(d => d.kind === 'seo_title_duplicate_across_routes')).toBe(true); + }); +}); + +describe('isSeoIndexable', () => { + it('returns false when metaRobots declares noindex', () => { + expect(isSeoIndexable({ anonStatus: 200, metaRobots: 'noindex, nofollow' })).toBe(false); + }); + + it('returns true for an anonymously-reachable 200 page with no noindex', () => { + expect(isSeoIndexable({ anonStatus: 200, metaRobots: null })).toBe(true); + }); + + it('returns false when an anonymous request is redirected away (auth-gated)', () => { + expect(isSeoIndexable({ anonStatus: 307, metaRobots: null })).toBe(false); + }); + + it('returns false for a non-2xx anonymous response', () => { + expect(isSeoIndexable({ anonStatus: 404, metaRobots: null })).toBe(false); + }); + + it('defaults to indexable when the anon probe is unavailable (null status)', () => { + expect(isSeoIndexable({ anonStatus: null, metaRobots: null })).toBe(true); + }); +}); diff --git a/packages/cli/src/classify/seo.ts b/packages/cli/src/classify/seo.ts index be8f1b0..6a5824c 100644 --- a/packages/cli/src/classify/seo.ts +++ b/packages/cli/src/classify/seo.ts @@ -10,8 +10,30 @@ export type SeoPageInput = { canonicalHref: string | null; h1Count: number; metaRobots: string | null; + /** + * Whether the page is search-indexable. SEO hygiene rules (title/meta/ + * canonical/h1, duplicate-title) only matter for indexable pages. Auth-gated + * routes (e.g. /admin/*) and noindex pages are not indexed, so those rules do + * not apply. Undefined ≡ indexable (backward compatible with callers that do + * not compute it). Populated upstream via {@link isSeoIndexable}. + */ + indexable?: boolean; }; +/** + * Decide whether a page is search-indexable from an anonymous-reachability + * probe and its robots meta. Search engines crawl anonymously, so a page that + * declares `noindex`, or that an unauthenticated request cannot fetch with a 2xx + * (e.g. an auth gate redirects it away), is not indexable and SEO hygiene rules + * do not apply to it. A null status (probe unavailable) is treated as indexable + * so a flaky probe never hides a real public-page issue. + */ +export function isSeoIndexable(input: { anonStatus: number | null; metaRobots: string | null }): boolean { + if (input.metaRobots?.toLowerCase().includes('noindex') === true) return false; + if (input.anonStatus === null) return true; + return input.anonStatus >= 200 && input.anonStatus < 300; +} + export type SeoCorpusInput = { pages: SeoPageInput[]; robotsTxt: string | null; @@ -54,8 +76,11 @@ export function classifySeoCorpus(input: SeoCorpusInput): BugDetection[] { // Per-page detections for (const page of pages) { const { pageRoute, title, metaDescription, canonicalHref, h1Count, metaRobots } = page; + // SEO hygiene rules only apply to search-indexable pages. Auth-gated/noindex + // pages are skipped. Undefined ≡ indexable (backward compatible). + const indexable = page.indexable !== false; - if (title === null || title.trim() === '') { + if (indexable && (title === null || title.trim() === '')) { detections.push({ kind: 'seo_title_missing', rootCause: `Page "${pageRoute}" has no element or the title is empty`, @@ -64,7 +89,7 @@ export function classifySeoCorpus(input: SeoCorpusInput): BugDetection[] { }); } - if (metaDescription === null || metaDescription.trim() === '') { + if (indexable && (metaDescription === null || metaDescription.trim() === '')) { detections.push({ kind: 'seo_meta_description_missing', rootCause: `Page "${pageRoute}" is missing <meta name="description"> or its content is empty`, @@ -73,7 +98,7 @@ export function classifySeoCorpus(input: SeoCorpusInput): BugDetection[] { }); } - if (canonicalHref === null && anyPageHasCanonical) { + if (indexable && canonicalHref === null && anyPageHasCanonical) { detections.push({ kind: 'seo_canonical_missing', rootCause: `Page "${pageRoute}" lacks <link rel="canonical"> while other pages in the corpus have one`, @@ -82,7 +107,7 @@ export function classifySeoCorpus(input: SeoCorpusInput): BugDetection[] { }); } - if (h1Count !== 1) { + if (indexable && h1Count !== 1) { detections.push({ kind: 'seo_h1_missing_or_multiple', rootCause: `Page "${pageRoute}" has ${h1Count} <h1> element(s) — exactly 1 is required`, @@ -117,6 +142,9 @@ export function classifySeoCorpus(input: SeoCorpusInput): BugDetection[] { // Cross-page: duplicate titles const titleGroups = new Map<string, string[]>(); for (const page of pages) { + // Non-indexable pages (auth-gated/noindex) do not pollute the duplicate-title + // cross-check — only routes a search engine would actually index count. + if (page.indexable === false) continue; if (page.title !== null && page.title.trim() !== '') { const key = page.title.toLowerCase().trim(); const group = titleGroups.get(key) ?? []; diff --git a/packages/cli/src/classify/state-change.test.ts b/packages/cli/src/classify/state-change.test.ts index c332738..9d01081 100644 --- a/packages/cli/src/classify/state-change.test.ts +++ b/packages/cli/src/classify/state-change.test.ts @@ -79,6 +79,31 @@ describe('classifyMissingStateChange — happy paths', () => { }); }); +describe('classifyMissingStateChange — new-window / download signal', () => { + it('returns null when the click opened a new window or triggered a download', () => { + // "Download label" button: onClick → window.open(url, '_blank'). The new tab + // carries the result; the current page legitimately shows no state change. + const r = classifyMissingStateChange( + makePre(), + makePost({ openedNewWindowOrDownload: true }), + makeAction({ selector: 'button[aria-label="Download label"]' }), + '/admin/orders/x', + ); + expect(r).toBeNull(); + }); + + it('still fires when openedNewWindowOrDownload is false and nothing else changed', () => { + const r = classifyMissingStateChange( + makePre(), + makePost({ openedNewWindowOrDownload: false }), + makeAction(), + '/admin/x', + ); + expect(r).not.toBeNull(); + expect(r!.kind).toBe('missing_state_change'); + }); +}); + describe('classifyMissingStateChange — fires when truly nothing happened', () => { it('emits when click had no observable signal at all', () => { const r = classifyMissingStateChange( diff --git a/packages/cli/src/classify/state-change.ts b/packages/cli/src/classify/state-change.ts index 404e703..d2ab3c2 100644 --- a/packages/cli/src/classify/state-change.ts +++ b/packages/cli/src/classify/state-change.ts @@ -33,6 +33,12 @@ export function classifyMissingStateChange( // If URL changed, or network completed, or there was a toast/error — not a missing state change if (urlChanged || hasToast || networkCompleted || hasConsoleError) return null; + // v0.54: the click opened a new window/tab or triggered a download (e.g. + // window.open(url, '_blank'), or an <a download>/target="_blank"). The result + // is carried off-page, so the current page legitimately shows no state change. + // Real spoonworks case: "Download label" → window.open('/api/.../download'). + if (postState.openedNewWindowOrDownload === true) return null; + // Option B (primary): ARIA signal — expanded/haspopup/controls changed → portal/popover opened if (ariaStateChanged(preState.ariaSnapshot, postState.ariaSnapshot)) return null; @@ -135,3 +141,45 @@ export const PORTAL_COUNT_SCRIPT = ` return document.body.querySelectorAll(sel).length; })() `; + +/** + * v0.54: Instrument window.open before a click fires, so a click that opens a + * new window/tab becomes observable. Counts the calls and suppresses the actual + * open (returns a benign stub) so the run does not leak orphan tabs. Read back + * with NEW_WINDOW_READ_SCRIPT after the action. + */ +export const NEW_WINDOW_INSTRUMENT_SCRIPT = ` +(function() { + window.__bhNewWindow = 0; + window.open = function() { + window.__bhNewWindow++; + return { closed: false, focus: function(){}, close: function(){}, blur: function(){}, postMessage: function(){}, location: { href: '' } }; + }; + return { ok: true }; +})() +`; + +/** + * v0.54: After a click, report whether it opened a new window/tab (via the + * window.open counter installed by NEW_WINDOW_INSTRUMENT_SCRIPT) or targeted a + * download/new tab via the clicked element's nearest anchor (target="_blank" or + * a download attribute). Relies on window.__bhAriaSelector being set to the click + * target (already done in the pre-action click block). + */ +export const NEW_WINDOW_READ_SCRIPT = ` +(function() { + var anchorNewTab = false; + var sel = window.__bhAriaSelector; + if (sel) { + var el = document.querySelector(sel); + if (el && el.closest) { + var a = el.closest('a'); + if (a) { + if (a.getAttribute('target') === '_blank') anchorNewTab = true; + if (a.hasAttribute('download')) anchorNewTab = true; + } + } + } + return { windowOpenCount: window.__bhNewWindow || 0, anchorNewTab: anchorNewTab }; +})() +`; diff --git a/packages/cli/src/phases/execute.ts b/packages/cli/src/phases/execute.ts index d438099..6d6e67b 100644 --- a/packages/cli/src/phases/execute.ts +++ b/packages/cli/src/phases/execute.ts @@ -29,7 +29,7 @@ import { classifyVitals } from '../classify/vitals.js'; import { classifyLongTasks } from '../classify/long-tasks.js'; import { classifyExcessiveRerenders } from '../classify/rerenders.js'; import { classifyNPlusOne, classifyDedupMissing, classifyCancelMissing } from '../classify/request-hygiene.js'; -import { classifyMissingStateChange, MUTATION_OBSERVER_START_SCRIPT, MUTATION_OBSERVER_STOP_SCRIPT, ARIA_SNAPSHOT_SCRIPT, PORTAL_COUNT_SCRIPT } from '../classify/state-change.js'; +import { classifyMissingStateChange, MUTATION_OBSERVER_START_SCRIPT, MUTATION_OBSERVER_STOP_SCRIPT, ARIA_SNAPSHOT_SCRIPT, PORTAL_COUNT_SCRIPT, NEW_WINDOW_INSTRUMENT_SCRIPT, NEW_WINDOW_READ_SCRIPT } from '../classify/state-change.js'; import type { AriaSnapshot } from '../types.js'; import { classifyVisualAnomaliesConsistent } from '../classify/vision.js'; import type { VisionClientInterface } from '../adapters/vision-client.js'; @@ -65,7 +65,7 @@ import { classifyA11yBaseline } from '../classify/a11y-baseline.js'; import { PlaywrightKeyboardTrapProbe } from '../adapters/keyboard-trap-probe.js'; import { FocusTracker } from '../adapters/focus-tracker.js'; import type { FocusAfterActionResult } from '../classify/a11y-baseline.js'; -import { classifySeoCorpus } from '../classify/seo.js'; +import { classifySeoCorpus, isSeoIndexable } from '../classify/seo.js'; import type { SeoPageInput } from '../classify/seo.js'; import { harEntriesToCsrfObservations } from '../adapters/har-writer.js'; import { detectMissingCsrf } from '../security/csrf-detector.js'; @@ -661,8 +661,27 @@ export async function runExecute(opts: ExecuteOptions): Promise<ExecuteResult> { if (seoEnabled === true && seoPageInputs.length > 0) { const origin = appBaseUrl ?? (pageUrls?.[0] !== undefined ? new URL(pageUrls[0].startsWith('http') ? pageUrls[0] : `http://localhost${pageUrls[0]}`).origin : ''); const robotsTxt = await fetchRobotsTxt(origin); + // Annotate each page with search-indexability via an anonymous reachability + // probe (search engines crawl unauthenticated). Auth-gated routes redirect + // (3xx) under an anonymous request, so SEO hygiene rules are skipped for them. + await Promise.all(seoPageInputs.map(async (page) => { + if (page.indexable !== undefined) return; + let anonStatus: number | null = null; + try { + const url = new URL(page.pageRoute, origin).toString(); + const res = await fetch(url, { redirect: 'manual', headers: {} }); + anonStatus = res.status; + } catch { + anonStatus = null; + } + page.indexable = isSeoIndexable({ anonStatus, metaRobots: page.metaRobots }); + })); seoDetections = classifySeoCorpus({ pages: seoPageInputs, robotsTxt, origin, suppressDuplicateTitles: seoSuppressDuplicateTitles }); - log.info('seo-corpus: complete', { pagesScraped: seoPageInputs.length, detections: seoDetections.length }); + log.info('seo-corpus: complete', { + pagesScraped: seoPageInputs.length, + detections: seoDetections.length, + indexablePages: seoPageInputs.filter(p => p.indexable !== false).length, + }); } return { @@ -929,6 +948,8 @@ async function executeUiTestInner( } const portalRes = await scope.evaluate(PORTAL_COUNT_SCRIPT).catch(() => null); prePortalCount = typeof portalRes?.value === 'number' ? portalRes.value : 0; + // v0.54: instrument window.open so a new-tab/download click is observable post-action. + await scope.evaluate(NEW_WINDOW_INSTRUMENT_SCRIPT).catch(() => null); } try { @@ -1215,6 +1236,7 @@ async function executeUiTestInner( // v0.45: post-action ARIA state + portal count (click actions only). let postAriaSnapshot: AriaSnapshot | undefined; let newPortalCount: number | undefined; + let openedNewWindowOrDownload: boolean | undefined; if (tc.action.kind === 'click' && tc.action.selector !== undefined) { const ariaRes = await scope.evaluate(ARIA_SNAPSHOT_SCRIPT).catch(() => null); const raw = ariaRes?.value as Record<string, unknown> | null | undefined; @@ -1224,6 +1246,12 @@ async function executeUiTestInner( const portalRes = await scope.evaluate(PORTAL_COUNT_SCRIPT).catch(() => null); const postPortalCount = typeof portalRes?.value === 'number' ? portalRes.value : 0; newPortalCount = Math.max(0, postPortalCount - prePortalCount); + // v0.54: did the click open a new window/tab or trigger a download? + const nwRes = await scope.evaluate(NEW_WINDOW_READ_SCRIPT).catch(() => null); + const nw = nwRes?.value as { windowOpenCount?: number; anchorNewTab?: boolean } | null | undefined; + if (nw !== null && nw !== undefined) { + openedNewWindowOrDownload = (nw.windowOpenCount ?? 0) > 0 || nw.anchorNewTab === true; + } } const preState: PreState = { @@ -1244,6 +1272,7 @@ async function executeUiTestInner( domMutationCount, ariaSnapshot: postAriaSnapshot, newPortalCount, + openedNewWindowOrDownload, }; // V24: classifyReactErrors replaces classifyConsoleErrors — it emits hydration_mismatch, diff --git a/packages/cli/src/types.ts b/packages/cli/src/types.ts index 78f757c..71f3eef 100644 --- a/packages/cli/src/types.ts +++ b/packages/cli/src/types.ts @@ -384,6 +384,13 @@ export type PostState = { ariaSnapshot?: AriaSnapshot; /** v0.45: Number of portal/popover elements newly added to document.body post-action. */ newPortalCount?: number; + /** + * v0.54: the action opened a new window/tab or triggered a download — + * window.open(), or a clicked <a target="_blank"> / <a download>. The result + * is carried off the current page, so classifyMissingStateChange treats the + * absence of in-page change as expected (returns null) rather than a bug. + */ + openedNewWindowOrDownload?: boolean; }; export type OccurrenceSummary = {