From f53c9dbd398d641c21a2ab2c48b699f1a2eb858a Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 14:26:11 -0700 Subject: [PATCH] fix(core): merge re-enqueue threw on every board with a renamed review column (#2819) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single most consequential finding from the u12 seam-gate work, picked up because batch-core (#2783) merged without it and it is now unowned. ## The defect `enqueueMergeQueueInTransaction` gates on the task's column being a review column, and takes the board's review columns as an optional trailing argument. - `moves.ts:487` and `moves.ts:1153` — the automatic handoff-to-review path — resolve and pass them. - The public `enqueueMergeQueue` wrapper (`async-merge-coordination.ts:246`), reached through `store.enqueueMergeQueue`, **did not**, so it fell back to `new Set(["in-review"])`. This is not the quiet legacy-id degradation most of these seams produce. The reject branch records `mergeQueue:enqueue-rejected` and **throws** `MergeQueueInvalidColumnError`. Its production callers are `merger.ts:7251` and `self-healing.ts:10329` — so on any board whose review lane is renamed, the merge and recovery re-enqueue paths failed outright while the handoff path kept working. ## Measured, not asserted With the fix reverted, the renamed case fails with the exact predicted error and the controls stay green: ``` × renamed vocabulary: a task in the RENAMED review lane enqueues for merge MergeQueueInvalidColumnError: Task KB-001 is in column 'checking', not 'in-review'; cannot enqueue ✓ default vocabulary: a task in the review lane enqueues for merge ✓ renamed vocabulary: a task in the WIP lane is still REJECTED ✓ default vocabulary: a task in the WIP lane is still REJECTED Tests 1 failed | 3 passed (4) ``` With it: `Tests 4 passed (4)`. ## About the suite **Differential.** The fixture is the builtin coding workflow with only its column ids renamed, so the sole difference between the two runs is vocabulary — a hand-built graph would test the fixture's own transition table as much as the code. It asserts the rename actually landed (`checking` present, `in-review` absent), so a surviving literal cannot pass by luck, and it walks the graph rather than jumping, because moves are transition-validated. **Both negatives included.** A WIP-lane task must still be REJECTED under each vocabulary. Supplying the real columns must not degrade into "every column is a review column", which would let work merge straight out of the WIP lane — the failure mode a careless version of this fix would introduce. ## Why nothing caught it Partial supply. Two of three call sites passed the argument, so a check asking "does SOME caller supply this?" reported the seam as satisfied, and the lifecycle-column census counted the conversion as done. Closing that one-supplier floor in `scripts/check-inert-flag-seams.mjs` (on #2772) is what surfaced it. ## Verification - `pnpm test:gate` green - new suite 4/4; neighbouring merge-queue suites (`taskstore-lifecycle`, `store-in-review-stall`, `runtime-lifecycle-async`) 29/29 - `tsc -p packages/core` 0, lint 0 - changeset included (`@runfusion/fusion` patch) Note: #2772 still carries a TEMPORARY per-call-site exemption for this seam. Once this lands, that exemption's staleness check will fail and I will remove it there — it cannot outlive the fix. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../merge-queue-renamed-review-column.md | 7 + ...queue-lease-renamed-review-lane.pg.test.ts | 151 +++++++++++++ ...rge-queue-renamed-review-column.pg.test.ts | 141 ++++++++++++ ...ueue-traitless-and-unresolvable.pg.test.ts | 200 ++++++++++++++++++ packages/core/src/store.ts | 34 ++- .../task-store/async-merge-coordination.ts | 168 +++++++++++++-- .../core/src/task-store/task-artifacts-ops.ts | 28 ++- packages/core/src/types/merge-queue.ts | 11 + 8 files changed, 718 insertions(+), 22 deletions(-) create mode 100644 .changeset/merge-queue-renamed-review-column.md create mode 100644 packages/core/src/__tests__/postgres/merge-queue-lease-renamed-review-lane.pg.test.ts create mode 100644 packages/core/src/__tests__/postgres/merge-queue-renamed-review-column.pg.test.ts create mode 100644 packages/core/src/__tests__/postgres/merge-queue-traitless-and-unresolvable.pg.test.ts diff --git a/.changeset/merge-queue-renamed-review-column.md b/.changeset/merge-queue-renamed-review-column.md new file mode 100644 index 0000000000..e02f29e067 --- /dev/null +++ b/.changeset/merge-queue-renamed-review-column.md @@ -0,0 +1,7 @@ +--- +"@runfusion/fusion": patch +--- + +summary: Fix merge re-enqueue failing on boards whose review column is renamed. +category: fix +dev: `enqueueMergeQueue` (and `store.enqueueMergeQueue`) now resolve the task's own review columns and forward them to `enqueueMergeQueueInTransaction`, which previously fell back to `new Set(["in-review"])` and threw `MergeQueueInvalidColumnError`. The two `moves.ts` handoff callers already supplied them; the merger and self-healing re-enqueue paths did not. diff --git a/packages/core/src/__tests__/postgres/merge-queue-lease-renamed-review-lane.pg.test.ts b/packages/core/src/__tests__/postgres/merge-queue-lease-renamed-review-lane.pg.test.ts new file mode 100644 index 0000000000..7da55195f3 --- /dev/null +++ b/packages/core/src/__tests__/postgres/merge-queue-lease-renamed-review-lane.pg.test.ts @@ -0,0 +1,151 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-14:35 (#2819 review — greptile, "renamed entries remain unleaseable"): + +A QUEUED CARD ON A RENAMED BOARD MUST ACTUALLY BE LEASEABLE. + +The first pass on this PR taught only the STALE SWEEP to resolve review lanes, so the row survived +cleanup — and then both eligibility predicates in `acquireMergeQueueLease` still demanded the literal +`in-review`, matched nothing, and the row sat in the queue forever. That is a worse failure than the +one it replaced: a deleted row at least emits `mergeQueue:auto-cleanup-stale-row`, while an +unleaseable row is a silent permanent stall with a healthy-looking queue. + +WHY THE SWEEP FIX ALONE LOOKED GREEN. Its test asserted the row still EXISTS after acquisition. Row +survival and row leaseability are different questions, and the sweep fix answered only the first — +the acquire returning `null` was consistent with an empty queue and nothing distinguished them. So +these cases assert the LEASE, which is the outcome the merger actually depends on. + +BOTH ACQUIRE MODES ARE COVERED. Targeted acquire (`targetTaskId`) and queue-head acquire are separate +code paths with separate predicates; the merger uses targeted, self-healing recovery uses the head. +Converting one and leaving the other is the half-converted-pair shape this program keeps hitting. + +WHY A LIVE STORE. The predicate is SQL evaluated by PostgreSQL against a column id that comes from +the task's own persisted workflow. A mock cannot exercise it at all. + +LANE. `.pg.test.ts`, skipped by `pgDescribe` when PostgreSQL is unreachable; throwaway per-file +database; never port 4040. +*/ + +import { it, expect, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../../index.js"; + +pgDescribe("merge-queue lease acquisition under a renamed review lane", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_mq_lease_renamed", + }); + + beforeAll(h.beforeAll); + beforeEach(h.beforeEach); + afterEach(h.afterEach); + afterAll(h.afterAll); + + /** + * The BUILTIN coding workflow with its column ids renamed and nothing else changed, so the only + * difference between the control and the defect case is the vocabulary. `checking` collides with no + * legacy literal, so a surviving `"in-review"` cannot pass by luck. + */ + async function seedRenamedWorkflow(): Promise { + const RENAME: Record = { + todo: "drafting", + "in-progress": "building", + "in-review": "checking", + done: "shipped", + }; + const rename = (id: string | undefined) => (id && RENAME[id]) ?? id; + const ir = JSON.parse(JSON.stringify(BUILTIN_CODING_WORKFLOW_IR)) as { + id: string; + nodes?: { column?: string }[]; + columns?: { id: string }[]; + }; + ir.id = "custom:renamed-review-merge-queue-lease"; + for (const node of ir.nodes ?? []) node.column = rename(node.column); + for (const column of ir.columns ?? []) column.id = rename(column.id) as string; + + const ids = (ir.columns ?? []).map((column) => column.id); + expect(ids).toContain("checking"); + expect(ids).not.toContain("in-review"); + + const created = await h.store().createWorkflowDefinition({ + name: "Renamed Review (merge queue lease)", + kind: "workflow", + ir, + } as never); + return (created as { id: string }).id; + } + + /** A card walked into its board's review lane and enqueued for merge. */ + async function seedQueuedTask(path: readonly string[], workflowId?: string): Promise { + const store = h.store(); + const task = await store.createTask({ title: "awaiting merge", description: "test", column: "todo" }); + if (workflowId) await store.writeTaskWorkflowSelection(task.id, workflowId, []); + for (const step of path) await store.moveTask(task.id, step as never); + store.taskCache.delete(task.id); + + /* Prove the fixture: an unqueued card would fail to lease for a reason unrelated to vocabulary. */ + const entry = await store.enqueueMergeQueue(task.id); + expect(entry.taskId).toBe(task.id); + return task.id; + } + + /* Control. Passes before and after the fix, so a generally broken acquire cannot hide below. */ + it("default vocabulary: a queued card can be leased by the queue head", async () => { + const id = await seedQueuedTask(["in-progress", "in-review"]); + + const lease = await h.store().acquireMergeQueueLease("worker-1", { leaseDurationMs: 60_000 }); + + expect(lease?.taskId).toBe(id); + }); + + /* + The defect, queue-head mode. Before the fix the head SELECT joined on + `tasks.column = 'in-review'`, matched no row on this board, and returned null — the merger saw an + empty queue while the card sat in it. + */ + it("renamed vocabulary: a queued card can be leased by the QUEUE HEAD", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedQueuedTask(["drafting", "building", "checking"], wf); + + const lease = await h.store().acquireMergeQueueLease("worker-1", { leaseDurationMs: 60_000 }); + + expect(lease?.taskId).toBe(id); + }); + + /* + The defect, targeted mode — the path the merger itself uses. Separate predicate, separate failure: + the candidate SELECT found nothing, so this recorded `mergeQueue:lease-target-unavailable` and + returned null without ever attempting the update. + */ + it("renamed vocabulary: a queued card can be leased by TARGETED acquire", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedQueuedTask(["drafting", "building", "checking"], wf); + + const lease = await h + .store() + .acquireMergeQueueLease("worker-1", { leaseDurationMs: 60_000, targetTaskId: id }); + + expect(lease?.taskId).toBe(id); + }); + + /* + The paired negative, and the reason this is a resolved-lane fix rather than a deleted predicate. A + card that has LEFT review must not be leaseable — otherwise the merger would merge work that was + pulled back for rework. Widening to "any column is a review column" would pass every case above and + break exactly this one. + */ + it("renamed vocabulary: a card moved BACK out of the review lane is not leaseable", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedQueuedTask(["drafting", "building", "checking"], wf); + await h.store().moveTask(id, "building" as never); + h.store().taskCache.delete(id); + + const lease = await h + .store() + .acquireMergeQueueLease("worker-1", { leaseDurationMs: 60_000, targetTaskId: id }); + + expect(lease).toBeNull(); + }); +}); diff --git a/packages/core/src/__tests__/postgres/merge-queue-renamed-review-column.pg.test.ts b/packages/core/src/__tests__/postgres/merge-queue-renamed-review-column.pg.test.ts new file mode 100644 index 0000000000..5a1127a845 --- /dev/null +++ b/packages/core/src/__tests__/postgres/merge-queue-renamed-review-column.pg.test.ts @@ -0,0 +1,141 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-02:20 (merge re-enqueue was BROKEN on a renamed board): + +`enqueueMergeQueueInTransaction` gates on the task's column being a review column, and takes the +board's review columns as an optional trailing argument. The two `moves.ts` callers — the automatic +handoff-to-review path — resolved and passed them. The public `enqueueMergeQueue` wrapper reached +through `store.enqueueMergeQueue` did not, so it fell back to `new Set(["in-review"])`. + +The consequence is not the usual quiet legacy-id degradation. The guard's reject branch records +`mergeQueue:enqueue-rejected` and THROWS `MergeQueueInvalidColumnError`. So on any board whose review +lane is renamed, every re-enqueue through the store method failed outright — and its production +callers are `merger.ts` and `self-healing.ts`, i.e. the merge and recovery paths. + +WHY IT SURVIVED. Partial supply. Two of three call sites passed the argument, so a check asking +"does SOME caller supply this?" reported the seam as satisfied, and the census counted the +conversion. Nothing looked at whether EVERY caller supplied it. + +THE CASES ARE DIFFERENTIAL: the same enqueue against two vocabularies whose roles are identical and +only the ids differ. `checking` collides with no legacy literal, so a surviving `"in-review"` cannot +pass by luck. +*/ + +import { it, expect, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../../index.js"; + +pgDescribe("merge-queue enqueue under a renamed review column", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_mq_renamed_review", + }); + + beforeAll(h.beforeAll); + beforeEach(h.beforeEach); + afterEach(h.afterEach); + afterAll(h.afterAll); + + /** + * The BUILTIN coding workflow with its column ids renamed — nothing else changed. + * + * Deriving from the builtin rather than hand-rolling a graph keeps the ONLY difference between the + * two runs the vocabulary, which is the whole differential claim. A hand-built fixture ends up + * testing the fixture's transition table as much as the code. + */ + async function seedRenamedWorkflow(): Promise { + const RENAME: Record = { + todo: "drafting", + "in-progress": "building", + "in-review": "checking", + done: "shipped", + }; + const rename = (id: string | undefined) => (id && RENAME[id]) ?? id; + const ir = JSON.parse(JSON.stringify(BUILTIN_CODING_WORKFLOW_IR)) as { + id: string; + nodes?: { column?: string }[]; + columns?: { id: string }[]; + }; + ir.id = "custom:renamed-review-merge-queue"; + for (const node of ir.nodes ?? []) node.column = rename(node.column); + for (const column of ir.columns ?? []) column.id = rename(column.id) as string; + + /* Prove the rename landed: without it a surviving "in-review" literal would pass by accident. */ + const ids = (ir.columns ?? []).map((column) => column.id); + expect(ids).toContain("checking"); + expect(ids).not.toContain("in-review"); + + const created = await h.store().createWorkflowDefinition({ + name: "Renamed Review (merge queue)", + kind: "workflow", + ir, + } as never); + return (created as { id: string }).id; + } + + /** + * A task walked into the board's review lane. `path` walks the graph rather than jumping, because + * moves are transition-validated and a direct hop is rejected under BOTH vocabularies. + */ + async function seedTaskInReviewLane(path: readonly string[], workflowId?: string): Promise { + const store = h.store(); + const task = await store.createTask({ + title: "awaiting merge", + description: "test", + column: "todo", + }); + if (workflowId) await store.writeTaskWorkflowSelection(task.id, workflowId, []); + for (const step of path) await store.moveTask(task.id, step as never); + store.taskCache.delete(task.id); + + /* Prove the fixture before asserting on it: a task that never reached the review lane would + fail to enqueue for a reason that has nothing to do with column vocabulary. */ + const seeded = await store.getTask(task.id); + expect(seeded.column).toBe(path[path.length - 1]); + return task.id; + } + + /* Control: the default vocabulary enqueues. Passes before and after the fix, so a generally + broken enqueue path cannot hide behind the renamed case below. */ + it("default vocabulary: a task in the review lane enqueues for merge", async () => { + const id = await seedTaskInReviewLane(["in-progress", "in-review"]); + + const entry = await h.store().enqueueMergeQueue(id); + + expect(entry.taskId).toBe(id); + }); + + /* + The defect. Before the fix this threw `MergeQueueInvalidColumnError`, because the wrapper omitted + the board's review columns and the guard compared `checking` against the legacy `in-review`. + */ + it("renamed vocabulary: a task in the RENAMED review lane enqueues for merge", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedTaskInReviewLane(["drafting", "building", "checking"], wf); + + const entry = await h.store().enqueueMergeQueue(id); + + expect(entry.taskId).toBe(id); + }); + + /* + The paired negative, and the reason this is a supply fix rather than a deleted guard: the gate must + still REJECT a task that is not in a review lane at all. Wiring the real columns must not degrade + into "every column is a review column", which would let work merge straight out of the WIP lane. + */ + it("renamed vocabulary: a task in the WIP lane is still REJECTED", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedTaskInReviewLane(["drafting", "building"], wf); + + await expect(h.store().enqueueMergeQueue(id)).rejects.toThrow(); + }); + + /* Same negative under the default vocabulary, so the rejection is not an artifact of the rename. */ + it("default vocabulary: a task in the WIP lane is still REJECTED", async () => { + const id = await seedTaskInReviewLane(["in-progress"]); + + await expect(h.store().enqueueMergeQueue(id)).rejects.toThrow(); + }); +}); diff --git a/packages/core/src/__tests__/postgres/merge-queue-traitless-and-unresolvable.pg.test.ts b/packages/core/src/__tests__/postgres/merge-queue-traitless-and-unresolvable.pg.test.ts new file mode 100644 index 0000000000..cb0777ed2d --- /dev/null +++ b/packages/core/src/__tests__/postgres/merge-queue-traitless-and-unresolvable.pg.test.ts @@ -0,0 +1,200 @@ +/* +FNXC:WorkflowResolvedColumns 2026-07-30-15:30 (#2819 review — greptile, two findings): + +THE TWO WAYS THIS CONVERSION COULD RE-CREATE THE BUG IT FIXES. + +Both are the same mistake in different clothes — treating "I have no answer" as "the answer is no +review lane" — and both land on boards that are not even custom. + + 1. TRAITLESS (v1-UPGRADED) BOARDS. `synthesizeDefaultColumns` (workflow-ir.ts:158) upgrades a v1 + graph by emitting the default column ids with `traits: []`. `resolveReviewColumns` therefore + returns EMPTY while the board's `in-review` column plainly exists and holds the card. Forwarding + that empty set to the enqueue guard makes it match nothing and throw + `MergeQueueInvalidColumnError` — moving the failure off custom boards and onto EVERY pre-v2 + project. Three states, not two: unreadable and traitless both take the legacy id; only a board + that expresses traits and still declares no review lane is answering. + + 2. FAILED RESOLUTION DURING THE STALE SWEEP. The sweep's SQL predicate is a candidate superset and + each row is verified per task before deletion. The first pass deleted a row whose resolution + FAILED, which is the original bug behind a narrower trigger: a transient workflow-read failure + silently drops a valid card out of the merge queue, with an audit event claiming + `reason: "not-in-review"` for a task sitting in review. Sparing is recoverable (the lease + predicates refuse it anyway, and the next sweep removes it once resolution succeeds); deleting + is not. + +WHY A LIVE STORE. Both defects live in the seam between a resolver and SQL evaluated by PostgreSQL. +A mock supplying a lifecycle struct would assert my own assumption about what the resolver returns, +which is the substitution that has produced vacuous tests throughout this program. + +LANE. `.pg.test.ts`, skipped by `pgDescribe` when PostgreSQL is unreachable; throwaway per-file +database; never port 4040. +*/ + +import { it, expect, beforeAll, beforeEach, afterEach, afterAll } from "vitest"; +import { + pgDescribe, + createSharedPgTaskStoreTestHarness, + type SharedPgTaskStoreHarness, +} from "../../__test-utils__/pg-test-harness.js"; +import { BUILTIN_CODING_WORKFLOW_IR } from "../../index.js"; + +pgDescribe("merge queue on traitless and unresolvable workflows", () => { + const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ + prefix: "fusion_mq_traitless", + }); + + beforeAll(h.beforeAll); + beforeEach(h.beforeEach); + afterEach(h.afterEach); + afterAll(h.afterAll); + + /** + * A v1 graph. The store upgrades it on read, and the upgrade emits the DEFAULT column ids with + * EMPTY trait arrays — the exact shape that makes `resolveReviewColumns` return nothing while + * `in-review` exists. Written as v1 on purpose: hand-authoring a v2 board with `traits: []` would + * be my reconstruction of the upgrade rather than the upgrade itself. + */ + async function seedV1Workflow(): Promise { + const created = await h.store().createWorkflowDefinition({ + name: "Legacy v1 (traitless after upgrade)", + kind: "workflow", + ir: { + version: "v1", + name: "legacy-v1", + nodes: [ + { id: "start", kind: "start" }, + { id: "impl", kind: "agent", seam: "execute" }, + { id: "review", kind: "agent", seam: "review" }, + { id: "end", kind: "end" }, + ], + edges: [ + { from: "start", to: "impl" }, + { from: "impl", to: "review" }, + { from: "review", to: "end" }, + ], + }, + } as never); + return (created as { id: string }).id; + } + + async function seedTaskInReview(workflowId: string): Promise { + const store = h.store(); + const task = await store.createTask({ title: "v1 card", description: "test", column: "todo" }); + await store.writeTaskWorkflowSelection(task.id, workflowId, []); + for (const step of ["in-progress", "in-review"]) await store.moveTask(task.id, step as never); + store.taskCache.delete(task.id); + + /* Prove the fixture before asserting on it. */ + expect((await store.getTask(task.id)).column).toBe("in-review"); + return task.id; + } + + /* + Finding 1. Before the three-state guard this threw `MergeQueueInvalidColumnError`: the upgraded + board resolved to an empty review set, so the guard compared `in-review` against nothing. + */ + it("a v1-upgraded (traitless) board still enqueues a card resting in `in-review`", async () => { + const wf = await seedV1Workflow(); + const id = await seedTaskInReview(wf); + + const entry = await h.store().enqueueMergeQueue(id); + + expect(entry.taskId).toBe(id); + }); + + /* + Finding 1, continued — the enqueue is worth nothing if the row cannot then be leased. The lease + predicate resolves the same set, so an empty answer would stall the queue rather than throw, which + is the quieter half of the same defect. + */ + it("a v1-upgraded (traitless) board can then LEASE the queued card", async () => { + const wf = await seedV1Workflow(); + const id = await seedTaskInReview(wf); + await h.store().enqueueMergeQueue(id); + + const lease = await h.store().acquireMergeQueueLease("worker-1", { leaseDurationMs: 60_000 }); + + expect(lease?.taskId).toBe(id); + }); + + /** + * A RENAMED board, needed for finding 2 and not interchangeable with the v1 one. + * + * The sweep's SQL selects a CANDIDATE SUPERSET — `column IS DISTINCT FROM 'in-review'` — and only + * candidates reach the per-task verification the finding is about. A v1-upgraded board uses the + * DEFAULT ids, so its card sits in `in-review` and is never a candidate: written against it, the + * case below passes with the fix reverted. I found that by mutating, not by reading. + */ + async function seedRenamedWorkflow(): Promise { + const RENAME: Record = { + todo: "drafting", + "in-progress": "building", + "in-review": "checking", + done: "shipped", + }; + const rename = (id: string | undefined) => (id && RENAME[id]) ?? id; + const ir = JSON.parse(JSON.stringify(BUILTIN_CODING_WORKFLOW_IR)) as { + id: string; + nodes?: { column?: string }[]; + columns?: { id: string }[]; + }; + ir.id = "custom:traitless-sweep-renamed"; + for (const node of ir.nodes ?? []) node.column = rename(node.column); + for (const column of ir.columns ?? []) column.id = rename(column.id) as string; + expect((ir.columns ?? []).map((c) => c.id)).toContain("checking"); + + const created = await h.store().createWorkflowDefinition({ + name: "Renamed (sweep verification)", + kind: "workflow", + ir, + } as never); + return (created as { id: string }).id; + } + + async function seedRenamedTaskInReview(workflowId: string): Promise { + const store = h.store(); + const task = await store.createTask({ title: "renamed card", description: "test", column: "todo" }); + await store.writeTaskWorkflowSelection(task.id, workflowId, []); + for (const step of ["drafting", "building", "checking"]) await store.moveTask(task.id, step as never); + store.taskCache.delete(task.id); + expect((await store.getTask(task.id)).column).toBe("checking"); + return task.id; + } + + /* + Finding 2. A resolver that THROWS stands in for a transient workflow-read failure. The row must + survive the sweep: deletion is unrecoverable, and the acquire that follows refuses the lease anyway + because it resolves the same lanes. Asserting the row is still queued afterwards is the point — + the previous behaviour deleted it and reported an empty queue as normal. + */ + it("a row whose workflow cannot be resolved is SPARED by the stale sweep, not deleted", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedRenamedTaskInReview(wf); + await h.store().enqueueMergeQueue(id); + + await h.store().acquireMergeQueueLease("worker-1", { + leaseDurationMs: 60_000, + resolveReviewColumnsFor: async () => { + throw new Error("transient workflow read failure"); + }, + }); + + expect(await h.store().getMergeQueuedTaskIdsAsync()).toContain(id); + }); + + /* + The paired negative: sparing on failure must not become "never clean anything up". A row whose task + has genuinely left review, with resolution WORKING, is still deleted. + */ + it("a row whose task genuinely left the review lane is still swept", async () => { + const wf = await seedRenamedWorkflow(); + const id = await seedRenamedTaskInReview(wf); + await h.store().enqueueMergeQueue(id); + await h.store().moveTask(id, "building" as never); + h.store().taskCache.delete(id); + + await h.store().acquireMergeQueueLease("worker-1", { leaseDurationMs: 60_000 }); + + expect(await h.store().getMergeQueuedTaskIdsAsync()).not.toContain(id); + }); +}); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 0e284ba63d..e48c0b16d8 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -126,7 +126,7 @@ import { createTaskBackendImpl, _createTaskInternalBackendImpl, createTaskImpl, import { getTaskImpl, listTasksImpl, searchTasksImpl, listTasksModifiedSinceImpl, getTaskVerificationRequestAsyncImpl } from "./task-store/reads.js"; import { updateTaskUnlockedImpl } from "./task-store/task-update.js"; import { __setTaskActivityLogLimitsForTesting } from "./task-store/comments.js"; -import { resolveReviewColumns, resolveTaskLifecycleColumns, type LifecycleColumns } from "./workflow-lifecycle-traits.js"; +import { declaresAnyLifecycleTrait, resolveReviewColumns, resolveTaskLifecycleColumns, type LifecycleColumns } from "./workflow-lifecycle-traits.js"; import { resolveProjectColumnsForRoles } from "./project-lane-vocabulary.js"; import { resolveWorkflowIrForTask } from "./workflow-ir-resolver.js"; // FNXC:RuntimeBackendAsync 2026-06-24-10:15: @@ -1449,8 +1449,17 @@ export class TaskStore extends EventEmitter { The message names the lanes the check actually used, keeping #2709's fix: telling an operator to move to a column their board does not have is worse than refusing. */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-15:20 (#2819 review — the same empty-set hole, found by + sweeping every `resolveReviewColumns` call site rather than only the one the reviewer named): + A v1-upgraded board resolves to an EMPTY review set while its `in-review` column plainly exists, + so this guard would refuse the operator's bypass on every pre-v2 project with the unhelpful + message "must be in a review lane". + */ const reviewIr = await resolveWorkflowIrForTask(this, task.id).catch(() => undefined); - const reviewColumns = reviewIr === undefined ? ["in-review"] : resolveReviewColumns(reviewIr); + const reviewColumns = reviewIr === undefined || !declaresAnyLifecycleTrait(reviewIr) + ? ["in-review"] + : resolveReviewColumns(reviewIr); if (!reviewColumns.includes(task.column)) { const named = reviewColumns.length > 0 ? reviewColumns.map((c: string) => `'${c}'`).join(" or ") : "a review lane"; throw new Error(`Cannot bypass review lane for ${id}: task is in '${task.column}', must be in ${named}`); @@ -1758,7 +1767,26 @@ export class TaskStore extends EventEmitter { * FNXC:RuntimeLifecycleAsync 2026-06-24-11:20: */ async acquireMergeQueueLease(workerId: string, opts: MergeQueueAcquireOptions): Promise { - return acquireMergeQueueLeaseImpl(this, workerId, opts); + /* + FNXC:WorkflowResolvedColumns 2026-07-31-01:25 (#2819 review — greptile): + Supplies the per-task review-lane resolver for the stale-row sweep. This is the production path, + so wiring it here is what makes the option live rather than one only tests fill. + */ + const withResolver: MergeQueueAcquireOptions = { + ...opts, + resolveReviewColumnsFor: opts.resolveReviewColumnsFor ?? (async (taskId: string) => { + /* + Three states, not two. `undefined` is "could not read"; an IR whose columns carry NO + lifecycle trait at all is a v1 graph upgraded by `synthesizeDefaultColumns`, whose + `in-review` column plainly exists and holds cards. Both take the legacy answer. Only a + board that expresses traits and still has no review lane is answering the question. + */ + const ir = await resolveWorkflowIrForTask(this, taskId).catch(() => undefined); + if (!ir || !declaresAnyLifecycleTrait(ir)) return new Set(["in-review"]); + return new Set(resolveReviewColumns(ir)); + }), + }; + return acquireMergeQueueLeaseImpl(this, workerId, withResolver); } /** diff --git a/packages/core/src/task-store/async-merge-coordination.ts b/packages/core/src/task-store/async-merge-coordination.ts index 36bc3c3063..953134ba0b 100644 --- a/packages/core/src/task-store/async-merge-coordination.ts +++ b/packages/core/src/task-store/async-merge-coordination.ts @@ -97,22 +97,49 @@ function leaseAvailable(now: string) { } /** - * Predicate: the queue row's task is still in the `in-review` column. + * Predicate: the queue row's task is still in a REVIEW lane. * * FNXC:MultiProjectIsolation 2026-07-10: when `projectId` is bound, the EXISTS * additionally requires the task to belong to this project so a project's * merger can only lease its OWN queue rows (merge_queue has no project_id, so * it is scoped transitively through its task on the shared embedded-PG cluster). + * + * FNXC:WorkflowResolvedColumns 2026-07-30-13:55 (#2819 review — greptile, + * "renamed entries remain unleaseable"): + * + * THE STALE SWEEP SPARED RENAMED ROWS AND THE LEASE PREDICATE THEN REFUSED THEM. + * + * The first pass fixed only `cleanupStaleMergeQueueRowsInTransaction`, so on a + * board whose review lane is called `signoff` the queue row survived — and then + * both eligibility predicates (targeted acquire and queue-head acquire) still + * demanded the literal `in-review`, matched nothing, and the row sat forever. + * Sparing a row you will never lease is worse than deleting it: deletion at + * least surfaces as an audit event, while this is a silent permanent stall. + * + * THE LANES ARE INJECTED INTO THE SQL rather than verified after the fact. The + * WHERE clause here is also the concurrency control — the acquire re-checks it + * inside `UPDATE ... WHERE` so a racing worker updates zero rows. Verifying the + * column in JS between SELECT and UPDATE would move that check outside the + * atomic step and let two workers lease the same task. `reviewColumns` is + * therefore a resolved set the caller supplies, and it goes into the predicate. */ -function taskStillInReview(projectId?: string) { +function taskStillInReview(projectId?: string, reviewColumns?: ReadonlySet) { const projectClause = projectId ? sql`AND ${schema.project.tasks.projectId} = ${projectId}` : sql``; + const lanes = [...(reviewColumns ?? new Set(["in-review"]))]; + /* + An EMPTY resolved set means the board declares no review lane at all. `inArray` with no + values is not portable across drizzle versions (it has both thrown and folded to FALSE), + so the FALSE is written explicitly. Nothing can be enqueued on such a board either — the + enqueue guard resolves the same set — so an unleaseable queue there is not a new stall. + */ + const laneClause = lanes.length > 0 ? inArray(schema.project.tasks.column, lanes) : sql`FALSE`; return sql` EXISTS ( SELECT 1 FROM ${schema.project.tasks} WHERE ${schema.project.tasks.id} = ${schema.project.mergeQueue.taskId} - AND ${schema.project.tasks.column} = 'in-review' + AND ${laneClause} ${projectClause} ) `; @@ -241,9 +268,19 @@ export async function enqueueMergeQueue( taskId: string, opts: MergeQueueEnqueueOptions = {}, audit?: { agentId?: string; runId?: string }, + /* + FNXC:WorkflowResolvedColumns 2026-07-30-02:10: + The board's review columns, forwarded to the in-transaction helper. Omitted, that helper falls back + to `new Set(["in-review"])` and REJECTS a task resting in a renamed review column — it is not a + quiet legacy-id degradation like most of these seams, it records `mergeQueue:enqueue-rejected` and + throws `MergeQueueInvalidColumnError`. Both `moves.ts` callers already resolve and pass this; this + wrapper did not, so the automatic handoff-to-review path worked on a renamed board while the manual + and recovery re-enqueue paths through `store.enqueueMergeQueue` (merger.ts, self-healing.ts) threw. + */ + reviewColumns?: ReadonlySet, ): Promise { return layer.transactionImmediate((tx) => - enqueueMergeQueueInTransaction(tx, taskId, opts, audit), + enqueueMergeQueueInTransaction(tx, taskId, opts, audit, reviewColumns), ); } @@ -260,6 +297,13 @@ export async function enqueueMergeQueue( export async function cleanupStaleMergeQueueRowsInTransaction( tx: DbTransaction, now: string, + /* + FNXC:WorkflowResolvedColumns 2026-07-31-01:20 (#2819 review — greptile): + Per-task review lanes, supplied by the caller. This runs inside an open transaction with only a + `tx` handle, so it cannot resolve a workflow itself — and the lanes are genuinely per task, because + queued tasks can run different workflows. + */ + resolveReviewColumnsFor?: (taskId: string) => Promise>, ): Promise { const staleRows = await tx .select({ @@ -277,7 +321,42 @@ export async function cleanupStaleMergeQueueRowsInTransaction( ), ); - if (staleRows.length === 0) return; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-01:20 (#2819 review — greptile): + THE SQL PREDICATE IS A SUPERSET NOW, NOT THE VERDICT. + + `column IS DISTINCT FROM 'in-review'` is evaluated by PostgreSQL, which cannot know a task's + workflow — so on a board whose review lane is named anything else, EVERY queued row matched and was + deleted at the start of lease acquisition. The merge queue filled (enqueue resolves lanes) and was + then emptied a moment later by this sweep, so nothing was ever merged and no error was raised: the + audit rows even claimed `reason: "not-in-review"` for tasks sitting in their board's review lane. + + Deleting is the destructive direction, so the SQL now only proposes CANDIDATES and each one is + verified against its own workflow before removal. Rows whose task is gone are stale regardless and + skip the check. + */ + const verifiedStale = resolveReviewColumnsFor === undefined + ? staleRows + : (await Promise.all(staleRows.map(async (row) => { + if (row.column === null) return row; + const reviewLanes = await resolveReviewColumnsFor(row.taskId).catch(() => undefined); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-15:10 (#2819 review — greptile, + "resolution failures delete valid rows"): + A FAILED RESOLUTION SPARES THE ROW. The first pass returned the row here, i.e. deleted it — + which reintroduced the exact bug this sweep fix exists to remove, just behind a narrower + trigger: any transient workflow-read failure silently dropped a valid card out of the merge + queue, and the audit event claimed `reason: "not-in-review"` for a task sitting in review. + + Sparing is safe in a way deleting is not. The lease predicates resolve the same lanes, so a + row spared here but genuinely stale is still refused a lease, and the next acquire re-runs + this sweep and removes it once resolution succeeds. Deleting is unrecoverable. + */ + if (reviewLanes === undefined) return undefined; + return reviewLanes.has(row.column) ? undefined : row; + }))).filter((row): row is typeof staleRows[number] => row !== undefined); + + if (verifiedStale.length === 0) return; // FNXC:TaskStoreMergeCoordination 2026-06-26-10:10: // Batch the cleanup to avoid an N+1: previously each stale row cost 2 @@ -286,12 +365,12 @@ export async function cleanupStaleMergeQueueRowsInTransaction( // acquired. Now the deletes are a single bulk DELETE ... WHERE IN (...) and // the audit events are a single bulk INSERT ... VALUES (...). Each metadata // payload is still per-row (the column/lease context differs per task). - const staleTaskIds = staleRows.map((row) => row.taskId); + const staleTaskIds = verifiedStale.map((row) => row.taskId); await tx .delete(schema.project.mergeQueue) .where(inArray(schema.project.mergeQueue.taskId, staleTaskIds)); - const auditValues = staleRows.map((row) => ({ + const auditValues = verifiedStale.map((row) => ({ id: randomUUID(), timestamp: now, taskId: row.taskId, @@ -353,20 +432,37 @@ export async function acquireMergeQueueLease( // must be scoped to this project so a project's merger can never lease (and // then merge in the wrong repo) another project's in-review task. const projectId = layer.projectId; + /* + FNXC:WorkflowResolvedColumns 2026-07-30-14:05 (#2819 review): + One resolution per task id per acquire, memoised, because the same id is asked + about twice on the targeted path (candidate SELECT, then the atomic UPDATE) and + the two MUST agree — resolving twice and getting different answers would let the + UPDATE's predicate diverge from the one that admitted the candidate. + */ + const laneCache = new Map>(); + const reviewLanesFor = async (taskId: string): Promise | undefined> => { + if (!opts.resolveReviewColumnsFor) return undefined; + const cached = laneCache.get(taskId); + if (cached) return cached; + const resolved = await opts.resolveReviewColumnsFor(taskId).catch(() => undefined); + if (resolved) laneCache.set(taskId, resolved); + return resolved; + }; return layer.transactionImmediate(async (tx) => { const now = opts.now ?? new Date().toISOString(); const leaseExpiresAt = new Date(Date.parse(now) + opts.leaseDurationMs).toISOString(); - await cleanupStaleMergeQueueRowsInTransaction(tx, now); + await cleanupStaleMergeQueueRowsInTransaction(tx, now, opts.resolveReviewColumnsFor); if (opts.targetTaskId) { // ── Targeted acquire: lease this specific task or fail ────────────── + const targetLanes = await reviewLanesFor(opts.targetTaskId); const candidateRows = await tx .select({ taskId: schema.project.mergeQueue.taskId }) .from(schema.project.mergeQueue) .where( and( eq(schema.project.mergeQueue.taskId, opts.targetTaskId), - taskStillInReview(projectId), + taskStillInReview(projectId, targetLanes), leaseAvailable(now), ), ) @@ -419,7 +515,7 @@ export async function acquireMergeQueueLease( .where( and( eq(schema.project.mergeQueue.taskId, opts.targetTaskId), - taskStillInReview(projectId), + taskStillInReview(projectId, targetLanes), leaseAvailable(now), ), ) @@ -448,10 +544,27 @@ export async function acquireMergeQueueLease( return entry; } - // ── Queue-head acquire: lease the highest-priority, earliest available row ── - // Select the candidate first (priority-first, FIFO within priority), then - // UPDATE it while re-checking availability to avoid a lost-update race. - const headRows = await tx + /* + ── Queue-head acquire: lease the highest-priority, earliest available row ── + + FNXC:WorkflowResolvedColumns 2026-07-30-14:20 (#2819 review): + THE HEAD SELECT CANNOT NAME THE LANES, BECAUSE IT DOES NOT YET KNOW THE TASK. + Each queued row can run a DIFFERENT workflow, so "is this row in review?" has a + different answer per row and the resolver is keyed by task id. The select is + therefore a CANDIDATE SUPERSET — ordered exactly as before — and each candidate + is resolved in turn until one qualifies. The chosen row's own resolved lanes then + go into the UPDATE's WHERE, so the atomic re-check is preserved: the row is only + leased if it is STILL both available and in one of ITS review lanes. + + Without a resolver (callers that cannot resolve workflows) the predicate stays + the literal and the loop stops at the first candidate — the previous behaviour. + + The candidate scan is bounded. The stale sweep above has already deleted rows + whose task left review, so under normal operation the first candidate qualifies; + the bound only caps the pathological case where the sweep could not resolve. + */ + const HEAD_CANDIDATE_SCAN_LIMIT = 25; + const candidateHeadRows = await tx .select({ taskId: schema.project.mergeQueue.taskId }) .from(schema.project.mergeQueue) .innerJoin( @@ -460,15 +573,35 @@ export async function acquireMergeQueueLease( ) .where( and( - eq(schema.project.tasks.column, "in-review"), // FNXC:MultiProjectIsolation 2026-07-10: only this project's tasks. taskProjectScope(layer), leaseAvailable(now), ), ) .orderBy(MERGE_QUEUE_PRIORITY_RANK, schema.project.mergeQueue.enqueuedAt) - .limit(1); - const head = headRows[0]; + .limit(opts.resolveReviewColumnsFor ? HEAD_CANDIDATE_SCAN_LIMIT : 1); + + let head: { taskId: string } | undefined; + let headLanes: ReadonlySet | undefined; + for (const candidate of candidateHeadRows) { + const lanes = await reviewLanesFor(candidate.taskId); + const eligible = await tx + .select({ taskId: schema.project.mergeQueue.taskId }) + .from(schema.project.mergeQueue) + .where( + and( + eq(schema.project.mergeQueue.taskId, candidate.taskId), + taskStillInReview(projectId, lanes), + leaseAvailable(now), + ), + ) + .limit(1); + if (eligible.length > 0) { + head = candidate; + headLanes = lanes; + break; + } + } if (!head) { return null; } @@ -483,6 +616,7 @@ export async function acquireMergeQueueLease( .where( and( eq(schema.project.mergeQueue.taskId, head.taskId), + taskStillInReview(projectId, headLanes), leaseAvailable(now), ), ) diff --git a/packages/core/src/task-store/task-artifacts-ops.ts b/packages/core/src/task-store/task-artifacts-ops.ts index a2e7e57dd0..067750e445 100644 --- a/packages/core/src/task-store/task-artifacts-ops.ts +++ b/packages/core/src/task-store/task-artifacts-ops.ts @@ -11,7 +11,7 @@ import { TaskStore } from "../store.js"; import { resolveProjectColumnsForRoles } from "../project-lane-vocabulary.js"; -import {resolveTaskLifecycleColumns} from "../workflow-lifecycle-traits.js"; +import {declaresAnyLifecycleTrait, resolveReviewColumns, resolveTaskLifecycleColumns} from "../workflow-lifecycle-traits.js"; import {resolveWorkflowIrForTask} from "../workflow-ir-resolver.js"; import { countAgentLogEntries, readAgentLogEntries } from "../agent-log-file-store.js"; import { toJsonNullable } from "../db.js"; @@ -85,7 +85,31 @@ export async function enqueueMergeQueueImpl(store: TaskStore, taskId: string, op FNXC:SqliteDualPathCleanup 2026-07-26-14:05: Merge-queue enqueue is PostgreSQL-only via enqueueMergeQueueAsync (column check, idempotent insert, audit). The SQLite enqueueMergeQueueSyncInternal arm is deleted. */ - return enqueueMergeQueueAsync(store.asyncLayer!, taskId, opts); + /* + FNXC:WorkflowResolvedColumns 2026-07-30-02:10: + Resolve the task's OWN review columns before enqueueing. Without them the in-transaction guard + falls back to `new Set(["in-review"])` and throws `MergeQueueInvalidColumnError` for a task + resting in a renamed review lane, which breaks the merger and self-healing re-enqueue paths on + every custom board. `undefined` on failure is deliberate and matches `moves.ts`: it makes the + guard fall back to the legacy id rather than to an empty set that matches nothing. + */ + /* + FNXC:WorkflowResolvedColumns 2026-07-30-15:15 (#2819 review — greptile, "traitless workflows reject re-enqueue"): + THREE STATES, NOT TWO. `undefined` (unreadable) was already handled. The missing one is an IR that + resolves but carries NO lifecycle trait on any column: `synthesizeDefaultColumns` upgrades a v1 + graph by emitting the default columns with `traits: []`, so `resolveReviewColumns` returns EMPTY + while the board's `in-review` column plainly exists and holds the card. Forwarding that empty set + made the guard match nothing and throw `MergeQueueInvalidColumnError` — the very failure this + conversion was fixing, moved from custom boards onto every pre-v2 project. + + Only a board that EXPRESSES traits and still has no review lane is answering the question; the + other two states take the legacy id. + */ + const reviewIr = await resolveWorkflowIrForTask(store, taskId).catch(() => undefined); + const reviewColumns = reviewIr && declaresAnyLifecycleTrait(reviewIr) + ? new Set(resolveReviewColumns(reviewIr)) + : undefined; + return enqueueMergeQueueAsync(store.asyncLayer!, taskId, opts, undefined, reviewColumns); } export function cleanupStaleMergeQueueRowsImpl(store: TaskStore, now: string): void { diff --git a/packages/core/src/types/merge-queue.ts b/packages/core/src/types/merge-queue.ts index e6b15e36bf..64121d2f3b 100644 --- a/packages/core/src/types/merge-queue.ts +++ b/packages/core/src/types/merge-queue.ts @@ -171,6 +171,17 @@ export interface MergeQueueAcquireOptions { /** If provided, the lease attempt targets this specific task first. * The task must be unexpired/available; otherwise falls back to normal queue-head selection. */ targetTaskId?: string; + /* + FNXC:WorkflowResolvedColumns 2026-07-31-01:25 (#2819 review — greptile): + Resolves a task's review lanes for the stale-row sweep that runs at the start of lease acquisition. + That sweep deletes rows whose task has left review, and its predicate is evaluated by PostgreSQL — + which cannot know a workflow. Without this the SQL literal deleted every queued row on a renamed + board, so the queue filled and emptied and nothing merged. + + Optional so a caller with only a data layer keeps today's behaviour; `TaskStore.acquireMergeQueueLease` + supplies it, which is the path production uses. + */ + resolveReviewColumnsFor?: (taskId: string) => Promise>; } export type MergeQueueReleaseOutcome =