Skip to content

test(ai): hold the no-grant reads open for a late request - #2546

Merged
KyleMit merged 1 commit into
mainfrom
claude/cq5-ai-spec-late-grant
Sep 30, 2026
Merged

KyleMit merged 1 commit into
mainfrom
claude/cq5-ai-spec-late-grant

Conversation

@KyleMit

@KyleMit KyleMit commented Sep 30, 2026 •

Copy link
Copy Markdown
Owner

Campaign unit F8 (lane C) of the ship-campaign parallel=3 run. Refs #2529 (tracking), Refs #2500 (source: its leftovers comment, follow-up 8, from audit 53).

Spec

web/tests/flows-ai.spec.ts has three negative cases that read grant.requests() once, as the page settles:

  • a fresh installation does not fetch an AI allowance or show the canvas action
  • a migrated BYO key reveals the AI button on the next launch
  • the AI button posts the drawing and reveals the generated result

A regression that sent the free-grant request late would pass them. Done when: a late grant request fails each of the three cases, a negative control proves it (injection present: each fixed case fails; injection absent: each passes), and the spec is stable across repeated targeted runs.

What changed

Only web/tests/flows-ai.spec.ts. No shared helper was touched.

  • web/tests/flows-ai.spec.ts:30: adds LATE_GRANT_REQUEST_WINDOW_MS = 750, with its WHY comment, and expectNoGrantRequest(page, grant), which waits out the window and then reads the count. The grant parameter is typed from mockFreeGrant's real return type.
  • :69, :205 and :249: each case's single read expect(grant.requests()).toBe(0) becomes await expectNoGrantRequest(page, grant).
  • The mixed import now carries type Page inline, and enableAiInSettings uses it in place of import('@playwright/test').Page.

Approach, and why the window is bounded rather than signal-driven

The brief preferred a deterministic signal that the send window has closed. None exists here.

  • The only sender is followEligibility in web/src/lib/state/freeGenerations.svelte.ts, which calls requestGrant(). That fetch goes out one await installationId() after the decision.
  • On the negative path, everything the decision writes is written synchronously inside the effect: the inactive grant state, and the unavailable state together with its freeGenerationBadgeHint storage write. All of it lands before the point where an awaited fetch would go. So no UI or storage state is ordered after a request the path might send.
  • A request that is actually late (an idle callback, a timer, a later effect) is by definition ordered after no page signal at all.
  • So I followed the documented pattern for proving a negative: docs/TESTING.md, "Writing flake-resistant specs", case (b). That means a named, bounded window after which the count is read, as coloring-pack-download-gate.spec.ts (IDLE_WORK_OBSERVATION_MS) and flows-settings-report.spec.ts (LATE_REPORT_SETTLE_MS) already do. A slower worker only lengthens the real wait, so the window cannot false-red.
  • I did not use a bare expect.poll(...).toBe(0): it passes on its first sample.

Sizing N from the path's own timing. Measured with temporary performance.now() logging (never committed) over three instrumented runs of the whole file:

Point (page time) Measured
decision → grant fetch dispatch 0.2–0.6 ms (positive cases)
hydrated decision 40–216 ms after navigation
original single read, fresh installation 562–678 ms
original single read, migrated BYO key 462–466 ms
original single read, access-code generation 2526–2663 ms

A grant request sent on time is therefore already counted by the old read, hundreds of milliseconds early. The window exists only for a late one. 750 ms is more than three times the whole navigation-to-request span, and it matches the repo's existing idle-work negative window. The cost is +0.75 s for each of the three tests.

Negative control

Two temporary injections went into the code path all three cases guard: followEligibility's not-ready branch, after hydration. Each sends one grant request once the page has "settled". Neither is committed; the diffs are below. Every cell is --repeat-each=3 at --workers=1 on port 5333, limited by --grep to the three cases.

  • A (input-quiet): fires 600 ms after the last pointer, click or key input.
  • B (input- and DOM-quiet): fires 600 ms after the last input or DOM mutation.

The access-code generation case needs B. Its flow has a reveal wait of more than 600 ms with no input, so A fires mid-flow, and even the original read catches that.

Case A, original spec A, fixed spec B, original spec B, fixed spec
fresh installation pass 3/3 (misses) fail 3/3 pass 3/3 (misses) pass 3/3 (¹)
migrated BYO key pass 3/3 (misses) fail 3/3 pass 3/3 (misses) pass 3/3 (¹)
access-code generation fail 3/3 (²) fail 3/3 pass 3/3 (misses) fail 3/3

Each failure is Expected: 0 / Received: 1 at the case's expectNoGrantRequest read.

  • (¹) The DOM keeps changing after the drawer opens, which keeps re-arming B. B's request therefore does not leave inside these two cases' window. This is the bound any finite window has: a request later than the window still slips past. Injection A covers these two cases.
  • (²) A fires in the access-code flow's mid-flow input gap, before the original read.

