fix(showcase): tighten verify-matrix drift guard + fail-loud boundaries; fix stale comment + flaky test

jpr5 · jpr5 · commit 41fae67618f8 · 2026-05-29T11:45:15.000-07:00
Closing hardening pass on the showcase deploy-gate's verify-matrix
resolver. The 7-agent review confirmed the gate is correct; this
commit fixes the residual rough edges.

- showcase_deploy.yml: correct the false §3 ok-non-empty comment.
  The empty-intersection case can coexist with redeploy_red=false
  (every redeploy succeeded, just none probe-eligible) — that's a
  correctly-green run, not a red one.
- showcase_deploy.yml: tighten the summary.json shape guard to catch
  PARTIAL drift (TOTAL&gt;0 &amp;&amp; WITH_STATUS&lt;TOTAL). The previous all-or-
  nothing TOTAL&gt;0 &amp;&amp; WITH_STATUS==0 check silently dropped drifted
  rows on a mixed summary. Validated locally on mixed/normal/empty/
  total-drift jq samples.
- resolve-verify-matrix.ts: add asSupportedEventName narrowing helper
  + use it in the CLI. Replaces the unchecked `as` cast — type system
  and runtime now tell one story. Resolver's internal eventName
  guard becomes defense-in-depth for direct (test) callers.
- resolve-verify-matrix.ts: make the workflow_run boundary total —
  summaryPresent MUST be exactly "true"/"false". Any other value
  (including "" from a step-id-rename wiring break) throws now
  instead of silently emitting has_services=false.
- resolve-verify-matrix.ts: drop the try/catch around
  fileURLToPath(import.meta.url) in `invokedDirectly`. The catch
  used to swallow ESM-interop failures and silently no-op the CLI
  (exit 0, no GITHUB_OUTPUT write → verify skipped = false-green).
- resolve-verify-matrix.ts: reword parseSsotServices JSDoc to
  distinguish schema-drift from truncation (the two are different
  failure modes, not one conflated story).
- showcase_build.yml: comment addendum on the redeploy-summary
  upload — swapping the guard to `if: always()` would red the
  legitimate services=='' path (no summary written), trading the
  already-closed false-green for a false-red on every non-buildable
  push.
- resolve-verify-matrix.cli.test.ts: switch to spawnSync so stderr
  is captured on both zero and non-zero exit (execFileSync only
  exposes stderr on throw). Hard-code two stable probe-eligible
  names ("aimock", "harness") for the sorted-CSV test rather than
  picking probe[0]/probe[1] off the live SSOT — the prior test was
  tautological (already-sorted in, sorted out) and would silently
  pass if the resolver did nothing.
- resolve-verify-matrix.cli.test.ts: add CLI coverage for the
  dropped-token ::warning:: path (FIX 3 — the entire drift-detection
  contract had zero CLI coverage), the unexpected-EVENT_NAME error
  (FIX 5), and the workflow_run-summary_present total boundary
  (FIX 7, both "" and "True" inputs).
- resolve-verify-matrix.test.ts: add unit coverage for the new
  workflow_run summaryPresent boundary (empty + "True" + the
  workflow_dispatch ignores-summaryPresent regression).

Red-green: 6 tests RED before code changes (FIX 3 warning, FIX 5
unknown EVENT_NAME, FIX 7 unit + CLI ×2 for "" and "True"); 79
tests GREEN after.

Validation: 4 vitest files / 79 tests passing; 87/87 ruby specs
passing; actionlint findings unchanged vs integration baseline
(8 → 8, identical diff); yaml.safe_load OK on both workflows.
diff --git a/.github/workflows/showcase_build.yml b/.github/workflows/showcase_build.yml
@@ -609,6 +609,13 @@ jobs:
         # gating, and `if-no-files-found: error` on `always()` then trips
         # because summary.json was never written. So neither relaxation
         # alone opens a false-green window.
+        #
+        # However, swapping the guard to `if: always()` would ALSO red the
+        # legitimate `services == ''` (nothing-to-redeploy) path — no
+        # summary.json is written there either, so `if-no-files-found:
+        # error` would trip on every push that didn't redeploy anything.
+        # Net effect: trades the (already-closed) false-green risk for a
+        # false-red on every non-buildable push. Don't do it.
         if: steps.changed.outputs.services != ''
         uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4
         with:
diff --git a/.github/workflows/showcase_deploy.yml b/.github/workflows/showcase_deploy.yml
@@ -154,13 +154,17 @@ jobs:
           # silently yields empty → redeploy_red=false AND ok_services=""
           # → resolve-verify-matrix skips verify on a real unverified
           # redeploy = green CI on a broken release. Refuse the ambiguity:
