From 0da19f7963ae2790f386d2be4c1a8e7befa2b53c Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 04:29:21 -0700 Subject: [PATCH] fix(core): a renamed archive lane was recorded as `done` in the eval corpus; flag the scheduler's two honest literals (#3100) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two pieces, both about the same distinction: which literals are worth **converting** and which are worth **naming**. ## Converted — the eval corpus was mislabelling renamed archive lanes `collectDeterministicSignals` writes `column` as a two-value eval-record field. Against the `archived` literal, a card resting in a renamed archive lane was recorded as `"done"`. No crash, no lifecycle decision — a **mislabelled row in the eval corpus**, which is a dataset every later comparison reads. That is the expensive kind of quiet: nothing fails, the numbers just drift. The collector is sync and pure (no store, no workflow), so the lane answer arrives as an optional parameter. `HybridEvaluatorService.evaluateTask` is async and already holds an optional store, which is where the resolution is paid; a store-less evaluator degrades to the legacy literal rather than failing. **Only the archived arm was ever wrong.** A renamed *complete* lane was, and remains, recorded as `"done"` — which is correct. So only that answer is resolved, and a third case pins that the widening did not turn every renamed lane into `"archived"`. ## Flagged, not converted — the scheduler's two honest literals These are the two `scheduler.ts` literals the sync-lane pass did not take, and **nothing in the file said why**. That silence is the problem: the obvious next move is to "finish the job" the way the other ten were converted, and that would make them **inert, not fixed**. `getTaskWorkflowSelectionImpl` returns `undefined` unconditionally under PostgreSQL, so `resolveTaskWorkflowIrSync` always answers with the default builtin IR — proved in `postgres/sync-workflow-ir-is-always-default.pg.test.ts`, and `check-inert-sync-lane-conversions` already baselines **twenty** guards in that state in this same file. They stay literal and **counted**, which is the honest state. An unconverted literal is visible to the census; an inert conversion leaves the backlog and takes the evidence with it. The note names the real blocker — a sync-capable workflow-selection reader — so the next pass does not spend a cycle discovering this the way I did. ## Measured - 3 new cases in `eval-signal-collector.test.ts` — file **5/5 pass**. - **MUTATION**: restoring the `archived` literal fails the renamed case and leaves **both** the legacy control and the renamed-complete negative green. The negative matters here: the fix must not turn every renamed lane into `"archived"`. - core eval suites — **4 files / 20 tests**; engine scheduler + evaluator — **14 files / 143 tests**. - `tsc --noEmit` clean in both packages; census `--strict`, `check-lane-wiring`, `check-inert-sync-lane-conversions`, `check-fnxc-future-dates` clean. ## Census Both files keep their counts, deliberately: - `eval-signal-collector.ts` — the remaining entry is the new parameter's documented default, which is the fallback doing its job. - `scheduler.ts` — the two literals this PR deliberately leaves visible. A census that fell here would mean the flags had been marked exempt, which would assert the code is fine. It is not fine; it is blocked, and those are different claims with different expiries. Co-authored-by: Claude Opus 5 (1M context) --- .../__tests__/eval-signal-collector.test.ts | 42 +++++++++++++++++++ packages/core/src/eval-signal-collector.ts | 27 +++++++++++- packages/engine/src/evaluator.ts | 12 +++++- packages/engine/src/scheduler.ts | 20 +++++++++ 4 files changed, 98 insertions(+), 3 deletions(-) diff --git a/packages/core/src/__tests__/eval-signal-collector.test.ts b/packages/core/src/__tests__/eval-signal-collector.test.ts index 93521817b3..312b841b61 100644 --- a/packages/core/src/__tests__/eval-signal-collector.test.ts +++ b/packages/core/src/__tests__/eval-signal-collector.test.ts @@ -55,4 +55,46 @@ describe("collectDeterministicSignals", () => { expect(signals.commitSummary.commitCount).toBe(0); expect(signals.logSummary).toEqual({ errorCount: 0, warningCount: 0, timingEntries: 0 }); }); + + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:58: + `column` is a two-value eval-record field, and against the `archived` literal a card resting in a + RENAMED archive lane was recorded as `"done"`. + + Not a crash and not a lifecycle decision — a mislabelled row in the eval corpus, which is a dataset + every later comparison reads. That is the expensive kind of quiet: nothing fails, the numbers just + drift. + + Only the ARCHIVED arm was ever wrong. A renamed COMPLETE lane was, and remains, recorded as + `"done"`, which is correct — so only the archived answer is resolved, and the third case below + pins that the widening did not turn every renamed lane into `"archived"`. + */ + it("records a RENAMED archive lane as archived", () => { + const task = makeTask({ column: "filed", log: [] }); + const signals = collectDeterministicSignals( + task, + { runId: "ER-3", startedAt: "2026-05-02T00:00:00.000Z" }, + { archivedColumns: new Set(["archived", "filed"]) }, + ); + expect(signals.column).toBe("archived"); + }); + + it("still records the legacy id as archived when no resolved answer is supplied", () => { + /* CONTROL: the parameter is optional, so an unwired caller must behave exactly as before. */ + const task = makeTask({ column: "archived", log: [] }); + const signals = collectDeterministicSignals(task, { runId: "ER-4", startedAt: "2026-05-02T00:00:00.000Z" }); + expect(signals.column).toBe("archived"); + }); + + it("records a renamed COMPLETE lane as done, not archived", () => { + /* The paired negative: the resolved set names the archive lanes only. A card in the board's + completion lane is `done`, which is what it always was and must stay. */ + const task = makeTask({ column: "shipped", log: [] }); + const signals = collectDeterministicSignals( + task, + { runId: "ER-5", startedAt: "2026-05-02T00:00:00.000Z" }, + { archivedColumns: new Set(["archived", "filed"]) }, + ); + expect(signals.column).toBe("done"); + }); }); diff --git a/packages/core/src/eval-signal-collector.ts b/packages/core/src/eval-signal-collector.ts index c5c766a15c..1c3034c75b 100644 --- a/packages/core/src/eval-signal-collector.ts +++ b/packages/core/src/eval-signal-collector.ts @@ -69,7 +69,28 @@ function collectCommitSummary(task: TaskDetail): DeterministicSignals["commitSum }; } -export function collectDeterministicSignals(task: TaskDetail, _run: EvalRunContext): DeterministicSignals { +/* +FNXC:WorkflowResolvedColumns 2026-07-31-23:55: +`archivedColumns` is an optional RESOLVED answer supplied by the caller; omitted, the `archived` +literal answers exactly as before. + +This collector is SYNC and pure — no store, no workflow — so the lane answer has to arrive as a +parameter. `HybridEvaluatorService.evaluateTask` is async and already holds an optional store, which +is where the resolution is paid. + +WHAT THE LITERAL COST. `column` is a two-value eval-record field, so a renamed ARCHIVE lane was +recorded as `"done"`. Not a crash and not a lifecycle decision — a mislabelled row in the eval +corpus, which is a dataset every later comparison reads. Wrong labels in evaluation data are quiet +in exactly the way that makes them expensive: nothing fails, the numbers just drift. + +A renamed COMPLETE lane is unaffected either way — it was, and remains, `"done"`, which is correct. +Only the archived arm was ever wrong, so only it is resolved. +*/ +export function collectDeterministicSignals( + task: TaskDetail, + _run: EvalRunContext, + options?: { archivedColumns?: ReadonlySet }, +): DeterministicSignals { const workflowSummary = countWorkflow(task.workflowStepResults); const logSummaryWithEvidence = summarizeLogs(task.log ?? []); const commitSummary = collectCommitSummary(task); @@ -110,7 +131,9 @@ export function collectDeterministicSignals(task: TaskDetail, _run: EvalRunConte return { taskId: task.id, - column: task.column === "archived" ? "archived" : "done", + column: (options?.archivedColumns ? options.archivedColumns.has(task.column) : task.column === "archived") + ? "archived" + : "done", executionStartedAt: task.executionStartedAt, executionCompletedAt: task.executionCompletedAt, timedExecutionMs: task.timedExecutionMs, diff --git a/packages/engine/src/evaluator.ts b/packages/engine/src/evaluator.ts index 7acbb1687d..c05a1f157f 100644 --- a/packages/engine/src/evaluator.ts +++ b/packages/engine/src/evaluator.ts @@ -4,6 +4,7 @@ import { normalizeCategoryScore, resolveScoreBand, resolveValidatorSettingsModel, + resolveProjectColumnsForRoles, EVAL_SCORE_CATEGORIES, type DeterministicSignals, type EvalScoreCategory, @@ -72,7 +73,16 @@ export class HybridEvaluatorService { settings: Partial, modelOverride?: EvaluatorModelOverride, ): Promise> { - const deterministicSignals = collectDeterministicSignals(task, run); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:55: + Resolve the archive lanes here, where an await is legal, and hand them to the sync collector. The + store is OPTIONAL on this service, so a store-less evaluator degrades to the legacy `archived` + literal — the collector's documented default — rather than failing. + */ + const archivedColumns = this.deps.store + ? await resolveProjectColumnsForRoles(this.deps.store, ["archived"]).catch(() => undefined) + : undefined; + const deterministicSignals = collectDeterministicSignals(task, run, { archivedColumns }); const model = resolveEvaluatorModel(settings, modelOverride); const evidenceBundle = this.deps.store ? await (this.deps.collectEvidence ?? collectTaskEvaluationEvidence)({ diff --git a/packages/engine/src/scheduler.ts b/packages/engine/src/scheduler.ts index b767593dd4..ec823f3570 100644 --- a/packages/engine/src/scheduler.ts +++ b/packages/engine/src/scheduler.ts @@ -1128,6 +1128,24 @@ export class Scheduler { this.lastAutoClaimFingerprint.set(task.id, nextFingerprint); this.options.snapshotManager?.invalidate("task:updated"); } + /* + FNXC:WorkflowResolvedColumns 2026-07-31-23:58 (FLAGGED AND LEFT COUNTED — do NOT convert with + `resolveTaskParkedColumnsSync`): + This literal and the `in-review` one further down are the two the sync-lane pass did not take, + and nothing in this file said why. Converting them the way the other ten were converted would + make them INERT, not fixed: `getTaskWorkflowSelectionImpl` returns `undefined` unconditionally + under PostgreSQL, so `resolveTaskWorkflowIrSync` always answers with the DEFAULT builtin IR and + every lane it yields is the legacy id (proved in `postgres/sync-workflow-ir-is-always-default.pg.test.ts`; + `check-inert-sync-lane-conversions` baselines the twenty guards already in that state here). + + They stay literal and COUNTED, which is the honest state: an unconverted literal is at least + visible to the census, while an inert conversion leaves the backlog and takes the evidence with + it. Both live in a synchronous `task:updated` listener, so the async resolver is unavailable + without reordering this handler against every other subscriber. + + Unblocking needs a sync-capable workflow-selection reader — one change that un-inerts every + sync-path conversion in this file at once. + */ // Track mission failure signals before moveTask clears failure metadata. if (task.sliceId && task.status === "failed") { if (task.column === "in-progress") this.failedTaskIds.add(task.id); @@ -1204,6 +1222,8 @@ export class Scheduler { } if (!this.options.prMonitor) return; + /* FNXC:WorkflowResolvedColumns 2026-07-31-23:58: the second of the two honest literals — see + the note on the mission-failure guard above for why converting it here would be inert. */ if (task.column !== "in-review") return; if (!task.prInfo) return;