Result: every case has a late request that the original single read misses and the fixed spec catches.

Injection absent, fixed spec: the whole file with --repeat-each=3 passed 30/30 (59.0 s). Baseline on fresh main 077c34f before any change: 10/10. That also shows the just-merged generate-image and safety-classifier changes did not disturb this spec.

Injection A (input-quiet), never committed
--- freeGenerations.original.svelte.ts	2026-09-30 01:07:33
+++ freeGenerations.injected-A-input-quiet.svelte.ts	2026-09-30 01:07:33
@@ -80,6 +80,30 @@
   return status.remaining;
 }
 
+// F8 NEGATIVE CONTROL INJECTION A (never committed): once the grant path
+// decides not to request, send one grant request anyway, LATE_GRANT_INJECTION_MS
+// after the page goes input-quiet (the later of the decision and the last input).
+const LATE_GRANT_INJECTION_MS = 600;
+let lateGrantInjected = false;
+function injectLateGrantRequest() {
+  if (lateGrantInjected) return;
+  lateGrantInjected = true;
+  const INPUTS = ['pointerdown', 'pointerup', 'click', 'keydown'] as const;
+  let timer: ReturnType<typeof setTimeout> | undefined;
+  const arm = () => {
+    clearTimeout(timer);
+    timer = setTimeout(() => {
+      for (const type of INPUTS) window.removeEventListener(type, arm, true);
+      console.log(`[F8] late grant injected t=${performance.now().toFixed(1)}`);
+      void installationId()
+        .then((id) => fetchGrantRemaining(id, new AbortController().signal))
+        .catch(() => {});
+    }, LATE_GRANT_INJECTION_MS);
+  };
+  for (const type of INPUTS) window.addEventListener(type, arm, true);
+  arm();
+}
+
 interface FreeGenerationsDeps {
   // Whether a credential is held, never the credential itself: the grant only
   // needs to know whether the parent is on the free tier.
@@ -190,6 +214,7 @@
     if (!ready || !online) {
       freeGenerationGrantRequest.cancel();
       if (persistedStateStatus.hydrated && !ready) {
+        injectLateGrantRequest();
         if (settings.aiCredentialKind() !== 'none') setFreeGenerationsUnavailable();
         else setFreeGenerationsInactive();
       }
Injection B (input- and DOM-quiet), never committed
--- freeGenerations.original.svelte.ts	2026-09-30 01:07:33
+++ freeGenerations.injected-B-dom-quiet.svelte.ts	2026-09-30 01:07:43
@@ -80,6 +80,38 @@
   return status.remaining;
 }
 
+// F8 NEGATIVE CONTROL INJECTION B (never committed): once the grant path
+// decides not to request, send one grant request anyway, LATE_GRANT_INJECTION_MS
+// after the page goes quiet: no input and no DOM mutation.
+const LATE_GRANT_INJECTION_MS = 600;
+let lateGrantInjected = false;
+function injectLateGrantRequest() {
+  if (lateGrantInjected) return;
+  lateGrantInjected = true;
+  const INPUTS = ['pointerdown', 'pointerup', 'click', 'keydown'] as const;
+  let timer: ReturnType<typeof setTimeout> | undefined;
+  const observer = new MutationObserver(() => arm());
+  const arm = () => {
+    clearTimeout(timer);
+    timer = setTimeout(() => {
+      observer.disconnect();
+      for (const type of INPUTS) window.removeEventListener(type, arm, true);
+      console.log(`[F8] late grant injected t=${performance.now().toFixed(1)}`);
+      void installationId()
+        .then((id) => fetchGrantRemaining(id, new AbortController().signal))
+        .catch(() => {});
+    }, LATE_GRANT_INJECTION_MS);
+  };
+  for (const type of INPUTS) window.addEventListener(type, arm, true);
+  observer.observe(document.documentElement, {
+    subtree: true,
+    childList: true,
+    attributes: true,
+    characterData: true,
+  });
+  arm();
+}
+
 interface FreeGenerationsDeps {
   // Whether a credential is held, never the credential itself: the grant only
   // needs to know whether the parent is on the free tier.
@@ -190,6 +222,7 @@
     if (!ready || !online) {
       freeGenerationGrantRequest.cancel();
       if (persistedStateStatus.hydrated && !ready) {
+        injectLateGrantRequest();
         if (settings.aiCredentialKind() !== 'none') setFreeGenerationsUnavailable();
         else setFreeGenerationsInactive();
       }

Commands run

  • SPLOTCH_E2E_PORT=5333 npm run test:e2e -- web/tests/flows-ai.spec.ts --workers=1: the baseline on main (10/10), the fixed spec --repeat-each=3 (30/30), and the control matrix above.
  • npm run check: exit 0.
  • npm run lint: exit 0.
  • npx prettier --check web/tests/flows-ai.spec.ts: clean.
  • SMOKE_PORT=5334 npm run test:browserless: all 5 checks passed.

Review and merge gate

  • Rival review, round 1 (Codex, review): no findings, with 0 findings and 0 unverified.
    • It ran the spec 10/10 through the broker.
    • It reproduced both controls independently. A failed all three fixed cases. B failed the access-code case and passed the two drawer-only cases, which is the bound this PR discloses.
    • It judged the no-deterministic-signal argument and the 750 ms budget sound.
    • Its strongest alternative, a harness-level fail-on-request stub, is drafted (not filed) in this comment.
    • Round two was skipped: round one changed no code.
  • Merge gate: path unrelated. Fetching main and running the reconcile-with-main survey found 0 incoming commits, so there was nothing to merge. Gated main is the merge base 077c34f.
  • CI on head 4e05670:
    • All applicable checks passed: Quality, Browserless tests, Release build smoke, Page-load performance, Tests 1–8, Firefox smoke, WebKit smoke, and ADR integrity.
    • Skipped by their own conditions: the WebKit commit gates (they run after merge, on tags, or on dispatch only) and Claude review (Dependabot PRs only).
    • Native compile and Centerline tracing are path-filtered and do not apply to this diff.

Caveats

  • The window is a bound, not a proof. A request sent more than LATE_GRANT_REQUEST_WINDOW_MS after the read still passes, as injection B shows for the two drawer-only cases.
  • No product code changed.

🤖 Generated with Claude Code

Three flows-ai negative cases read the free-grant request count once, as
the page settled, so a regression that sent the grant request late (behind
idle work, a timer, or a later effect) passed them. Each now reads the
count after LATE_GRANT_REQUEST_WINDOW_MS through expectNoGrantRequest.

Negative control: a temporary injection in followEligibility's not-ready
branch that sends one grant request once the page goes quiet passed all
three original cases and fails each fixed case.

Refs #2529
Refs #2500

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@KyleMit KyleMit left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Independent review by the codex rival agent, round 1, of pull request 2546.

No merge-blocking defects found in the full diff. Reviewed all three call sites, grant dispatch, credential hydration, the shared mock, repository guidance, and the PR’s timing/control evidence. Type checking and lint passed; the complete targeted Playwright spec passed 10/10 through the handler.

Independently reproduced both temporary controls: A failed all three cases with Expected: 0 / Received: 1; B failed the access-code generation case while the two drawer-only cases passed, matching the disclosed limitation. Splitting the controls fairly demonstrates additional coverage in each case. All injections were reverted and the worktree is clean.

The negative-path state and storage writes precede the possible awaited fetch; neither supplies a deterministic signal after a would-be request. The 750 ms window is a reasonable empirical observation budget, with no code-defined deadline supporting a uniquely better value. Its coverage remains bounded: credential hydration continues in the background, so the comment’s claim that on-time requests are already counted describes measurements rather than a universal ordering guarantee. The final access-code assertion retains requests counted throughout the preceding flow.

The strongest alternative is a harness-level fail-on-request stub: it would report forbidden traffic immediately, improve diagnostics, and prevent callers from forgetting a final count assertion. It would still need an observation window before teardown. The local helper is acceptable for these three cases under the documented bounded-negative pattern.

@KyleMit

KyleMit commented Sep 30, 2026

Copy link
Copy Markdown
Owner Author

Drafted follow-ups (not filed)

Drafts for the user to review. None has been opened.

1. Let mockFreeGrant fail at the forbidden request itself

  • Suggested labels: type:test, area:ai, priority:low
  • Source: the round-one rival review's adversarial pass on this PR.

Body. The three negative cases in web/tests/flows-ai.spec.ts now prove that no free-grant request happens by waiting LATE_GRANT_REQUEST_WINDOW_MS and then reading grant.requests(). If a request slips through, the failure is a bare Expected: 0 / Received: 1 at the read. It does not say when the request went out or what triggered it.

A harness-level reply in web/tests/ai-harness.ts would report forbidden traffic at the request, and it would give the failure a better message: the request's URL and page time, plus the test step in progress. Something like a 'forbidden' reply that records a violation, or makes the route handler throw.

It would still need the same observation window before teardown to catch a late request. So this changes the diagnostics, not the coverage. Worth doing only if a second spec comes to need a "never requested" grant mock.

Done when: a forbidden-grant mock reports the request's timing in its failure, the three cases use it, and a negative control shows each case still fails on a late request.

@KyleMit
KyleMit merged commit 3a34443 into main Sep 30, 2026
19 checks passed
@KyleMit
KyleMit deleted the claude/cq5-ai-spec-late-grant branch September 30, 2026 05:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant