diff --git a/packages/engine/src/__tests__/recovery-policy-safety.test.ts b/packages/engine/src/__tests__/recovery-policy-safety.test.ts index 7882e6c93f..59666cc03e 100644 --- a/packages/engine/src/__tests__/recovery-policy-safety.test.ts +++ b/packages/engine/src/__tests__/recovery-policy-safety.test.ts @@ -28,6 +28,7 @@ import type { Task, WorkflowIr } from "@fusion/core"; import { decideRecovery, + isLifecycleMutatingAction, isSuppressedBySafeguard, resolveColumnRecovery, RECOVERY_POLICY_KEYS, @@ -127,42 +128,82 @@ describe("recovery policy safety invariant — safeguards live OUTSIDE the polic }); describe("behavioral: a policy that tries to disable a safeguard has no effect", () => { - it("cannot switch off the user-pause safeguard", () => { - /* - The load-bearing case for the `surface` action. A user-paused card must not - receive engine-authored signals, and no policy may say otherwise. - */ - const paused = task({ userPaused: true } as Partial); + /* + FNXC:WorkflowRecoveryPolicy 2026-07-27-21:20 (U4 — RE-RATIFIED, narrowed): + The user-pause safeguard means NEVER MUTATE LIFECYCLE STATE of a user-paused + card. It does NOT mean never observe one. - // Honest policy: suppressed. - const honest = decideRecovery(paused, ir(), LATER); - expect(honest).toEqual({ suppressed: "user-paused" }); - - // Hostile policy attempting every spelling of "ignore the pause": identical. - const hostile = decideRecovery( - paused, - ir({ ignoreUserPause: true, respectUserPause: false, userPaused: false }), - LATER, - ); - expect(hostile).toEqual({ suppressed: "user-paused" }); + The broad case below is NARROWED, not deleted. Keeping both halves is the + point: a future reader must be able to tell the scoping was DELIBERATE rather + than eroded by someone who found the broad rule inconvenient. + */ + it("SUPPRESSES a lifecycle-mutating action on a user-paused card", () => { + /* The half that must never weaken: the engine must not move, rebound, + archive, requeue or resume a card behind the operator who paused it. */ + for (const action of ["rebound", "archive", "requeue", "resume"] as const) { + expect(isSuppressedBySafeguard({ userPaused: true }, action)).toBe("user-paused"); + expect(isLifecycleMutatingAction(action)).toBe(true); + } }); - it("still acts on an unpaused card, so the guard is not vacuously suppressing everything", () => { - /* Without this, a reconciler that suppressed EVERYTHING would pass the test - above while doing nothing — the "dead guard passes tests" failure mode. */ + it("PERMITS an observational action on a user-paused card", () => { + /* + The half that was wrong before. `surface` writes no lifecycle state — it + tells the operator what their own paused card is doing. Suppressing it + turned `surfaceStalePausedTodos`, whose entire purpose is reporting paused + cards, into a diagnostic that silently reported nothing. + */ + expect(isSuppressedBySafeguard({ userPaused: true }, "surface")).toBeUndefined(); + expect(isLifecycleMutatingAction("surface")).toBe(false); + }); + + it("still surfaces a stale user-paused card end-to-end", () => { + /* The scoping must reach the actual reconcile outcome, not just the + predicate — otherwise the sweep is still blind in practice. */ + const paused = task({ userPaused: true } as Partial); + const outcome = decideRecovery(paused, ir(), LATER); + expect(outcome).toEqual({ + decision: expect.objectContaining({ taskId: "FN-1", action: "surface" }), + }); + }); + + it("suppression is BY ACTION, so no policy can opt a mutating action out", () => { + /* + A hostile policy attempting every spelling of "ignore the pause" changes + nothing: the scoping keys on the action's own nature, never on anything a + workflow author can write. + */ + const hostile = ir({ ignoreUserPause: true, respectUserPause: false, userPaused: false }); + expect(resolveColumnRecovery(hostile, "drafting")).toBeDefined(); + for (const action of ["rebound", "archive", "requeue", "resume"] as const) { + expect(isSuppressedBySafeguard({ userPaused: true }, action)).toBe("user-paused"); + } + }); + + it("a NEW action is suppressed by default, because observational is an allow-list", () => { + /* Guards the direction of the default: adding a mutating action without + touching the safeguard must fail closed, not open. */ + expect(isLifecycleMutatingAction("rebound")).toBe(true); + expect(isSuppressedBySafeguard({ userPaused: true }, "rebound")).toBe("user-paused"); + }); + + it("gates on userPaused, NOT on the broader paused flag", () => { + /* + Chosen deliberately: `paused` also covers automation pauses (dispatch-storm) + that carry no operator intent to respect, and the two fields diverge + (branch-group-ops.ts:128). The safeguard defers to a HUMAN decision, so it + keys on the field that records one. + */ + expect(isSuppressedBySafeguard({ userPaused: false }, "rebound")).toBeUndefined(); + expect(isSuppressedBySafeguard({ userPaused: undefined }, "rebound")).toBeUndefined(); + }); + + it("acts on an unpaused card, so the guard is not vacuously suppressing everything", () => { const outcome = decideRecovery(task(), ir(), LATER); expect(outcome).toEqual({ decision: expect.objectContaining({ taskId: "FN-1", action: "surface", code: "stale-paused-todo" }), }); }); - - it("routes the user-pause decision through the single safeguard chokepoint", () => { - /* Pins that suppression comes from `isSuppressedBySafeguard` and not from an - incidental check elsewhere, so there stays exactly one place to audit. */ - expect(isSuppressedBySafeguard({ userPaused: true }, "surface")).toBe("user-paused"); - expect(isSuppressedBySafeguard({ userPaused: false }, "surface")).toBeUndefined(); - expect(isSuppressedBySafeguard({ userPaused: undefined }, "surface")).toBeUndefined(); - }); }); describe("scope limits stated rather than implied", () => { diff --git a/packages/engine/src/recovery-reconciler.ts b/packages/engine/src/recovery-reconciler.ts index 1395c224c4..4762e06810 100644 --- a/packages/engine/src/recovery-reconciler.ts +++ b/packages/engine/src/recovery-reconciler.ts @@ -185,21 +185,66 @@ export function resolveColumnRecovery(ir: WorkflowIr, columnId: string): Workflo } /* -FNXC:WorkflowRecoveryPolicy 2026-07-27-14:05 (U4): -THE safeguard chokepoint. Every safeguard suppression flows through here so there -is exactly one place to audit, and so the safety test has a single seam to assert -against. Returns the suppressing safeguard, or `undefined` when none applies. +FNXC:WorkflowRecoveryPolicy 2026-07-27-21:10 (U4 — RE-RATIFIED, narrowed): +Every recovery action, whether or not the policy vocabulary can author it yet. -`surface` writes no lifecycle state, so only the user-pause safeguard is -load-bearing for it — a user-paused card must not have engine-authored signals -attributed to it. The remaining five gate lifecycle-mutating actions and are -wired when those actions land. +Declared beyond the authorable set on purpose. The user-pause safeguard is scoped +BY ACTION, so the scoping is only testable if the mutating actions exist as +values — and a rule that cannot be tested is a rule that erodes. `parseWorkflowIr` +keeps a CLOSED action list, so nothing here becomes authorable by being named. +*/ +export type RecoveryActionKind = "surface" | "rebound" | "archive" | "requeue" | "resume"; + +/** + * OBSERVATIONAL actions: they read state and report it. They write no lifecycle + * field, move no card, and change nothing an operator would have to undo. + */ +const OBSERVATIONAL_ACTIONS: ReadonlySet = new Set(["surface"]); + +/** True when an action changes lifecycle state rather than merely reporting it. */ +export function isLifecycleMutatingAction(action: RecoveryActionKind): boolean { + return !OBSERVATIONAL_ACTIONS.has(action); +} + +/* +FNXC:WorkflowRecoveryPolicy 2026-07-27-21:10 (U4 — THE safeguard chokepoint): +Every safeguard suppression flows through here, so there is exactly one place to +audit and the safety test has a single seam to assert against. + +THE INVARIANT, in the words that make the distinction survive a future reader: + + The user-pause safeguard means NEVER MUTATE LIFECYCLE STATE of a user-paused + card. It does NOT mean never observe one. + +The purpose of respecting a pause is to stop the engine acting on a card BEHIND +the operator who paused it — moving it, rebounding it, archiving it, resuming it. +A read-only diagnostic does the opposite: it tells that same operator what their +paused card is doing. Blinding them to their own paused work is not safety; it is +the engine deciding they should not be told. + +This was ratified BROADLY first (suppress every action on a user-paused card) and +re-ratified narrowly after that reading was caught suppressing the very sweeps it +was meant to protect: `surfaceStalePausedTodos` exists to report cards that have +sat paused too long, so the broad rule turned a diagnostic into one that silently +reported nothing. Scoping is therefore BY ACTION, never by sweep — a sweep cannot +opt itself out, and a new mutating action is suppressed by default because +`OBSERVATIONAL_ACTIONS` is an allow-list. + +WHICH FIELD, chosen deliberately rather than inherited from whatever was nearest: +this gate reads `userPaused` — the explicit operator park — NOT `paused`. The two +diverge (see `branch-group-ops.ts:128`: "userPaused remains true but legacy +`paused` is false"). `paused` also covers automation pauses such as +dispatch-storm, which are engine-authored and carry no operator intent to respect, +so gating lifecycle mutation on `paused` would suppress recovery from the engine's +own throttles. The safeguard exists to defer to a HUMAN decision, so it keys on +the field that records one. */ export function isSuppressedBySafeguard( task: Pick, - action: RecoveryDecision["action"], + action: RecoveryActionKind, ): "user-paused" | undefined { - if (action === "surface" && task.userPaused === true) return "user-paused"; + if (!isLifecycleMutatingAction(action)) return undefined; + if (task.userPaused === true) return "user-paused"; return undefined; }