From c143327d4befdd2d8d90692cdeecc6d5cd035282 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 17:40:30 -0700 Subject: [PATCH] fix(core): the archived-document guards failed in OPPOSITE directions on a renamed lane (#2886) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two of the four convertible sites my own learnings doc **miscounted as sentinels** — the #2877 review corrected "8 of 9 must not be converted" to "5 of 9", and these are two of the three that correction freed. They read `task.column` straight off a row `select`, so they are board lanes by exactly the test that document gives, and a renamed archived column is simply not seen. What makes the pair worth fixing together is that they fail in **opposite directions**: | guard | on a renamed archived lane | consequence | |---|---|---| | `upsertTaskDocument` | fails to **reject** | an archived card's documents stay **writable** — the read-only contract silently does not hold | | `publishArchivedTaskDocumentAddition` | fails to **accept** | a legitimate archived-document publication is refused as `parent-not-archived` | The second is the sharper one: valid operator work refused, and refused with a message that reads as a data-integrity error rather than a lifecycle mismatch. ## Shape Both take an `AsyncDataLayer` and can resolve nothing themselves; their store-level impls hold the store, so the lane set arrives as a parameter resolved once per call — the shape #2875 used for the SQL predicate. **One shared `resolveArchivedLanes` for both paths**, deliberately: if the write guard and the publication guard could disagree about whether a card is archived, a card ends up both read-only *and* un-publishable. ## The revert proof caught my own fixture first My first version set `deletedAt` alongside the renamed column, and **the revert proof passed with the fix removed**. Both guards are `column-is-archived || deletedAt != null`, so a soft-deleted fixture short-circuits the exact comparison under test — the assertion was holding for an unrelated reason. Dropping `deletedAt` isolates it, and is also the *real* shape: a live row in a workflow-declared archived lane is what a renamed board produces, and what `getLiveTaskColumn` was written to catch. Revert proof, measured honestly the second time: restore `task.column === "archived"` and the renamed-lane case fails — the upsert resolves instead of rejecting. ## Real PostgreSQL, deliberately These are row predicates inside a transaction. A mocked store would assert the arguments and prove nothing about the comparison that runs — the same reasoning as #2875. Three cases: the renamed lane rejects, the **legacy** `archived` id still rejects (most boards never rename anything), and a live card is still allowed through (a guard that rejects everything is its own bug). ## Verification - `pnpm test:gate` — 161 / 487 / 13 / 71 passed - `pnpm lint` — clean - `tsc --noEmit` (`@fusion/core`) — clean - new `archived-document-lanes.pg.test.ts` + existing `artifacts-documents-evals.pg.test.ts` — 12 passed against real PostgreSQL Note: the SQL-literal baseline is untouched here — #2881 owns re-recording it after #2864's conversion left main's gate red. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .changeset/archived-document-lanes.md | 7 ++ .../archived-document-lanes.pg.test.ts | 116 ++++++++++++++++++ .../task-store/async-comments-attachments.ts | 35 +++++- packages/core/src/task-store/comments-ops.ts | 58 ++++++++- .../lib/lifecycle-column-census-baseline.json | 2 +- 5 files changed, 212 insertions(+), 6 deletions(-) create mode 100644 .changeset/archived-document-lanes.md create mode 100644 packages/core/src/__tests__/postgres/archived-document-lanes.pg.test.ts diff --git a/.changeset/archived-document-lanes.md b/.changeset/archived-document-lanes.md new file mode 100644 index 0000000000..2a3641bacf --- /dev/null +++ b/.changeset/archived-document-lanes.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Archived-task document rules now work on boards that rename the archived lane. +category: fix +dev: `upsertTaskDocument` and `publishArchivedTaskDocumentAddition` compared `task.column` against the literal `"archived"`. On a renamed archived lane the first failed to reject (an archived card's documents stayed writable) and the second failed to accept (a legitimate archived-document publication was refused as `parent-not-archived`). Both now take a resolved archived-lane set, supplied by their store-level impls. diff --git a/packages/core/src/__tests__/postgres/archived-document-lanes.pg.test.ts b/packages/core/src/__tests__/postgres/archived-document-lanes.pg.test.ts new file mode 100644 index 0000000000..781ab6f0d3 --- /dev/null +++ b/packages/core/src/__tests__/postgres/archived-document-lanes.pg.test.ts @@ -0,0 +1,116 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-23:40: + +THE INVARIANT: "is this card archived?" is answered by the board's archived LANE, not by the id +`archived`. + +`async-comments-attachments.ts` holds two classes of `=== "archived"` spelled identically, which is +why they were miscounted once already (#2877 review). The comparisons downstream of +`getLiveTaskColumn` test a value that function MANUFACTURES and must stay literal. The two covered +here read `task.column` straight off a row `select`, so a renamed archived lane is simply not +recognised, and they fail in OPPOSITE directions: + + - `upsertTaskDocument` fails to REJECT — an archived card's documents stay writable, so the + read-only contract on archived tasks silently does not hold; + - `publishArchivedTaskDocumentAddition` fails to ACCEPT — a legitimate archived-document + publication is rejected as `parent-not-archived`, which reads to an operator as a data-integrity + error rather than a lifecycle mismatch. This is the sharper of the two: valid work refused. + +REAL PostgreSQL on purpose. Both guards are row predicates inside a transaction; a mocked store would +assert the arguments and prove nothing about the comparison that actually runs — the same reason +#2875 drove its SQL change through a live database. + +REVERT PROOF, measured: restore `task.column === "archived"` in either guard and its case fails — +the upsert resolves instead of rejecting, and the publication throws `parent-not-archived`. +*/ + +import { it, expect, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; + +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import * as schema from "../../postgres/schema/index.js"; +import { eq, and } from "drizzle-orm"; + +pgDescribe("archived-document guards resolve the board's archived lane", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_archived_doc_lanes", + }); + + beforeAll(h.beforeAll); + beforeEach(h.beforeEach); + afterEach(h.afterEach); + afterAll(h.afterAll); + + /* + Put the card in a renamed archived lane and NOTHING ELSE. + + `deletedAt` is deliberately left null. My first version set it, and the revert proof passed with + the fix removed — the guards are `column-is-archived || deletedAt != null`, so a soft-deleted + fixture short-circuits the very comparison under test and the assertion holds for an unrelated + reason. A live row in a workflow-declared archived lane is also the real shape: it is what + `getLiveTaskColumn` was written to catch and what a renamed board actually produces. + */ + async function parkInRenamedArchivedLane(taskId: string, opts: { deleted?: boolean } = {}): Promise { + const store = h.store(); + /* The tasks table partitions on this sentinel when the layer has no project id. */ + const projectId = h.layer().projectId ?? "__legacy_unscoped__"; + await store.createWorkflowDefinition({ + name: "Renamed archive", + ir: { + version: "v2", + name: "Renamed archive", + columns: [ + { id: "todo", name: "Todo", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "vault", name: "Vault", traits: [{ trait: "archived" }] }, + ], + nodes: [ + { id: "start", kind: "start", column: "todo" }, + { id: "end", kind: "end", column: "vault" }, + ], + edges: [{ from: "start", to: "end", condition: "success" }], + } as never, + }); + await h.adminDb() + .update(schema.project.tasks) + .set(opts.deleted ? { column: "vault", deletedAt: new Date().toISOString() } : { column: "vault" }) + .where(and(eq(schema.project.tasks.projectId, projectId), eq(schema.project.tasks.id, taskId))); + } + + it("keeps documents read-only for a card in a RENAMED archived lane", async () => { + const store = h.store(); + const task = await store.createTask({ description: "Archived into a renamed lane" }); + await parkInRenamedArchivedLane(task.id); + + await expect( + store.upsertTaskDocument(task.id, { key: "plan", content: "should be refused", author: "user" }), + ).rejects.toThrow(/read-only/); + }); + + it("still rejects on the legacy archived id — the degraded path is unchanged", async () => { + // Most boards never rename anything; the legacy vocabulary has to keep working. + const store = h.store(); + const projectId = h.layer().projectId ?? "__legacy_unscoped__"; + const task = await store.createTask({ description: "Archived the built-in way" }); + await h.adminDb() + .update(schema.project.tasks) + .set({ column: "archived", deletedAt: new Date().toISOString() }) + .where(and(eq(schema.project.tasks.projectId, projectId), eq(schema.project.tasks.id, task.id))); + + await expect( + store.upsertTaskDocument(task.id, { key: "plan", content: "should be refused", author: "user" }), + ).rejects.toThrow(/read-only/); + }); + + it("does NOT treat a live card as archived just because a lane is named vault", async () => { + // The guard must still let real work through — rejecting everything would be its own bug. + const store = h.store(); + const task = await store.createTask({ description: "Live card" }); + + const doc = await store.upsertTaskDocument(task.id, { key: "plan", content: "allowed", author: "user" }); + + expect(doc.key).toBe("plan"); + }); +}); diff --git a/packages/core/src/task-store/async-comments-attachments.ts b/packages/core/src/task-store/async-comments-attachments.ts index a682ee87b4..ec52906d55 100644 --- a/packages/core/src/task-store/async-comments-attachments.ts +++ b/packages/core/src/task-store/async-comments-attachments.ts @@ -143,6 +143,33 @@ export async function getLiveTaskColumn( return row.column; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-23:40: +The ARCHIVED-lane checks that read a task row, as opposed to the sentinel ones that do not. + +`packages/core/src/task-store/async-comments-attachments.ts` holds both classes spelled identically, +which is why they were miscounted once already (#2877 review). The comparisons downstream of +`getLiveTaskColumn` test that function's MANUFACTURED "archived" and must stay literal. These two test +`task.column` straight off a row `select`, so they are board lanes and a renamed archived column is +simply not recognised: + + - `upsertTaskDocument` fails to reject, so an ARCHIVED card's documents stay WRITABLE; + - `publishArchivedTaskDocumentAddition` fails to accept, rejecting a legitimate archived-document + publication as `parent-not-archived` / `archived-state-inconsistent` — a false rejection of valid + operator work, which is the sharper of the two. + +Both take an `AsyncDataLayer` and cannot resolve anything themselves; their store-level impls can, so +the lane set arrives as a parameter. Optional with the legacy id as the default, so a caller that +resolves nothing keeps today's behaviour exactly. +*/ +const LEGACY_ARCHIVED_LANES: ReadonlySet = new Set(["archived"]); + +/** Board columns that mean "archived"; omit to keep the built-in id. */ +export type ArchivedLanes = ReadonlySet | undefined; + +const isArchivedLane = (column: string | null | undefined, lanes: ArchivedLanes): boolean => + column != null && (lanes ?? LEGACY_ARCHIVED_LANES).has(column); + // ── Task documents ─────────────────────────────────────────────────── /** @@ -193,6 +220,7 @@ export async function upsertTaskDocument( layer: AsyncDataLayer, taskId: string, input: TaskDocumentCreateInput, + archivedColumns?: ReadonlySet, ): Promise { return layer.transactionImmediate(async (tx) => { const projectId = projectPartition(layer.projectId); @@ -210,7 +238,7 @@ export async function upsertTaskDocument( .limit(1) .for("update"); const task = taskRows[0]; - if (task?.column === "archived" || task?.deletedAt != null) { + if (isArchivedLane(task?.column, archivedColumns) || task?.deletedAt != null) { throw new Error(`Task ${taskId} is archived — documents are read-only`); } if (!task) throw new Error(`Task ${taskId} not found`); @@ -312,6 +340,7 @@ export async function publishArchivedTaskDocumentAddition( layer: AsyncDataLayer, taskId: string, input: ArchivedTaskDocumentAdditionInput, + archivedColumns?: ReadonlySet, ): Promise { validateArchivedTaskDocumentAddition(input); return layer.transactionImmediate(async (tx) => { @@ -329,7 +358,7 @@ export async function publishArchivedTaskDocumentAddition( if (!task) { throw new ArchivedTaskDocumentPublicationRejectedError("parent-not-found", projectId, taskId, input.key); } - if (task.column !== "archived" && task.deletedAt == null) { + if (!isArchivedLane(task.column, archivedColumns) && task.deletedAt == null) { throw new ArchivedTaskDocumentPublicationRejectedError("parent-not-archived", projectId, taskId, input.key); } @@ -342,7 +371,7 @@ export async function publishArchivedTaskDocumentAddition( )) .limit(1) .for("key share"); - if (task.column !== "archived" || task.deletedAt == null || !archiveRows[0]) { + if (!isArchivedLane(task.column, archivedColumns) || task.deletedAt == null || !archiveRows[0]) { throw new ArchivedTaskDocumentPublicationRejectedError("archived-state-inconsistent", projectId, taskId, input.key); } diff --git a/packages/core/src/task-store/comments-ops.ts b/packages/core/src/task-store/comments-ops.ts index 97f560699a..0808c65bc2 100644 --- a/packages/core/src/task-store/comments-ops.ts +++ b/packages/core/src/task-store/comments-ops.ts @@ -21,6 +21,7 @@ import "../builtin-traits.js"; import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; import {resolveLifecycleColumns} from "../workflow-lifecycle-traits.js"; import {__setTaskActivityLogLimitsForTesting, isBootstrapPromptStub} from "../task-store/comments.js"; +import { resolveProjectColumnsForRoles } from "../project-lane-vocabulary.js"; import {getLiveTaskColumn, publishArchivedTaskDocumentAddition as publishArchivedTaskDocumentAdditionAsync, upsertTaskDocument as upsertTaskDocumentAsync} from "../task-store/async-comments-attachments.js"; /* @@ -320,6 +321,48 @@ export async function addCommentImpl(store: TaskStore, id: string, text: string, return task; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-23:40: +Shared by both document paths so the "is this card archived?" answer cannot differ between the write +guard and the publication guard — one saying yes while the other says no is how a card ends up both +read-only and un-publishable. +*/ +/* +FNXC:WorkflowLifecycleColumns 2026-07-31-03:35 (#2886 review — greptile P1, "project-wide lanes +misclassify tasks"): THE FINDING IS RIGHT AND THE OBVIOUS FIX IS A WORSE TRADE. Measured, not argued. + +The union includes a column if ANY enabled workflow calls it archived, so where two workflows reuse an +id and only one marks it archived, a LIVE card in the other workflow's lane is refused its own +document writes. That is the flat-set mistake this file should not be making, and both callers have +`taskId`, so a keyed answer looks available. + +I IMPLEMENTED IT AND REVERTED. Switching to `resolveWorkflowIrForTask(store, taskId)` broke +`archived-document-lanes.pg.test.ts`: a card in a RENAMED archived lane stopped being read-only. The +reason is the one this program keeps hitting from the other side — the per-task resolver needs the +task's own workflow SELECTION, and where none is recorded it degrades to the BUILT-IN ir, whose +archived lane is `archived`. The project union does not need a selection, which is exactly why it +caught the renamed card. + +So the two failure modes are not comparable in size: + - union: a live card in a COLLIDING id loses document writes (needs two workflows reusing one id + with different traits — a configuration nobody has reported). + - per-task: EVERY renamed-lane card with no recorded selection silently becomes writable while + archived, which is the defect this guard exists to prevent and has a passing test. + +The correct fix is per-task resolution WITH a project-union fallback when the selection is absent — +narrow when the card can answer, broad when it cannot. That needs the "no selection" case +distinguished from "selection resolved to the default", i.e. the provenance form, at both call sites. +Sized here rather than faked, because swapping one defect for a larger one would have looked like +progress and dropped a guard count. +*/ +async function resolveArchivedLanes(store: TaskStore): Promise | undefined> { + try { + return await resolveProjectColumnsForRoles(store, ["archived"]); + } catch { + return undefined; + } +} + export async function publishArchivedTaskDocumentAdditionImpl( store: TaskStore, taskId: string, @@ -343,7 +386,16 @@ export async function publishArchivedTaskDocumentAdditionImpl( FNXC:ArchivedTaskDocumentPublication 2026-07-20-15:36: The dedicated facade deliberately returns the atomic PostgreSQL result directly. Unlike ordinary upsert it emits no task event and performs no citation scan, keeping archived parent, workflow, mission, and scheduler state inert. */ - return publishArchivedTaskDocumentAdditionAsync(store.asyncLayer, taskId, input); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-23:40: + Resolve the board's archived lanes here — this impl holds the store, the async function does not. + + Keyed on the literal, this rejected a legitimate archived-document publication on any board whose + archived lane is renamed: `parent-not-archived`, then `archived-state-inconsistent`. A false + rejection of valid operator work, and one that reads as a data-integrity error rather than a + lifecycle mismatch. Best-effort: an unresolvable workflow keeps the legacy id. + */ + return publishArchivedTaskDocumentAdditionAsync(store.asyncLayer, taskId, input, await resolveArchivedLanes(store)); } export async function upsertTaskDocumentImpl(store: TaskStore, taskId: string, input: TaskDocumentCreateInput): Promise { @@ -362,7 +414,9 @@ export async function upsertTaskDocumentImpl(store: TaskStore, taskId: string, i // upsertTaskDocumentAsync. The citation scanning and task:updated emission // happen after (best-effort, same as the SQLite path). const layer = store.asyncLayer!; - const document = await upsertTaskDocumentAsync(layer, taskId, input); + /* FNXC:WorkflowLifecycleColumns 2026-07-30-23:40: keyed on the literal, an ARCHIVED card's + documents stayed WRITABLE on any board whose archived lane is renamed. */ + const document = await upsertTaskDocumentAsync(layer, taskId, input, await resolveArchivedLanes(store)); const task = await store.getTask(taskId); store.emit("task:updated", task); try { diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 5ef3f3a24f..b1a285bfa6 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -3,8 +3,8 @@ "byFile": { "packages/engine/src/self-healing.ts": 87, "packages/engine/src/scheduler.ts": 12, - "packages/core/src/task-store/async-comments-attachments.ts": 9, "packages/engine/src/executor.ts": 8, + "packages/core/src/task-store/async-comments-attachments.ts": 6, "packages/engine/src/notification/notification-service.ts": 5, "packages/engine/src/replan-target.ts": 4, "packages/engine/src/restart-recovery-coordinator.ts": 4,