Tell an already-converted fallback literal from an unconverted guard — 19 of 19 dashboard scan hits were the former (#2677)

The batch phase is about to hand per-file guard lists to cheap workers, and the census currently cannot distinguish **"not yet converted"** from **"converted, with a documented degradation."** ## The measurement that makes this a class, not a preference A proximity scan for *"legacy literal near a role-resolved call"* — the heuristic that produced #2670 and #2672 from the engine — returned **19 hits across the dashboard and zero defects.** Every one was: ```ts if (flags) return flags.hold === true || flags.countsTowardWip === true; return column === "todo" || column === "in-progress"; // reachable only without traits ``` That literal is **correct**: it answers for callers with no resolved column metadata, which is the case `resolveLifecycleColumns` returns `undefined`-for-the-whole-struct to preserve. A worker told to "convert" it would delete the only answer available when traits are absent. ## And the difference is structural, so the parser can see it In **both** engine defects the literal sat in a **separate statement beside resolved data**, not in a fallback branch. Proximity cannot tell those apart; an AST can. `traitFallback` flags the ternary form and the **early-return** form (which is how most are actually written), and deliberately does **not** flag a fallback whose test is itself a column-*name* check — otherwise any `if/else` over column names would launder itself. ## Reported beside the backlog, not subtracted from it ``` COLUMN guards (the backlog): 746 of the column guards, 9 are trait-fallback branches (already converted) ``` A fallback literal is still a literal and should go when the trait path becomes unconditional. This only says which **kind** of work it is. **Advisory, and structurally so:** `traitFallback` never changes `kind`, and the count lives *outside* `totals`. My first attempt put it in `totals` and broke two existing suites that correctly deep-equal that shape — an advisory number does not belong in the structure that defines the bar. ## Revert proof Forcing `traitFallback: false` fails **3 of 33** (both fallback forms, plus the kind-unchanged case). The two *negative* cases pass under the revert — which is the point: they assert what must **not** be flagged, and a classifier that flags nothing satisfies them trivially. Worth stating, because a revert proof that only counts failures would look stronger than it is. ## Baseline Re-recorded: `executor.ts` 87 → 85 was **main's own drift** from #2568 landing, so `--strict` was red on main again. #2668 made the re-record possible; the auto-tighten (coordinator item 2) is still open, and this is the third time in this program that a legitimate merge has left the gate red for everyone else. ## Verification 62/62 across both census suites, `pnpm test:gate` **71/71**, `--strict` exits 0, `pnpm lint` clean. 🤖 Generated with [Claude Code](https://claude.com/claude-code)  ## Summary by CodeRabbit * **New Features** * Added reporting for legacy column comparisons found in trait-fallback branches. * Census results now include a separate count for these fallback-related column guards. * Human-readable reports display the new metric alongside the existing backlog totals. * **Tests** * Added coverage for fallback detection across ternary, early-return, and conditional patterns. * Added safeguards to prevent false positives in resolved-data and column-name checks. * **Maintenance** * Updated baseline census metrics to reflect revised classifications.  --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 02:33:19 -07:00
parent 30e287a29e
commit 543f4a556c
3 changed files with 301 additions and 0 deletions
--- a/packages/engine/src/tests/lifecycle-column-census-ast.test.ts
+++ b/packages/engine/src/tests/lifecycle-column-census-ast.test.ts
@@ -326,3 +326,171 @@ describe("state, phase and result enums are not column guards", () => {
    expect(result.totals.column).toBe(1);
  });
 });
