From b9b7d1480420299a4d240091c0e503e488557ea0 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 07:06:48 -0700 Subject: [PATCH] =?UTF-8?q?fix(core):=20a=20type=20that=20taught=20the=20w?= =?UTF-8?q?rong=20invariant=20=E2=80=94=20staleness=20signal=20column=20na?= =?UTF-8?q?rrowed=20to=20legacy=20ids=20(#3159)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Type-only. No runtime behaviour changes**, and I would rather say that than let a green suite imply otherwise. ## The type described a guard that no longer exists `TaskAgeStalenessSignal.column` was typed `"in-progress" | "in-review"` and filled through a cast carrying this justification: ```ts // The guard above proves `column` is one of these two legacy ids ... (#1403) const activeColumn = task.column as "in-progress" | "in-review"; ``` True when written. The guard now reads: ```ts const wipColumn = context.lifecycle?.wip ?? "in-progress"; const reviewColumn = context.lifecycle?.review ?? "in-review"; if (task.column !== wipColumn && task.column !== reviewColumn) return undefined; ``` So on a renamed board it proves the column is `building` or `checking` — and the cast asserted the **opposite** of what the guard established. The runtime was always fine; the real id passed straight through. ## The damage is in what the type taught A consumer writing `signal.column === "building"` got a **compile error** saying the comparison was impossible. The type actively instructed callers that `=== "in-progress"` is exhaustive — the exact guard shape this program spends its time removing. This is the second instance of the shape today. The first was `dashboard/src/server.ts`: ```ts moveTask(taskId: string, column: "todo", options?: …): Promise; ``` which made the type system **reject** a resolved target (#3158). Neither was a constraint anyone chose — both were inferred from a single legacy call site and then hardened into an assertion about live data. **A type narrowed to legacy ids is a lint against fixing the code**, and it is invisible to every gate this program has: the census counts comparisons, the move-target ratchet counts arguments, and neither looks at type positions. ## The test is a characterization, and says so No runtime test can differentiate a type-level fix — **`tsc` is what differentiates it**. The added case pins a value that was already correct, so a future narrowing has something to break against besides a compile error nobody sees until they hit it. I have labelled it in the file rather than presenting it as a regression test. ## Verification | | result | |---|---| | `tsc` — core, engine, dashboard (app + src) | **0 errors** each | | `task-age-staleness` | **17 passed** | | all three staleness suites | **28 passed** | | census `--strict` | exit 0 | All three consumers of `.column` only display or compare it (`taskAgeStalenessCopy.ts`, `TaskDetailModal`, a `TaskCard` memo comparison), so nothing downstream narrows on the widened type. ## Scope note I scanned for this class and the raw pattern is noisy — 192 candidates, almost all object-literal **values** (`status: "archived"`), agent roles, and unrelated `type: "done"` stream events. This one and `server.ts` are the two I could confirm as genuine type-position narrowings on a *task column*. I have not filed an issue for the class because I cannot yet separate it from the noise reliably; if a cheap discriminator turns up, it is worth a ratchet like the move-target one. Co-authored-by: Claude Opus 5 (1M context) --- .../src/__tests__/task-age-staleness.test.ts | 29 +++++++++++++++++ packages/core/src/task-age-staleness.ts | 32 ++++++++++++++++--- 2 files changed, 56 insertions(+), 5 deletions(-) diff --git a/packages/core/src/__tests__/task-age-staleness.test.ts b/packages/core/src/__tests__/task-age-staleness.test.ts index 0aa5cebdc3..9f6c68daef 100644 --- a/packages/core/src/__tests__/task-age-staleness.test.ts +++ b/packages/core/src/__tests__/task-age-staleness.test.ts @@ -206,4 +206,33 @@ describe("age staleness resolves its lanes by ROLE", () => { // With a 1ms wip warning and a ~999h review warning, only the wip threshold can produce critical. expect(wipSignal?.level).toBe("critical"); }); + + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:59: + CHARACTERIZATION, and it is honest about being one: this pins the value the signal already emitted. + + `TaskAgeStalenessSignal.column` was typed `"in-progress" | "in-review"` and filled through a cast + whose comment said "the guard above proves `column` is one of these two legacy ids". That stopped + being true when the guard was converted to compare against the RESOLVED wip/review lanes — from then + on the cast asserted the opposite of what the guard established, while the runtime happily passed the + renamed id through. + + So widening the type changes NO behaviour, and no runtime test can differentiate it; the thing that + differentiates is `tsc`. Under the old narrow type a consumer writing `signal.column === "building"` + got a compile error telling them the comparison was impossible — the type actively taught the wrong + invariant. This case exists so the value is at least pinned, and so a future narrowing has something + to break against besides a type error nobody sees until they hit it. + + The `as never` casts on the fixtures below are the same shape and stay: `ColumnId` is a union with a + `string & {}` member, and these are deliberately ids no board declares. + */ + it("reports the RENAMED lane id it matched, not a legacy one", () => { + const signal = getTaskAgeStalenessSignal( + { ...baseTask, column: "building" as never, columnMovedAt: STALE_MOVED_AT }, + { now: NOW, lifecycle: RENAMED }, + ); + + expect(signal).toBeDefined(); + expect(signal?.column).toBe("building"); + }); }); diff --git a/packages/core/src/task-age-staleness.ts b/packages/core/src/task-age-staleness.ts index 37205bda71..e8f35b7299 100644 --- a/packages/core/src/task-age-staleness.ts +++ b/packages/core/src/task-age-staleness.ts @@ -9,7 +9,21 @@ export interface TaskAgeStalenessSignal { ageMs: number; warningThresholdMs: number; criticalThresholdMs: number; - column: "in-progress" | "in-review"; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:59: + WIDENED from `"in-progress" | "in-review"`, because the guard that fills it was converted and this + type was left describing the old one. + + `getTaskAgeStalenessSignal` resolves the lanes it accepts (`context.lifecycle?.wip ?? …`), so on a + renamed board this field legitimately holds `building` or `checking`. The narrow type therefore + asserted something FALSE about live data, and forced a cast to keep saying it — see the note at the + assignment. + + A type narrowed to legacy ids is not a harmless leftover: it tells every consumer that a + `=== "in-progress"` comparison is exhaustive, which is exactly the guard this program spends its + time removing. All three consumers today only display or compare the value, so widening is safe. + */ + column: string; paused: boolean; } @@ -63,10 +77,18 @@ export function getTaskAgeStalenessSignal( if (task.column !== wipColumn && task.column !== reviewColumn) { return undefined; } - // The guard above proves `column` is one of these two legacy ids; the - // `ColumnId` union's `string & {}` member can't be excluded by literal `!==` - // narrowing, so the cast is provably safe here (#1403). - const activeColumn = task.column as "in-progress" | "in-review"; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:59: + THE CAST IS GONE, and the comment it carried had become false. + + It read: "the guard above proves `column` is one of these two legacy ids" (#1403). That was true + when the guard compared against the literals. The guard now compares against `wipColumn` / + `reviewColumn`, which are RESOLVED — so on a renamed board it proves the column is `building` or + `checking`, and the cast was asserting the opposite of what the guard established. + + Nothing needs casting now that the field's type matches what the guard admits. + */ + const activeColumn = task.column; if (task.mergeDetails?.mergeConfirmed === true) { return undefined; }