fix(core): a renamed archive lane was recorded as done in the eval corpus; flag the scheduler's two honest literals (#3100)
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) <noreply@anthropic.com>
This commit is contained in:
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
@@ -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<string> },
|
||||
): 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,
|
||||
|
||||
@@ -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<Settings>,
|
||||
modelOverride?: EvaluatorModelOverride,
|
||||
): Promise<Omit<EvalTaskResultCreateInput, "taskId" | "taskSnapshot">> {
|
||||
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)({
|
||||
|
||||
@@ -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;
|
||||
|
||||
|
||||
Reference in New Issue
Block a user