root/fusion
1
0
Fork 0
Code Issues Pull Requests Actions 1 Packages Projects Releases Wiki Activity

FN-6634: remove engine merge trait hook registration

Browse Source 
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
This commit is contained in:
gsxdsm
2026-06-18 06:41:22 -07:00
parent 3d28b3b212
commit ab8ecb2206
4 changed files with 106 additions and 139 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
.changeset/fn-6634-merge-trait-hook-collision.md Normal file
View File

@@ -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.

103
packages/engine/src/__tests__/merge-trait.test.ts
View File

@@ -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<void>;
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<void>;
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();
}

1
packages/engine/src/index.ts
View File

@@ -184,7 +184,6 @@ export {
type AutostashHandle,
} from "./merger.js";
export {
registerMergeTraitHooks,
resolveMergePolicy,
type ResolvedMergePolicy,
type MergeFileScopeMode,

136
packages/engine/src/merge-trait.ts
View File

@@ -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<Task, "id" | "priority">): Promise<void> {
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.
*/