From ab8ecb2206e7c5cb442201bc2aa4a16be44b16d6 Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Thu, 18 Jun 2026 06:41:22 -0700 Subject: [PATCH] FN-6634: remove engine merge trait hook registration Remove the engine-side merge trait hook registration so core remains the sole owner of in-review field effects. - Stop exporting or registering engine merge-trait onEnter/onExit hooks that collide with core workflow move effects. - Document the store-owned merge queue handoff and core-owned scheduler-state clearing contract. - Update merge-trait tests to assert ctx-shaped hook invocation and in-review scheduler state cleanup. - Add a patch changeset for the published CLI package. Files changed: .changeset/fn-6634-merge-trait-hook-collision.md | 5 + packages/engine/src/__tests__/merge-trait.test.ts | 103 +++++++++------- packages/engine/src/index.ts | 1 - packages/engine/src/merge-trait.ts | 138 +++++++--------------- 4 files changed, 107 insertions(+), 140 deletions(-) Fusion-Task-Id: FN-6634 Fusion-Task-Lineage: fe7e2259-e354-4457-afd3-7ff25e5a1504 --- .../fn-6634-merge-trait-hook-collision.md | 5 + .../engine/src/__tests__/merge-trait.test.ts | 103 +++++++------ packages/engine/src/index.ts | 1 - packages/engine/src/merge-trait.ts | 136 +++++------------- 4 files changed, 106 insertions(+), 139 deletions(-) create mode 100644 .changeset/fn-6634-merge-trait-hook-collision.md diff --git a/.changeset/fn-6634-merge-trait-hook-collision.md b/.changeset/fn-6634-merge-trait-hook-collision.md new file mode 100644 index 0000000000..acacf101d6 --- /dev/null +++ b/.changeset/fn-6634-merge-trait-hook-collision.md @@ -0,0 +1,5 @@ +--- +"@runfusion/fusion": patch +--- + +Stop the engine from registering merge-trait hooks that collided with core's in-review field-effects adapter and could crash workflow-column moves. diff --git a/packages/engine/src/__tests__/merge-trait.test.ts b/packages/engine/src/__tests__/merge-trait.test.ts index 80fa0741c9..8df120f7a9 100644 --- a/packages/engine/src/__tests__/merge-trait.test.ts +++ b/packages/engine/src/__tests__/merge-trait.test.ts @@ -40,11 +40,7 @@ import { type WorkflowIr, } from "@fusion/core"; -import { - resolveMergePolicy, - registerMergeTraitHooks, - __resetMergeTraitRegistrationForTests, -} from "../merge-trait.js"; +import { resolveMergePolicy } from "../merge-trait.js"; import { assertSquashOverlapsFileScope, enforceSquashFileScopeInvariant, @@ -489,52 +485,77 @@ describe("lost-work guard trio is non-configurable (KTD-6 regression)", () => { }); }); -// ── 4. merge trait hooks: enqueue (onEnter) drives queue, never inline ─────── - -describe("merge trait hooks — enqueue-only, queue-driven", () => { - beforeEach(() => { - __resetMergeTraitRegistrationForTests(); - registerMergeTraitHooks(); - }); - - it("registers real onEnter/onExit impls in the registry (not degraded no-ops)", () => { - const onEnter = getTraitRegistry().resolveTraitHook("merge", "onEnter"); - const onExit = getTraitRegistry().resolveTraitHook("merge", "onExit"); - expect(onEnter.impl).toBeDefined(); - expect(onEnter.warning).toBeUndefined(); // a real impl is registered - expect(onExit.impl).toBeDefined(); - expect(onExit.warning).toBeUndefined(); - }); - - it("onEnter enqueues onto the persisted merge queue and never awaits a merge", async () => { +// ── 4. merge.onEnter is the core field-effects hook, invoked as impl(ctx) ───── +// +// Regression for the unhandled rejection in the hold-release sweep +// (TypeError: Cannot read properties of undefined (reading 'id')). The engine +// used to register a `mergeOnEnter(store, task)` impl here; the ONLY caller +// (`applyDefaultWorkflowMoveEffects`) invokes the hook as `impl(ctx)` with a +// single `DefaultWorkflowMoveContext` (no store handle), so `task` bound to +// `undefined` and `task.id` threw. The merge hook is now owned by core +// (`applyInReviewEnterEffects`, registered via `registerDefaultWorkflowHooks`); +// the enqueue is store-owned on the handoff path. Importing this engine module +// must NOT clobber that registration. +describe("merge.onEnter — core field-effects hook (no engine clobber)", () => { + it("resolves to a real impl that is safe to invoke as impl(ctx) (single arg)", async () => { const fx = await makeStoreFixture(); try { - const onEnter = getTraitRegistry().resolveTraitHook("merge", "onEnter").impl as ( - s: TaskStore, - t: { id: string; priority?: string }, - ) => Promise; + const onEnter = getTraitRegistry().resolveTraitHook("merge", "onEnter"); + expect(onEnter.impl).toBeDefined(); + expect(onEnter.warning).toBeUndefined(); // a real impl, not a degraded no-op + + // The contract the bug violated: the ONLY caller invokes the hook with a + // single `DefaultWorkflowMoveContext` (no store, no second `task` arg). A + // `(store, task)`-shaped impl would deref `undefined.id` and throw here. + // Order-independent: this guards the signature regardless of which module + // registered last. const task = await fx.store.getTask(fx.taskId); - await onEnter(fx.store, { id: task.id, priority: task.priority }); - // Exactly one queue entry; the merge itself is NOT performed by the hook. - expect(fx.peekQueue(fx.taskId)).toBeTruthy(); - const after = await fx.store.getTask(fx.taskId); - expect(after.column).toBe("in-review"); // hook did not move the card + const ctx = { + task, + fromColumn: "in-progress", + toColumn: "in-review", + moveSource: "scheduler", + bypassGuards: false, + movedAt: new Date().toISOString(), + settings: undefined, + options: {}, + resetSteps: () => {}, + }; + expect(() => (onEnter.impl as (c: unknown) => void)(ctx)).not.toThrow(); } finally { await fx.cleanup(); } }); - it("onEnter is idempotent: re-running (crash-replay) holds exactly one entry", async () => { + it("a flag-ON move into in-review does not throw and clears scheduler state", async () => { const fx = await makeStoreFixture(); try { - const onEnter = getTraitRegistry().resolveTraitHook("merge", "onEnter").impl as ( - s: TaskStore, - t: { id: string; priority?: string }, - ) => Promise; - const task = await fx.store.getTask(fx.taskId); - await onEnter(fx.store, { id: task.id, priority: task.priority }); - await onEnter(fx.store, { id: task.id, priority: task.priority }); - expect(fx.queueCount()).toBe(1); + // Fresh card in in-progress with scheduler dispatch state that MUST be + // cleared on review entry (else it permanently blocks the merge gate). + const created = await fx.store.createTask({ + title: "review-entry", + description: "x", + column: "in-progress", + branch: "fusion/fn-review-entry", + baseBranch: "main", + steps: [], + status: "queued", + blockedBy: "SOME-OTHER", + overlapBlockedBy: "SOME-OTHER", + } as never); + + // The exact wiring that crashed: moveTask → moveTaskInternal → + // applyDefaultWorkflowMoveEffects → merge.onEnter, invoked as impl(ctx). + const moved = await fx.store.moveTask(created.id, "in-review", { + moveSource: "scheduler", + allowDirectInReviewMove: true, + } as never); + + expect(moved.column).toBe("in-review"); + // applyInReviewEnterEffects ran: scheduler dispatch state is cleared. + expect(moved.status).toBeUndefined(); + expect(moved.blockedBy).toBeUndefined(); + expect(moved.overlapBlockedBy).toBeUndefined(); } finally { await fx.cleanup(); } diff --git a/packages/engine/src/index.ts b/packages/engine/src/index.ts index 9593491ebc..7055ea0ff3 100644 --- a/packages/engine/src/index.ts +++ b/packages/engine/src/index.ts @@ -184,7 +184,6 @@ export { type AutostashHandle, } from "./merger.js"; export { - registerMergeTraitHooks, resolveMergePolicy, type ResolvedMergePolicy, type MergeFileScopeMode, diff --git a/packages/engine/src/merge-trait.ts b/packages/engine/src/merge-trait.ts index a300d64590..ddfb2bfe1d 100644 --- a/packages/engine/src/merge-trait.ts +++ b/packages/engine/src/merge-trait.ts @@ -3,32 +3,13 @@ * * The merge trait turns merge/PR orchestration, merge strategy, squash posture * and file-scope enforcement mode into *configuration* over the substrate merge - * capability (KTD-6). This module owns two things: - * - * 1. The merge trait's hook implementations, registered into core's trait - * registry via the `registerTraitHookImpl` DI seam (mirrors - * `setCreateFnAgent`): - * - `onEnter` → enqueue the task onto the *persisted* merge-request - * queue (reuse the store's existing enqueue path). It NEVER awaits a - * merge inline; completion is driven by the merge-queue worker loop - * (`ProjectEngine.pickNextMergeTaskId` → `aiMergeTask` → - * `store.moveTask(id, "done")`) and resolved via the queue, so a - * graph walk / transition never blocks on a merge (the plan-002 - * deadlock hazard). - * - `onExit` → leaving the merge column dequeues a pending request. - * The store already performs this in-lock inside `moveTaskInternal` - * (`dequeueMergeQueueOnColumnExit`, a private method); the hook - * delegates to that existing mechanism rather than reimplementing the - * dequeue (see the onExit impl note). It is registered so the registry - * resolves a real impl (not a degraded no-op + audit warning). - * - * 2. `resolveMergePolicy` — a small read-through resolver consulted by - * `merger.ts` at its existing policy-knob read sites. When the - * `workflowColumns` flag is ON it reads the merge-trait config from the - * task's resolved workflow; otherwise (and when the workflow's merge - * trait carries no config, e.g. the built-in default workflow) it falls - * back to the existing settings knobs (`directMergeCommitStrategy`, - * `mergeStrategy`, scope settings) for back-compat. + * capability (KTD-6). This module owns `resolveMergePolicy` — a small + * read-through resolver consulted by `merger.ts` at its existing policy-knob + * read sites. When the `workflowColumns` flag is ON it reads the merge-trait + * config from the task's resolved workflow; otherwise (and when the workflow's + * merge trait carries no config, e.g. the built-in default workflow) it falls + * back to the existing settings knobs (`directMergeCommitStrategy`, + * `mergeStrategy`, scope settings) for back-compat. * * The three 2026-05-23 lost-work guards stay in `merger.ts` mechanics and are * UNREACHABLE from this config (KTD-6 / R10): sibling `fusion/fn-*` merge-target @@ -39,7 +20,6 @@ import { isWorkflowColumnsEnabled, - registerTraitHookImpl, resolveWorkflowIrForTask, type DirectMergeCommitStrategy, type Settings, @@ -48,7 +28,6 @@ import { type WorkflowIr, type WorkflowIrColumn, } from "@fusion/core"; -import { mergerLog } from "./logger.js"; // ── Resolved merge policy ──────────────────────────────────────────────────── @@ -189,73 +168,36 @@ export async function resolveMergePolicy( }; } -// ── Merge trait hook implementations (DI into core's trait registry) ───────── +// ── Merge trait hooks: owned by core, NOT this module ──────────────────────── +// +// The engine deliberately does NOT register `merge` onEnter/onExit impls. +// +// `merge.onEnter` is invoked by core's `applyDefaultWorkflowMoveEffects` as +// `impl(ctx)` with a single `DefaultWorkflowMoveContext` — an in-lock, +// pre-commit, in-memory field-mutation phase that carries NO store handle. Core +// registers the correct ctx-shaped impl (`applyInReviewEnterEffects`) via +// `registerDefaultWorkflowHooks()`; it clears the in-review scheduler state +// (`status: queued`, `blockedBy`, `overlapBlockedBy`) and mirrors the flag-OFF +// inline block in `store.ts`. +// +// An earlier version of this module registered a `mergeOnEnter(store, task)` +// impl here that enqueued onto the merge queue. That was wrong on three counts: +// 1. Signature mismatch — the only caller passes `ctx`, so `store`/`task` bound +// to `(ctx, undefined)` and dereferencing `task.id` threw at runtime +// (TypeError: cannot read 'id' of undefined) during the hold-release sweep. +// 2. Slot collision — it clobbered core's field-effects adapter on the +// last-write-wins registry, dropping the in-review state clears. +// 3. Redundant responsibility — the queue enqueue is in-txn and store-owned on +// the handoff path (`store.ts` enqueues on `fromHandoff`, shared by both +// flag states), and direct non-handoff entry into `in-review` is audited as +// a handoff-invariant violation. There is no sanctioned entry into the +// merge column that needs a hook-driven enqueue. +// +// `merge.onExit` similarly needs no impl: the store dequeues in-lock via the +// private `dequeueMergeQueueOnColumnExit` on every move (lease-aware), and the +// ctx move-effects path never invokes `merge.onExit` at all. -/** - * onEnter: enqueue the task onto the persisted merge-request queue. NEVER awaits - * a merge (KTD-6) — the merge-queue worker loop drives the actual merge and the - * subsequent move to the `complete`-flagged column. Delegates to the store's - * existing `enqueueMergeQueue` so the queue mechanics (audit, priority, - * idempotent ON CONFLICT insert) are not reimplemented. - * - * Idempotent: `enqueueMergeQueue` is `ON CONFLICT(taskId) DO NOTHING`, so a - * crash-then-rerun (recovery sweep replaying `transitionPending` hooks) holds - * exactly one queue entry. - * - * Invoked by the store's post-commit hook runner with `(store, task)`. - */ -async function mergeOnEnter(store: TaskStore, task: Pick): Promise { - try { - store.enqueueMergeQueue(task.id, { priority: task.priority }); - } catch (err) { - // Enqueue rejects (e.g. task not in the merge column) degrade to a no-op: - // the card is never stranded and the queue is never corrupted. The store - // already audits the rejection. - const message = err instanceof Error ? err.message : String(err); - mergerLog.warn(`merge enqueue skipped for task ${task.id}: ${message}`); - } -} - -/** - * onExit: leaving the merge column dequeues a pending (unleased) request. - * - * NOTE (design / delegation): the store ALREADY performs dequeue-on-column-exit - * in-lock inside `moveTaskInternal` via the private - * `dequeueMergeQueueOnColumnExit`, which runs unconditionally on every move and - * owns the lease-aware semantics (drop an unleased entry; audit a leased one as - * a stale-lease event). The merge trait's onExit therefore *delegates to that - * existing mechanism* — it does not reissue a dequeue (which would be a - * redundant second pass and could not see the lease columns without a store API - * change the prompt forbids). Registering the hook makes the registry resolve a - * real impl (not a degraded no-op + audit warning) and documents that the - * substrate, not the trait, owns the dequeue mechanic (KTD-6: traits configure - * and invoke capabilities; they never reimplement them). - */ -function mergeOnExit(): void { - // Intentional no-op: dequeue is owned by the store's in-lock - // `dequeueMergeQueueOnColumnExit` (see note above). -} - -let registered = false; - -/** - * Register the merge trait's hook implementations into core's shared trait - * registry. Idempotent (guarded), so importing this module (or calling it from - * engine startup) more than once is safe. Mirrors the `setCreateFnAgent` DI - * pattern: core declares the hook descriptors; the engine supplies the impls. - */ -export function registerMergeTraitHooks(): void { - if (registered) return; - registered = true; - registerTraitHookImpl("merge", "onEnter", mergeOnEnter as never); - registerTraitHookImpl("merge", "onExit", mergeOnExit as never); -} - -/** Test-only: re-arm registration so a fresh registry can be exercised. */ -export function __resetMergeTraitRegistrationForTests(): void { - registered = false; -} - -// Register on import (idempotent) so the engine's trait registry resolves real -// merge-hook impls without a separate wiring call. -registerMergeTraitHooks(); +/* +FNXC:MergeTrait 2026-06-18-13:05: +The engine must not register `merge.onEnter` or `merge.onExit` hooks because core owns the ctx-shaped `merge.onEnter` field-effects adapter. Keeping hook registration out of this module prevents last-write-wins registry collisions and preserves in-review scheduler-state clears under workflow columns. +*/