diff --git a/packages/engine/src/__tests__/lifecycle-column-census-ast.test.ts b/packages/engine/src/__tests__/lifecycle-column-census-ast.test.ts index cc84622358..8695697f78 100644 --- 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); + }); +}); diff --git a/scripts/lib/lifecycle-column-census-ast.mjs b/scripts/lib/lifecycle-column-census-ast.mjs index 54bbb3d4a7..7f926add12 100644 --- 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])), diff --git a/scripts/lifecycle-column-census.mjs b/scripts/lifecycle-column-census.mjs index 59e3cfdeb5..281b598a6a 100644 --- 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 —