+
+/*
+FNXC:LifecycleColumnCensus 2026-08-01-01:30:
+
+A LEGACY LITERAL IN A FALLBACK BRANCH IS NOT BACKLOG. The converted shape is
+
+    if (flags) return flags.hold === true || flags.countsTowardWip === true;
+    return column === "todo" || column === "in-progress";      // reachable only without traits
+
+and that literal is CORRECT — it answers for callers with no resolved column metadata, which is the case
+`resolveLifecycleColumns` returns `undefined`-for-the-whole-struct to preserve. Telling a batch worker to
+"convert" it means telling them to delete the only answer available when traits are absent.
+
+MEASURED, which is why this is a class and not a preference: a proximity scan for "legacy literal near a
+role-resolved call" over the dashboard returned 19 hits and ZERO defects, all this shape. The same scan over
+the engine found two real defects (#2670, #2672) — and in BOTH the literal was in a SEPARATE STATEMENT beside
+resolved data, not in a fallback branch. That difference is structural, so the parser can see it; proximity
+cannot.
+
+Advisory only: `traitFallback` never changes `kind`, so a wrong hint cannot move the bar. Repo-wide it flags
+8 of 746 column guards — 8 and not 9 since the #2677 review: the `hold` hint was matching inside `threshold`,
+which had marked one live guard as already-converted.
+*/
+describe("a literal in a trait-fallback branch is flagged as such", () => {
+  const fallbacksIn = (source: string) => census(source).filter((f) => (f as { traitFallback?: boolean }).traitFallback).length;
+
+  it("flags the ternary form", () => {
+    const source = `export function isHold(flags: F | undefined, columnId: string) {
+      return flags ? flags.hold === true : columnId === "todo";
+    }`;
+
+    expect(fallbacksIn(source)).toBe(1);
+  });
+
+  it("flags the early-return form, which is how most of them are written", () => {
+    const source = [
+      "function mayEdit(column: string, flags?: F) {",
+      "  if (flags) return flags.hold === true || flags.countsTowardWip === true;",
+      `  return column === "todo" || column === "in-progress";`,
+      "}",
+    ].join("\n");
+
+    // Two literals in one return, both in the fallback.
+    expect(fallbacksIn(source)).toBe(2);
+  });
+
+  it("does NOT flag a literal in a separate statement beside resolved data — the #2670 / #2672 shape", () => {
+    /*
+    This is the distinction the whole class exists for. Both engine defects looked like this: resolved lane
+    data in scope, and a guard next to it still matching a name. Flagging these as fallbacks would have hidden
+    exactly the two bugs the scan found.
+    */
+    const source = [
+      "async function park(task: T) {",
+      "  const lanes = await resolveLifecycleColumns(ir);",
+      `  if (task.column === "done" || task.column === "archived") return false;`,
+      "  await move(task.id, lanes.hold);",
+      "}",
+    ].join("\n");
+
+    expect(fallbacksIn(source)).toBe(0);
+  });
+
+  it("does NOT flag a fallback branch whose test is itself a column-name check", () => {
+    // `if (column === "todo")` tests a NAME, not resolved data, so its else branch is not a trait fallback —
+    // otherwise any if/else over column names would launder itself.
+    const source = [
+      "function f(column: string) {",
+      `  if (column === "todo") return 1;`,
+      `  return column === "in-progress" ? 2 : 3;`,
+      "}",
+    ].join("\n");
+
+    expect(fallbacksIn(source)).toBe(0);
+  });
+
+  it("does NOT flag when the trait branch only returns CONDITIONALLY — a return token is not termination", () => {
+    /*
+    FNXC:LifecycleColumnCensus 2026-07-30-10:05 (PR #2677 review — greptile):
+    The detector used to search the branch for the word `return`. Here the branch contains one, but it is
+    nested under its own `if`, so with `flags` present control still falls through and the literal below
+    IS evaluated. That makes it a live guard, and flagging it as a fallback would quietly remove a real
+    guard from the backlog this census is trusted to report.
+    */
+    const source = [
+      "function mayEdit(column: string, flags?: F) {",
+      "  if (flags) {",
+      "    if (flags.hold === true) return true;",
+      "  }",
+      `  return column === "todo";`,
+      "}",
+    ].join("\n");
+
+    expect(fallbacksIn(source)).toBe(0);
+  });
+
+  it("still flags a trait branch that terminates on EVERY path", () => {
+    /* The paired positive: if/else where both arms return is genuine termination, so the literal below
+       really is the no-traits answer. "Not a bare return token" must not become "no early returns". */
+    const source = [
+      "function mayEdit(column: string, flags?: F) {",
+      "  if (flags) {",
+      "    if (flags.hold === true) return true;",
+      "    else return false;",
+      "  }",
+      `  return column === "todo";`,
+      "}",
+    ].join("\n");
+
+    expect(fallbacksIn(source)).toBe(1);
+  });
+
+  it("does NOT flag when the trait branch ends in a non-terminating statement after a nested return", () => {
+    /* Last-statement-wins: the block's final statement is a call, so the branch falls through. */
+    const source = [
+      "function mayEdit(column: string, flags?: F) {",
+      "  if (flags) {",
+      "    if (flags.hold === true) return true;",
+      "    log(flags);",
+      "  }",
+      `  return column === "todo";`,
+      "}",
+    ].join("\n");
+
+    expect(fallbacksIn(source)).toBe(0);
+  });
+
+  it("does NOT treat a hint embedded in a longer identifier as trait data", () => {
+    /*
+    FNXC:LifecycleColumnCensus 2026-07-30-10:40 (PR #2677 review — coderabbit):
+    `hold` is a hint, and it is a substring of `threshold`. A branch testing an unrelated
+    threshold was read as testing resolved trait data, which marks the live guard below it as an
+    already-converted fallback. `flags` inside `myflags` had the same problem.
+    */
+    for (const test of ["staleThreshold > 0", "opts.threshold != null", "household.size", "myflags"]) {
+      const source = [
+        "function f(column: string) {",
+        `  if (${test}) return 1;`,
+        `  return column === "todo";`,
+        "}",
+      ].join("\n");
+
+      expect(fallbacksIn(source)).toBe(0);
+    }
+  });
+
+  it("still sees the genuine trait forms the hints exist for", () => {
+    /* The paired positive: tightening the boundary must not stop matching real trait reads. */
+    for (const test of ["flags.hold === true", "flags?.hold", "lifecycleRoles.hold", "flags"]) {
+      const source = [
+        "function f(column: string, flags?: F) {",
+        `  if (${test}) return 1;`,
+        `  return column === "todo";`,
+        "}",
+      ].join("\n");
+
+      expect(fallbacksIn(source)).toBe(1);
+    }
+  });
+
+  it("leaves `kind` alone, so a wrong hint cannot move the bar", () => {
+    const source = `const x = flags ? flags.hold === true : columnId === "todo";`;
+    const finding = census(source)[0] as { kind: string; traitFallback?: boolean };
+
+    expect(finding.kind).toBe("column");
+    expect(finding.traitFallback).toBe(true);
+  });
+});
--- a/scripts/lib/lifecycle-column-census-ast.mjs
+++ b/scripts/lib/lifecycle-column-census-ast.mjs
@@ -224,6 +224,114 @@ function columnPropertyLiteral(node) {
  return LEGACY_COLUMN_IDS.includes(node.initializer.text) ? node.initializer.text : undefined;
 }

