From 73338502e504fe4604c8741a0e5dc4d55f26bf15 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Wed, 29 Jul 2026 22:23:32 -0700 Subject: [PATCH] fix(test) + E2E: re-green main's lifecycle release leg, and prove the MERGED board + REVISE rework (#2634) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit **Second batch.** Three commits, no production code. `pnpm test:gate` green, `pnpm lint` clean, all three E2E suites together **3 files / 41 tests, exit 0**. ## 1. main's lifecycle E2E is RED right now — this fixes it Independently of my work, on a detached `origin/main`: **2 failed / 18 passed**. Scenarios 1 and 2 fail with `sweep.released` **empty**. **Cause:** `seedTask` relied on task creation's PROMPT.md, which is a bootstrap seed (`"# \n\n"`). FN-7648's `isUnplannedForExecution` reads that file for any card resting in an intake- **or** hold-trait column and refuses to move an unplanned card into a processing column. The sweep reported `held: [{ reason: "move-rejected-or-no-slot" }]`. **That is the gate working.** The fixture was asking the scheduler to release a card that had never been specified. The fix is the one the graph-entry contract doc already prescribes: *"Scheduler/release test fixtures must model a card that cleared the gate ... A held unreviewed card is the gate working."* `seedTask` now writes a planned PROMPT.md. **Verified it repairs main, not just this branch:** applying only that file to a detached `origin/main` leaves scenarios 1 and 2 **passing**, with the 4 residual failures being scenarios 3 and 6 — which need the fixture-options commit main does not have. ### I was wrong in #2627 and this corrects it In #2627 I named the in-transaction capacity gate (#2488/#2499) as the likely cause. **It was not.** Two hypotheses died, both recorded in the code comment so nobody re-runs them: | Hypothesis | Result | |---|---| | E2E settings lack `maxConcurrent` → capacity gate rejects the move | added `maxConcurrent`/`maxWorktrees` → **still 2 failed**. Not the cause. | | the move itself is refused | a direct `moveTask(id, wip)` → **succeeded**. Never the blocker. | Only then did probing the two release gates give `isTaskBlockedOnApproval=false`, `isUnplannedForExecution=true`, and dumping the file show the stub. I've flagged the wrong lead on #2627 too — a plausible-sounding cause pointed at another worker's PR is worse than no lead. ## 2. E2E evidence: the MERGED intake+hold board U11's shape — one column carrying intake **and** hold — had no end-to-end coverage; every prior E2E drove intake and hold as separate columns. - shared fixture gains opt-in `mergedIntakeAndHold`, plus `MERGED_VOCAB` (legacy ids, so a failure is attributable to the **role** merge alone) and `MERGED_RENAMED_VOCAB` (ids move too). - lifecycle scenario 3 drives the full spine: planning runs **in place** on the dual-role column, the real `runHoldReleaseSweep` releases **from** it, the graph runs to complete. - 4 merge-safeguard cases on the merged board (finalize, proofless refusal with the same reason, merged+renamed landing no legacy id, at-most-once). ## 3. E2E evidence: a REVISE routes back through rework The plan's `InReview → InProgress: review requests changes` had **no** live-engine evidence on any board — the fixture's review seam always succeeded. Two things the engine taught me, both corrected here: - the **IR validator refused** my rework edge: it is only legal into a node with `config.reworkRegion: true`. A real contract, and the validator catching it is the system working. `exec` now declares it (the shape the builtin uses on `merge-attempt`). - my first assertion was wrong. A REVISE does **not** leave the card in wip — rework re-enters `exec` within the same run, review approves on its second call, and the card finishes at complete. The evidence is the **seam sequence** `["planning","execute","review","execute","review","merge"]`, not an intermediate column the run has already passed. Asserting the final column alone would have been satisfied by a graph that ignored the REVISE entirely. ## Both families are mutation-attributed | Scenario | Mutation | Result | |---|---|---| | 3 — merged intake+hold | `isHeldTask` treats intake/hold as exclusive | **exactly its 2 tests** fail | | 6 — REVISE → rework | disable rework re-entry in `workflow-graph-executor` | **exactly its 2 tests** fail | Both fixture options are opt-in; the two pre-existing suites are behaviourally unchanged (27 → 29 → 41 passed across the additions, no existing assertion touched). ## Still not shipped: safeguard 2's graph E2E Attempted twice, deleted both times. Attempt 1 passed and then survived mutating `merge-gate` to ignore `task.autoMerge` — the card parked on the review column's `merge-blocker` trait, not the gate. Attempt 2 removed that trait to isolate the gate, and the **control** case parked too. Isolating it needs a merge path mirroring the builtin (`merge-gate → merge node → end`) rather than a direct edge to `end` — a real redesign, not a speculative edit. The enforcement that holds today is `allowInReviewMergeProcessing` in `project-engine` (unit-mutation verified, NEW=9; gated via #2526). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) --- .../u9-safeguard-baseline.md | 56 ++++++ .../__tests__/_workflow-vocabulary-fixture.ts | 64 ++++++- .../workflow-lifecycle-live-e2e.pg.test.ts | 181 +++++++++++++++++- ...kflow-merge-safeguards-live-e2e.pg.test.ts | 59 +++++- 4 files changed, 348 insertions(+), 12 deletions(-) diff --git a/docs/plans/workflow-owned-merge-stack/u9-safeguard-baseline.md b/docs/plans/workflow-owned-merge-stack/u9-safeguard-baseline.md index 754da3a217..d2fbb5b05d 100644 --- a/docs/plans/workflow-owned-merge-stack/u9-safeguard-baseline.md +++ b/docs/plans/workflow-owned-merge-stack/u9-safeguard-baseline.md @@ -116,6 +116,62 @@ same family of error this program is trying to stamp out. Harness: baseline run → assert patch applied (a no-op patch is an abort, not a pass) → mutated run → `comm -13` the sorted fail-sets → restore → assert clean. +## Why the merge-region nodes cannot be tested as nodes (and what U9 actually is) + +Three attempts to give safeguard 2 (`autoMerge:false` is terminal-until-human) a +graph-level E2E all failed, and the third failure found the reason. It is the single +most useful thing in this document for whoever lands the conversion. + +`workflow-graph-executor.ts:310` says it plainly: + +> Until the workflow interpreter owns merge policy end-to-end, graph execution treats +> any entry into this region as the terminal legacy `merge` seam so observable +> lifecycle behavior stays byte-identical with the legacy executor. + +`MERGE_REGION_KINDS` is `merge-gate`, `merge-attempt`, `manual-merge-hold`, +`retry-backoff`, `recovery-router`, `branch-group-member-integration`, +`branch-group-promotion`. **Entering any of them short-circuits to the legacy merge +seam, so none of their handlers ever runs.** Consequences worth stating separately, +because each one burned an attempt: + +- `createMergeGateHandler` (`merge-runner.ts:44`) DOES read + `task.autoMerge !== false && settings.autoMerge !== false` and emit + `auto-on`/`auto-off` — and is **never called by the graph**. A test that drives a + graph and expects the gate to park an `autoMerge:false` card will watch the card + complete, no matter how the IR is shaped. +- The `outcome:auto-on` / `outcome:auto-off` edges the builtin coding IR declares are + therefore unreachable in graph execution today. +- This is the mechanism behind the very first U9 finding + (`u9-merge-region-node-config-authority.test.ts`): the IR's merge-region config is + unread not because a handler ignores it, but because the executor bypasses the whole + region. + +**So U9's conversion, stated concretely, is: stop short-circuiting +`MERGE_REGION_KINDS` to the legacy seam and let those nodes run.** Everything else — +S06's capability extraction, S07's completion handoff, S08's queue processing — hangs +off that one behavioural change, and the safeguard table above is the contract it must +not break. Safeguard 2 in particular has NO node-level representation today, so a +conversion that simply enables the region without carrying the `autoMerge` contract +into it would let an `autoMerge:false` card merge on PR-readiness alone. + +Attempts, recorded so they are not repeated: +1. Fixture with an auto-off edge alongside its existing bare `success` edge — the + `autoMerge:false` card COMPLETED, because both edges match + `{ outcome: "success", value: "auto-off" }` and `end` won on target-id sort. (Real + IR-authoring hazard; the builtin routes `outcome:auto-on` explicitly for this + reason.) +2. Dropped `merge-blocker` from the review column to isolate the gate — broke the route + to `end`, so the CONTROL card parked too and the flag was still not the + discriminator. +3. Mirrored the builtin region (`merge-gate --auto-on--> merge-attempt --> end`, + `--auto-off--> manual-hold`) — control passed, `autoMerge:false` still completed. + That is what exposed the short-circuit: the nodes are not executing. + +Testing safeguard 2 at the node level requires a harness that does NOT substitute +merge-region nodes, which is a change to the executor's substitution boundary rather +than a test addition — i.e. it becomes possible as part of the conversion, not before +it. + ## What this baseline does not cover - **Reviewer-lane safeguards.** This is the merge lane only. The review nodes diff --git a/packages/engine/src/__tests__/_workflow-vocabulary-fixture.ts b/packages/engine/src/__tests__/_workflow-vocabulary-fixture.ts index 06b164cfcb..a225d331a8 100644 --- a/packages/engine/src/__tests__/_workflow-vocabulary-fixture.ts +++ b/packages/engine/src/__tests__/_workflow-vocabulary-fixture.ts @@ -53,8 +53,51 @@ export interface LifecycleIrOptions { suite's IR stays byte-identical to what it was written against — a shared fixture must not silently change an existing suite's subject. */ readonly mergeOrchestration?: boolean; + /* FNXC:MergedPlanningColumn 2026-07-29-23:50 (U9 E2E evidence — the merged board): + Adds the `intake` trait to the HOLD column, so ONE column carries intake + hold — + which is exactly the shape U11 shipped on the default lineage (Planning, id `todo`, + no `triage` column at all). Until now every E2E here drove a board with intake and + hold as SEPARATE columns, so nothing proved the merged shape end-to-end; a guard + that silently keys on "the column that is only a hold" passes on the default and + renamed vocabularies and goes wrong only here. + + OPT-IN for the same reason as `mergeOrchestration`: a shared fixture must not + silently change an existing suite's subject. */ + readonly mergedIntakeAndHold?: boolean; + /* FNXC:ReviewRework 2026-07-30-01:10 (U9 E2E evidence — the review half): + Adds the `review --failure--> exec` rework edge, so a REVISE verdict routes the + card BACK to the wip column. Without it the review node has only a success edge + and every E2E here drives review as a pass-through, which means the plan's + `InReview --> InProgress: review requests changes` transition had no live-engine + evidence on ANY board. + + Opt-in like the other options: this changes the graph's reachable shape, and a + shared fixture must not alter an existing suite's subject. */ + readonly reviewRework?: boolean; } +/** + * The MERGED board: intake and hold are one column, as on the operator's real default + * workflow after U11. Ids deliberately overlap the legacy enum (`todo` is genuinely the + * merged Planning id there) so this is not a rename test — it isolates the merge of two + * ROLES onto one column from any change of vocabulary. + */ +export const MERGED_VOCAB: Vocabulary = { + hold: "todo", + wip: "in-progress", + review: "in-review", + complete: "done", +}; + +/** A merged board that ALSO renames: both variables move at once, which is the shape a + * custom workflow author actually produces. */ +export const MERGED_RENAMED_VOCAB: Vocabulary = { + hold: "planning", + wip: "building", + review: "checking", + complete: "shipped", +}; + export function lifecycleIr(v: Vocabulary, id: string, options: LifecycleIrOptions = {}): WorkflowIr { return { version: "v2", @@ -64,7 +107,10 @@ export function lifecycleIr(v: Vocabulary, id: string, options: LifecycleIrOptio { id: v.hold, name: "Hold", - traits: [{ trait: "hold", config: { release: "capacity" } }], + traits: [ + ...(options.mergedIntakeAndHold ? [{ trait: "intake" }] : []), + { trait: "hold", config: { release: "capacity" } }, + ], /* U4 workflow-declared recovery policy (#2478). Declared on the HOLD column of both vocabularies from the one builder, so the reconciler's role resolution is exercised against a renamed column with nothing else differing. */ @@ -89,7 +135,18 @@ export function lifecycleIr(v: Vocabulary, id: string, options: LifecycleIrOptio nodes: [ { id: "start", kind: "start", column: v.hold }, { id: "plan", kind: "prompt", column: v.hold, config: { seam: "planning" } }, - { id: "exec", kind: "prompt", column: v.wip, config: { seam: "execute" } }, + { + id: "exec", + kind: "prompt", + column: v.wip, + /* `reworkRegion` is required by the IR validator for any rework-edge TARGET + ("only legal ... into a top-level rework region head") — the same shape the + builtin coding IR uses on `merge-attempt`. Declared only when the rework edge + exists, so the non-rework IR stays byte-identical. */ + config: options.reviewRework + ? { seam: "execute", reworkRegion: true, maxReworkCycles: 3 } + : { seam: "execute" }, + }, { id: "review", kind: "prompt", column: v.review, config: { seam: "review" } }, /* A real merge-class node. The IR validator REFUSES a `merge-blocker` column with no reachable merge-class node ("the gate can never clear without one") — discovered by this @@ -105,6 +162,9 @@ export function lifecycleIr(v: Vocabulary, id: string, options: LifecycleIrOptio { from: "exec", to: "review", condition: "success" }, { from: "review", to: "merge-gate", condition: "success" }, { from: "merge-gate", to: "end", condition: "success" }, + ...(options.reviewRework + ? [{ from: "review", to: "exec", condition: "failure", kind: "rework" }] + : []), ], } as WorkflowIr; } diff --git a/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts b/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts index f844909cac..dfa896c66a 100644 --- a/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts +++ b/packages/engine/src/__tests__/workflow-lifecycle-live-e2e.pg.test.ts @@ -52,7 +52,7 @@ import { import { WorkflowGraphTaskRunner, type WorkflowColumnBoundaryHooks } from "../workflow-graph-task-runner.js"; import { createExecutorColumnBoundaryHooks } from "../workflow-column-boundary-hooks.js"; import { runHoldReleaseSweep } from "../hold-release.js"; -import { DEFAULT_VOCAB, RENAMED_VOCAB, HOLD_STALENESS_MS, lifecycleIr, type Vocabulary } from "./_workflow-vocabulary-fixture.js"; +import { DEFAULT_VOCAB, RENAMED_VOCAB, MERGED_VOCAB, MERGED_RENAMED_VOCAB, HOLD_STALENESS_MS, lifecycleIr, type Vocabulary } from "./_workflow-vocabulary-fixture.js"; import { SelfHealingManager } from "../self-healing.js"; import { reconcileRecovery } from "../recovery-reconciler.js"; @@ -67,6 +67,25 @@ interface SeamLog { ANY entry into the merge region (merge-gate included) onto the legacy `merge` seam, so the walk below genuinely reaches the merge lane before the terminal column. Scripting it is the same substitution `testMode` makes; the column move that follows is real. */ +/* +FNXC:ReviewRework 2026-07-30-01:20 (U9 E2E evidence — the review half): +Seams whose REVIEW fails its first call and succeeds afterwards, so the graph takes +the `review --failure--> exec` rework edge exactly once. `OK` everywhere else, so the +only behavioural difference from `scriptedSeams` is the verdict. +*/ +function revisingSeams(log: SeamLog) { + const base = scriptedSeams(log); + let reviewCalls = 0; + return { + ...base, + review: async () => { + log.calls.push("review"); + reviewCalls += 1; + return reviewCalls === 1 ? { outcome: "failure" as const, value: "REVISE" } : OK; + }, + }; +} + function scriptedSeams(log: SeamLog) { const seam = (name: string) => async () => { log.calls.push(name); @@ -101,8 +120,8 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { * `createWorkflowDefinition` allocates its own `WF-###` and IGNORES the `id` in the input — * binding a task to the id we passed in silently resolves to the DEFAULT builtin IR, which is * exactly how a renamed-workflow fixture can pass while testing nothing. */ - async function seedWorkflow(v: Vocabulary, key: string): Promise<{ workflowId: string; ir: WorkflowIr }> { - const ir = lifecycleIr(v, `custom:${key}`); + async function seedWorkflow(v: Vocabulary, key: string, merged = false, reviewRework = false): Promise<{ workflowId: string; ir: WorkflowIr }> { + const ir = lifecycleIr(v, `custom:${key}`, { mergedIntakeAndHold: merged, reviewRework }); const created = await h.store().createWorkflowDefinition({ name: `Lifecycle ${key}`, kind: "workflow", @@ -119,6 +138,36 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { { taskId, applyDefaultWorkflowSteps: false } as never, ); await store.writeTaskWorkflowSelection(taskId, workflowId, []); + /* + FNXC:WorkflowLifecycleColumns 2026-07-30-04:10 (release-leg fixture fix): + The card needs a PLANNED PROMPT.md before the sweep will release it. Task creation + leaves a bootstrap seed ("# \n\n"), and FN-7648's + `isUnplannedForExecution` reads that file for any card resting in an intake- OR + hold-trait column and refuses to move an unplanned card into a processing column — + so the sweep reported `held: [{ reason: "move-rejected-or-no-slot" }]` and released + nothing, which is the gate WORKING, not a scheduler defect. + + Diagnosed rather than guessed, and two hypotheses died on the way: adding + `maxConcurrent`/`maxWorktrees` to the E2E settings changed nothing (so the + in-transaction capacity gate from #2488/#2499 was NOT the cause), and a direct + `moveTask(id, wip)` succeeded (so the move itself was never the blocker). Probing the + two release gates showed `isTaskBlockedOnApproval=false`, + `isUnplannedForExecution=true`, and dumping the file showed the stub. + + This is the fixture modelling a card that cleared specification, which + `docs/solutions/architecture-patterns/workflow-node-column-placement-and-graph-entry-contract.md` + states outright: "Scheduler/release test fixtures must model a card that cleared the + gate ... A held unreviewed card is the gate working." + */ + const { writeFileSync, mkdirSync } = await import("node:fs"); + const { join } = await import("node:path"); + const dir = join(store.getTasksDir(), taskId); + mkdirSync(dir, { recursive: true }); + writeFileSync( + join(dir, "PROMPT.md"), + `# ${taskId}\n\n## Context\nA planned spec, so the release sweep does not classify this card as an unplanned seed.\n\n## Steps\n### Step 1\n- [ ] do the planned work\n`, + "utf-8", + ); store.taskCache.delete(taskId); return task as Task; } @@ -165,7 +214,13 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { }); } - function makeRunner(taskId: string, workflowId: string, log: SeamLog, moveMarks: string[] = []) { + function makeRunner( + taskId: string, + workflowId: string, + log: SeamLog, + moveMarks: string[] = [], + seamsFactory?: (l: SeamLog) => unknown, + ) { const store = h.store(); const runId = `${taskId}:workflow`; return new WorkflowGraphTaskRunner({ @@ -176,7 +231,7 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { getTask: (id: string) => store.getTask(id), }, runId, - seams: scriptedSeams(log) as never, + seams: (seamsFactory ?? scriptedSeams)(log) as never, runCustomNode: async () => { throw new Error("no custom node should run in this lifecycle shape"); }, @@ -195,8 +250,8 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { * The full lifecycle, driven for one vocabulary. Returns everything observed so the two * vocabularies can be compared field-for-field rather than eyeballed. */ - async function driveLifecycle(taskId: string, v: Vocabulary, key: string) { - const { workflowId } = await seedWorkflow(v, key); + async function driveLifecycle(taskId: string, v: Vocabulary, key: string, merged = false) { + const { workflowId } = await seedWorkflow(v, key, merged); await seedTask(taskId, v, workflowId); const events: WorkflowLifecycleEvent[] = []; @@ -305,6 +360,47 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { }); }); + /* + FNXC:MergedPlanningColumn 2026-07-30-00:20 (U9 E2E evidence — the merged board): + Scenarios 1 and 2 both drive a board where intake and hold are SEPARATE columns. + The operator's real default workflow no longer looks like that: U11 merged them into + one Planning column. The release path is where that matters — `runHoldReleaseSweep` + has to release a card FROM a column that is simultaneously the intake column, and a + resolver that treats "intake" and "hold" as mutually exclusive roles would leave the + card parked forever with no error. + + Two variants: MERGED_VOCAB keeps legacy ids so a failure is attributable to the ROLE + merge alone, and MERGED_RENAMED_VOCAB moves ids too, which is what a custom workflow + author actually produces. + */ + describe("scenario 3 — MERGED intake+hold column (U11's shape)", () => { + it("releases and completes a card whose hold column is ALSO the intake column", async () => { + const r = await driveLifecycle("FN-E2E-MERGED", MERGED_VOCAB, "merged-vocab", true); + + expect(r.columnsObserved.atCreate).toBe(MERGED_VOCAB.hold); + // Planning runs IN PLACE on the merged column — it must not be treated as a + // pre-intake staging lane the card has to leave first. + expect(r.columnsObserved.afterPlanning).toBe(MERGED_VOCAB.hold); + // The claim this scenario exists for: capacity release works from a dual-role column. + expect(r.sweep.released).toContain("FN-E2E-MERGED"); + expect(r.columnsObserved.afterRelease).toBe(MERGED_VOCAB.wip); + expect(r.columnsObserved.afterRun).toBe(MERGED_VOCAB.complete); + expect(r.seamCalls).toEqual(["planning", "execute", "review", "merge"]); + }); + + it("does the same on a board that is BOTH merged and renamed", async () => { + const r = await driveLifecycle("FN-E2E-MR", MERGED_RENAMED_VOCAB, "merged-renamed", true); + + expect(r.columnsObserved.atCreate).toBe(MERGED_RENAMED_VOCAB.hold); + expect(r.sweep.released).toContain("FN-E2E-MR"); + expect(r.columnsObserved.afterRelease).toBe(MERGED_RENAMED_VOCAB.wip); + expect(r.columnsObserved.afterRun).toBe(MERGED_RENAMED_VOCAB.complete); + // No leg may touch a legacy id on this board. + const legacy = new Set(["triage", "todo", "in-progress", "in-review", "done", "archived"]); + for (const observed of Object.values(r.columnsObserved)) expect(legacy.has(observed)).toBe(false); + }); + }); + describe("scenario 4 — the post-commit event seam under a real move", () => { it("delivers a well-formed TaskTransitioned to a real subscriber for a RENAMED move", async () => { const r = await driveLifecycle("FN-E2E-5", RENAMED_VOCAB, "events-renamed"); @@ -657,6 +753,77 @@ pgDescribe("live lifecycle E2E: real graph + real PostgreSQL store", () => { } }); }); + /* + FNXC:ReviewRework 2026-07-30-01:30 (U9 E2E evidence — the review half): + The plan's target lifecycle includes `InReview --> InProgress: review requests + changes`, and until now NO board proved it against a live engine: the fixture's + review seam always succeeded, so every E2E drove review as a pass-through. + + This drives a real REVISE. The graph takes the `review --failure--> exec` rework + edge, which is a BACKWARD column move (review -> wip) through the real boundary + controller and the real `store.moveTask` — the move class most likely to be refused + by an adjacency or trait guard, and the one a rework loop cannot work without. + + Run on the merged board as well as the renamed one: rework re-enters the wip column, + whose trait set differs on a merged board, so a guard that resolves the rework target + by elimination ("the column that is not intake and not review") behaves differently + there. + */ + describe("scenario 6 — a REVISE verdict routes the card back to wip", () => { + async function driveRevise(taskId: string, v: Vocabulary, key: string, merged: boolean) { + const { workflowId } = await seedWorkflow(v, key, merged, true); + await seedTask(taskId, v, workflowId); + const log: SeamLog = { calls: [] }; + const marks: string[] = []; + + // Leg 1: plan in the hold column, park at the capacity boundary. + await makeRunner(taskId, workflowId, log, marks, revisingSeams).run(await detail(taskId), settings); + // Leg 2: the real sweep releases into wip. + await runHoldReleaseSweep(h.store(), { now: () => Date.now() }); + const afterRelease = await persistedColumn(taskId); + // Leg 3: execute -> review -> REVISE -> rework back to exec. + const items = await h.store().listWorkflowWorkItemsForTask(taskId, { kinds: ["task"] }); + const resumeNode = items.find((i) => ["held", "runnable", "running"].includes(i.state))?.nodeId; + const leg3 = await makeRunner(taskId, workflowId, log, marks, revisingSeams) + .run(await detail(taskId), settings, resumeNode); + return { afterRelease, afterRevise: await persistedColumn(taskId), calls: log.calls, leg3 }; + } + + /* + MEASURED, and it corrected the assertion I first wrote. A REVISE does not leave the + card resting in wip: the rework edge re-enters `exec` WITHIN THE SAME run, review is + called again, approves, and the card finishes at complete. So the observable proof + that rework happened is the SEAM SEQUENCE — execute appears twice, the second time + after a review — not an intermediate column, which the run has already moved past by + the time the leg returns. + + Asserting the final column alone would have been satisfied by a graph that ignored + the REVISE entirely and went straight to merge, which is exactly the failure this + scenario is for. + */ + it("re-enters exec on a REVISE and only completes after the second review (renamed board)", async () => { + const r = await driveRevise("FN-E2E-REV", RENAMED_VOCAB, "revise-renamed", false); + + expect(r.afterRelease).toBe(RENAMED_VOCAB.wip); + // The rework edge was traversed: execute ran a SECOND time, after a review. + expect(r.calls).toEqual(["planning", "execute", "review", "execute", "review", "merge"]); + // And the loop resolved rather than spinning — the card reached complete. + expect(r.afterRevise).toBe(RENAMED_VOCAB.complete); + }); + + it("does the same on a MERGED board, without bouncing to the dual-role column", async () => { + const r = await driveRevise("FN-E2E-REV-M", MERGED_VOCAB, "revise-merged", true); + + expect(r.afterRelease).toBe(MERGED_VOCAB.wip); + expect(r.calls).toEqual(["planning", "execute", "review", "execute", "review", "merge"]); + expect(r.afterRevise).toBe(MERGED_VOCAB.complete); + // Rework must re-enter wip, not the dual-role Planning column: on a merged board + // intake and hold share an id, so an elimination-based target resolution lands + // there. A second "planning" call in the sequence above would reveal that. + expect(r.calls.filter((c) => c === "planning")).toHaveLength(1); + }); + }); + }); /* diff --git a/packages/engine/src/__tests__/workflow-merge-safeguards-live-e2e.pg.test.ts b/packages/engine/src/__tests__/workflow-merge-safeguards-live-e2e.pg.test.ts index f2f92570dd..f6c6f4d61a 100644 --- a/packages/engine/src/__tests__/workflow-merge-safeguards-live-e2e.pg.test.ts +++ b/packages/engine/src/__tests__/workflow-merge-safeguards-live-e2e.pg.test.ts @@ -41,7 +41,7 @@ import { type SharedPgTaskStoreHarness, } from "../../../core/src/__test-utils__/pg-test-harness.js"; import { finalizeProvenAutoMergeTask } from "../auto-merge-finalization.js"; -import { DEFAULT_VOCAB, RENAMED_VOCAB, lifecycleIr, type Vocabulary } from "./_workflow-vocabulary-fixture.js"; +import { DEFAULT_VOCAB, RENAMED_VOCAB, MERGED_VOCAB, MERGED_RENAMED_VOCAB, lifecycleIr, type Vocabulary } from "./_workflow-vocabulary-fixture.js"; pgDescribe("live merge safeguards E2E: real store, real refusals", () => { const h: SharedPgTaskStoreHarness = createSharedPgTaskStoreTestHarness({ @@ -55,11 +55,11 @@ pgDescribe("live merge safeguards E2E: real store, real refusals", () => { /** The store allocates its own `WF-###` and ignores the id in the input; binding to * the one we passed would silently resolve to the DEFAULT builtin IR instead. */ - async function seedWorkflow(v: Vocabulary, key: string): Promise { + async function seedWorkflow(v: Vocabulary, key: string, merged = false): Promise { const created = await h.store().createWorkflowDefinition({ name: `Merge safeguards ${key}`, kind: "workflow", - ir: lifecycleIr(v, `custom:${key}`, { mergeOrchestration: true }), + ir: lifecycleIr(v, `custom:${key}`, { mergeOrchestration: true, mergedIntakeAndHold: merged }), } as never); return (created as { id: string }).id; } @@ -257,4 +257,57 @@ pgDescribe("live merge safeguards E2E: real store, real refusals", () => { expect(column).toBe(RENAMED_VOCAB.review); expect(["todo", "in-progress", "in-review", "done", "archived"]).not.toContain(column); }); + /* ── The MERGED board — U11's actual shape ──────────────────────────────────── + Every case above drives a board where intake and hold are SEPARATE columns. The + operator's real default workflow no longer looks like that: U11 merged them into + one Planning column (id `todo`, no `triage` at all). A guard that keys on "the + column that is only a hold" passes on the default AND renamed vocabularies and + goes wrong only here, which is precisely why this needed its own family rather + than another row in an existing one. + + Two variants on purpose: MERGED_VOCAB keeps the legacy ids (so a failure is + attributable to the ROLE merge alone), MERGED_RENAMED_VOCAB moves both variables + at once (the shape a custom workflow author actually produces). */ + it("finalizes a proven-merged card on a MERGED intake+hold board", async () => { + const wf = await seedWorkflow(MERGED_VOCAB, "merged", true); + await seedProvenMergedTask("FN-SG-MERGED", MERGED_VOCAB, wf); + + const result = await finalize("FN-SG-MERGED"); + + expect(result.outcome).toBe("done"); + expect(await persistedColumn("FN-SG-MERGED")).toBe(MERGED_VOCAB.complete); + }); + + it("refuses a proofless card on a MERGED board, with the same reason", async () => { + const wf = await seedWorkflow(MERGED_VOCAB, "merged-noproof", true); + await seedProvenMergedTask("FN-SG-MERGED-NOPROOF", MERGED_VOCAB, wf, { mergeDetails: null }); + + const result = await finalize("FN-SG-MERGED-NOPROOF"); + + expect({ outcome: result.outcome, reason: (result as { reason?: string }).reason }) + .toEqual({ outcome: "blocked", reason: "missing-merge-confirmation" }); + expect(await persistedColumn("FN-SG-MERGED-NOPROOF")).toBe(MERGED_VOCAB.review); + }); + + it("finalizes on a board that is BOTH merged and renamed, landing no legacy id", async () => { + const wf = await seedWorkflow(MERGED_RENAMED_VOCAB, "merged-renamed", true); + await seedProvenMergedTask("FN-SG-MR", MERGED_RENAMED_VOCAB, wf); + + const result = await finalize("FN-SG-MR"); + + expect(result.outcome).toBe("done"); + const column = await persistedColumn("FN-SG-MR"); + expect(column).toBe(MERGED_RENAMED_VOCAB.complete); + expect(["todo", "triage", "in-progress", "in-review", "done", "archived"]).not.toContain(column); + }); + + it("at-most-once holds on a merged+renamed board too", async () => { + const wf = await seedWorkflow(MERGED_RENAMED_VOCAB, "merged-once", true); + await seedProvenMergedTask("FN-SG-MR-ONCE", MERGED_RENAMED_VOCAB, wf); + + expect((await finalize("FN-SG-MR-ONCE")).outcome).toBe("done"); + expect((await finalize("FN-SG-MR-ONCE")).outcome).toBe("already-done"); + expect(await persistedColumn("FN-SG-MR-ONCE")).toBe(MERGED_RENAMED_VOCAB.complete); + }); + });