From 4afb32ef98c13e192e52346351e5bd585697f911 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Fri, 31 Jul 2026 04:46:58 -0700 Subject: [PATCH] test(core): cover the untested log-entry archive gate; correct a deferral that named the wrong blocker (#3110) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit I converted `audit-ops.ts`'s archived gate, measured, and **backed it out**. Both halves of that are the deliverable. ## The old deferral was stale on its own terms It declined the conversion because *"the fix is the same one `getLiveTaskColumn` needs"* and doing one of the pair would leave them disagreeing. But `getLiveTaskColumn` now **takes** a resolved `archivedColumns` set, and both of its callers already pass `await resolveArchivedLanes(store)` — including the sentinel path **twenty lines up in this same function**. The pair it worried about was already half-converted, and this arm was the half out of step. Converting it would have made them *agree*. That is the third deferral I have found this session whose stated blocker had dissolved. A deferral note records the blocker at the moment it was written, and nothing re-checks it. ## The real blocker is one neither note named `archived-column-gate-parity.test.ts` failed my conversion, and its reasoning is correct and not obvious. This gate has **three encodings**: 1. TypeScript comparisons 2. Drizzle `eq`/`ne` predicates 3. raw SQL templates Converting only the TypeScript arm makes them **diverge**: the gate would call the row archived while the SQL side still returns it as live — a log write rejected by its gate while its parent is listed as live. Every builtin workflow names the column `archived`, so all three agree *by accident* on every board we ship, and nothing except that parity test can see the split. Unblocking means converting all three together — the SQL sides need the resolved id as a query-build value, including inside `for update` transactions that receive no store today — or declaring `archived` a non-renameable system column. That test lays out both options and owns the inventory that has to move in the same commit. I am not doing it here; it is a different change from a lane conversion. ## What ships **The corrected note**, and **a test for a gate that had no coverage in any form**. The test asserts the legacy refusal and — the case that matters more — that a **live lane is not refused**. A gate that refused everything would satisfy a one-sided test and silently break every log write on the board. The renamed case is recorded as a **deliberate, explained omission** rather than left as a silent hole, so the next reader knows it is a decision. ## Measured - 3 new cases pass; the parity gate passes. - **MUTATION**, on the conversion before I reverted it: restoring the literal failed the renamed case. The conversion *worked* — which is exactly why the parity gate mattered. A working change can still be the wrong change. - The live-lane negative asserts **the gate did not fire**, not that the call succeeded: past the gate the fast path performs a real Drizzle write this fake layer cannot serve, so asserting success would drag a database fixture into a test about a lane comparison, and asserting a bare rejection would pass even if the gate *had* fired. - `src/__tests__/{log-entry,archive,cold-storage,unarchive}*` — **7 files / 23 tests pass**. - `tsc --noEmit -p packages/core` clean; census `--strict`, `check-fnxc-future-dates` clean. ## Census **No movement — nothing converted, deliberately.** The count stays where it is because the gate is blocked, not because it is fine. Co-authored-by: Claude Opus 5 (1M context) --- .../log-entry-archived-lane-gate.test.ts | 98 +++++++++++++++++++ packages/core/src/task-store/audit-ops.ts | 23 +++++ 2 files changed, 121 insertions(+) create mode 100644 packages/core/src/__tests__/log-entry-archived-lane-gate.test.ts diff --git a/packages/core/src/__tests__/log-entry-archived-lane-gate.test.ts b/packages/core/src/__tests__/log-entry-archived-lane-gate.test.ts new file mode 100644 index 0000000000..8d3166a85c --- /dev/null +++ b/packages/core/src/__tests__/log-entry-archived-lane-gate.test.ts @@ -0,0 +1,98 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-31-23:20: +THE "ARCHIVED TASKS ARE LOG-READ-ONLY" GATE HAD NO TEST AT ALL — this adds one, and records why it +deliberately does NOT assert the renamed-lane case. + +`logEntryImpl`'s fast path refuses a log write when the parent task is archived, asking with the +`archived` literal. On a renamed board a card the operator filed away keeps accepting log writes. +The gap is narrow rather than absent because `deletedAt` covers the soft-delete half and that is the +common path — which is also why it stayed invisible. + +I CONVERTED IT, AND BACKED THE CONVERSION OUT. `archived-column-gate-parity.test.ts` caught it, and +its reasoning is correct and not obvious: this gate has THREE encodings — TypeScript comparisons, +Drizzle `eq`/`ne` predicates, and raw SQL templates — and converting only the TypeScript arm makes +them DIVERGE. TS would call the row archived while the SQL side still returns it as live: a log write +rejected by its gate while its parent is listed as live. Every builtin workflow names the column +`archived`, so all three agree by accident on every board we ship and nothing else can see the split. + +So the renamed case stays uncovered ON PURPOSE, and the file says so rather than quietly omitting it. +Unblocking it means converting all three encodings together (the SQL sides need the resolved id as a +query-build value, including inside `for update` transactions that receive no store today), or +declaring `archived` a non-renameable system column — the choice that parity test lays out. + +What IS asserted is the gate's existing behaviour, which had no coverage at all: it refuses the +legacy archived id, and — the case that matters more — it does NOT refuse a live lane. A gate that +refused everything would have satisfied a one-sided test and broken every log write on the board. + +`readTaskRow` is mocked because the subject is the gate, not the row read. +*/ + +import { describe, expect, it, vi } from "vitest"; +import type { TaskStore } from "../store.js"; + +const { readTaskRowMock } = vi.hoisted(() => ({ readTaskRowMock: vi.fn() })); +vi.mock("../task-store/async-persistence.js", async (importOriginal) => ({ + ...(await importOriginal>()), + readTaskRow: readTaskRowMock, +})); + +const { logEntryImpl } = await import("../task-store/audit-ops.js"); + +/** Archive lane is `filed`; the board declares no column called `archived`. */ +const RENAMED_IR = { + version: "v2", id: "wf-renamed", name: "renamed", nodes: [], edges: [], + columns: [ + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + { id: "filed", name: "Filed", traits: [{ trait: "archived" }] }, + ], +}; + +function storeWith(ir: unknown): TaskStore { + return { + withTaskLock: vi.fn(async (_id: string, fn: () => Promise) => fn()), + listWorkflowDefinitions: vi.fn(async () => [{ ir }]), + asyncLayer: { db: {}, projectId: "p1" }, + isWatching: false, + taskCache: new Map(), + emit: vi.fn(), + /* Reached only on the NON-archived path; enough for the write to complete. */ + updateTaskLogFields: vi.fn(async () => undefined), + } as unknown as TaskStore; +} + +function row(column: string) { + return { id: "KB-1", column, deletedAt: null, log: [] }; +} + +describe("the log-entry archive gate", () => { + /* + DELIBERATELY ABSENT: "refuses a log write to a card in a RENAMED archive lane". That is the + behaviour this gate SHOULD have and does not, and it cannot be fixed in the TypeScript arm alone — + see the header. Written down rather than left as a silent hole, so the next reader knows the + omission is a decision and not an oversight. + */ + it("refuses a log write to the legacy `archived` column", async () => { + readTaskRowMock.mockResolvedValue(row("archived")); + + await expect(logEntryImpl(storeWith(RENAMED_IR), "KB-1", "did a thing")) + .rejects.toThrow(/archived — logging is read-only/); + }); + + /* + The paired negative, and the one that matters most: a gate that refused everything would satisfy + both cases above and silently break every log write on the board. + */ + it("does NOT refuse a log write to a live lane", async () => { + readTaskRowMock.mockResolvedValue(row("building")); + + /* + Asserted as "the GATE did not fire", not as "the call succeeded". Past the gate the fast path + performs a real Drizzle write, which this fake layer cannot serve — so the call still rejects, + with an unrelated error. Asserting success would drag a database fixture into a test about a lane + comparison, and asserting a bare rejection would pass even if the gate HAD fired. + */ + const error = await logEntryImpl(storeWith(RENAMED_IR), "KB-1", "did a thing").catch((err: unknown) => err); + expect(String(error)).not.toMatch(/logging is read-only/); + }); +}); diff --git a/packages/core/src/task-store/audit-ops.ts b/packages/core/src/task-store/audit-ops.ts index 32cc3d4d1f..51918b77ef 100644 --- a/packages/core/src/task-store/audit-ops.ts +++ b/packages/core/src/task-store/audit-ops.ts @@ -204,6 +204,29 @@ export async function logEntryImpl(store: TaskStore, id: string, action: string, archived-lane set threaded into a low-level, project-scoped read — and doing it in one of the two places would leave the pair disagreeing about what "archived" means. Recorded so the census keeps pointing at it with the reason attached. + + FNXC:WorkflowResolvedColumns 2026-07-31-23:25 (THE STATED BLOCKER IS STALE, AND THE REAL ONE IS + BIGGER — I converted this, measured, and backed it out): + The note above is out of date on its own terms: `getLiveTaskColumn` now TAKES a resolved + `archivedColumns` set and both callers already pass `await resolveArchivedLanes(store)` — the + sentinel path twenty lines up in this same function is one of them. So the pair it worries about + is already half-converted, and this arm is the half that is out of step. + + The REAL blocker is one neither note named. `archived-column-gate-parity.test.ts` failed my + conversion and is right: this gate has THREE encodings — TypeScript comparisons, Drizzle + `eq`/`ne` predicates, and raw SQL templates — and converting only the TypeScript arm makes them + DIVERGE. This gate would call the row archived while the SQL side still returns it as live: a + log write rejected by its gate while its parent is listed as live. Every builtin workflow names + the column `archived`, so all three agree by accident on every board we ship and nothing else + can see the split. + + Unblocking means converting all three encodings together — the SQL sides need the resolved id as + a query-build value, including inside `for update` transactions that receive no store today — or + declaring `archived` a non-renameable system column. That parity test lays out both options and + owns the inventory that has to move in the same commit. + + Behaviour here is otherwise now covered by `log-entry-archived-lane-gate.test.ts`, which had no + test at all before and which records the renamed case as a deliberate, explained omission. */ if (pgRow.column === "archived" || pgRow.deletedAt != null) { throw new Error(`Task ${id} is archived — logging is read-only`);