+
+/*
+FNXC:LifecycleColumnCensus 2026-08-01-01:10:
+A LEGACY LITERAL IN A FALLBACK BRANCH IS NOT BACKLOG. The converted shape across the dashboard is
+
+    if (flags) return flags.hold === true || flags.countsTowardWip === true;
+    return column === "todo" || column === "in-progress";      // <- reachable only without traits
+
+and that second literal is CORRECT: it answers for callers that have no resolved column metadata, which is
+the distinction `resolveLifecycleColumns` returns `undefined`-for-the-whole-struct to preserve. Counting it
+as an unconverted guard tells a batch worker to convert code that is already converted — and "convert it"
+there means deleting the only answer available when traits are absent.
+
+MEASURED, which is why this is worth a class rather than a preference: a proximity scan for
+"legacy literal near a role-resolved call" over the dashboard returned 19 hits and ZERO defects, every one
+this shape. The same scan over the engine found two real defects (#2670, #2672) — and in BOTH the literal was
+in a separate statement beside resolved data, not in a fallback branch. That is the whole difference, and it
+is structural, so the parser can see it.
+
+Reported as its own line rather than removed from the total: a fallback literal is still a literal, and the
+day the trait path is unconditional it should be deleted. This distinguishes "not yet converted" from
+"converted, with a documented degradation".
+*/
+const TRAIT_TEST_HINTS = [
+  "flags", "Flags", "columnFlags", "lifecycle", "roles", "trait", "resolveColumnFlags", "columnHasFlag",
+  "resolveLifecycleColumns", "intake", "hold", "countsTowardWip", "mergeOrchestration", "mergeBlocker",
+];
+
+/*
+FNXC:LifecycleColumnCensus 2026-07-30-10:40 (PR #2677 review — coderabbit):
+HINTS MUST NOT MATCH INSIDE A LONGER IDENTIFIER. The leading class was `[.?\w]`, so the `hold`
+hint matched `threshold`, `staleThreshold`, `household`, `stronghold`, `withhold` — and `flags`
+matched `myflags`. Any branch testing an unrelated `threshold` was then read as testing resolved
+trait data, which marks a live legacy guard as an already-converted fallback.
+
+The `\w` was also REDUNDANT, which is why removing it costs nothing: the third alternative
+`\bhold\b` already matches `flags.hold` and `flags?.hold`, because `.` is a non-word character
+and so supplies the word boundary itself. The `\w` alternative added only the false positives.
+*/
+/** True when `text` reads as a test for resolved trait data rather than for a column name. */
+function testsTraitData(text) {
+  return TRAIT_TEST_HINTS.some((hint) => new RegExp(`[.?]${hint}\\b|\\b${hint}\\s*[?.]|\\b${hint}\\b`).test(text))
+    && !new RegExp(`(===|!==)\\s*["'](${LEGACY_COLUMN_IDS.join("|")})["']`).test(text);
+}
+
+/*
+FNXC:LifecycleColumnCensus 2026-07-30-10:05 (PR #2677 review — greptile):
+A RETURN TOKEN IS NOT TERMINATION. The early-return detector used to ask whether the `then`
+branch CONTAINED the word `return` anywhere. A branch that only returns conditionally —
+`if (flags) { if (x) return a; }` — satisfies that while still falling through, so the
+literal after it is REACHABLE with traits present and is a live guard.
+
+The misclassification runs in the dangerous direction: it removes a real guard from the
+backlog the census is trusted to report, and it does so silently. This asks whether the
+branch DEFINITELY terminates instead, which is a property of structure rather than of the
+presence of a token.
+*/
+function alwaysTerminates(stmt) {
+  if (!stmt) return false;
+  if (ts.isReturnStatement(stmt) || ts.isThrowStatement(stmt)) return true;
+  if (ts.isBlock(stmt)) {
+    const statements = stmt.statements ?? [];
+    return statements.length > 0 && alwaysTerminates(statements[statements.length - 1]);
+  }
+  /* Only terminates when BOTH arms do — a missing else is exactly the fall-through case. */
+  if (ts.isIfStatement(stmt)) {
+    return alwaysTerminates(stmt.thenStatement) && alwaysTerminates(stmt.elseStatement);
+  }
+  return false;
+}
+
+/**
+ * True when this comparison sits in the FALLBACK branch of a conditional whose test reads resolved
+ * trait data — i.e. it is the documented answer for callers without traits, not an unconverted guard.
+ */
+function isTraitFallback(node, sourceFile) {
+  let current = node;
+  while (current.parent && !ts.isSourceFile(current.parent)) {
+    const parent = current.parent;
+    if (ts.isConditionalExpression(parent) && parent.whenFalse === current) {
+      if (testsTraitData(parent.condition.getText(sourceFile))) return true;
+    }
+    if (ts.isIfStatement(parent)) {
+      /*
+      Two shapes count: the explicit `else`, and the EARLY-RETURN form — `if (flags) return ...;` followed by
+      the literal as the next statement, which is how most of these are actually written. The early-return
+      case is detected by looking at the preceding sibling statement rather than at an else branch.
+      */
+      if (parent.elseStatement === current && testsTraitData(parent.expression.getText(sourceFile))) return true;
+    }
+    if (ts.isBlock(parent) || ts.isSourceFile(parent)) {
+      const statements = parent.statements ?? [];
+      const index = statements.indexOf(current);
+      for (let i = index - 1; i >= 0 && i >= index - 2; i -= 1) {
+        const prior = statements[i];
+        if (ts.isIfStatement(prior)
+          && prior.elseStatement === undefined
+          && testsTraitData(prior.expression.getText(sourceFile))
+          && alwaysTerminates(prior.thenStatement)) {
+          return true;
+        }
+      }
+    }
+    current = parent;
+  }
+  return false;
+}
+
 /** Parse one file and classify every comparison against a legacy column id. */
 export function findComparisons(filePath, source) {
  const sourceFile = ts.createSourceFile(filePath, source, ts.ScriptTarget.Latest, true, ts.ScriptKind.TSX);
@@ -246,6 +354,11 @@ export function findComparisons(filePath, source) {
          columnId: parts.literal,
          receiver,
          kind: deliberate ? "deliberate" : isRole ? "role" : isStatus ? "status" : "column",
+          /*
+          Advisory only — it never changes `kind`, so a wrong hint cannot move the bar. It exists so a batch
+          worker can tell an unconverted guard from an already-converted site's documented fallback.
+          */
+          traitFallback: isTraitFallback(node, sourceFile),
        });
      }
    }
