U4: recovery policy as an OVERRIDE LAYER (unset defers to the operator setting) (#2482)

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)


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

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

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
gsxdsm
2026-07-27 15:24:19 -07:00
committed by GitHub
parent 3578903b16
commit 2dfac47917
3 changed files with 345 additions and 25 deletions

View File

@@ -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> = {}): 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);
}
});
});
});

View File

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

View File

@@ -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<keyof WorkflowColumnRecovery, true> =
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<string, WorkflowIr>;
}
@@ -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;
}