test(engine): pin the evaluator's archived-lane read — the service had no test at all (#3224)

## What Pins the **evaluator's archived-lane read**. Test-only — no product change. `HybridEvaluatorService.evaluateTask` resolves the board's archived lanes and hands them to `collectDeterministicSignals`, which decides which of a task's related rows count as archived when scoring a run. **The service had no test anywhere in the repo.** Four test files import the module; none construct or exercise it. So this conversion was unobservable for the simplest possible reason — *nothing ran the code*. That is a different failure from the ones this audit has been finding (harnesses that run the code but cannot see the difference), and worth distinguishing: no amount of fixture care helps when the entry point is never called. ## Measured | | default (control) | renamed | differential | |---|---|---|---| | converted | pass | pass | pass | | blinded to `["archived"]` | pass | **FAIL** | **FAIL** | ``` converted: Test Files 1 passed (1) / Tests 3 passed (3) blinded: Test Files 1 failed (1) / Tests 2 failed | 1 passed (3) the 4 files importing evaluator.ts, plus this one: 5 files/76 tests, all green lint clean; fnxc-future-dates: none added; census unchanged ``` Per the rule I documented in #3223, the blind was confirmed applied with `git diff --stat` **before** the run rather than trusting the tool's exit code. ## What breaks without it On a board whose archived lane is `vaulted`, the evaluator hands the collector the legacy `{archived}` set. Rows resting in `vaulted` are not recognised as archived, and the deterministic half of every evaluation score is computed from a wrong picture of the task's history. **Nothing errors, the run completes, the number is just wrong** — which is why it survived unnoticed. ## Pinned without faking a provider response The assertion is about what the collector *receives*, which is decided before any model call. `collectDeterministicSignals` is mocked to record its arguments and throw a sentinel; the test asserts the resolved lane set and stops. This is deliberate over the obvious alternative of feeding `runPrompt` a canned AI payload: `deps.runPrompt` is injectable so either approach is offline, but a canned payload has to satisfy `parseAiResponse` and every `EVAL_SCORE_CATEGORIES` entry, and would silently rot into a maintenance burden on a test whose subject is one `Set`. Reversible if someone later wants full end-to-end evaluator coverage — that is a different test, not this one. ## Completes the engine audit With this, every `resolveProjectColumnsForRoles` call site in `packages/engine` has been blinded: | file | resolvers | result | |---|---|---| | `self-healing.ts` | 64 | 21 pinned, 1 recorded inert by construction, remainder mapped | | `executor.ts` | 2 | both already covered | | `scheduler.ts` | 1 | uncovered → pinned (#3219, merged) | | `triage.ts` | 1 | uncovered → pinned (#3221) | | `restart-recovery-coordinator.ts` | 1 | already covered | | `notification-service.ts` | 1 | already covered | | `evaluator.ts` | 1 | uncovered → pinned (this PR) | `project-engine.ts:5154` takes `roles` as a **parameter**, so it is a generic wrapper with no fixed role set to blind — flagged rather than guessed at; its callers are where the question belongs. **`packages/core`'s 17 files remain entirely unaudited** and I am claiming nothing about them.  ## Summary by CodeRabbit * **Tests** * Added regression coverage to verify reliable resolution of archived workflow lanes. * Covered both the default archived-lane name and custom renamed configurations. * Confirmed compatibility with legacy archived-lane naming behavior.  --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 12:20:08 -07:00
parent 76c73238a0
commit 12c4ab5a6e
1 changed files with 137 additions and 0 deletions
--- a/packages/engine/src/tests/evaluator-archived-lanes-renamed.test.ts
+++ b/packages/engine/src/tests/evaluator-archived-lanes-renamed.test.ts
@@ -0,0 +1,137 @@
+import { describe, expect, it, vi } from "vitest";
+import type { Settings, Task, TaskStore, WorkflowIr } from "@fusion/core";
+
+/*
+FNXC:WorkflowResolvedColumns 2026-07-31-19:45:
+THE EVALUATOR'S ARCHIVED-LANE READ, on a RENAMED board.
+
+`HybridEvaluatorService.evaluateTask` resolves the board's archived lanes and hands them to
+`collectDeterministicSignals`, which uses them to decide which of a task's related rows count as
+archived when scoring a run.
+
+WHY THIS FILE EXISTS. `HybridEvaluatorService` had NO test anywhere in the repo — the module was
+imported by four test files, none of which construct or exercise it. So the archived-lane conversion
+was unobservable for the simplest possible reason: nothing ran the code.
+
+HOW IT IS PINNED WITHOUT FAKING AN AI RESPONSE. The assertion is about what the collector RECEIVES,
+which is decided before any model call. `collectDeterministicSignals` is mocked to record its
+arguments and then throw a sentinel, so the test asserts the resolved lane set and stops — no canned
+provider payload to drift out of sync with `parseAiResponse`, and no network. `runPrompt` is
+injectable via deps, so this would be offline either way; the sentinel keeps the test about ONE
+thing.
+
+WHAT BREAKS WITHOUT THE CONVERSION. On a board whose archived lane is `vaulted`, the evaluator hands
+the collector the legacy `{archived}` set. Rows resting in `vaulted` are then not recognised as
+archived, and the deterministic half of every evaluation score is computed from a wrong picture of
+the task's history. It is a silent scoring defect: nothing errors, the run completes, the number is
+just wrong.
+
+DIFFERENTIAL. Both vocabularies run the same workflow SHAPE with identical traits; only the ids
+differ. The default-vocabulary run is the control — it passes with or without the conversion.
+*/
+
+const STOP = "STOP_AFTER_SIGNALS";
+const collectSpy = vi.fn(() => {
+  throw new Error(STOP);
+});
+
+vi.mock("@fusion/core", async (importOriginal) => {
+  const actual = await importOriginal<typeof import("@fusion/core")>();
+  return { ...actual, collectDeterministicSignals: collectSpy };
+});
+
+const { HybridEvaluatorService } = await import("../evaluator.js");
+
+const WF = "custom:renamed-archive";
+
+function ir(archivedColumn: string): WorkflowIr {
+  return {
+    version: "v2",
+    id: WF,
+    nodes: [],
+    edges: [],
+    columns: [
+      { id: "todo", label: "Hold", traits: [{ trait: "hold", config: { release: "capacity" } }] },
+      { id: "done", label: "Complete", traits: [{ trait: "complete" }] },
+      { id: archivedColumn, label: "Archived", traits: [{ trait: "archived" }] },
+    ],
+  } as unknown as WorkflowIr;
+}
+
+function createStore(archivedColumn: string): TaskStore {
+  const selection = { workflowId: WF, stepIds: [] };
+  return {
+    listTasks: vi.fn(async () => []),
+    getSettings: vi.fn(async () => ({} as Settings)),
+    getTask: vi.fn(async () => null),
+    logEntry: vi.fn(async () => undefined),
+    getRootDir: vi.fn(() => "/tmp/project"),
+    getTasksDir: vi.fn(() => "/tmp/project/.fusion/tasks"),
+    on: vi.fn(),
+    off: vi.fn(),
+    getTaskWorkflowSelection: vi.fn(() => selection),
+    getTaskWorkflowSelectionAsync: vi.fn(async () => selection),
+    getWorkflowDefinition: vi.fn(async () => ({ ir: ir(archivedColumn) })),
+    /* FNXC:WorkflowResolvedColumns 2026-07-31-23:30: without this the resolver hands back legacy ids
+       only, so the conversion under test cannot be observed and the suite passes on the unconverted
+       code — the blinding failure this file guards. */
+    listWorkflowDefinitions: vi.fn(async () => [{ ir: ir(archivedColumn) }]),
+  } as unknown as TaskStore;
+}
+
+/** Returns the archived lane set the evaluator actually handed to the collector. */
+async function archivedLanesHandedToCollector(archivedColumn: string): Promise<string[]> {
+  collectSpy.mockClear();
+  const service = new HybridEvaluatorService({
+    cwd: "/tmp/project",
+    store: createStore(archivedColumn),
+    runPrompt: async () => "{}",
+  });
+
+  const task = { id: "FN-1", title: "t", description: "", column: "done", steps: [], log: [] } as unknown as Task;
+  await expect(
+    service.evaluateTask(task as never, { runId: "run-1" } as never, {} as Settings),
+  ).rejects.toThrow(STOP);
+
+  expect(collectSpy).toHaveBeenCalledTimes(1);
+  const options = collectSpy.mock.calls[0]![2] as { archivedColumns?: ReadonlySet<string> } | undefined;
+  return [...(options?.archivedColumns ?? [])].sort();
+}
+
+describe("evaluator resolves the board's own archived lanes", () => {
+  it("default vocabulary: hands the collector the archived lane", async () => {
+    expect(await archivedLanesHandedToCollector("archived")).toContain("archived");
+  });
+
+  it("renamed vocabulary: hands the collector the RENAMED archived lane", async () => {
+    expect(await archivedLanesHandedToCollector("vaulted")).toContain("vaulted");
+  });
+
+  it("both vocabularies resolve their OWN lane — no column-id literal survives on this path", async () => {
+    const renamed = await archivedLanesHandedToCollector("vaulted");
+    const legacy = await archivedLanesHandedToCollector("archived");
+    /*
+    FNXC:WorkflowResolvedColumns 2026-07-31-23:30 (#3224 review — "reject legacy archived identifiers"):
+    MEASURED, AND THE LEGACY ID IS SUPPOSED TO BE THERE.
+
+    The review asked for an exact single-column set. Asserting that fails: the renamed board resolves
+    to `["archived", "vaulted"]`. That is `resolveProjectColumnsForRoles`' documented legacy FLOOR —
+    it unions `LEGACY_COLUMN_IDS_BY_ROLE` in so a row whose workflow cannot be resolved still
+    classifies, and the helper's contract says the result is "never empty: the legacy ids are"
+    included. Pinning an exact set here would encode the opposite of the design and fail the moment
+    anyone read the helper's own docstring.
+
+    The review's underlying worry is real but belongs to a different layer: a project that renames its
+    archive lane AND has an unrelated column literally named `archived` would over-match. That is the
+    known cost of the union, recorded in project-union-versus-per-task-lanes.md, and the answer there
+    is per-task resolution at sites where over-inclusion is unsafe — not a narrower set here.
+
+    So: assert the renamed lane IS resolved (the conversion works) and that the two boards differ,
+    and pin the floor explicitly so its presence is documented rather than incidental.
+    */
+    expect(renamed).toContain("vaulted");
+    expect(legacy).not.toContain("vaulted");
+    /* The legacy floor, asserted rather than tolerated. */
+    expect(renamed).toContain("archived");
+  });
+});