@@ -268,6 +381,14 @@ export function findComparisons(filePath, source) {

 /** Aggregate findings into the four headline counts plus per-file and per-column breakdowns. */
 export function summarize(findings) {
+  /*
+  FNXC:LifecycleColumnCensus 2026-08-01-01:55:
+  Reported ALONGSIDE `totals`, not inside it. `totals` is the four-class contract other suites deep-equal, so
+  adding a key there breaks assertions that are correctly strict about the shape — my first attempt did
+  exactly that and failed two existing cases. An advisory number does not belong in the structure that
+  defines the bar.
+  */
+  let traitFallbackCount = 0;
  const totals = { column: 0, role: 0, status: 0, deliberate: 0 };
  const byColumnId = {};
  const byFile = new Map();
@@ -302,6 +423,7 @@ export function summarize(findings) {
      continue;
    }
    totals[finding.kind] += 1;
+    if (finding.kind === "column" && finding.traitFallback) traitFallbackCount += 1;
    if (finding.kind === "deliberate") {
      /*
      FNXC:WorkflowLifecycleColumns 2026-07-31-10:00 (PR #2661 review — greptile P1, same class again):
@@ -324,6 +446,8 @@ export function summarize(findings) {

  return {
    totals,
+    /* Advisory, deliberately OUTSIDE `totals`: see the note on `summarize`. */
+    traitFallbackCount,
    byColumnId,
    byFile: [...byFile].sort((a, b) => b[1] - a[1] || a[0].localeCompare(b[0])),
    deliberateByFile: [...deliberateByFile].sort((a, b) => b[1] - a[1] || a[0].localeCompare(b[0])),
--- a/scripts/lifecycle-column-census.mjs
+++ b/scripts/lifecycle-column-census.mjs
@@ -98,6 +98,15 @@ if (json) {
  console.log(`  STATUS comparisons (not guards): ${summary.totals.status}`);
  console.log(`  DELIBERATE-LITERAL (reviewed): ${summary.totals.deliberate}`);
  /*
+  FNXC:LifecycleColumnCensus 2026-08-01-01:40:
+  Reported BESIDE the backlog, not subtracted from it. A fallback literal is still a literal and should go
+  when the trait path becomes unconditional — but it is an ALREADY-CONVERTED site's documented degradation,
+  not unconverted work, and a batch worker told to convert it would delete the only answer available to a
+  caller without traits. Measured: 19 of 19 dashboard proximity hits were this shape and none was a defect,
+  while both engine defects (#2670, #2672) were literals in a separate statement instead.
+  */
+  console.log(`  of the column guards, ${summary.traitFallbackCount ?? 0} are trait-fallback branches (already converted)`);
+  /*
  FNXC:LifecycleColumnCensus 2026-07-29-19:40:
  Reported BESIDE the backlog, never inside it. A `column: "todo"` source query decides which rows
  a sweep even considers, so it can kill a sweep whose per-task guard was correctly converted —