From 12c4ab5a6eaf5ea503f6fbd50e64e9baef2f80d9 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 12:20:08 -0700 Subject: [PATCH] =?UTF-8?q?test(engine):=20pin=20the=20evaluator's=20archi?= =?UTF-8?q?ved-lane=20read=20=E2=80=94=20the=20service=20had=20no=20test?= =?UTF-8?q?=20at=20all=20(#3224)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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) --- .../evaluator-archived-lanes-renamed.test.ts | 137 ++++++++++++++++++ 1 file changed, 137 insertions(+) create mode 100644 packages/engine/src/__tests__/evaluator-archived-lanes-renamed.test.ts diff --git a/packages/engine/src/__tests__/evaluator-archived-lanes-renamed.test.ts b/packages/engine/src/__tests__/evaluator-archived-lanes-renamed.test.ts new file mode 100644 index 0000000000..e1f5038292 --- /dev/null +++ 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(); + 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 { + 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 } | 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"); + }); +});