U4 vertical slice: recovery-policy reconciler + ratified safety invariant (measured: engine ~780, real cost is a settings migration) - #2478
Conversation
|
Warning Review limit reached
Next review available in: 4 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThe PR adds an optional column recovery policy and an unwired recovery-decision engine.
Confidence Score: 4/5The PR appears safe to merge after tightening the recovery parser’s runtime allow-list coverage. Unknown recovery-policy properties currently survive parsing and serialization even though the safety test claims the parser prevents such keys from entering live policies. Files Needing Attention: packages/core/src/workflow-ir.ts; packages/engine/src/tests/recovery-policy-safety.test.ts
|
| Filename | Overview |
|---|---|
| packages/core/src/workflow-ir-types.ts | Adds optional recovery-policy and stale-action types to workflow columns. |
| packages/core/src/workflow-ir.ts | Adds recovery-policy validation and prevents policies from being discarded during v1 downgrade. |
| packages/engine/src/recovery-reconciler.ts | Adds pure recovery decisions, per-task workflow resolution, caching, and role-based policy lookup. |
| packages/engine/src/tests/recovery-policy-safety.test.ts | Adds structural and behavioral tests for the recovery-policy safety boundary. |
| packages/core/src/tests/workflow-ir-column-recovery.test.ts | Covers recovery validation, serialization, and downgrade preservation. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart LR
JSON[Workflow JSON] --> Parse[Parse and validate IR]
Parse --> IR[Workflow IR]
Task[Task snapshot] --> Resolve[Resolve task workflow]
IR --> Resolve
Resolve --> Decide[Evaluate column recovery policy]
Decide --> Suppress[Apply user-pause safeguard]
Suppress --> Decision[Return surface decision]
Reviews (3): Last reviewed commit: "fix(core,engine): recovery policy — v2-o..." | Re-trigger Greptile
…on, type-driven ratchet (PR #2478 review) Three review findings, all verified real and all mutation-verified. P1 — RECOVERY POLICY LOST ON DOWNGRADE (workflow-ir.ts). `downgradeIrToV1IfPure` did not treat `recovery` as v2-only. The v1 shape has no `columns` at all, so a workflow with v1-compatible nodes and default-shaped columns serialized WITHOUT columns and PERMANENTLY DISCARDED the authored policy on save. Silent data loss: the card just stops being reconciled, with no error and nothing in the diff to explain it. `recovery` now forces v2 exactly like `agent`, and — matching the `optionalSteps` rule — PRESENCE is the signal, so even `recovery: {}` forces v2, because downgrading would still mutate the persisted shape. P1 — RECOVERY SCHEMA UNVALIDATED (workflow-ir.ts). `parseWorkflowIr` persisted a negative or non-finite `stalenessMs`, an `onStale` missing its `code`, and unsupported actions, and `decideRecovery` consumed them directly. A negative threshold makes every card instantly stale; a non-finite one makes no card ever stale. Both are silent and both surface as a sweep behaving inexplicably at 3am rather than as a save that was refused. `validateColumnRecovery` mirrors `validateColumnAgent` and runs from the same loop. The action list is closed on purpose, so adding an action to the type without teaching the reconciler fails at authoring time. It also rejects a HALF-DECLARED policy (threshold without action, or action without threshold). Either half alone silently does nothing, which is precisely the "present but inert" failure this program keeps finding. P2 — THE RATCHET GUARDED THE FIXTURE, NOT THE TYPE (recovery-policy-safety). The structural check read keys off a test fixture, so adding a safeguard-adjacent property to `WorkflowColumnRecovery` left the advertised guarantee unenforced — the fixture simply never set it. Labelled P2 but it is the ratchet protecting the ratified safety invariant, so it is treated as the most important of the three. It is now driven off a compiler-enforced `Record<keyof WorkflowColumnRecovery, true>` manifest. That manifest lives in PRODUCTION code deliberately: the engine tsconfig excludes the __tests__ directory, so a compile-time assertion placed in the test file would never be checked by `tsc` and the ratchet would have been decorative — the same class of mistake as the finding itself. Two-stage effect, both verified: adding a key to the interface FAILS THE BUILD (`Property 'ignoreDependencies' is missing in type ... but required in type 'Record<keyof WorkflowColumnRecovery, true>'`), and listing it in the manifest to silence the build then FAILS THE TEST. Either way a human must re-state the safety argument. MUTATION VERIFICATION (each fix proven load-bearing): remove the downgrade guard → 3 tests fail unwire the parse validator → 13 tests fail add an interface key only → build fails in production code add key + manifest entry → 2 safety tests fail The downgrade suite carries an explicit CONTROL — "the fixture WITHOUT a recovery policy really does downgrade to v1" — and it earned its place immediately: the first fixture placed its nodes in `triage`/`done`, which forces v2 on PLACEMENT, so every downgrade assertion would have passed for the wrong reason. Seamless nodes default to `todo` (`defaultColumnForNode`); the fixture now matches. Verified: 20 new IR tests + 24 safety tests green; 143 passed across the five related core suites; tsc clean in core and engine; lint clean; merge gate green (299+10+71). The one `settings-parity` failure (`agentToolOutputMaxChars` overlap) is PRE-EXISTING and fails identically with these changes stashed. Fusion-Task-Id: KB-U4
4bbe137 to
7441bd3
Compare
f43f4db to
d3b9783
Compare
…econciler, measured The smallest end-to-end slice of the U4 reshape, built to MEASURE the real line cost before committing to the full policy table. The survey's ~900-line reconciler figure was reasoned, not prototyped; this replaces it with numbers. WHAT LANDS (all additive; nothing is wired, no behavior changes): `WorkflowColumnRecovery` on the IR column — one key, `stalenessMs` + `onStale`, 42 lines including the safety rationale. Optional and omitted when unset, so every existing workflow serializes byte-identically. `recovery-reconciler.ts` — 176 lines. One engine that walks live cards, resolves each card's policy from ITS OWN workflow (per task, shared `irCache`, so a 400-card board across three workflows reads three IRs), and returns decisions. Decision and application are separate so the safety boundary is assertable without running an engine. `recovery-policy-safety.test.ts` — 156 lines, one-time, encoding the RATIFIED invariant: the six safeguards live OUTSIDE the policy table and must never become expressible as workflow policy, because an author could then write a workflow that overrides an operator's explicit stop. MEASURED COST vs the ~900 estimate: reconciler engine + IR types = 222 lines for one action (`surface`) and one safeguard. Extrapolating the remaining actions — `rebound` (target resolution, attempt budgets, backward-move proof, five more safeguards) ≈ +350, `archive` ≈ +50, the `budgets`/`dependencies` keys ≈ +150 — lands near 780. So ~900 was a good estimate for the ENGINE, and the vertical slice does not move it much. THE ESTIMATE'S REAL MISS IS NOT LINES. 16 of the 34 POLICY sweeps read an OPERATOR SETTING today — ~17 distinct policy-threshold keys including `stalePausedTodoThresholdMs`, `inReviewStalledThresholdMs`, `taskStuckTimeoutMs`, `doneAutoArchiveDays`, `maxPostReviewFixes`. Moving those sweeps into workflow policy is not a code refactor; it is a SETTINGS MIGRATION with operator-visible blast radius, and it needs three decisions the line estimate never surfaced: does workflow policy override the global setting or defer to it, what happens to existing projects that configured the settings, and does an unset policy inherit the setting or the built-in default. That is the gating question for the full table, not the reconciler's size. WHY THE SWEEP IS NOT RETIRED HERE. Retiring `surfaceStalePausedTodos` requires builtin:coding to declare the policy AND `stalePausedTodoThresholdMs` to migrate, or the behavior silently disappears for every existing project. That is the settings migration above and belongs behind its own decision, not smuggled into a measurement slice. The reconciler is therefore unwired — deliberately dead code, for exactly as long as it takes to get that decision. Both halves of the safety invariant are MUTATION-VERIFIED, because a safety test that cannot fail is worse than none: making the reconciler honor a policy field that disables the user-pause safeguard fails it, and adding an unreviewed key to the policy schema fails it. A third test asserts the reconciler still ACTS on an unpaused card, so a reconciler that suppressed everything cannot pass by doing nothing. Verified: tsc clean in core and engine, lint clean, merge gate green (299+10+71), 23 safety tests green, workflow-lifecycle-traits green. Fusion-Task-Id: KB-U4
…on, type-driven ratchet (PR #2478 review) Three review findings, all verified real and all mutation-verified. P1 — RECOVERY POLICY LOST ON DOWNGRADE (workflow-ir.ts). `downgradeIrToV1IfPure` did not treat `recovery` as v2-only. The v1 shape has no `columns` at all, so a workflow with v1-compatible nodes and default-shaped columns serialized WITHOUT columns and PERMANENTLY DISCARDED the authored policy on save. Silent data loss: the card just stops being reconciled, with no error and nothing in the diff to explain it. `recovery` now forces v2 exactly like `agent`, and — matching the `optionalSteps` rule — PRESENCE is the signal, so even `recovery: {}` forces v2, because downgrading would still mutate the persisted shape. P1 — RECOVERY SCHEMA UNVALIDATED (workflow-ir.ts). `parseWorkflowIr` persisted a negative or non-finite `stalenessMs`, an `onStale` missing its `code`, and unsupported actions, and `decideRecovery` consumed them directly. A negative threshold makes every card instantly stale; a non-finite one makes no card ever stale. Both are silent and both surface as a sweep behaving inexplicably at 3am rather than as a save that was refused. `validateColumnRecovery` mirrors `validateColumnAgent` and runs from the same loop. The action list is closed on purpose, so adding an action to the type without teaching the reconciler fails at authoring time. It also rejects a HALF-DECLARED policy (threshold without action, or action without threshold). Either half alone silently does nothing, which is precisely the "present but inert" failure this program keeps finding. P2 — THE RATCHET GUARDED THE FIXTURE, NOT THE TYPE (recovery-policy-safety). The structural check read keys off a test fixture, so adding a safeguard-adjacent property to `WorkflowColumnRecovery` left the advertised guarantee unenforced — the fixture simply never set it. Labelled P2 but it is the ratchet protecting the ratified safety invariant, so it is treated as the most important of the three. It is now driven off a compiler-enforced `Record<keyof WorkflowColumnRecovery, true>` manifest. That manifest lives in PRODUCTION code deliberately: the engine tsconfig excludes the __tests__ directory, so a compile-time assertion placed in the test file would never be checked by `tsc` and the ratchet would have been decorative — the same class of mistake as the finding itself. Two-stage effect, both verified: adding a key to the interface FAILS THE BUILD (`Property 'ignoreDependencies' is missing in type ... but required in type 'Record<keyof WorkflowColumnRecovery, true>'`), and listing it in the manifest to silence the build then FAILS THE TEST. Either way a human must re-state the safety argument. MUTATION VERIFICATION (each fix proven load-bearing): remove the downgrade guard → 3 tests fail unwire the parse validator → 13 tests fail add an interface key only → build fails in production code add key + manifest entry → 2 safety tests fail The downgrade suite carries an explicit CONTROL — "the fixture WITHOUT a recovery policy really does downgrade to v1" — and it earned its place immediately: the first fixture placed its nodes in `triage`/`done`, which forces v2 on PLACEMENT, so every downgrade assertion would have passed for the wrong reason. Seamless nodes default to `todo` (`defaultColumnForNode`); the fixture now matches. Verified: 20 new IR tests + 24 safety tests green; 143 passed across the five related core suites; tsc clean in core and engine; lint clean; merge gate green (299+10+71). The one `settings-parity` failure (`agentToolOutputMaxChars` overlap) is PRE-EXISTING and fails identically with these changes stashed. Fusion-Task-Id: KB-U4
7441bd3 to
3225811
Compare
…rt an unreachable export Main moved while this branch was open and #2478 added a new lifecycle-role census site — the exact drift the table exists to absorb. Covering it cost one entry. Two findings came out of covering it, both established by mutation rather than by reading the code: 1. THE ROW DOES NOT COVER WHAT THE CENSUS SAID. The census flagged recovery-reconciler.ts:198 as a resolveLifecycleColumns site. Destroying that role resolution leaves all 18 tests green, because decideRecovery looks policy up by COLUMN ID and never consults a role. The row is relabelled to what it actually proves and is mutation-verified against that instead: keying the reconciler's policy lookup on the `todo` literal fails exactly its renamed test. 2. resolveRoleRecovery IS AN UNREACHABLE EXPORT. It is the only use of resolveLifecycleColumns in that file and has no production caller anywhere. So that census line is not a live converted site. Reported in the ledger, not fixed here: it is production code owned by the U4 slice, and whether its consumer is still to land or it should be deleted is its author's call. The table gains an explicit `observability` field. `persisted-row` is the strong form; `returned-decision` is recorded as WEAKER EVIDENCE and the reconciler row uses it, because reconcileRecovery decides and does not apply — there is no row to read. Naming it in the type is what keeps the ledger truthful about what "covered" means per site, instead of letting a return-value assertion pass as observed state. Verified: 18/18 green, engine tsc clean, pnpm test:gate green (299+10+71).
…r 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 -->
…est unproven-sites ledger (#2485) Follow-up to #2475 (merged). Test-only, plus one test-utility seam. ## Why a table #2475 proved one converted sweep. The count has since gone to **three**, twice while this work was open — `surfaceStalePausedTodos` appeared during #2475's review, and #2478 landed `recovery-reconciler.ts` while this branch was open. A suite with a bespoke `describe` per sweep is a coverage claim that quietly becomes false. Replaced with a table of `(seed, run, acted, roles, observability)`. The driver derives four assertions per entry: | | positive | negative | |---|---|---| | **renamed vocabulary** | acts on the card | inert in a non-target column | | **default vocabulary** | acts (regression floor) | inert | Adding a converted sweep is **one entry** — the #2478 site proved that in practice, not in principle. `actsOnRole`/`inertRole` are keys of `Vocabulary`, not column strings, so an entry cannot hardcode `todo` and pass for the wrong reason. ## Two findings, both from mutation rather than reading **1. The census was wrong about `recovery-reconciler.ts:198.`** It was flagged as a `resolveLifecycleColumns` site, so the row was first labelled as covering it. **Destroying that role resolution leaves all 18 tests green** — `decideRecovery` looks policy up by *column id* and never consults a role. The row is relabelled to what it actually proves, and mutation-verified against that instead: keying the reconciler's policy lookup on the `todo` literal fails exactly its renamed test. **2. `resolveRoleRecovery` is an unreachable export.** It is the only use of `resolveLifecycleColumns` in that file and has **no production caller anywhere** in engine, core, or dashboard. So that census line is not a live converted site — it is a helper written ahead of its consumer. **Not fixed here:** it is production code owned by the U4 slice, and whether the consumer is still to land or it should be deleted is its author's call. ## Observability is now explicit in the type `persisted-row` is the strong form. `returned-decision` is recorded as **weaker evidence** and the reconciler row uses it, because `reconcileRecovery` decides and does not apply — there is no row to read. Naming it in the type is what stops a return-value assertion from quietly passing as observed state, and it is what keeps the ledger truthful per site. ## Harness seam `PgTestHarness` now exposes its raw admin SQL client. The store **stamps** `updatedAt`/`columnMovedAt` on every write, so `updateTask` cannot express an aged row at all — the patch is accepted and the value silently replaced with `now`. **Found by the new case failing on BOTH vocabularies**, which is what distinguishes a broken fixture from a broken guard. Seeding only; assertions still read back through the real `getTask` path. ## Mutation verification | Mutation | Result | |---|---| | revert **only** `recoverStrandedCompletedTodoTasks`'s resolution | **exactly** that row's renamed test fails | | revert **only** `surfaceStalePausedTodos`'s resolution | **exactly** its own renamed test fails | | reconciler policy lookup keyed on `todo` | exactly the reconciler row's renamed test fails | | `resolveRoleRecovery` role resolution destroyed | **nothing fails** → finding #2 | | `hold-release` `isHeldTask` keyed on `todo` | 5 of 18 fail; default spine survives | | `markMoveInFlight` dropped | both spine tests fail | Per-site verification matters here: three rows could all be riding one guard. They are not. ## The honest number **Proven end to end: 5** (two self-healing sweeps, the reconciler's policy lookup at the weaker observability, hold-release's capacity release, and the graph boundary + `moveTask` + post-commit bus). **Not proven: 11 call sites** — `merger.ts:324-326`, `merger-ai.ts:1022,1039`, `auto-merge-finalization.ts:20-22`, `executor.ts:1763,6339,6341`, `self-healing.ts:713,6732`, `mesh-lease-manager.ts:61`, `task-agent-sync.ts:59`, `core/task-store/reads.ts:130`, `core/live-agent-count.ts:63-75`, and four dashboard route sites. The ledger lives in the file, not just here, so it stays with the code. ## Where the table does not fit — reported, not papered over The **merge/rebound family** cannot be a table row: those sweeps have no observable persisted effect without a real git repository, so `acted` cannot be written against the row at all. They need an engine-slow real-git lane. The dashboard sites need an HTTP route test with a live store. Both are different lanes, not missing entries. ## Verification - 18/18 green; engine + core `tsc --noEmit` clean; `pnpm test:gate` green (299 + 10 + 71) - full core PG suite run (the harness is shared): 1036 passed, 3 failed in `central-archive-secrets` and `workflow-settings-project-identity` — **reproduce identically with this change stashed**, pre-existing 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Stacked on #2477. Base is
feature/workflow-vocabulary-u4-delete-dep-blocked— do not merge before it.The smallest end-to-end slice of the U4 reshape, built to measure the real cost before committing to the full policy table. The survey's ~900-line reconciler figure was reasoned, not prototyped; this replaces it with numbers.
Everything here is additive and unwired. No behavior changes.
What lands
WorkflowColumnRecovery(IR)stalenessMs+onStale. Optional and omitted when unset, so existing workflows serialize byte-identically.recovery-reconciler.tsirCache— a 400-card board across three workflows reads three IRs), returns decisions. Decision and application are separate so the safety boundary is assertable without running an engine.recovery-policy-safety.test.tsMeasured cost vs the ~900 estimate
Engine + IR types = 222 lines for one action (
surface) and one safeguard.Extrapolating the rest —
rebound(target resolution, attempt budgets, backward-move proof, five more safeguards) ≈ +350,archive≈ +50, thebudgets/dependencieskeys ≈ +150 — lands near 780.So ~900 was a good estimate for the engine, and the vertical slice does not move it much. That is the answer to the question asked.
But the estimate's real miss is not lines
16 of the 34 POLICY sweeps read an operator setting today — ~17 distinct policy-threshold keys, including
stalePausedTodoThresholdMs,inReviewStalledThresholdMs,taskStuckTimeoutMs,doneAutoArchiveDays,maxPostReviewFixes.Moving those sweeps into workflow policy is not a code refactor — it is a settings migration with operator-visible blast radius, and it needs three decisions the line estimate never surfaced:
That is the gating question for the full table — not the reconciler's size.
Why the sweep is not retired here
Retiring
surfaceStalePausedTodosrequires builtin:coding to declare the policy andstalePausedTodoThresholdMsto migrate — or the behavior silently disappears for every existing project. That is the settings migration above, and it belongs behind its own decision rather than smuggled into a measurement slice.The reconciler is therefore unwired — deliberately dead code, for exactly as long as it takes to get that decision.
The ratified safety invariant
The six safeguards (user pause,
autoMerge:false, dependency, capacity, merge-proof, at-most-once) live outside the policy table. A workflow must never be able to author a safety invariant away.Encoded two ways, because either alone is defeatable:
Both halves mutation-verified, because a safety test that cannot fail is worse than none:
A third test asserts the reconciler still acts on an unpaused card, so a reconciler that suppressed everything cannot pass by doing nothing.
Scope limits stated rather than implied
Only the
surfaceaction is implemented, so only its relevant safeguard is wired.surfacemutates no lifecycle state; the other five gate lifecycle-mutating actions that do not exist yet, and wiring them now would be untestable dead code. A test records this so the absence reads as deliberate and must be updated whenreboundlands.Verification
tsc --noEmitclean in core and engine;pnpm lintcleanworkflow-lifecycle-traitsgreenNo changeset:
@fusion/coreand@fusion/engineare private.🤖 Generated with Claude Code