diff --git a/scripts/__tests__/lifecycle-census-inverted-fallback.test.mjs b/scripts/__tests__/lifecycle-census-inverted-fallback.test.mjs new file mode 100644 index 0000000000..128a115063 --- /dev/null +++ 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"]); +}); diff --git a/scripts/lib/lifecycle-column-census-ast.mjs b/scripts/lib/lifecycle-column-census-ast.mjs index 540dc93b6c..65094c46a4 100644 --- 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