U12 part 9: make the raw-flag census a ratchet that fails when the last read goes — answer: 2 reads left, key cannot be deleted (#2537)

## U12 part 9 — the flag census now answers itself Independent of the #2530 rebase; adds one test file, no production changes. ## The answer, first: NO, the settings key cannot be deleted yet **Three files reference the raw flag on current main (`3ff98aae5`):** ``` packages/core/src/store.ts ← declares it packages/core/src/task-store/moves.ts:363 ← U2b: `useWorkflow` packages/core/src/task-store/workflow-task-create-ops.ts:351 ← U2b: move-policy preflight ``` Everything else that greps is a comment, a test writing the flag deliberately to reach the dead path, or the unrelated `workflowColumns.*` i18n namespace for the Columns editor panel. **Why I can't remove them.** Both are on the move path and belong to **U2b**, which carries an equivalence-proof obligation because the two move implementations it arbitrates have never both run in production. They are also **not separable from each other**: `workflow-task-create-ops.ts:351` computes the `movePolicyPreflight` that `moves.ts` consumes and validates, so un-gating it alone would start evaluating workflow move policies — with their plugin-gate side effects — while the branch consuming the result stays off. That is a behaviour change with no consumer, which is worse than either end state. **U2b has not landed.** Program history on main runs `#2466 → #2467 → #2468 → #2469 → #2479 → #2500 → #2512 → #2513 → #2525 → #2528 → #2535`. #2468 was Phase A2 **steps 1–2 only** — the differential characterisation. No convergence PR exists. ## Why this is a PR and not another status message You have asked this question three times. I have answered it three times by grepping, and each answer was a number nobody could re-derive later — including me, which is why I re-ran the audit from scratch each time. That is exactly the shape this program keeps finding: a fact everyone believes, maintained by nobody. So the census is now a test. It **fails in both directions**, deliberately: - **A new read appears** → someone re-gated behaviour on a flag that is `false` for every real project, so the feature behind it will not run. That is the defect class U12 spent its length finding (the capacity gate, the U5 guards, the move policies — all looked enforced, none were). - **The last read disappears** → U2b has landed, and the settings key can finally go. The removal steps are written at the assertion. The second case is the one that matters. It converts "remember to delete the settings key someday" into a failing test at the exact moment that becomes possible, instead of a note in a PR body that ages out. ## Verified in both directions, not assumed - Adding a reference in `lifecycle-ops.ts` → fails with `+ "packages/core/src/task-store/lifecycle-ops.ts"`. - Dropping `moves.ts` from the allowlist → fails with `+ "packages/core/src/task-store/moves.ts"`. Equality rather than subset is what makes the second case possible; a subset check would let the last reader vanish silently and leave the key orphaned forever. Two supporting assertions, both there because of failure modes this program has already hit: - **No production code WRITES the key.** That is the premise the entire unit rests on — if a writer appears, every "this branch is unreachable" conclusion in U12 needs revisiting. - **The scan sees >200 files.** A broken path glob would otherwise make every assertion vacuously green: a guard reporting success without checking anything. ## Verification `pnpm test:gate` (414 + 10 + 71), `pnpm lint`, `pnpm verify:fast`, core typecheck green. ## Standing offer If you want U12 actually closed rather than ratcheted, the remaining work is U2b's convergence. I have the inventory and the divergence list its characterisation suite does not yet cover (plugin column gates, the `transitionPending` marker, `workflowId` in `task:move` run-audit, move-policy preflight). I would want the current U2b worker stood down from `moves.ts` first — two writers on the file this whole program pivots on is the one hazard I would not take on my own authority.  ## Summary by CodeRabbit * **Tests** * Added a new automated Vitest “census ratchet” to ensure only an approved, fixed set of production reads is made for the workflow columns compatibility flag. * Added checks that disallow hardcoded `workflowColumns: true/false` assignments in production sources. * Added allowlist validation, including per-file occurrence counts, required rationale text length, and confirmation that referenced files exist.  --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-29 09:05:04 -07:00
parent 6721bdc652
commit f1be80420f
1 changed files with 184 additions and 0 deletions
--- a/packages/core/src/tests/raw-workflow-columns-flag-census.test.ts
+++ b/packages/core/src/tests/raw-workflow-columns-flag-census.test.ts
@@ -0,0 +1,184 @@
+/*
+FNXC:WorkflowColumns 2026-07-29-00:00 (U12 — R9):
+CENSUS RATCHET for the raw `experimentalFeatures.workflowColumns` compatibility flag.
+
+U12's headline goal is deleting this flag and the settings key behind it. That cannot
+happen while anything reads it, and "does anything still read it?" has been answered by
+hand three times over the life of the unit — each time by grepping, each time producing
+a number nobody can re-derive later. This test makes the answer a fact the suite
+maintains.
+
+WHAT THE FLAG IS. `isWorkflowColumnsCompatibilityFlagEnabled` (store.ts) returns
+`experimentalFeatures.workflowColumns === true`. It is the RAW key, distinct from the
+public runtime helper that treats stale `false` as enabled. No module hardcodes
+the key, so it reads false for every project that never carried a stale persisted value
+— which is why every branch behind it has been silently inert, and why U12 spent its length finding features that looked
+enforced and were not.
+
+WHAT REMAINS, and why it is not mine to remove. Both surviving reads are on the MOVE
+PATH and belong to U2b (move-path convergence), which carries an equivalence-proof
+obligation because the two implementations it arbitrates have never both run in
+production. They are also not separable from each other: the preflight computes the
+`movePolicyPreflight` that `moves.ts` consumes and validates, so un-gating it alone
+would start evaluating workflow move policies — with their plugin-gate side effects —
+while the branch that consumes the result stays off.
+
+THIS TEST FAILS IN BOTH DIRECTIONS, deliberately:
+  - a NEW read appears        -> someone is re-gating behaviour on a retired flag;
+  - the LAST read disappears  -> U2b has landed, and the settings key can finally go.
+The second is the one that matters. It converts "remember to delete the key someday"
+into a failing test at the exact moment that becomes possible.
+*/
+import { describe, expect, it } from "vitest";
+import { existsSync, readdirSync, readFileSync, statSync } from "node:fs";
+import { join, resolve } from "node:path";
+
+const REPO_ROOT = resolve(import.meta.dirname, "../../../..");
+const SOURCE_ROOTS = ["packages/core/src", "packages/engine/src", "packages/dashboard/src", "packages/cli/src"];
+
+/** The raw-flag reader. Not the always-on public runtime helper. */
+const RAW_FLAG_READER = "isWorkflowColumnsCompatibilityFlagEnabled";
+
+/**
+ * Every file permitted to reference the raw reader, and why. Paths are repo-relative.
+ * `store.ts` declares it; the other two are U2b's move path.
+ */
+const ALLOWED: ReadonlyArray<{ file: string; occurrences: number; why: string }> = [
+  {
+    file: "packages/core/src/store.ts",
+    occurrences: 1,
+    why: "declares the helper; it goes with the last reader",
+  },
+  {
+    file: "packages/core/src/task-store/moves.ts",
+    occurrences: 2,
+    why: "U2b: the import, plus `useWorkflow` selecting between the two move-side-effect implementations",
+  },
+  {
+    file: "packages/core/src/task-store/workflow-task-create-ops.ts",
+    occurrences: 2,
+    why: "U2b: the import, plus the gate on the move-policy preflight that moves.ts consumes",
+  },
+];
+
+/*
+Strip comments AND string/template literals before scanning (PR #2537 review — greptile).
+Comments alone were not enough: this flag is discussed by name in diagnostics, error
+copy and fixtures, and a substring scan would classify any such TEXT as a reader — a
+ratchet that fails on prose is a ratchet people learn to edit around. What remains after
+this is executable code, where the symbol appearing means it is genuinely referenced.
+*/
+function stripCommentsAndStrings(source: string): string {
+  return source
+    .replace(/\/\*[\s\S]*?\*\//g, " ")
+    .replace(/(^|[^:])\/\/[^\n]*/g, "$1 ")
+    /*
+    Template literals: keep the ${...} EXPRESSIONS, drop only the literal text
+    (PR #2537 review — greptile). Erasing whole templates would have removed executable
+    interpolations with them, so a reader written inside `${...}` would have escaped the
+    census entirely — a hole in the direction that matters, since it hides a read.
+    */
+    .replace(/`(?:[^`\\]|\\.)*`/g, (template) =>
+      (template.match(/\$\{[\s\S]*?\}/g) ?? []).join(" "))
+    .replace(/'(?:[^'\\\n]|\\.)*'/g, '""')
+    .replace(/"(?:[^"\\\n]|\\.)*"/g, '""');
+}
+
+function collectSourceFiles(dir: string, out: string[]): void {
+  if (!existsSync(dir)) return;
+  for (const entry of readdirSync(dir)) {
+    if (entry === "__tests__" || entry === "node_modules" || entry === "dist" || entry === "__test-utils__") continue;
+    const full = join(dir, entry);
+    if (statSync(full).isDirectory()) {
+      collectSourceFiles(full, out);
+      continue;
+    }
+    if (entry.endsWith(".ts") || entry.endsWith(".tsx")) out.push(full);
+  }
+}
+
+describe("raw workflowColumns flag census (U12)", () => {
+  const files: string[] = [];
+  for (const root of SOURCE_ROOTS) collectSourceFiles(join(REPO_ROOT, root), files);
+
+  it("scans a non-trivial production source set, so an empty sweep cannot pass", () => {
+    // Without this, a broken path glob would make every assertion below vacuously true —
+    // the "guard that reports success without checking anything" failure mode.
+    expect(files.length).toBeGreaterThan(200);
+  });
+
+  it("the raw flag is read ONLY by the known move-path sites, at the known COUNT", () => {
+    /*
+    COUNTS, not just file names (PR #2537 review — CodeRabbit). A per-file allowlist has
+    a hole exactly where it matters least visibly: a NEW raw-flag read added inside
+    `moves.ts` — already an allowed file — would have passed silently. Pinning the
+    occurrence count per file means the census notices a third read in a file that is
+    permitted two.
+
+    Whole-word matching, so a longer identifier that merely contains this one is not
+    counted. Deliberately NOT a full AST parse: that is a heavy lift for a guard whose
+    job is to notice movement, and the count already fails on the case that motivated
+    it. If this ever needs to distinguish a call from a re-export, parse then.
+    */
+    const wholeWord = new RegExp(`\\b${RAW_FLAG_READER}\\b`, "g");
+    const readers = files
+      .map((file) => ({
+        file: file.slice(REPO_ROOT.length + 1),
+        occurrences: (stripCommentsAndStrings(readFileSync(file, "utf8")).match(wholeWord) ?? []).length,
+      }))
+      .filter((entry) => entry.occurrences > 0)
+      .sort((a, b) => a.file.localeCompare(b.file));
+
+    const allowed = ALLOWED
+      .map((entry) => ({ file: entry.file, occurrences: entry.occurrences }))
+      .sort((a, b) => a.file.localeCompare(b.file));
+
+    /*
+    Equality, not subset. A subset check would let the last reader vanish silently and
+    leave the settings key orphaned forever, which is precisely the outcome this exists
+    to prevent.
+
+    If this fails because a reader was ADDED — a new file, or a higher count in an
+    existing one: do not edit ALLOWED to make it pass. A new read re-gates behaviour on
+    a flag that is false for every project that never carried a stale value, so the
+    feature behind it will not run.
+
+    If this fails because a reader was REMOVED: U2b has landed. Delete
+    `isWorkflowColumnsCompatibilityFlagEnabled`, drop `workflowColumns` from
+    `HIDDEN_EXPERIMENTAL_FEATURE_KEYS` in the dashboard SettingsModal only after
+    confirming stale persisted values still render nothing, and delete this file.
+    */
+    expect(readers).toEqual(allowed);
+  });
+
+  it("no production SOURCE LITERAL writes the key", () => {
+    /*
+    SCOPE, corrected (PR #2537 review — greptile). An earlier version of this claimed
+    "no production code writes the key", which overclaims and contradicts a correction
+    made earlier in this same unit (PR #2512, greptile P1): `settings-schema.ts`
+    explicitly TOLERATES stale persisted values, and the generic settings-update path —
+    settings import, configuration rollback — persists experimental-feature entries
+    assembled from RUNTIME data. A `true` can absolutely reach storage that way, on an
+    upgraded project that carried one.
+
+    A source scan cannot see that and must not pretend to. What it does prove is
+    narrower and still worth pinning: no module hardcodes the key, so nothing in the
+    product deliberately turns the flag on. That is the property behind "every read is
+    false for a project that never carried a stale value" — not an absolute.
+    */
+    const writers = files.filter((file) => {
+      const code = stripCommentsAndStrings(readFileSync(file, "utf8"));
+      return /workflowColumns\s*:\s*(true|false)/.test(code);
+    }).map((file) => file.slice(REPO_ROOT.length + 1));
+
+    expect(writers).toEqual([]);
+  });
+
+  it("records why each remaining reader survives, so the list cannot become folklore", () => {
+    // Cheap, but it forces the next person to state a reason when they touch the list.
+    for (const entry of ALLOWED) {
+      expect(entry.why.length).toBeGreaterThan(20);
+      expect(existsSync(join(REPO_ROOT, entry.file))).toBe(true);
+    }
+  });
+});