-          # if the file has entries but none carry an ok|error status,
-          # fail loud here so enforce-redeploy-gate reds the workflow
-          # (resolve-matrix.result == 'failure' fans into the gate).
+          # if the file has entries but ANY entry is missing a valid
+          # ok|error status (partial drift — some rows on the legacy schema,
+          # some on the new one), fail loud here so enforce-redeploy-gate
+          # reds the workflow (resolve-matrix.result == 'failure' fans into
+          # the gate). The previous TOTAL>0 && WITH_STATUS==0 check was
+          # all-or-nothing and silently dropped the drifted rows on a mixed
+          # summary.
           TOTAL=$(jq 'length' "$SUMMARY")
           WITH_STATUS=$(jq '[.[] | select(.status == "ok" or .status == "error")] | length' "$SUMMARY")
-          if [ "$TOTAL" -gt 0 ] && [ "$WITH_STATUS" -eq 0 ]; then
-            echo "::error::summary.json has $TOTAL entries but none with status ok|error (schema drift?)"
+          if [ "$TOTAL" -gt 0 ] && [ "$WITH_STATUS" -lt "$TOTAL" ]; then
+            echo "::error::summary.json shape drift: $WITH_STATUS of $TOTAL entries have status ok|error"
             exit 1
           fi
           # Per spec §3: the workflow MUST turn red on any staging
@@ -203,9 +207,10 @@ jobs:
         #       → intersect ok_services (SSOT key OR dispatchName aliases)
         #         with probe.staging-eligible SSOT services. has_services
         #         reflects CSV emptiness. When the intersection collapses
-        #         to empty (e.g. every ok service is non-probe-eligible),
-        #         verify is skipped and `enforce-redeploy-gate` reds the
-        #         workflow independently on redeploy_red=true.
+        #         to empty, verify is skipped; if there were per-service
+        #         errors, enforce-redeploy-gate reds the workflow — otherwise
+        #         the run is correctly green (every redeploy succeeded, just
+        #         none probe-eligible).
         run: npx tsx showcase/scripts/resolve-verify-matrix.ts
 
   verify:
diff --git a/showcase/scripts/__tests__/resolve-verify-matrix.cli.test.ts b/showcase/scripts/__tests__/resolve-verify-matrix.cli.test.ts
@@ -23,7 +23,7 @@
  *   3. workflow_dispatch + no summary → full probe-eligible set, has_services=true.
  *   4. workflow_dispatch + service=<unknown> → non-zero exit, stderr ::error::Unknown service.
  */
-import { execFileSync } from "node:child_process";
+import { spawnSync } from "node:child_process";
 import { mkdtempSync, readFileSync, writeFileSync } from "node:fs";
 import { tmpdir } from "node:os";
 import { dirname, join, resolve } from "node:path";
@@ -55,35 +55,22 @@ function runCli(env: Record<string, string>): SpawnResult {
     GITHUB_OUTPUT: outputPath,
     ...env,
   };
-  try {
-    execFileSync("npx", ["tsx", SCRIPT], {
-      cwd: REPO_ROOT,
-      env: fullEnv,
-      stdio: ["ignore", "pipe", "pipe"],
-    });
-    return {
-      status: 0,
-      stderr: "",
-      output: readFileSync(outputPath, "utf-8"),
-    };
-  } catch (e) {
-    // execFileSync throws on non-zero exit; the thrown error carries
-    // `.status` and `.stderr` (Buffer when stdio captures).
-    const err = e as {
-      status?: number;
-      stderr?: Buffer | string;
-    };
-    return {
-      status: typeof err.status === "number" ? err.status : 1,
-      stderr:
-        typeof err.stderr === "string"
-          ? err.stderr
-          : err.stderr
-            ? err.stderr.toString("utf-8")
-            : "",
-      output: readFileSync(outputPath, "utf-8"),
-    };
-  }
+  // spawnSync (rather than execFileSync) so we capture stderr on BOTH
+  // zero and non-zero exit. execFileSync exposes stderr only on throw,
+  // which means the ::warning:: drift path (exit 0 + stderr text) is
+  // invisible to the test harness. spawnSync returns a single object
+  // regardless of status, so we inspect `status` and `stderr` directly.
+  const res = spawnSync("npx", ["tsx", SCRIPT], {
+    cwd: REPO_ROOT,
+    env: fullEnv,
+    stdio: ["ignore", "pipe", "pipe"],
+    encoding: "utf-8",
+  });
+  return {
+    status: typeof res.status === "number" ? res.status : 1,
+    stderr: typeof res.stderr === "string" ? res.stderr : "",
+    output: readFileSync(outputPath, "utf-8"),
+  };
 }
 
 // Pull two real probe-eligible names off the actual SSOT so the test
