From ab715cbd39b18580ff61cc367b002ce7b0664524 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 30 Jul 2026 07:05:05 -0700 Subject: [PATCH] =?UTF-8?q?fleet:=20default-workflow-hooks.ts=207=20?= =?UTF-8?q?=E2=86=92=200=20=E2=80=94=20every=20duration=20display=20read?= =?UTF-8?q?=20ZERO=20on=20a=20renamed=20board=20(#2734)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Claiming `default-workflow-hooks.ts` (7 → **0**), verified free against every open PR's diff first. ## Not a vocabulary tidy — three silent zeroes This file's header names it for the default workflow, but the store runs it on the flag-ON path for **every** workflow: the trait registry resolves each hook by **trait id**, not by workflow. `reopen-semantics-by-role.test.ts` already documents that exact hazard for the reopen predicates. The **timing, completion and in-review hooks had the same defect** and were not part of that conversion. On a renamed board, with nothing thrown and nothing logged: - **`applyTimingEffects`** accrues `cumulativeActiveMs` while a card sits in the WIP lane. With the lane named, the exit test never fires — so **no active time is ever accrued**, and `productivity-analytics.ts`, `task-timing.ts` and every duration display read **zero**. - **`applyCompletionTimingEffects`** never stamps `executionCompletedAt`, so a finished card looks unfinished to anything reading that field. - **`applyInReviewEnterEffects`** returns early, leaving the recovery counters set. The file already had the idiom — `ctx.lifecycleColumns`, `planningColumnsOf`, `liveWorkColumnsOf` with `LEGACY_` fallbacks — so this adds no abstraction. One deliberate detail: `applyTimingEffects` resolves the WIP lane **once into a local** rather than reading it twice. The exit test and the re-entry test have to agree about which column is WIP, or a rename makes the accounting count an interval twice, or not at all. ## A test that would have lied to me I wrote the new cases through `applyDefaultWorkflowMoveEffects` first, and **all three failed on the DEFAULT lineage too**. The dispatcher resolves hooks by trait, and neither test IR declares the `timing` trait, so those hooks never ran at all. That failure looks exactly like a conversion bug. Going through the dispatcher would have been testing the trait registry's wiring rather than this change — so the cases call the converted functions directly, and the reason is recorded in the test. ## Revert proof — all three, each naming the renamed lineage | reverted | failure | |---|---| | the `in-progress` literals | `renamed lineage accrued no active time: expected undefined to be 300000` | | the `done` literal | `renamed lineage did not stamp completion: expected undefined to be '2026-07-30T00:00:00.000Z'` | | the `in-review` literal | `renamed lineage kept its recovery counter: expected 3 to be undefined` | Every case runs on **both** lineages and the default one passes either way — which is the point of running it. ## A finding I did not act on **`evaluateMergeBlockerGuard` appears exactly once in the repo — its own definition.** And the file header says it is "implemented as the `evaluateDefaultWorkflowGuards` reader", which does not exist either. The merge-blocker guard hook is **defined and never consulted**. I converted it (trailing optional lifecycle param, matching `DefaultWorkflowMoveContext`) but did not delete it: the header states this file is a deliberate parallel of `store.ts`'s flag-off path so the two can be parity-checked, which makes removing it a scope call for whoever owns that convergence — not something to decide inside a conversion. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **22 passed** across `default-workflow-hooks` + `reopen-semantics-by-role` · core `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../moves-review-set-from-producer.pg.test.ts | 97 +++++++++ .../reopen-semantics-by-role.test.ts | 200 ++++++++++++++++++ packages/core/src/default-workflow-hooks.ts | 71 ++++++- packages/core/src/task-store/moves.ts | 28 ++- .../lib/lifecycle-column-census-baseline.json | 1 - 5 files changed, 390 insertions(+), 7 deletions(-) create mode 100644 packages/core/src/__tests__/postgres/moves-review-set-from-producer.pg.test.ts diff --git a/packages/core/src/__tests__/postgres/moves-review-set-from-producer.pg.test.ts b/packages/core/src/__tests__/postgres/moves-review-set-from-producer.pg.test.ts new file mode 100644 index 0000000000..b0dc394d87 --- /dev/null +++ b/packages/core/src/__tests__/postgres/moves-review-set-from-producer.pg.test.ts @@ -0,0 +1,97 @@ +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-15:10 (PR #2734 review — greptile, on my own code): + +THE PRODUCER IS THE THING UNDER TEST, not the hook. + +`moves.ts` builds `lifecycleColumnSets.review` and hands it to the default-workflow hooks. It built +that set from `columnsWithFlag(ir, "mergeOrchestration")` alone, so a workflow hosting review on a +`humanReview`-only lane produced an EMPTY review set — and `applyInReviewEnterEffects` then never ran +for a card plainly in review, leaving the recovery counters it clears set. + +WHY THIS FILE EXISTS RATHER THAN A UNIT CASE. My first attempt asserted the hook directly, passing +`lifecycleColumnSets: { review: ["signoff"] }` by hand. That exercises the hook — which was already +correct — and passes with the producer's bug fully in place. Reverting the producer left it green. +Only driving a real move through a real store reaches the code that was wrong. + +LANE. `.pg.test.ts`, skipped by `pgDescribe` when no PostgreSQL is reachable, so the merge gate is +unaffected. Throwaway per-file database; never port 4040. +*/ +import { beforeAll, beforeEach, afterEach, afterAll, expect, it } from "vitest"; +import "@fusion/core"; // registers the built-in column traits + +import { pgDescribe, createSharedPgTaskStoreTestHarness } from "../../__test-utils__/pg-test-harness.js"; + +pgDescribe("moves.ts supplies EVERY review lane to the lifecycle hooks", () => { + const harness = createSharedPgTaskStoreTestHarness({ prefix: "fusion_moves_review_set" }); + + beforeAll(harness.beforeAll); + afterAll(harness.afterAll); + beforeEach(async () => { await harness.beforeEach(); }); + afterEach(async () => { await harness.afterEach(); }); + + it("runs the in-review enter effects for a humanReview-ONLY lane", async () => { + const store = harness.store(); + const definition = await store.createWorkflowDefinition({ + name: "human-review-only", + ir: { + version: "v2", + name: "human-review-only", + columns: [ + { id: "backlog", name: "Backlog", traits: [{ trait: "intake" }, { trait: "hold" }] }, + { id: "building", name: "Building", traits: [{ trait: "wip" }] }, + /* No `merge` trait: review is hosted by human-review alone. */ + { 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); + + const task = await store.createTask({ description: "human-review card", workflowId: definition.id } as never); + await store.moveTask(task.id, "building" as never, { bypassGuards: true } as never); + + /* + The witness: `applyInReviewEnterEffects` clears `recoveryRetryCount`. Seed it so its absence after + the move can only mean the enter-effects ran. + */ + await store.updateTask(task.id, { recoveryRetryCount: 3 } as never); + store.taskCache.delete(task.id); + expect((await store.getTask(task.id)).recoveryRetryCount).toBe(3); + + await store.moveTask(task.id, "signoff" as never, { bypassGuards: true } as never); + + store.taskCache.delete(task.id); + const moved = await store.getTask(task.id); + expect(moved.column).toBe("signoff"); + expect(moved.recoveryRetryCount).toBeUndefined(); + }); + + it("does NOT run them entering a lane that carries no review trait", async () => { + /* The negative half: widening the set must not make every column a review lane. */ + const store = harness.store(); + const definition = await store.createWorkflowDefinition({ + name: "human-review-only-neg", + ir: { + version: "v2", + name: "human-review-only-neg", + 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); + + const task = await store.createTask({ description: "non-review move", workflowId: definition.id } as never); + await store.updateTask(task.id, { recoveryRetryCount: 3 } as never); + + await store.moveTask(task.id, "building" as never, { bypassGuards: true } as never); + + store.taskCache.delete(task.id); + expect((await store.getTask(task.id)).recoveryRetryCount).toBe(3); + }); +}); diff --git a/packages/core/src/__tests__/reopen-semantics-by-role.test.ts b/packages/core/src/__tests__/reopen-semantics-by-role.test.ts index 1e6a4b5dd6..b400a73ed6 100644 --- a/packages/core/src/__tests__/reopen-semantics-by-role.test.ts +++ b/packages/core/src/__tests__/reopen-semantics-by-role.test.ts @@ -30,7 +30,10 @@ import { __resetTraitRegistryForTests } from "../trait-registry.js"; import { registerBuiltinTraits } from "../builtin-traits.js"; import { __resetDefaultWorkflowHooksForTests, + applyCompletionTimingEffects, applyDefaultWorkflowMoveEffects, + applyInReviewEnterEffects, + applyTimingEffects, isReopenIntoPlanning, registerDefaultWorkflowHooks, type DefaultWorkflowMoveContext, @@ -241,3 +244,200 @@ describe("the store's reopen check and the hooks' cannot disagree", () => { ]); }); }); + +/* +FNXC:WorkflowResolvedColumns 2026-07-31-04:40 (fleet phase — the same safety argument, applied to TIMING): +The header of this file explains why the reopen predicates had to stop naming the default lineage: these +hooks run on the flag-ON path for EVERY workflow, because the trait registry resolves each hook by trait +id, not by workflow. The timing, completion and in-review hooks had the same defect and were not part of +that conversion. + +What it costs on a renamed board, none of it throwing: + - `applyTimingEffects` accrues `cumulativeActiveMs` while a card sits in the WIP lane. With the lane + named, the exit test never fires, so NO active time is accrued and every duration display — + `productivity-analytics.ts`, `task-timing.ts` — reads zero. + - `applyCompletionTimingEffects` never stamps `executionCompletedAt`, so a finished card looks + unfinished to anything reading that field. + - `applyInReviewEnterEffects` returns early, so the recovery counters it clears stay set. + +THE HOOKS ARE CALLED DIRECTLY here, not through `applyDefaultWorkflowMoveEffects`. I wrote it through the +dispatcher first and all three cases failed on the DEFAULT lineage too: the dispatcher resolves hooks by +TRAIT, and neither test IR declares the `timing` trait, so those hooks never ran at all. Going through the +dispatcher would have tested the trait registry's wiring, not this conversion — and would have looked like +a conversion bug. + +REVERT CHECK, measured (all three run): restoring the `in-progress` literals leaves `cumulativeActiveMs` +undefined on the renamed board; restoring the `done` literal leaves `executionCompletedAt` unset; +restoring the `in-review` literal leaves `recoveryRetryCount` at 3. Every case runs on BOTH lineages, and +the default one passes either way — which is the point of running it. +*/ +describe("timing, completion and in-review effects are keyed on ROLES", () => { + const LINEAGES = [ + { label: "default", ir: DEFAULT_IR, wip: "in-progress", review: "in-review", complete: "done" }, + { label: "renamed", ir: RENAMED_IR, wip: "building", review: "checking", complete: "shipped" }, + ] as const; + + it("accrues active time leaving the WIP lane on both lineages", () => { + for (const { label, ir, wip, review } of LINEAGES) { + const ctx = makeCtx(ir, wip, review, { + task: { + id: "FN-2", + column: review, + columnMovedAt: "2026-07-30T00:05:00.000Z", + executionStartedAt: "2026-07-30T00:00:00.000Z", + steps: [], + dependencies: [], + workflowStepResults: [], + } as unknown as Task, + }); + applyTimingEffects(ctx); + expect(ctx.task.cumulativeActiveMs, `${label} lineage accrued no active time`).toBe(5 * 60_000); + } + }); + + it("stamps executionCompletedAt on entry to the complete lane on both lineages", () => { + for (const { label, ir, review, complete } of LINEAGES) { + const ctx = makeCtx(ir, review, complete); + applyCompletionTimingEffects(ctx); + expect(ctx.task.executionCompletedAt, `${label} lineage did not stamp completion`).toBe( + ctx.task.columnMovedAt, + ); + } + }); + + it("clears the recovery counters on entry to the review lane on both lineages", () => { + for (const { label, ir, wip, review } of LINEAGES) { + const ctx = makeCtx(ir, wip, review, { + task: { + id: "FN-3", + column: review, + columnMovedAt: "2026-07-30T00:00:00.000Z", + recoveryRetryCount: 3, + steps: [], + dependencies: [], + workflowStepResults: [], + } as unknown as Task, + }); + applyInReviewEnterEffects(ctx); + expect(ctx.task.recoveryRetryCount, `${label} lineage kept its recovery counter`).toBeUndefined(); + } + }); +}); + +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-09:05 (PR #2734 review — greptile): +SECONDARY LANES. `LifecycleColumns` names one column per role by design (#2721 pinned that), so a +workflow declaring `countsTowardWip` on two columns had a second WIP lane the hooks did not recognise. + +The timing consequence is the concrete one: a move BETWEEN two WIP lanes looked like an exit from WIP +followed by a re-entry, so `cumulativeActiveMs` closed and reopened a segment the card never left. +*/ +describe("timing hooks honour EVERY lane carrying the role", () => { + const ctx = (fromColumn: string, toColumn: string, sets?: { wip?: readonly string[] }) => { + const task = { + id: "FN-1", + columnMovedAt: "2026-07-30T10:00:00.000Z", + executionStartedAt: "2026-07-30T09:00:00.000Z", + cumulativeActiveMs: 0, + } as never as { cumulativeActiveMs?: number }; + return { + task, + ctx: { + task, + fromColumn, + toColumn, + movedAt: "2026-07-30T10:00:00.000Z", + lifecycleColumns: { wip: "building", complete: "shipped" }, + lifecycleColumnSets: sets, + resetSteps: () => {}, + } as never, + }; + }; + + it("does NOT close the active segment when a card moves between two WIP lanes", () => { + const { task, ctx: c } = ctx("building", "building-two", { wip: ["building", "building-two"] }); + + applyTimingEffects(c); + + // Still inside WIP, so no segment was accrued on the way out. + expect(task.cumulativeActiveMs).toBe(0); + }); + + it("DOES close it when the card genuinely leaves every WIP lane", () => { + const { task, ctx: c } = ctx("building", "signoff", { wip: ["building", "building-two"] }); + + applyTimingEffects(c); + + expect(task.cumulativeActiveMs).toBeGreaterThan(0); + }); + + it("runs the review enter-effects for a humanReview-ONLY lane", () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-15:10 (PR #2734 review — greptile, on my own code): + The producer built this set from `mergeOrchestration` alone, so a workflow hosting review on a + `humanReview`- or `mergeBlocker`-only lane skipped `applyInReviewEnterEffects` entirely — the + recovery counters it clears stayed set for a card plainly in review. + + Fixed by using core's `resolveReviewColumns` (#2730) rather than a fifth inline union. That is the + BROAD set, which is right here: these hooks ASK the question and move nothing on the answer. A + caller that admits and then MOVES wants the narrow single lane — #2750 documents the split. + */ + const task = { + id: "FN-HR", + column: "signoff", + columnMovedAt: "2026-07-30T00:00:00.000Z", + recoveryRetryCount: 3, + steps: [], + dependencies: [], + workflowStepResults: [], + } as unknown as Task; + + applyInReviewEnterEffects({ + task, + fromColumn: "building", + toColumn: "signoff", + movedAt: "2026-07-30T00:00:00.000Z", + lifecycleColumns: { wip: "building" }, + lifecycleColumnSets: { review: ["signoff"] }, + resetSteps: () => {}, + } as never); + + expect(task.recoveryRetryCount).toBeUndefined(); + }); + + it("treats an EMPTY set as 'no lane carries this role', not as a missing answer", () => { + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-10:40 (PR #2734 review — greptile): + `lifecycleColumnSets` is populated only when the caller resolved an IR, so an empty array means the + board was READ and declares no WIP lane. The first version guarded on `length > 0` and fell back to + the singular id and then the legacy name, so a traitless column merely NAMED `in-progress` accrued + timing as though it were the WIP lane. + + Undefined means "could not read"; empty means "read, and the answer is none". Same distinction as + #2731's `?? {}` and #2733's refusal to invent a complete column — which I applied in both of those + and then got wrong here. + */ + const { task, ctx: c } = ctx("in-progress", "signoff", { wip: [] }); + /* + The singular role must be ABSENT for this to discriminate. With `lifecycleColumns.wip` set, the + buggy fallback lands on that name and answers false anyway — my first version of this case did + exactly that and passed with the defect in place. Only when the singular is absent does the bug + reach the LEGACY name and treat a traitless `in-progress` column as WIP. + */ + (c as unknown as { lifecycleColumns: Record }).lifecycleColumns = { complete: "shipped" }; + + applyTimingEffects(c); + + // No WIP lane exists, so leaving `in-progress` is not leaving WIP and nothing accrues. + expect(task.cumulativeActiveMs).toBe(0); + }); + + it("falls back to the singular role when no set is supplied", () => { + /* Additive: a caller that does not pass sets keeps exactly the previous behaviour. */ + const { task, ctx: c } = ctx("building", "signoff"); + + applyTimingEffects(c); + + expect(task.cumulativeActiveMs).toBeGreaterThan(0); + }); +}); diff --git a/packages/core/src/default-workflow-hooks.ts b/packages/core/src/default-workflow-hooks.ts index db54a88e53..17ec67cf95 100644 --- a/packages/core/src/default-workflow-hooks.ts +++ b/packages/core/src/default-workflow-hooks.ts @@ -56,8 +56,19 @@ export function evaluateMergeBlockerGuard( task: Pick, fromColumn: string, toColumn: string, + /* + FNXC:WorkflowResolvedColumns 2026-07-31-04:20 (fleet phase): + The moving task's resolved lifecycle columns, so the review -> complete crossing this guard fires on + is a ROLE crossing. OPTIONAL and last, matching `DefaultWorkflowMoveContext.lifecycleColumns`: absent, + the legacy ids answer, which is the same degraded contract `planningColumnsOf` and + `liveWorkColumnsOf` already use in this file. + */ + lifecycleColumns?: LifecycleColumns | undefined, ): GuardVerdict { - if (fromColumn === "in-review" && toColumn === "done") { + if ( + fromColumn === (lifecycleColumns?.review ?? "in-review") + && toColumn === (lifecycleColumns?.complete ?? "done") + ) { return getTaskMergeBlocker(task); } return undefined; @@ -116,10 +127,51 @@ export interface DefaultWorkflowMoveContext { * only in that no-basis case, never as a substitute for an absent role. */ lifecycleColumns?: LifecycleColumns | undefined; + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-09:05 (PR #2734 review — greptile): + THE SET-SHAPED COMPANION, because `LifecycleColumns` names ONE column per role by design — #2721 + pinned that as the contract, not a gap. A workflow may put `countsTowardWip` (or `complete`, or + `mergeOrchestration`) on several columns, and these hooks ask "is this card IN that role", which is + membership. + + Concretely for the timing hook: with a single id, a card moving between two WIP lanes looked like an + EXIT from WIP followed by a re-entry, so `cumulativeActiveMs` closed and reopened a segment the card + never left — and a card living only in the secondary lane accrued nothing at all. + + Optional and additive: absent, every read falls back to the singular struct and then the legacy id, + so nothing changes for the default lineage or for a caller that does not supply it. The caller in + `moves.ts` already holds the IR, so populating it costs no extra read. + */ + lifecycleColumnSets?: { wip?: readonly string[]; complete?: readonly string[]; review?: readonly string[] } | undefined; /** Reset all steps to pending + currentStep 0 (store owns the impl). */ resetSteps: () => void; } +/* +FNXC:WorkflowLifecycleColumns 2026-07-30-10:40 (PR #2734 review — greptile, and it is the same +distinction I had just applied twice elsewhere and then got wrong here): +AN EMPTY SET IS AN ANSWER, NOT AN ABSENT ONE. + +`lifecycleColumnSets` is supplied only when the caller resolved a workflow IR, so `set !== undefined` +already means "the board was read". An EMPTY array then says "no column carries this role" — and the +`length > 0` guard treated that as no-basis and fell back to the singular id, then to the legacy name. +The consequence: on a valid v2 workflow with no WIP lane, a traitless column happening to be NAMED +`in-progress` accrued timing as though it were one. + +Same shape as #2731 (`?? {}` for resolved-but-roleless flags) and #2733 (refuse rather than invent a +complete column). Undefined means "could not read"; empty means "read, and the answer is none". +*/ +/** Membership for a role: the resolved SET when the caller supplied one, else the singular id, else the legacy id. */ +function inRole( + column: string, + set: readonly string[] | undefined, + single: string | undefined, + legacy: string, +): boolean { + if (set !== undefined) return set.includes(column); + return column === (single ?? legacy); +} + // ── Field-mutation effects (applied in-lock, before commit) ─────────────────── // // These mirror the inline flag-off mutations in store.ts exactly. They run as @@ -145,7 +197,16 @@ export interface DefaultWorkflowMoveContext { */ export function applyTimingEffects(ctx: DefaultWorkflowMoveContext): void { const { task, fromColumn, toColumn } = ctx; - if (fromColumn === "in-progress" && toColumn !== "in-progress") { + /* + FNXC:WorkflowResolvedColumns 2026-07-31-04:20 (fleet phase): + `cumulativeActiveMs` accrues while a card sits in the WIP lane, so both halves of the segment + boundary must name that lane by ROLE. Keyed on one resolved value rather than two independent reads: + the exit test and the re-entry test have to agree about which column is WIP or a rename makes the + accounting count the same interval twice, or not at all. + */ + const isWip = (column: string) => + inRole(column, ctx.lifecycleColumnSets?.wip, ctx.lifecycleColumns?.wip, "in-progress"); + if (isWip(fromColumn) && !isWip(toColumn)) { const segmentStartMs = Date.parse(task.executionStartedAt ?? task.columnMovedAt ?? ctx.movedAt); const segmentEndMs = Date.parse(task.columnMovedAt ?? ctx.movedAt); const segmentDeltaMs = @@ -154,7 +215,7 @@ export function applyTimingEffects(ctx: DefaultWorkflowMoveContext): void { : 0; task.cumulativeActiveMs = Math.max(0, task.cumulativeActiveMs ?? 0) + segmentDeltaMs; } - if (toColumn === "in-progress") { + if (isWip(toColumn)) { task.cumulativeActiveMs ??= 0; if (!task.firstExecutionAt) task.firstExecutionAt = task.columnMovedAt; if (!task.executionStartedAt) task.executionStartedAt = task.columnMovedAt; @@ -165,7 +226,7 @@ export function applyTimingEffects(ctx: DefaultWorkflowMoveContext): void { /** Stamp `executionCompletedAt` on entry to a completion column. */ export function applyCompletionTimingEffects(ctx: DefaultWorkflowMoveContext): void { const { task, toColumn } = ctx; - if (toColumn === "done" && !task.executionCompletedAt) { + if (inRole(toColumn, ctx.lifecycleColumnSets?.complete, ctx.lifecycleColumns?.complete, "done") && !task.executionCompletedAt) { task.executionCompletedAt = task.columnMovedAt; } } @@ -298,7 +359,7 @@ export function isReopenIntoPlanning( * block in store.ts. */ export function applyInReviewEnterEffects(ctx: DefaultWorkflowMoveContext): void { const { task, toColumn } = ctx; - if (toColumn !== "in-review") return; + if (!inRole(toColumn, ctx.lifecycleColumnSets?.review, ctx.lifecycleColumns?.review, "in-review")) return; // Do not snapshot the global autoMerge setting here. Undefined means "follow // the live global setting"; only an explicit task value should stay sticky. task.recoveryRetryCount = undefined; diff --git a/packages/core/src/task-store/moves.ts b/packages/core/src/task-store/moves.ts index 195a43eb85..a7db9f6dd0 100644 --- a/packages/core/src/task-store/moves.ts +++ b/packages/core/src/task-store/moves.ts @@ -26,7 +26,7 @@ import { evaluateTransitionInvariants, } from "../workflow-transition-policy.js"; import {type DefaultWorkflowMoveContext, applyDefaultWorkflowMoveEffects, isReopenIntoPlanning} from "../default-workflow-hooks.js"; -import {resolveLifecycleColumns} from "../workflow-lifecycle-traits.js"; +import {columnsWithFlag, resolveLifecycleColumns, resolveReviewColumns} from "../workflow-lifecycle-traits.js"; import {makeTransitionRejection, makeTransitionPending} from "../transition-types.js"; import {writeTransitionPendingAsync, clearTransitionPendingAsync} from "./async-transition-pending.js"; import type {WorkflowIr} from "../workflow-ir-types.js"; @@ -862,6 +862,32 @@ export async function moveTaskInternalImpl(store: TaskStore, id: string, toColum names on every workflow, so a renamed board got no reopen effects at all. */ lifecycleColumns: moveLifecycleColumns, + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-09:05 (PR #2734 review — greptile): + The SET-shaped roles beside the singular ones. `workflowIr` is already in hand here, so these + cost three trait scans and no extra read — and they are what let the timing hook treat a move + BETWEEN two WIP lanes as staying in WIP rather than as an exit plus a re-entry. + */ + lifecycleColumnSets: workflowIr === undefined + ? undefined + : { + wip: columnsWithFlag(workflowIr, "countsTowardWip"), + complete: columnsWithFlag(workflowIr, "complete"), + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-15:10 (PR #2734 review — greptile, on my own + code): `mergeOrchestration` ALONE was the wrong set. A workflow may host review on a + `humanReview`- or `mergeBlocker`-only lane, and entering it then skipped + `applyInReviewEnterEffects` entirely — the enter-effects simply did not run for a card + plainly in review. + + `resolveReviewColumns` (core, merged in #2730) is the shared answer to exactly this + question, so this becomes the first consumer to use it instead of a fifth inline union. + Note it is the BROAD set — correct here, because these hooks ASK "is this card in a review + lane" and do not move anything on the answer. A caller that ADMITS and then MOVES wants the + narrow single lane instead; #2750 documents that split at the helper. + */ + review: resolveReviewColumns(workflowIr), + }, }; /* FNXC:WorkflowLifecycleColumns 2026-07-30-08:10: the store's own copy of the reopen diff --git a/scripts/lib/lifecycle-column-census-baseline.json b/scripts/lib/lifecycle-column-census-baseline.json index abcdd690da..01b438badf 100644 --- a/scripts/lib/lifecycle-column-census-baseline.json +++ b/scripts/lib/lifecycle-column-census-baseline.json @@ -9,7 +9,6 @@ "packages/dashboard/app/components/TaskContextMenu.tsx": 9, "packages/dashboard/src/github-tracking-reconciler.ts": 9, "packages/engine/src/notification/notification-service.ts": 9, - "packages/core/src/default-workflow-hooks.ts": 7, "packages/dashboard/app/components/Column.tsx": 7, "packages/core/src/live-agent-count.ts": 6, "packages/core/src/task-merge.ts": 6,