From fad5094587c1d63011d0c89e35d9957d124490ad Mon Sep 17 00:00:00 2001
From: Tom Boucher <trekkie@nomorestars.com>
Date: Thu, 9 Jul 2026 09:36:33 -0400
Subject: [PATCH] =?UTF-8?q?fix(#2089):=20redesign=20scanner=20property=20t?=
 =?UTF-8?q?est=20=E2=80=94=20logic/results,=20not=20wall-clock?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replaces the timing-dependent execFileSync(timeout:5000) approach with a
spawnSync-based runHook that tests the scanner's RESULT (exit code + output
shape), never how long it takes.

Root design flaw in the prior approach: execFileSync's timeout (5000ms)
was identical to the scanner's own internal setTimeout(5000ms), creating a
non-repeatable race (F.I.R.S.T. violation: not Repeatable). Under concurrent
test-chunk load — which #2089's 3 new cursor test files redistribute —
node22's event-loop scheduling let execFileSync's SIGTERM win the race,
producing err.status=null → exitCode=1 → spurious property-test failure.

Redesign (F.I.R.S.T.):
- spawnSync (not execFileSync): non-zero exits return a result object,
  not an exception — cleaner for property tests
- Non-serializable payloads (BigInt, circular refs, Symbol) are SKIPPED:
  the scanner receives JSON via stdin, so these values are outside its
  protocol — JSON.stringify throwing is a test-harness artifact, not a
  scanner defect
- 30s safety-net timeout is NOT a test assertion: scanner exits in <100ms;
  30s only catches a genuinely hung process (6x the scanner's own 5s
  internal timer → no race possible)
- Assertions check exit===0 and output structure, never timing

qa-test-architect pipeline: risk=HIGH (security boundary); automation=
subprocess (real shipped hook); test-cases cover happy/boundary/negative/
independence; verified via gsd-test.
---
 .../read-injection-scanner.property.test.cjs  | 62 ++++++++++++++-----
 1 file changed, 45 insertions(+), 17 deletions(-)

diff --git a/tests/read-injection-scanner.property.test.cjs b/tests/read-injection-scanner.property.test.cjs
index fb891c9c1..eac847542 100644
--- a/tests/read-injection-scanner.property.test.cjs
+++ b/tests/read-injection-scanner.property.test.cjs
@@ -12,35 +12,63 @@
  *
  * Invoked as a subprocess (the hook reads a JSON payload on stdin and has no
  * exported surface), so this exercises the real shipped hook end-to-end.
+ *
+ * F.I.R.S.T. design:
+ *   Fast     — spawnSync is synchronous; scanner exits in <100ms for any input.
+ *   Isolated — each invocation is a fresh subprocess; no shared state.
+ *   Repeatable — no wall-clock assertion; the 30s safety-net timeout is 6x the
+ *               scanner's own internal 5s timer and is never tested against.
+ *               Tests assert on the scanner's RESULT (exit code + output shape),
+ *               never on timing.
+ *   Self-Val  — assertions check exit===0 and output is empty or valid JSON.
+ *   Timely    — written alongside the scanner (#1577); hardened for #2089.
  */
 
 const { describe, test } = require('node:test');
 const assert = require('node:assert/strict');
-const { execFileSync } = require('node:child_process');
+const { spawnSync } = require('node:child_process');
 const path = require('node:path');
 const fc = require('./helpers/fast-check-setup.cjs');
 
 const HOOK_PATH = path.join(__dirname, '..', 'hooks', 'gsd-read-injection-scanner.js');
 
+/**
+ * Run the scanner hook with a payload and return its result.
+ *
+ * Uses spawnSync (not execFileSync) so non-zero exits return a result object
+ * rather than throwing — cleaner for property tests that assert on exit code.
+ *
+ * Non-serializable payloads (BigInt, circular refs, Symbol, undefined) are
+ * SKIPPED: the scanner receives JSON via stdin, so these values can never
+ * reach it. JSON.stringify throwing is a test-harness artifact (fc.anything()
+ * generates values outside the JSON domain), not a scanner defect.
+ *
+ * The 30s safety-net timeout is NOT a test assertion. The scanner exits in
+ * <100ms for any input; its own internal setTimeout(5000) guarantees exit
+ * even if stdin never closes (impossible here — spawnSync's `input:` pipes
+ * and closes stdin). The ceiling only catches a genuinely hung process (a
+ * real defect) without racing the scanner's internal timer.
+ */
 function runHook(payload) {
+  let input;
   try {
-    const stdout = execFileSync(process.execPath, [HOOK_PATH], {
-      input: JSON.stringify(payload),
-      encoding: 'utf-8',
-      // 10s — double the scanner's own 5s internal stdin-timeout
-      // (hooks/gsd-read-injection-scanner.js:109). Under concurrent test
-      // load (crowded run-tests.cjs chunks), node22's event-loop scheduling
-      // can delay the scanner's stdin 'end' handler past 5s, racing the
-      // scanner's process.exit(0) against this timeout's SIGTERM. A 10s
-      // ceiling gives the scanner's own 5s exit a 5s buffer to win the race
-      // deterministically on every node version. (#2089)
-      timeout: 10000,
-      stdio: ['pipe', 'pipe', 'pipe'],
-    });
-    return { exitCode: 0, stdout: stdout.trim() };
-  } catch (err) {
-    return { exitCode: err.status ?? 1, stdout: (err.stdout || '').toString().trim() };
+    input = JSON.stringify(payload);
+  } catch {
+    return { exitCode: 0, stdout: '', skipped: true };
   }
+
+  const result = spawnSync(process.execPath, [HOOK_PATH], {
+    input,
+    encoding: 'utf-8',
+    timeout: 30000,
+    stdio: ['pipe', 'pipe', 'pipe'],
+  });
+
+  return {
+    exitCode: result.status ?? 1,
+    stdout: (result.stdout || '').trim(),
+    signal: result.signal,
+  };
 }
 
 // Injection-shaped fragments so the regex-matching path is exercised, not just clean text.