@@ -122,22 +109,135 @@ describe("resolve-verify-matrix CLI (end-to-end against real SSOT)", () => {
   it(
     "workflow_run + summary_present + two real ok_services → sorted CSV + has_services=true",
     () => {
-      const probe = realProbeEligibleNames();
-      // Pick TWO probe-eligible names. Use a CSV order that is NOT
-      // already sorted so the sort-on-intersection assertion is real.
-      // probe[0] sorts before probe[1] (alphabetical); feed reversed.
-      const picked = [probe[0], probe[1]];
-      const reversedCsv = `${picked[1]},${picked[0]}`;
-      const sortedCsv = picked.join(",");
+      // Hard-code two real, stable probe-eligible SSOT names rather than
+      // picking probe[0]/probe[1] off the live list. The earlier
+      // probe-index version was tautological: it pulled sorted names from
+      // the SSOT, reversed them, fed them back, and asserted the resolver
+      // re-sorted to the same order — which would silently pass even on a
+      // resolver that did nothing (because probe[0] < probe[1] is the
+      // ALREADY-sorted SSOT order). Also: if the SSOT ever shrank to <2
+      // probe-eligible entries, the test would silently assert
+      // `services_csv=undefined,undefined`.
+      //
+      // `aimock` and `harness` are foundational infra services (not
+      // integration slots), so they will not churn out of the SSOT. We
+      // assert they're both still probe-eligible at runtime; if either
+      // ever leaves, this test fails LOUD with a specific message rather
+      // than silently degrading.
+      const probe = new Set(realProbeEligibleNames());
+      expect(probe.has("aimock")).toBe(true);
+      expect(probe.has("harness")).toBe(true);
+      // Feed unsorted so the sort-on-intersection assertion is real:
+      // "harness,aimock" must come out as "aimock,harness".
       const r = runCli({
         EVENT_NAME: "workflow_run",
         SUMMARY_PRESENT: "true",
-        OK_FROM_REDEPLOY: reversedCsv,
+        OK_FROM_REDEPLOY: "harness,aimock",
         DISPATCH_SERVICE: "",
       });
       expect(r.status).toBe(0);
-      expect(r.output).toBe(
-        `services_csv=${sortedCsv}\nhas_services=true\n`,
+      expect(r.output).toBe("services_csv=aimock,harness\nhas_services=true\n");
+    },
+    30_000,
+  );
+
+  // -----------------------------------------------------------------------
+  // FIX 3 — surface SSOT/build drift via ::warning::ok_services tokens
+  // dropped (no SSOT match). The intersection logic already silently drops
+  // unmatched tokens from the verify matrix; the WARNING is the entire
+  // drift-detection contract operators rely on to notice that a redeploy
+  // reported success for a service the SSOT has forgotten about (or vice
+  // versa). Without coverage here it could silently regress to "no warning
+  // emitted" and the gate would keep working while losing its early-warning
+  // signal.
+  // -----------------------------------------------------------------------
+  it(
+    "workflow_run + ok_services with bogus token → ::warning:: lists dropped tokens, real ones still verify",
+    () => {
+      const r = runCli({
+        EVENT_NAME: "workflow_run",
+        SUMMARY_PRESENT: "true",
+        // `svc-bogus` is not in the SSOT under any spelling; `aimock` is
+        // a real probe-eligible service. The verify CSV must drop the
+        // bogus token AND the wrapper must `::warning::` so the dropped
+        // token surfaces in the workflow log as an annotation.
+        OK_FROM_REDEPLOY: "svc-bogus,aimock",
+        DISPATCH_SERVICE: "",
+      });
+      expect(r.status).toBe(0);
+      expect(r.output).toContain("services_csv=aimock\n");
+      expect(r.output).toContain("has_services=true\n");
+      expect(r.stderr).toMatch(
+        /::warning::ok_services tokens dropped \(no SSOT match\): svc-bogus/,
+      );
+    },
+    30_000,
+  );
+
+  // -----------------------------------------------------------------------
+  // FIX 5 — EVENT_NAME must be exactly 'workflow_run' or 'workflow_dispatch'.
+  // The CLI used to do an unchecked `as` cast on EVENT_NAME, so a typo or
+  // accidental new trigger ('push', 'schedule') would compile-pass at the
+  // type layer and only fail deep inside the resolver. Make the boundary
+  // total: a narrowing helper rejects unknown EVENT_NAME values up front,
+  // matching the resolver's own runtime guard with a SINGLE consistent
+  // story (type system + runtime agree).
+  // -----------------------------------------------------------------------
+  it(
+    "EVENT_NAME=push → non-zero exit with ::error::resolve-verify-matrix: unexpected EVENT_NAME",
+    () => {
+      const r = runCli({
+        EVENT_NAME: "push",
+        SUMMARY_PRESENT: "",
+        OK_FROM_REDEPLOY: "",
+        DISPATCH_SERVICE: "",
+      });
+      expect(r.status).not.toBe(0);
+      expect(r.stderr).toMatch(
+        /::error::resolve-verify-matrix: unexpected EVENT_NAME 'push'/,
+      );
+    },
+    30_000,
+  );
+
+  // -----------------------------------------------------------------------
+  // FIX 7 — workflow_run requires SUMMARY_PRESENT to be exactly "true" or
+  // "false". The check-redeploy-summary step always sets one of those two
+  // values, so any other input (including "" from a step-id wiring break,
+  // or "True" from a case-typo) means the wiring is broken and the gate
+  // would silently fall through to the intersection branch. Fail loud
+  // rather than silently emitting has_services=false on a real redeploy.
+  // workflow_dispatch ignores SUMMARY_PRESENT and must NOT trigger this.
+  // -----------------------------------------------------------------------
+  it(
+    "EVENT_NAME=workflow_run + SUMMARY_PRESENT='' → non-zero exit with workflow_run requires summary_present",
+    () => {
+      const r = runCli({
+        EVENT_NAME: "workflow_run",
+        SUMMARY_PRESENT: "",
+        OK_FROM_REDEPLOY: "",
+        DISPATCH_SERVICE: "",
+      });
+      expect(r.status).not.toBe(0);
+      expect(r.stderr).toMatch(
+        /::error::resolve-verify-matrix: workflow_run requires summary_present in \{true,false\}, got ''/,
+      );
+    },
+    30_000,
+  );
+
+  it(
+    "EVENT_NAME=workflow_run + SUMMARY_PRESENT='True' (case typo) → non-zero exit",
+    () => {
+      const r = runCli({
+        EVENT_NAME: "workflow_run",
+        SUMMARY_PRESENT: "True",
+        OK_FROM_REDEPLOY: "",
+        DISPATCH_SERVICE: "",
+      });
+      expect(r.status).not.toBe(0);
+      expect(r.stderr).toMatch(
+        /::error::resolve-verify-matrix: workflow_run requires summary_present in \{true,false\}, got 'True'/,
       );
     },
     30_000,
diff --git a/showcase/scripts/__tests__/resolve-verify-matrix.test.ts b/showcase/scripts/__tests__/resolve-verify-matrix.test.ts
@@ -257,6 +257,58 @@ describe("resolveVerifyMatrix", () => {
       }),
     ).toThrow(/::error::resolve-verify-matrix: unexpected eventName 'schedule'/);
   });
+
+  // ---------------------------------------------------------------------
+  // FIX 7 — make the workflow_run boundary total. summaryPresent MUST be
+  // exactly "true" or "false" on workflow_run (check-redeploy-summary
+  // always sets one of the two). Any other value (including the empty
+  // string from a future step-id-rename wiring break, or "True" from a
+  // case-typo) used to fall through to the intersection branch and
+  // silently emit has_services=false — indistinguishable from a
+  // legitimate skip. Throw instead. workflow_dispatch ignores
+  // summaryPresent and must NOT throw.
+  // ---------------------------------------------------------------------
+  it("workflow_run + summaryPresent='' → throws (boundary is total)", () => {
+    expect(() =>
+      resolveVerifyMatrix({
+        eventName: "workflow_run",
+        summaryPresent: "",
+        okFromRedeploy: "",
+        dispatchService: "",
+        ssotServices: fixtureServices,
+      }),
+    ).toThrow(
+      /::error::resolve-verify-matrix: workflow_run requires summary_present in \{true,false\}, got ''/,
+    );
+  });
+
+  it("workflow_run + summaryPresent='True' (case typo) → throws", () => {
+    expect(() =>
+      resolveVerifyMatrix({
+        eventName: "workflow_run",
+        summaryPresent: "True",
+        okFromRedeploy: "",
+        dispatchService: "",
+        ssotServices: fixtureServices,
+      }),
+    ).toThrow(
+      /::error::resolve-verify-matrix: workflow_run requires summary_present in \{true,false\}, got 'True'/,
+    );
+  });
+
+  it("workflow_dispatch ignores summaryPresent (any value) — does NOT throw on empty", () => {
+    // workflow_dispatch path never reads summaryPresent; it must keep
+    // working even when the wrapper passes the default "".
+    const out = resolveVerifyMatrix({
+      eventName: "workflow_dispatch",
+      summaryPresent: "",
+      okFromRedeploy: "",
+      dispatchService: "all",
+      ssotServices: fixtureServices,
+    });
+    expect(out.servicesCsv).toBe("svc-a,svc-b,svc-c,svc-d");
+    expect(out.hasServices).toBe(true);
+  });
 });
 
 // -------------------------------------------------------------------------
diff --git a/showcase/scripts/resolve-verify-matrix.ts b/showcase/scripts/resolve-verify-matrix.ts
