From b0b9d1b373685bbaef6951caeffdb0a5e37097a7 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 07:36:00 -0700 Subject: [PATCH] =?UTF-8?q?fleet:=20store.ts=2012=20=E2=86=92=2011=20+=20n?= =?UTF-8?q?ames=20the=20sync-dependency-loop=20class=20blocking=20~10=20si?= =?UTF-8?q?tes=20across=203=20clusters=20(#2709)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claiming **`packages/core/src/store.ts`** (12). One conversion and a triage — because **10 of the 12 share a single blocking shape** that is worth naming once rather than rediscovering per file. ## Census before/after | | before | after | |---|---:|---:| | `store.ts` column guards | **12** | **11** | Baseline re-recorded; `--strict` exits 0. ## Converted: 1 **1386** — the in-review guard inside `withTaskLock(id, async () => …)`. Already async, and `this` **is** the store, so `resolveTaskLifecycleColumns(this, task.id)` resolves the review role with `in-review` as the fallback. Import added; nothing else in the method changes. ## The blocking class — 6 sites, and it is not specific to this file **1772, 1791 ×2, 1874 ×2, 1916, 1917, 1933** all read **another task's** column — a dependency's, a blocker's, an overlap candidate's — inside **synchronous callbacks over a prefetched `taskById` map**: ```ts const unresolvedDeps = (task.dependencies ?? []).filter((depId) => { const dep = taskById.get(depId); return dep && !dep.deletedAt && dep.column !== "done" && dep.column !== "archived"; }); ``` This is not a substitution. Each dependency may belong to a **different workflow**, so the role must be resolved *per dep* — N async resolutions inside a sync `filter`, on a path that deliberately prefetches into a map precisely to avoid per-item I/O. Two honest options: 1. **Prefetch lifecycle columns alongside `taskById`** and pass a resolved map into these predicates. Keeps them synchronous, one resolution per distinct workflow rather than per dep. This is the one I'd argue for. 2. Accept per-dep resolution and make the callbacks async — changes the shape of dependency evaluation. Both are design changes with real cost, so this is flagged rather than guessed. **The same shape appears in at least two other clusters I've worked**: `TaskDetailModal`'s `overlapBlockerTask.column` (#2696) and `register-task-workflow-routes`' dependency-summary pair (#2700), both flagged for this exact reason. **Worth one decision covering all three** rather than three separate judgement calls by three workers. ## Also flagged: 3 **1610** and **1739** — enclosing-scope async-ness and store access not established at those points, so not guessed. **1933** belongs to the sync-filter family above. ## Verification `pnpm test:gate` **GREEN** (158 + 487 + 10 + 71) · `pnpm lint` clean · core `tsc` clean · `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit * **Bug Fixes** * Updated failed pre-merge review bypass validation to support custom workflow boards. * Tasks can now bypass the step when placed in the board’s configured review lane. * Improved error messages to identify the correct review column when bypassing is not allowed. --------- Co-authored-by: Claude Opus 5 (1M context) --- .../src/__tests__/store-bypass-review.test.ts | 76 +++++++++++++++++++ packages/core/src/store.ts | 40 +++++++++- .../lib/lifecycle-column-census-baseline.json | 2 +- 3 files changed, 113 insertions(+), 5 deletions(-) diff --git a/packages/core/src/__tests__/store-bypass-review.test.ts b/packages/core/src/__tests__/store-bypass-review.test.ts index 522712025d..9982eda394 100644 --- a/packages/core/src/__tests__/store-bypass-review.test.ts +++ b/packages/core/src/__tests__/store-bypass-review.test.ts @@ -182,4 +182,80 @@ pgDescribe("TaskStore.bypassFailedPreMergeReviewStep", () => { expect(latestFailedPreMergeStep(updated)).toBeUndefined(); }); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-01:10 (PR #2709 review — greptile): + THE REJECTION MUST NAME THE COLUMN THE CHECK USED. The guard was converted to the resolved review + lane while the message still said `in-review`, so on a custom board an operator was refused and + then told to move the card to a column their board does not have — through both the CLI and the + dashboard, with nothing in the error to reveal the real target. + + That is worse than an unconverted guard. An inert guard fails visibly; this one refuses CORRECTLY + and then misdirects, so the operator's next three attempts are all wrong for a reason the product + told them. + */ + it("accepts a humanReview-ONLY lane, which the singular `.review` excluded", async () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-16:05 (PR #2718 review — greptile): + `.review` is the single `mergeOrchestration` column, so a board hosting review on a `humanReview`- + only lane failed this guard — `TaskContextMenu` offered "Bypass failed review" (it asks by ROLE) + and the store refused it. The operator's only escape from a stranded failed pre-merge step returned + a conflict. + + The BROAD set is right here because this guard refuses or permits and moves nothing; #2750 + documents why a caller that admits and then MOVES wants the narrow lane instead. + */ + const definition = await store().createWorkflowDefinition({ + name: "human-review-bypass", + ir: { + version: "v2", + name: "human-review-bypass", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "signoff", name: "Sign-off", traits: [{ trait: "human-review" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + }, + } as never); + + await store().createTaskWithReservedId( + { description: "human-review bypass", column: "signoff", workflowId: definition.id } as never, + { taskId: "FN-HRB", applyDefaultWorkflowSteps: false }, + ); + await store().updateTask("FN-HRB", { workflowStepResults: [failedStep()] }); + + /* Passes the lane guard; any later refusal is a different gate, which is the point. */ + await expect( + store().bypassFailedPreMergeReviewStep("FN-HRB", { reason: "operator override" } as never), + ).resolves.toBeDefined(); + }); + + it("names the board's OWN review column when refusing a card that is elsewhere", async () => { + const definition = await store().createWorkflowDefinition({ + name: "renamed-review", + ir: { + version: "v2", + name: "renamed-review", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + { id: "validating", name: "Validating", traits: [{ trait: "merge" }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + nodes: [{ id: "start", kind: "start", column: "backlog" }, { id: "end", kind: "end", column: "shipped" }], + edges: [{ from: "start", to: "end" }], + }, + } as never); + + await store().createTaskWithReservedId( + { description: "renamed board card", column: "building", workflowId: definition.id } as never, + { taskId: "FN-RENAMED", applyDefaultWorkflowSteps: false }, + ); + + await expect( + store().bypassFailedPreMergeReviewStep("FN-RENAMED", { reason: "operator override" } as never), + ).rejects.toThrow(/must be in 'validating'/); + }); }); diff --git a/packages/core/src/store.ts b/packages/core/src/store.ts index 50cda328dd..2f84c0aa3f 100644 --- a/packages/core/src/store.ts +++ b/packages/core/src/store.ts @@ -126,6 +126,8 @@ 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 } from "./workflow-lifecycle-traits.js"; +import { resolveWorkflowIrForTask } from "./workflow-ir-resolver.js"; // FNXC:RuntimeBackendAsync 2026-06-24-10:15: // Async helper imports for backend-mode (AsyncDataLayer/PostgreSQL) delegation. // persistence/allocator/settings/search/lifecycle/merge/archive helpers preserve @@ -749,7 +751,7 @@ export class TaskStore extends EventEmitter { return reconcileStaleSymbolLocksAsync(this); } - /** FNXC:SymbolLock 2026-07-31-10:00: FN-8306 resolves only durable task declarations; PROMPT is never re-read here. */ + /** FNXC:SymbolLock 2026-07-30-10:00: FN-8306 resolves only durable task declarations; PROMPT is never re-read here. */ async resolveTaskSymbols(taskId: string): Promise { try { return resolveTaskSymbolsForTask(await this.getTask(taskId)); @@ -1383,8 +1385,38 @@ export class TaskStore extends EventEmitter { const dir = this.taskDir(id); const task = await this.readTaskJson(dir); - if (task.column !== "in-review") { - throw new Error(`Cannot bypass review lane for ${id}: task is in '${task.column}', must be in 'in-review'`); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-01:10 (PR #2709 review — greptile): + THE MESSAGE MUST NAME THE COLUMN THE CHECK ACTUALLY USED. The guard was converted to the + resolved review lane while the rejection still said `in-review`, so on a custom board an + operator was told to move the card to a column their board does not have — through both the + CLI and the dashboard, with no way to discover the real answer from the error. + + Wrong guidance is worse than an unconverted guard: an inert guard fails visibly, while this + one refuses correctly and then sends the operator somewhere that does not exist. Resolved once + into a local so the check and the message cannot drift apart again. + */ + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-16:05 (PR #2718 review — greptile, on the guard I + converted in #2709): + EVERY REVIEW LANE, because `.review` is the single `mergeOrchestration` column. A board hosting + review on a `humanReview`- or `mergeBlocker`-only lane failed this check, so `TaskContextMenu` + offered "Bypass failed review" (it asks by ROLE) and the store refused it — the operator's only + escape from a stranded failed pre-merge step returned a conflict. + + THE BROAD SET IS RIGHT HERE, and that is a decision rather than a default: this guard REFUSES or + PERMITS an operator action and moves nothing, so admitting every lane where review happens cannot + send a card anywhere the engine disagrees with. #2750 documents the split — a caller that admits + and then MOVES wants the narrow single lane instead. + + 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. + */ + const reviewIr = await resolveWorkflowIrForTask(this, task.id).catch(() => undefined); + const reviewColumns = reviewIr === undefined ? ["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}`); } if (task.paused) { throw new Error(`Cannot bypass review lane for ${id}: task is paused`); @@ -2399,7 +2431,7 @@ Issue #2149 requires read-only type filtering to occur in the file-store before the three U5 reconciliation guards (now unconditional) and the three v1-IR rollback-compat persistence sites (now unconditional, same stored bytes). - FNXC:WorkflowColumns 2026-07-31-04:00 (U12): `isWorkflowColumnsCompatibilityFlagEnabled` is now + FNXC:WorkflowColumns 2026-07-30-04:00 (U12): `isWorkflowColumnsCompatibilityFlagEnabled` is now DELETED TOO. Its last two readers were the move path — `moves.ts` and the preflight in `workflow-task-create-ops.ts` — and both were un-gated in one commit because the preflight computes what `moves.ts` consumes. diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index 89497c6e07..9791094941 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": 110, "packages/engine/src/executor.ts": 15, - "packages/core/src/store.ts": 12, "packages/engine/src/scheduler.ts": 12, + "packages/core/src/store.ts": 11, "packages/core/src/task-store/async-comments-attachments.ts": 9, "packages/dashboard/app/components/TaskContextMenu.tsx": 9, "packages/engine/src/notification/notification-service.ts": 9,