fix(census): the header claimed 0 trait-fallback branches while sites of that shape existed (#2874)

The census header has been printing `of the column guards, 0 are trait-fallback branches (already converted)` while sites of exactly that shape exist. I flagged this on #2842 as a suspected classifier gap; this confirms and fixes it. ## The miss Only `cond ? trait : literal` was recognised. The other spelling — a **negative** test with the literal on the **true** branch — is what a caller writes once it hoists its resolved lanes: ```ts complete: completeLanes === undefined ? columnId === "done" : completeLanes.includes(columnId) ``` That is `github-tracking-state.ts:245-246` — a fully converted resolver whose two degraded arms were reported as unconverted debt. **The backlog read higher than the remaining work**, and a reader chasing it was sent to lines that are already correct. Second half of the miss: `completeLanes` matches no hint. Adding `Lanes` to the hint list does **not** work, and the reason is itself a prior fix — hints are word-bounded because the unbounded form once let `hold` match `threshold` and `household`. `\bLanes\b` cannot match inside `completeLanes`, where the boundary does not exist. So resolved-lane identifiers get an explicit suffix rule. ## Both guards on the new rule exist because I broke them while writing it Worth stating, because each failure ran in the **dangerous direction** — marking a *live* line "already converted", which removes a real guard from a backlog people trust: | mistake | what it excused | |---|---| | widened the shared `testsTraitData` | fed the ancestor-walking rules too, which marked `step.status === "done" \|\| step.status === "in-progress"` at `register-task-workflow-routes.ts:941` — a step-**status** comparison, not a column guard — as converted | | let the new rule walk ancestors | excused any literal inside a block governed by a negative lane test | Measured: the count went to **6 with two of them wrong** before I caught it. The rule is now immediate-parent-only with its widened identifier match local to it, and reports exactly the **2 real sites**. ## Verification - Census: **176 guards, 2 trait-fallback** (was 176 / 0). The total is unchanged — this sub-count is diagnostic and does not move the ratchet, so `--strict` exits 0 with no baseline re-record. - 5 cases in `scripts/__tests__/lifecycle-census-inverted-fallback.test.mjs`, including both negatives that pin the mistakes above plus one for the suffix rule not over-reaching (`airplanes` is not a lane test). - `pnpm lint` clean; gate green (161/487/13/71).  ## Summary by CodeRabbit * **Bug Fixes** * Improved lifecycle analysis accuracy for trait fallback logic, including inverted conditions, legacy fallback syntax, and null or undefined checks. * Added safeguards to avoid misclassifying complex conditions, unrelated identifiers, and nested expressions. * Improved handling of lifecycle lane and column naming patterns. * **Tests** * Expanded coverage for valid and invalid fallback scenarios, identifier boundaries, parent-expression restrictions, and property-path checks.  --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 16:43:31 -07:00
parent 6099f028e4
commit ee8ae1eb23
2 changed files with 188 additions and 0 deletions
--- a/scripts/tests/lifecycle-census-inverted-fallback.test.mjs
+++ b/scripts/tests/lifecycle-census-inverted-fallback.test.mjs
@@ -0,0 +1,96 @@
+/*
+FNXC:LifecycleColumnCensus 2026-07-30-22:40:
+
+THE CENSUS HEADER SAID "0 ARE TRAIT-FALLBACK BRANCHES" WHILE SITES OF THAT SHAPE EXISTED.
+
+Only `cond ? trait : literal` was recognised. The other spelling — a NEGATIVE test with the literal on
+the TRUE side — is what a caller writes once it hoists its resolved lanes:
+
+  completeLanes === undefined ? columnId === "done" : completeLanes.includes(columnId)
+
+That is a fully converted resolver whose degraded arms were reported as unconverted debt, so the
+backlog read higher than the remaining work and a reader chasing it was sent to correct lines.
+
+BOTH GUARDS BELOW EXIST BECAUSE I BROKE THEM WHILE WRITING THE FIX, and each failure ran in the
+dangerous direction — marking a LIVE line "already converted":
+
+  - widening the shared `testsTraitData` predicate fed the ancestor-walking rules too, which then
+    excused a step-STATUS comparison in `register-task-workflow-routes.ts:941`;
+  - letting the new rule walk ancestors excused any literal inside a block governed by a negative
+    lane test.
+
+So the rule is immediate-parent-only and its widened identifier match is local to it.
+*/
+import test from "node:test";
+import assert from "node:assert/strict";
+
+import { findComparisons } from "../lib/lifecycle-column-census-ast.mjs";
+
+const fallbacks = (src) =>
+  findComparisons("t.ts", src).filter((f) => f.traitFallback).map((f) => f.columnId);
+
+test("an inverted fallback — negative test, literal on the TRUE branch — is recognised", () => {
+  const src = 'const c = completeLanes === undefined ? columnId === "done" : completeLanes.includes(columnId);';
+  assert.deepEqual(fallbacks(src), ["done"]);
+});
+
+test("the classic form still counts", () => {
+  const src = 'const c = columnFlags ? columnFlags.complete : columnId === "done";';
+  assert.deepEqual(fallbacks(src), ["done"]);
+});
+
+test("a POSITIVE condition with the literal on the true branch is a LIVE guard, not a fallback", () => {
+  /* The direction that must not be excused: nothing here says the trait data was absent. */
+  const src = 'const c = columnFlags ? columnId === "done" : other;';
+  assert.deepEqual(fallbacks(src), []);
+});
+
+test("a lane test does NOT excuse a literal elsewhere in the same block", () => {
+  /*
+  The ancestor-walk over-reach. `step.status === "done"` is not a column guard at all, and letting the
+  new rule climb marked exactly this shape as converted.
+  */
+  const src = `
+    function f() {
+      if (completeLanes === undefined) {
+        return steps.find((step) => step.status === "done" || step.status === "in-progress");
+      }
+      return null;
+    }`;
+  assert.deepEqual(fallbacks(src), []);
+});
+
+test("an identifier merely CONTAINING a lane word is not a lane test", () => {
+  /* `\\bLanes\\b` cannot match inside `completeLanes`, which is why the suffix rule exists — but it
+     must not over-reach onto unrelated names either. */
+  const src = 'const c = airplanes === undefined ? columnId === "done" : other;';
+  assert.deepEqual(fallbacks(src), []);
+});
+
+/*
+FNXC:LifecycleColumnCensus 2026-07-30-23:20 (#2874 review — greptile P2):
+A COMPOUND condition is not a simple absence test. `completeLanes === undefined || forceLegacy` has a
+second disjunct that can select the true branch with lane data PRESENT, so the literal there is a live
+guard. The unanchored check accepted it, which removes a real guard from the backlog — the direction
+this rule exists to stop.
+*/
+
+test("a compound absence test is NOT treated as a fallback", () => {
+  const src = 'const c = completeLanes === undefined || forceLegacy ? columnId === "done" : other;';
+  assert.deepEqual(fallbacks(src), []);
+});
+
+test("a conjunction is refused too", () => {
+  const src = 'const c = completeLanes === undefined && legacyMode ? columnId === "done" : other;';
+  assert.deepEqual(fallbacks(src), []);
+});
+
+test("the simple negated form still counts", () => {
+  const src = 'const c = !completeLanes ? columnId === "done" : completeLanes.includes(columnId);';
+  assert.deepEqual(fallbacks(src), ["done"]);
+});
+
+test("a property-path absence test still counts", () => {
+  const src = 'const c = lifecycle?.completeLanes === undefined ? columnId === "done" : other;';
+  assert.deepEqual(fallbacks(src), ["done"]);
+});
--- a/scripts/lib/lifecycle-column-census-ast.mjs
+++ b/scripts/lib/lifecycle-column-census-ast.mjs
@@ -329,6 +329,23 @@ const TRAIT_TEST_HINTS = [
  "resolveLifecycleColumns", "intake", "hold", "countsTowardWip", "mergeOrchestration", "mergeBlocker",
 ];

+/*
+FNXC:LifecycleColumnCensus 2026-07-30-22:15 (the other half of the inverted-fallback miss):
+A CAMEL-CASE SUFFIX CANNOT BE A HINT, so resolved-lane variables get their own rule.
+
+A resolver that hoists its answer names it `completeLanes`, `activeLanes`, `reviewLanes`,
+`archivedLanes` — the shape used across analytics, the glasses notifier and the GitHub tracking
+classifier. Adding `Lanes` to the hint list does NOT work, and the reason is a fix rather than an
+oversight: hints are word-bounded because the unbounded form once let `hold` match `threshold` and
+`household`. `\bLanes\b` therefore cannot match inside `completeLanes`, where the boundary the
+regex needs does not exist.
+
+So the suffix is matched explicitly. It is narrow on purpose — an identifier ENDING in `Lanes` or
+`Columns` — which covers the resolved-lane naming without reopening the substring problem: `planes`
+does not end in `Lanes` (case-sensitive), and `columnsRendered` does not end in `Columns`.
+*/
+const RESOLVED_LANE_IDENTIFIER = /\b[A-Za-z_$][A-Za-z0-9_$]*(?:Lanes|Columns)\b/;
+
 /*
 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`
@@ -376,6 +393,38 @@ function alwaysTerminates(stmt) {
 * 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.
 */
+/**
+ * True when a condition asks "is the trait data ABSENT?" rather than "is it present?".
+ *
+ * Form only, deliberately: `=== undefined`, `== null`, `=== null` or a leading `!`. Anything else is
+ * treated as a positive test, so an unrecognised spelling leaves the site COUNTED — the safe
+ * direction for a backlog measurement, since over-counting sends a reader to a correct line while
+ * under-counting hides a live guard.
+ */
+function isNegativeTraitTest(text) {
+  /*
+  FNXC:LifecycleColumnCensus 2026-07-30-23:15 (#2874 review — greptile P2, "compound absence tests
+  over-classify"): SIMPLE conditions only.
+
+  The equality check was unanchored, so `completeLanes === undefined || forceLegacy` passed — and its
+  second disjunct can select the true branch with lane data PRESENT, making the literal a live guard
+  rather than a fallback. Marking a live line "already converted" is the direction that removes a real
+  guard from the backlog, which is the failure this whole rule was added to stop doing.
+
+  A compound condition is not something to reason about here: whether the literal is reachable with
+  traits present depends on the other operand. Refusing them leaves those sites COUNTED, which is the
+  safe answer for a measurement — over-counting sends a reader to a correct line, under-counting hides
+  a live one.
+
+  ANCHORING IS WHAT DOES THE WORK, not a separate compound check. I wrote one — `if (/[|&]{2}/) return
+  false` — and mutation showed it was dead: `^...$` already refuses anything with an operand beside
+  the comparison. A redundant guard carrying a comment that claims it is load-bearing is worse than no
+  guard, because the next reader trusts it instead of the anchors.
+  */
+  return /^\s*[A-Za-z_$][\w.$?[\]"'`]*\s*(===|==)\s*(undefined|null)\s*$/.test(text.trim())
+    || /^\s*!\s*[A-Za-z_$][\w.$?[\]"'`]*\s*$/.test(text.trim());
+}
+
 function isTraitFallback(node, sourceFile) {
  let current = node;
  while (current.parent && !ts.isSourceFile(current.parent)) {
@@ -383,6 +432,49 @@ function isTraitFallback(node, sourceFile) {
    if (ts.isConditionalExpression(parent) && parent.whenFalse === current) {
      if (testsTraitData(parent.condition.getText(sourceFile))) return true;
    }
+    /*
+    FNXC:LifecycleColumnCensus 2026-07-30-21:55 (an INVERTED fallback the census counted as backlog):
+    THE LITERAL IS SOMETIMES THE `whenTrue` BRANCH, BECAUSE THE CONDITION ASKS THE QUESTION BACKWARDS.
+
+    Only `cond ? trait : literal` was recognised. The other spelling is at least as common once a
+    caller resolves its lanes up front:
+
+      complete: completeLanes === undefined ? columnId === "done" : completeLanes.includes(columnId)
+
+    That is `github-tracking-state.ts:245`, a fully converted resolver whose two degraded arms the
+    census reports as unconverted debt. The consequence is not cosmetic: the census header says
+    "0 are trait-fallback branches (already converted)" while sites of exactly that shape exist, so
+    the remaining number reads higher than the remaining WORK and a reader chasing the backlog is
+    sent to lines that are already correct.
+
+    A NEGATIVE trait test plus the true branch is the same statement as a positive test plus the
+    false branch. Recognising it needs the condition to be negative in FORM — `=== undefined`,
+    `== null`, or a leading `!` — because a positive condition with the literal on the true side is a
+    live guard, not a fallback, and must keep counting.
+
+    IMMEDIATE PARENT ONLY (`current === node`), unlike the sibling rules that walk ancestors, and this
+    is measured rather than cautious. With the walk, any literal ANYWHERE inside a block governed by a
+    negative lane test was marked converted — including
+    `step.status === "done" || step.status === "in-progress"` in
+    `register-task-workflow-routes.ts:941`, a STEP-STATUS comparison that is not a column guard at
+    all. Marking a live line "already converted" is the dangerous direction for a backlog measurement,
+    so this rule only fires where the literal IS the ternary's true branch, which is the shape it was
+    written for.
+    */
+    if (current === node && ts.isConditionalExpression(parent) && parent.whenTrue === current) {
+      const condition = parent.condition.getText(sourceFile);
+      /*
+      The lane-suffix rule is applied HERE ONLY, not folded into `testsTraitData`. Widening that
+      shared predicate fed the ancestor-walking rules too, and they promptly marked
+      `step.status === "done" || step.status === "in-progress"` at
+      `register-task-workflow-routes.ts:941` — a STEP-STATUS comparison, not a column guard — as an
+      already-converted fallback. Measured, not hypothetical: the count went to 6 with two of them
+      wrong. A widening that reaches rules it was not reasoned about is how a measurement quietly
+      starts excusing live lines.
+      */
+      if (isNegativeTraitTest(condition)
+        && (testsTraitData(condition) || RESOLVED_LANE_IDENTIFIER.test(condition))) return true;
+    }
    if (ts.isIfStatement(parent)) {
      /*
      Two shapes count: the explicit `else`, and the EARLY-RETURN form — `if (flags) return ...;` followed by