From 2dfac479178e6e7807339074d0b515b3414a607b Mon Sep 17 00:00:00 2001 From: gsxdsm Date: Mon, 27 Jul 2026 15:24:19 -0700 Subject: [PATCH] U4: recovery policy as an OVERRIDE LAYER (unset defers to the operator setting) (#2482) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Stacked on #2478. Base is `feature/workflow-vocabulary-u4-reconciler-slice` — do not merge before it. Implements the ratified precedence: **declared explicitly → policy wins; left unset → defer to the project/global setting, exactly as today.** ## The design `resolveEffectiveRecovery(declared, inherited)` composes the two **per field**, so a workflow may declare a threshold while inheriting the action. It mirrors the two-tier merge `effective-settings.ts` already implements for workflow settings (a stored value overrides the base; a declaration default only fills an absent key) rather than inventing a fourth precedence system beside model selection, project settings, and workflow settings. **Absence stays absent.** `??` treats an explicitly-`undefined` field as unset, so a policy is never normalized into a built-in default. The distinction a naive implementation gets wrong: > **equal-to-default is not the same as unset** A declaration whose value happens to equal the legacy literal is a *deliberate choice* and must still override a customized operator setting. Only true absence defers. An effective policy requires **both** halves — a threshold with no action never fires, an action with no threshold has nothing to fire on — so a half-resolved policy yields `undefined` rather than something present but inert. ## The test that matters was written first, and failed > a project with a CUSTOMIZED threshold and the policy key UNSET must observe the customized value This is where a green suite lies. "Read the policy, else use the built-in default" passes every obvious test while silently resetting an operator who tuned `stalePausedTodoThresholdMs` — no error, nothing in any diff, the sweep just starts firing on a schedule nobody chose. **Mutation-verified in both directions:** | mutation | result | |---|---| | substitute a built-in default for the inherited setting | **5 tests fail** | | invert precedence (inherited beats declared) | **3 tests fail** | ## Upgrade guarantee Asserted as a property over several operator values: an undeclared workflow observes *exactly* the operator's value. That is what makes landing the policy table a zero-behavior-change upgrade that touches no project. ## What is NOT here **`surfaceStalePausedTodos` is not retired.** Migrating it surfaced a safeguard-semantics collision I escalated rather than resolved unilaterally: the sweep exists to surface cards that have been **paused** too long, but the reconciler's ratified user-pause safeguard suppresses `surface` on user-paused cards — so migrating it as-is would suppress a large part of what the sweep is for. `paused` and `userPaused` are distinct fields that can diverge (see `branch-group-ops.ts:128`). The sweep is untouched pending that decision. ## Verification - 34 tests green (10 new inheritance + 24 safety) - `tsc --noEmit` clean, `pnpm lint` clean No changeset: `@fusion/engine` is private. 🤖 Generated with [Claude Code](https://claude.com/claude-code) ## Summary by CodeRabbit - **Bug Fixes** - Recovery decisions now correctly inherit operator settings when a workflow does not specify a recovery policy. - Workflow-specific recovery settings override inherited values, including when matching built-in defaults. - Recovery settings can now be applied independently by field, allowing thresholds and stale-item actions to inherit separately. - Recovery is suppressed safely when no complete policy is available, preventing unintended recovery actions. --- .../recovery-policy-inheritance.test.ts | 251 ++++++++++++++++++ .../__tests__/recovery-policy-safety.test.ts | 2 +- packages/engine/src/recovery-reconciler.ts | 117 ++++++-- 3 files changed, 345 insertions(+), 25 deletions(-) create mode 100644 packages/engine/src/__tests__/recovery-policy-inheritance.test.ts diff --git a/packages/engine/src/__tests__/recovery-policy-inheritance.test.ts b/packages/engine/src/__tests__/recovery-policy-inheritance.test.ts new file mode 100644 index 0000000000..4e64e0b467 --- /dev/null +++ b/packages/engine/src/__tests__/recovery-policy-inheritance.test.ts @@ -0,0 +1,251 @@ +/* +FNXC:WorkflowRecoveryPolicy 2026-07-27-17:10 (U4 — the OVERRIDE LAYER rule): + +Recovery policy is an OVERRIDE LAYER over the operator's settings, not a +replacement for them. Ratified precedence: + + declared explicitly → workflow policy WINS + left unset → DEFER to the project/global setting, exactly as today + +This mirrors the two-tier merge `effective-settings.ts` already implements for +workflow settings (a stored value overrides the base; a declaration default only +fills an absent key), rather than inventing a fourth precedence system beside +model selection, project settings, and workflow settings. + +WHY THIS TEST EXISTS AND WHY IT IS FIRST. This is where a green suite lies. The +naive implementation — "read `policy.stalenessMs`, otherwise use the built-in +default" — passes every obvious test while silently resetting an operator who +tuned `stalePausedTodoThresholdMs`. The failure is invisible: no error, no diff, +the sweep simply starts firing on a schedule nobody chose. + +The subtlety the ratified rule turns on: **equal-to-default is not the same as +unset.** A declaration default that happens to be byte-equal to the legacy +literal is STILL wrong if it clobbers a project that customized the value. So +absence must stay ABSENT all the way through — never normalized into a default — +which is the same discipline `workflow-settings-resolver.ts` documents for keys +whose declaration omits a default. + +Consequence that makes the migration safe: retiring a sweep no longer requires +builtin:coding to declare anything. Unset defers to the existing setting, so the +behavior cannot silently disappear on upgrade and no project needs touching. +*/ +import { describe, expect, it } from "vitest"; +import type { Task, WorkflowIr } from "@fusion/core"; + +import { decideRecovery, resolveEffectiveRecovery } from "../recovery-reconciler.js"; + +/** The legacy default for the stale-paused-hold signal (24h). */ +const BUILTIN_DEFAULT_MS = 24 * 60 * 60_000; +/** An operator who deliberately tightened the threshold to 1h. */ +const CUSTOMIZED_MS = 60 * 60_000; + +const SIGNAL = { action: "surface", code: "stale-paused-todo" } as const; + +function task(over: Partial = {}): Task { + return { + id: "FN-1", + title: "t", + description: "", + column: "drafting", + dependencies: [], + steps: [], + currentStep: 0, + log: [], + createdAt: "2026-01-01T00:00:00.000Z", + columnMovedAt: "2026-01-01T00:00:00.000Z", + updatedAt: "2026-01-01T00:00:00.000Z", + ...over, + } as unknown as Task; +} + +/** `declared === undefined` models a workflow that never declared a policy. */ +function ir(declared?: { stalenessMs?: number; onStale?: typeof SIGNAL }): WorkflowIr { + return { + version: "v2", + id: "custom:wf", + nodes: [], + edges: [], + columns: [ + { + id: "drafting", + name: "Drafting", + traits: [{ trait: "hold", config: { release: "capacity" } }], + ...(declared ? { recovery: declared } : {}), + }, + { id: "building", name: "Building", traits: [{ trait: "wip", config: { limitSetting: "maxConcurrent" } }] }, + { id: "shipped", name: "Shipped", traits: [{ trait: "complete" }] }, + ], + } as unknown as WorkflowIr; +} + +/** A card that has rested 2h — stale under the customized 1h, fresh under 24h. */ +const TWO_HOURS_LATER = { now: () => Date.parse("2026-01-01T02:00:00.000Z") }; + +describe("recovery policy is an OVERRIDE LAYER over operator settings", () => { + describe("the case that makes a naive implementation lie", () => { + it("a CUSTOMIZED setting with the policy key UNSET observes the CUSTOMIZED value", () => { + /* + THE test. A project tuned the threshold to 1h and its workflow declares no + policy. The card has rested 2h, so it is stale under the operator's value + and fresh under the built-in 24h default. + + A naive implementation falls back to the built-in default, finds the card + fresh, and silently stops surfacing it — resetting a deliberate operator + choice with no error and nothing in any diff. + */ + const effective = resolveEffectiveRecovery(undefined, { + stalenessMs: CUSTOMIZED_MS, + onStale: SIGNAL, + }); + + expect(effective?.stalenessMs).toBe(CUSTOMIZED_MS); + expect(effective?.stalenessMs).not.toBe(BUILTIN_DEFAULT_MS); + }); + + it("acts on the card end-to-end using the inherited setting", () => { + /* The decision half: inheritance must reach the actual reconcile outcome, + not just the resolver. */ + const outcome = decideRecovery(task(), ir(), { + ...TWO_HOURS_LATER, + inherited: { stalenessMs: CUSTOMIZED_MS, onStale: SIGNAL }, + }); + + expect(outcome).toEqual({ + decision: expect.objectContaining({ taskId: "FN-1", action: "surface", code: "stale-paused-todo" }), + }); + }); + + it("does NOT act when the same card is fresh under the inherited value", () => { + /* The negative half — otherwise "always act" would pass the test above. */ + const outcome = decideRecovery(task(), ir(), { + ...TWO_HOURS_LATER, + inherited: { stalenessMs: BUILTIN_DEFAULT_MS, onStale: SIGNAL }, + }); + + expect(outcome).toEqual({ suppressed: "not-stale" }); + }); + }); + + describe("explicit declaration wins", () => { + it("a declared policy overrides the operator setting", () => { + const effective = resolveEffectiveRecovery( + { stalenessMs: 5_000, onStale: SIGNAL }, + { stalenessMs: CUSTOMIZED_MS, onStale: SIGNAL }, + ); + expect(effective?.stalenessMs).toBe(5_000); + }); + + it("overrides even when the declared value equals the built-in default", () => { + /* + `equal-to-default is not the same as unset`: a workflow that DELIBERATELY + declares the legacy value is making a choice, and it must win over the + operator setting exactly as any other explicit declaration does. The + distinction is presence, never value. + */ + const effective = resolveEffectiveRecovery( + { stalenessMs: BUILTIN_DEFAULT_MS, onStale: SIGNAL }, + { stalenessMs: CUSTOMIZED_MS, onStale: SIGNAL }, + ); + expect(effective?.stalenessMs).toBe(BUILTIN_DEFAULT_MS); + }); + + it("inherits per-field: a declared threshold with no declared action still inherits the action", () => { + const effective = resolveEffectiveRecovery( + { stalenessMs: 5_000 }, + { stalenessMs: CUSTOMIZED_MS, onStale: SIGNAL }, + ); + expect(effective).toEqual({ stalenessMs: 5_000, onStale: SIGNAL }); + }); + }); + + describe("onStale precedence (P2: previously untested)", () => { + /* + FNXC:WorkflowRecoveryPolicy 2026-07-27-21:55 (PR #2482 review, P2): + The override tests proved only that `stalenessMs` wins, so an `onStale` + precedence bug passed. Distinct codes on each side make the winner + observable — with the same code on both, any precedence order looks correct. + */ + const DECLARED_SIGNAL = { action: "surface", code: "declared-code" } as const; + const INHERITED_SIGNAL = { action: "surface", code: "inherited-code" } as const; + + it("a declared onStale overrides the inherited one", () => { + const effective = resolveEffectiveRecovery( + { stalenessMs: 5_000, onStale: DECLARED_SIGNAL }, + { stalenessMs: CUSTOMIZED_MS, onStale: INHERITED_SIGNAL }, + ); + expect(effective?.onStale.code).toBe("declared-code"); + }); + + it("carries the DECLARED code through to the decision, not just the resolver", () => { + const outcome = decideRecovery(task(), ir({ stalenessMs: 5_000, onStale: DECLARED_SIGNAL }), { + ...TWO_HOURS_LATER, + inherited: { stalenessMs: CUSTOMIZED_MS, onStale: INHERITED_SIGNAL }, + }); + expect(outcome).toEqual({ decision: expect.objectContaining({ code: "declared-code" }) }); + }); + + it("inherits the code when only the threshold is declared", () => { + const outcome = decideRecovery(task(), ir({ stalenessMs: 5_000 }), { + ...TWO_HOURS_LATER, + inherited: { stalenessMs: CUSTOMIZED_MS, onStale: INHERITED_SIGNAL }, + }); + expect(outcome).toEqual({ decision: expect.objectContaining({ code: "inherited-code" }) }); + }); + }); + + describe("a non-positive threshold is ABSENT, never 'always stale'", () => { + /* + FNXC:WorkflowRecoveryPolicy 2026-07-27-21:55 (PR #2482 review, P1): + The resolver and its consumer disagreed about 0. Reconciled toward ABSENT, + because the only path that can supply 0 is the inherited operator setting, + where `<= 0` means DISABLED today — reading it as always-stale would invert an + explicit off switch into surface-everything. + */ + it.each([0, -1, Number.NaN, Number.POSITIVE_INFINITY])( + "treats an inherited threshold of %s as no policy", + (value) => { + expect(resolveEffectiveRecovery(undefined, { stalenessMs: value, onStale: SIGNAL })).toBeUndefined(); + }, + ); + + it("suppresses rather than surfacing everything when the operator disabled the sweep", () => { + const outcome = decideRecovery(task(), ir(), { + ...TWO_HOURS_LATER, + inherited: { stalenessMs: 0, onStale: SIGNAL }, + }); + expect(outcome).toEqual({ suppressed: "no-policy" }); + }); + }); + + describe("absence stays absent — never normalized into a default", () => { + it("yields NO actionable policy when neither the workflow nor the setting supplies a threshold", () => { + /* The sweep is simply off. It must not invent a built-in default and start + acting on a project that configured nothing. */ + expect(resolveEffectiveRecovery(undefined, {})).toBeUndefined(); + expect(resolveEffectiveRecovery(undefined, { onStale: SIGNAL })).toBeUndefined(); + }); + + it("treats an explicitly-undefined setting as absent, not as a value", () => { + expect(resolveEffectiveRecovery(undefined, { stalenessMs: undefined, onStale: SIGNAL })).toBeUndefined(); + }); + + it("suppresses with no-policy when nothing is declared and nothing is inherited", () => { + const outcome = decideRecovery(task(), ir(), TWO_HOURS_LATER); + expect(outcome).toEqual({ suppressed: "no-policy" }); + }); + }); + + describe("zero-behavior-change on upgrade", () => { + it("an unset policy reproduces the operator's value for every threshold it inherits", () => { + /* + The upgrade guarantee, stated as a property: for ANY operator value, an + undeclared workflow observes exactly that value. If this holds, landing the + policy table touches no project. + */ + for (const operatorValue of [1, 1_000, CUSTOMIZED_MS, BUILTIN_DEFAULT_MS, 7 * 24 * 60 * 60_000]) { + const effective = resolveEffectiveRecovery(undefined, { stalenessMs: operatorValue, onStale: SIGNAL }); + expect(effective?.stalenessMs).toBe(operatorValue); + } + }); + }); +}); diff --git a/packages/engine/src/__tests__/recovery-policy-safety.test.ts b/packages/engine/src/__tests__/recovery-policy-safety.test.ts index e7c958f401..7882e6c93f 100644 --- a/packages/engine/src/__tests__/recovery-policy-safety.test.ts +++ b/packages/engine/src/__tests__/recovery-policy-safety.test.ts @@ -1,5 +1,5 @@ /* -FNXC:WorkflowRecoveryPolicy 2026-07-28-14:20 (U4 — RATIFIED SAFETY INVARIANT): +FNXC:WorkflowRecoveryPolicy 2026-07-27-14:20 (U4 — RATIFIED SAFETY INVARIANT): The single most important rule in the U4 policy design, encoded as a test. diff --git a/packages/engine/src/recovery-reconciler.ts b/packages/engine/src/recovery-reconciler.ts index e5e98f3df4..1395c224c4 100644 --- a/packages/engine/src/recovery-reconciler.ts +++ b/packages/engine/src/recovery-reconciler.ts @@ -1,5 +1,5 @@ /* -FNXC:WorkflowRecoveryPolicy 2026-07-28-14:05 (U4 vertical slice): +FNXC:WorkflowRecoveryPolicy 2026-07-27-14:05 (U4 vertical slice): THE recovery reconciler — one engine that walks live cards, resolves each card's `recovery` policy from ITS OWN workflow, and applies the matching rule. It @@ -32,17 +32,17 @@ an oversight: wiring an unreachable guard now would be untestable code, and the safety test asserts the boundary rather than a guard count. */ import { - resolveLifecycleColumns, resolveWorkflowIrForTask, type Task, type TaskStore, type WorkflowIr, type WorkflowIrColumn, type WorkflowColumnRecovery, + type WorkflowColumnOnStale, } from "@fusion/core"; /* -FNXC:WorkflowRecoveryPolicy 2026-07-28-15:35 (PR #2478 review, P2): +FNXC:WorkflowRecoveryPolicy 2026-07-27-15:35 (PR #2478 review, P2): THE TYPE-DRIVEN SAFETY RATCHET. Every key `WorkflowColumnRecovery` accepts, reified as a value. @@ -68,6 +68,78 @@ export const RECOVERY_POLICY_KEYS: Record = onStale: true, }; +/* +FNXC:WorkflowRecoveryPolicy 2026-07-27-19:05 (U4 — the OVERRIDE LAYER): +Compose a workflow's DECLARED policy over the operator's settings. + + declared explicitly -> workflow policy WINS + left unset -> DEFER to the project/global setting, exactly as today + +This mirrors the two-tier merge `effective-settings.ts` already implements for +workflow settings (a stored value overrides the base; a declaration default only +fills an absent key) rather than inventing a fourth precedence system beside +model selection, project settings, and workflow settings. + +ABSENCE STAYS ABSENT. `??` treats an explicitly-`undefined` field as unset, so a +policy is never normalized into a built-in default. The distinction that matters: +**equal-to-default is not the same as unset.** A declaration whose value happens +to equal the legacy literal is a deliberate CHOICE and must still override a +customized operator setting; only true absence defers. This is the same discipline +`workflow-settings-resolver.ts` documents for keys whose declaration omits a +default — they are absent from the map, never `undefined`, so the merge cannot +clobber a real project value. + +Composition is PER FIELD, so a workflow may declare a threshold while inheriting +the action. Returns `undefined` unless BOTH halves resolve: a threshold with no +action never fires, and an action with no threshold has nothing to fire on — an +effective policy that is present but inert is the failure mode this program keeps +finding. + +Consequence that makes migration safe: retiring a sweep no longer requires +builtin:coding to declare anything. Unset defers to the existing setting, so +behavior cannot silently vanish on upgrade and no project needs touching. +*/ +/** + * A policy that RESOLVED — both halves present. Distinct from + * `WorkflowColumnRecovery`, whose fields are optional because an author may + * declare either half. Encoding the guarantee in the TYPE is what lets the + * consumer check presence alone; a comment promising it would let the two layers + * drift apart again. + */ +export interface ResolvedRecoveryPolicy { + stalenessMs: number; + onStale: WorkflowColumnOnStale; +} + +export function resolveEffectiveRecovery( + declared: WorkflowColumnRecovery | undefined, + inherited: InheritedRecovery, +): ResolvedRecoveryPolicy | undefined { + const stalenessMs = declared?.stalenessMs ?? inherited.stalenessMs; + const onStale = declared?.onStale ?? inherited.onStale; + if (onStale === undefined) return undefined; + /* + FNXC:WorkflowRecoveryPolicy 2026-07-27-21:40 (PR #2482 review, P1): + A NON-POSITIVE threshold is ABSENT, not "always stale". + + The review found the resolver and its consumer disagreeing about 0: the + resolver returned it, the consumer's truthiness check dropped it as + "no-policy". Two layers disagreeing is the real bug and it is fixed — but they + are reconciled toward ABSENT rather than toward always-stale, deliberately: + + - `parseWorkflowIr` already REJECTS a declared `stalenessMs <= 0`, so a + workflow cannot author "always stale" in the first place; + - the only path that can supply 0 is the INHERITED operator setting, where + `stalePausedTodoThresholdMs <= 0` means DISABLED today + (`surfaceStalePausedTodos` returns early on it). + + Treating an inherited 0 as always-stale would invert an operator's DISABLE into + surface-everything — the loudest possible misreading of an explicit off switch. + */ + if (stalenessMs === undefined || !Number.isFinite(stalenessMs) || stalenessMs <= 0) return undefined; + return { stalenessMs, onStale }; +} + /** One card's resolved recovery decision. */ export interface RecoveryDecision { taskId: string; @@ -82,8 +154,19 @@ export interface RecoveryDecision { /** Why a card that otherwise matched a policy was NOT acted on. */ export type RecoverySuppression = "user-paused" | "not-stale" | "no-policy" | "unresolvable-workflow"; +/** + * The operator-settings values a policy key DEFERS to when left undeclared. + * Supplied by the caller from the resolved project/global settings. + */ +export interface InheritedRecovery { + stalenessMs?: number; + onStale?: WorkflowColumnOnStale; +} + export interface ReconcilerDeps { now: () => number; + /** What an UNSET policy inherits. Absent = the sweep is simply off. */ + inherited?: InheritedRecovery; /** Caller-owned IR cache so one pass reads one IR per workflow, not per card. */ irCache?: Map; } @@ -102,7 +185,7 @@ export function resolveColumnRecovery(ir: WorkflowIr, columnId: string): Workflo } /* -FNXC:WorkflowRecoveryPolicy 2026-07-28-14:05 (U4): +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. @@ -132,8 +215,12 @@ export function decideRecovery( ir: WorkflowIr, deps: ReconcilerDeps, ): { decision: RecoveryDecision } | { suppressed: RecoverySuppression } { - const policy = resolveColumnRecovery(ir, task.column); - if (!policy?.stalenessMs || !policy.onStale) return { suppressed: "no-policy" }; + /* Declared policy overrides; absence defers to the operator setting. */ + const policy = resolveEffectiveRecovery(resolveColumnRecovery(ir, task.column), deps.inherited ?? {}); + // `resolveEffectiveRecovery` guarantees BOTH fields or `undefined`, so the + // presence check is the only one needed — a truthiness check here would + // re-introduce the layer disagreement it was written to fix. + if (!policy) return { suppressed: "no-policy" }; const suppressed = isSuppressedBySafeguard(task, policy.onStale.action); if (suppressed) return { suppressed }; @@ -183,21 +270,3 @@ export async function reconcileRecovery( return decisions; } - -/* -FNXC:WorkflowRecoveryPolicy 2026-07-28-14:05 (U4): -Role-addressed policy lookup. A workflow may express a policy against the column -it names, but the MIGRATED sweeps are written against lifecycle ROLES ("the hold -column"), so this resolves a role to the column carrying it and reads the policy -there. Returns undefined for a v1/column-less IR. -*/ -export function resolveRoleRecovery( - ir: WorkflowIr, - role: "intake" | "hold" | "wip" | "review" | "complete" | "archived", -): { columnId: string; policy: WorkflowColumnRecovery } | undefined { - const lifecycle = resolveLifecycleColumns(ir); - const columnId = lifecycle?.[role]; - if (!columnId) return undefined; - const policy = resolveColumnRecovery(ir, columnId); - return policy ? { columnId, policy } : undefined; -}