fix: kill SEO-on-admin + new-window/download false positives (real-app precision 13.6%→~67%)

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.

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> = {}): 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);
+  });
+});

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 <title> 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) ?? [];

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(

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 };
+})()
+`;