U4: retire the surfacing family onto one policy-driven runner (measured: trim is ~34% of the estimate) - #2487
Merged
Merged
Conversation
…t observation (U4, re-ratified) The safeguard was ratified broadly — suppress every recovery action on a user-paused card — and that reading was caught suppressing the very sweeps it was meant to protect. `surfaceStalePausedTodos` exists to report cards that have sat paused too long; routing it through a reconciler that suppresses paused cards turns a diagnostic into one that silently reports nothing. RE-RATIFIED INVARIANT, written into the code because the distinction is the whole point and a future reader will otherwise re-broaden it: The user-pause safeguard means NEVER MUTATE LIFECYCLE STATE of a user-paused card. It does NOT mean never observe one. Respecting a pause exists to stop the engine acting on a card BEHIND the operator who paused it — moving, rebounding, archiving, resuming. 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. SCOPED BY ACTION, never by sweep, so a sweep cannot opt itself out. `OBSERVATIONAL_ACTIONS` is an ALLOW-LIST, so a newly added mutating action is suppressed by default — the scoping fails closed. `RecoveryActionKind` deliberately names actions the policy vocabulary cannot yet author (`rebound`, `archive`, `requeue`, `resume`). The scoping is only testable if those exist as values, and a rule that cannot be tested is a rule that erodes. `parseWorkflowIr` keeps a CLOSED action list, so nothing becomes authorable by being named here. WHICH FIELD, chosen deliberately rather than inherited from whatever was nearest: the gate reads `userPaused`, not `paused`. The two diverge — `branch-group-ops.ts` comments that `userPaused` remains true while legacy `paused` is false — and `paused` also covers engine-authored automation pauses (dispatch-storm) that carry no operator intent to respect. Gating on `paused` would suppress recovery from the engine's own throttles. The safeguard defers to a HUMAN decision, so it keys on the field that records one. The broad test case is NARROWED, NOT DELETED, and both halves are kept: one proving mutation is still suppressed, one proving observation is now permitted. A future reader must be able to tell the scoping was deliberate rather than eroded by someone who found the broad rule inconvenient. Mutation-verified in BOTH directions, since either error is silent: re-broaden (suppress observation) -> 2 tests fail over-narrow (rebound treated observational) -> 3 tests fail Verified: 38 tests green (28 safety + 10 inheritance), tsc clean, lint clean, merge gate green (299+10+71). Fusion-Task-Id: KB-U4
…iven runner (U4) Migrates `surfaceStalePausedTodos`, `surfaceStalePausedReviews` and `surfaceInReviewStalled` together, because they were three copies of one skeleton and a fix applied to one had to be remembered for the other two. One runner owns the mechanism; each sweep is now the part that was genuinely its own. Thresholds are POLICY-OVER-SETTING. A workflow declaring `recovery.stalenessMs` on the role's column wins; one declaring nothing keeps reading its operator setting exactly as before — so this changes nothing for an existing project and requires no workflow to be edited. #2484's characterization suite is the floor and passes unchanged. TWO UNCONVERTED SITES FOUND IN CORE, without which this migration would have been cosmetic for two of the three: `getStalePausedReviewSignal` and `getInReviewStalledSignal` both hardcoded `task.column !== "in-review"`. `getStalePausedTodoSignal` gained the equivalent `holdColumn` parameter back in B1; its two siblings were missed, so they silently stopped matching for any workflow that renames its review column. Both now take `reviewColumn`, defaulting to the legacy id so every other caller is byte-identical. THREE BUGS THIS WORK INTRODUCED AND THE TESTS CAUGHT — worth recording, because each was silent in the diff: 1. I dropped the ENGINE ACTIVATION FLOOR from all three signal calls. Wall-clock the engine was not running for is not quiet time, so every sweep would have reported cards as stale purely because the engine restarted. Caught by the PRE-EXISTING suite, not by my own characterization floor — which is exactly why the floor was not sufficient on its own. 2. I passed `task.column` as the signal's role column, which makes the signal's own column check compare a value against itself — tautologically true, so the check was silently deleted. Now the RESOLVED role column is passed. 3. The role gate was conditional on the role resolving, so it was silently ABSENT for exactly the workflows whose role failed to resolve. It is now unconditional, with the legacy id as the fallback, so an unresolvable workflow behaves as before rather than losing the gate. The shared test is a TABLE — one row per sweep, every invariant asserted for all three: threshold inheritance, declared-policy override, renamed role column, the negative case outside the role column, at-most-once, the activation floor, both pause gates, non-positive-threshold disable, and soft-delete. Adding a fourth surfacing sweep means adding a row. Its log mock APPENDS to the card's log, because the at-most-once dedup reads that log and a call-recording mock cannot observe suppression at all. AT-MOST-ONCE stays in the runner, outside the policy table: a workflow must not be able to author repeated notification of the same signal. Surfacing is OBSERVATIONAL, so per the re-ratified invariant these sweeps report on user-paused cards — which is their entire purpose. MEASURED LINE DELTA, which corrects the survey estimate downward: self-healing.ts -192 / +95 = -97 new runner file = +209 core signal conversions = +23 NET = +135 Three sweeps migrated INCREASED total lines by 135 while shrinking self-healing.ts by 97. Per sweep: -32 in self-healing.ts. Break-even is 6.5 sweeps. Extrapolated to all 34 POLICY sweeps: -1,099 in self-healing.ts, -890 net once the runner is amortized. My survey estimated -2,612 for that bucket, so the MEASURED figure is 34% of it. The reconciler size was never the issue — the estimate assumed per-sweep bodies collapse to almost nothing, and they do not: each keeps a real eligibility predicate, signal call and operator message. Verified: 33 shared-family tests green; 471 passed across five engine suites with the single known PRE-EXISTING `archiveStaleDoneTasks` failure; 44 core signal tests green; tsc clean in core and engine; lint clean; merge gate green (299+10+71). Fusion-Task-Id: KB-U4
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change adds a shared runner for three surfacing sweeps, supports configurable review columns, shares surfacing cycles across self-healing flows, and narrows user-pause suppression to lifecycle-mutating recovery actions. Tests cover thresholds, role mapping, deduplication, safeguards, and merge-lane behavior. ChangesSurfacing and recovery safety
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SelfHealingManager
participant runSurfacingSweep
participant WorkflowIR
participant TaskStore
SelfHealingManager->>runSurfacingSweep: run sweep specification with shared cycle
runSurfacingSweep->>WorkflowIR: resolve threshold and role column
runSurfacingSweep->>TaskStore: evaluate tasks and inspect logs
runSurfacingSweep->>TaskStore: record eligible surfacing
runSurfacingSweep-->>SelfHealingManager: return surfaced count
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
Greptile SummaryConsolidates three stale/stalled surfacing sweeps onto a shared policy-driven runner.
Confidence Score: 5/5The PR appears safe to merge after the previously reported auto-merge eligibility regression was corrected. No blocking failure remains; the current stalled-review specification passes effective settings into the restored auto-merge eligibility gate before signal evaluation, excluding globally disabled and manually owned review work.
|
| Filename | Overview |
|---|---|
| packages/engine/src/self-healing.ts | Replaces three duplicated surfacing implementations with shared-cycle specifications and restores the previously omitted auto-merge eligibility gate. |
| packages/engine/src/surfacing-sweeps.ts | Introduces the common policy-aware runner for role resolution, threshold selection, freshness checks, deduplication, and diagnostic logging. |
| packages/core/src/in-review-stalled.ts | Makes the stalled-review signal honor the workflow-resolved review column while preserving the legacy default. |
| packages/core/src/stale-paused-review.ts | Makes stale paused-review detection honor the workflow-resolved review column while preserving existing callers. |
| packages/engine/src/recovery-reconciler.ts | Narrows user-pause suppression to lifecycle-mutating recovery actions while allowing observational surfacing. |
| packages/engine/src/tests/surfacing-family-shared.test.ts | Adds shared regression coverage for policy inheritance, renamed roles, deduplication, activation and pause gates, task partitioning, and auto-merge eligibility. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
Cycle[Self-healing cycle] --> Snapshot[Open shared surfacing cycle]
Snapshot --> Settings[Load settings]
Snapshot --> Tasks[Load task snapshot]
Snapshot --> Cache[Create workflow IR cache]
Settings --> Runner[Policy-driven surfacing runner]
Tasks --> Runner
Cache --> Runner
Runner --> Todo[Stale paused hold tasks]
Runner --> Review[Stale paused review tasks]
Runner --> Stalled[Active stalled review tasks]
Todo --> Log[Append operator diagnostic]
Review --> Log
Stalled --> AutoMerge{Auto-merge processing allowed?}
AutoMerge -->|Yes| Log
AutoMerge -->|No| Skip[Skip task]
Reviews (3): Last reviewed commit: "perf(engine): share one board snapshot a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
…opped (PR #2487 review, P1) `surfaceInReviewStalled` lost `allowsAutoMergeProcessing` when it moved onto the shared runner, while STILL passing `autoMerge: true` to its signal — so the code asserted an eligibility it no longer checked. With global auto-merge off and no per-task override, or with a manually opened PR, the sweep treated the task as auto-merge-eligible and reported it. `autoMerge:false` is one of the six safeguards and is meant to be terminal-until-human. ROOT CAUSE, not just the symptom: the runner's `isEligible(task)` had no access to settings, so a settings-dependent safeguard had nowhere to live. Anything the interface could not express was silently dropped at migration time and nothing failed. `isEligible` now takes `settings`, and the comment records why — the missing parameter IS the bug, and re-adding the call without widening the interface would leave the next settings-dependent gate equally droppable. The `autoMerge: true` argument to the signal is retained and is now sound again: it is only correct BECAUSE the restored gate proves it. SIX-SAFEGUARD AUDIT of the migrated runner, one row per safeguard, with the test that proves each — requested instead of another green suite: 1. USER PAUSE — consulted, and deliberately NOT applied. Surfacing is observational and the runner mutates no lifecycle field, so per the re-ratified invariant a user-paused card is still reported (the entire point of the two paused sweeps). NEW test: "surfaces a user-paused card, because surfacing is observational", asserted for all three sweeps, plus the existing safety-suite pair. 2. AUTOMERGE:FALSE — the P1. Restored. NEW tests: off-with-no-override does not surface; per-task override does; global-on does. The middle one is the discriminator, without which "never surfaces" would pass by being broken. 3. DEPENDENCY — NOT APPLICABLE, stated rather than assumed. The safeguard protects lifecycle MOVES against unmet dependencies; surfacing performs no move, so there is no dependency decision to respect. No test, because there is no behavior to assert. 4. CAPACITY — NOT APPLICABLE, same reasoning: no move, so no slot is consumed and no capacity limit is engaged. 5. MERGE-PROOF — consulted, via the active-merge id, the executing-task set and `isMergeLaneOwned`. NEW tests: the task currently being merged is not surfaced; an actively executing task is not surfaced. Reporting either would tell an operator work is stalled while it is in flight. 6. AT-MOST-ONCE — consulted, in the runner and outside the policy table, so a workflow cannot author repeated notification. Covered by the table's at-most-once row for all three sweeps. Mutation-verified, because a safeguard test that cannot fail is worse than none: reintroducing the P1 (dropping the auto-merge gate) fails 1 test; removing the at-most-once dedup fails 2. This is the fourth self-inflicted bug in this slice and they share one shape: every one was a SAFEGUARD OR FLOOR silently disappearing during a change that looked like a move — the activation floor, the tautological role-column check, the conditional role gate, and now this. None was caught by my own characterization floor; two were caught by the PRE-EXISTING suite and this one by review. The lesson is recorded here rather than in a commit nobody re-reads: a migration is not a move, and every guard the old body ran must be named and re-proven, not assumed to have travelled. Verified: 41 family tests green (7 new), 447 passed across three engine suites with the single known PRE-EXISTING `archiveStaleDoneTasks` failure, tsc clean, lint clean, merge gate green (299+10+71). Fusion-Task-Id: KB-U4
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
packages/engine/src/__tests__/recovery-policy-safety.test.ts (3)
165-167: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert
codealongsideactionhere.The
codeis what downstream notification routing keys on, and this is the test that proves the narrowing reaches the real outcome. Pinning it costs one key.♻️ Include the policy code in the assertion
expect(outcome).toEqual({ - decision: expect.objectContaining({ taskId: "FN-1", action: "surface" }), + decision: expect.objectContaining({ taskId: "FN-1", action: "surface", code: "stale-paused-todo" }), });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/recovery-policy-safety.test.ts` around lines 165 - 167, Update the outcome assertion in the recovery-policy safety test to verify the policy decision’s code alongside taskId and action. Use the expected narrowed code value produced by the test scenario, preserving the existing objectContaining assertion structure.
183-188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicates the loop at Lines 143-146 and does not test the default direction it names.
Both assertions for
"rebound"are already covered above. Fail-closed-for-a-future-action is a property of the allow-list, not of an existing member, so asserting the allow-list contents is what would actually catch someone widening the union without revisitingOBSERVATIONAL_ACTIONS.♻️ Assert the allow-list itself rather than a member already covered
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"); + const ALL: readonly RecoveryActionKind[] = ["surface", "rebound", "archive", "requeue", "resume"]; + expect(ALL.filter((a) => !isLifecycleMutatingAction(a))).toEqual(["surface"]); });Requires importing the
RecoveryActionKindtype.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/recovery-policy-safety.test.ts` around lines 183 - 188, Replace the duplicate “rebound” assertions in the test with an assertion on the allow-list itself, using RecoveryActionKind to type the expected observational actions. Verify OBSERVATIONAL_ACTIONS contains only the intended allow-listed actions so adding a future mutating action without updating the safeguard remains fail-closed.
176-181: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThis test never feeds
hostileto the code under test, so it cannot fail for the reason its name states.Line 177 only proves the policy resolved; the loop then calls
isSuppressedBySafeguardwith a literal{ userPaused: true }, which is byte-identical to the loop at Lines 143-146. Nothing connects the hostile keys to a suppression outcome. Route it throughdecideRecoverywith a mutatingonStale.actionso the policy actually reaches the chokepoint.♻️ Make the hostile policy reach the decision path
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"); - } + for (const action of ["rebound", "archive", "requeue", "resume"] as const) { + const authored = ir({ + ignoreUserPause: true, + respectUserPause: false, + onStale: { action, code: "stale-paused-todo" }, + }); + const outcome = decideRecovery(task({ userPaused: true } as Partial<Task>), authored, LATER); + expect(outcome).toEqual({ suppressed: "user-paused" }); + }Note the
ir()helper spreadsextraPolicyKeysafteronStale, so the override above replaces the default"surface"action as intended.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/__tests__/recovery-policy-safety.test.ts` around lines 176 - 181, Update the hostile-policy test around resolveColumnRecovery to exercise the policy through decideRecovery instead of directly checking isSuppressedBySafeguard with a literal userPaused object. Provide a mutating onStale.action override in the ir() input so the hostile policy reaches the recovery decision chokepoint, then assert the resulting suppression behavior while preserving the existing hostile-policy setup.packages/engine/src/recovery-reconciler.ts (1)
233-240: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueHardcoded
branch-group-ops.ts:128citation will rot in both files. TheuserPaused/pauseddivergence rationale is duplicated with a pinned line number that no tooling keeps current; the next edit to that file silently invalidates both references.
packages/engine/src/recovery-reconciler.ts#L233-L240: cite the symbol or the quoted invariant rather thanbranch-group-ops.ts:128.packages/engine/src/__tests__/recovery-policy-safety.test.ts#L190-L199: drop the line number and point at the source rationale instead of restating it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/engine/src/recovery-reconciler.ts` around lines 233 - 240, Remove the hardcoded branch-group-ops.ts:128 line citation from the rationale at the recovery reconciler’s userPaused gate, replacing it with the relevant symbol or quoted invariant. Apply the same citation cleanup in packages/engine/src/__tests__/recovery-policy-safety.test.ts lines 190-199 by referencing the source rationale without a line number; no other behavior changes are needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/engine/src/self-healing.ts`:
- Around line 8046-8072: Update runSurfacing and the adjacent
surfaceStalePausedTodos, surfaceStalePausedReviews, and surfaceInReviewStalled
calls to share one non-slim task snapshot and one irCache per startup or
maintenance cycle. Reuse SurfacingRunnerDeps.irCache when invoking
runSurfacingSweep, and clear the shared cache at the start or end of
runStartupRecovery and runMaintenance to preserve per-cycle freshness.
---
Nitpick comments:
In `@packages/engine/src/__tests__/recovery-policy-safety.test.ts`:
- Around line 165-167: Update the outcome assertion in the recovery-policy
safety test to verify the policy decision’s code alongside taskId and action.
Use the expected narrowed code value produced by the test scenario, preserving
the existing objectContaining assertion structure.
- Around line 183-188: Replace the duplicate “rebound” assertions in the test
with an assertion on the allow-list itself, using RecoveryActionKind to type the
expected observational actions. Verify OBSERVATIONAL_ACTIONS contains only the
intended allow-listed actions so adding a future mutating action without
updating the safeguard remains fail-closed.
- Around line 176-181: Update the hostile-policy test around
resolveColumnRecovery to exercise the policy through decideRecovery instead of
directly checking isSuppressedBySafeguard with a literal userPaused object.
Provide a mutating onStale.action override in the ir() input so the hostile
policy reaches the recovery decision chokepoint, then assert the resulting
suppression behavior while preserving the existing hostile-policy setup.
In `@packages/engine/src/recovery-reconciler.ts`:
- Around line 233-240: Remove the hardcoded branch-group-ops.ts:128 line
citation from the rationale at the recovery reconciler’s userPaused gate,
replacing it with the relevant symbol or quoted invariant. Apply the same
citation cleanup in packages/engine/src/__tests__/recovery-policy-safety.test.ts
lines 190-199 by referencing the source rationale without a line number; no
other behavior changes are needed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cde8e169-91bd-4a0c-8cd8-c9086c656a49
📒 Files selected for processing (7)
packages/core/src/in-review-stalled.tspackages/core/src/stale-paused-review.tspackages/engine/src/__tests__/recovery-policy-safety.test.tspackages/engine/src/__tests__/surfacing-family-shared.test.tspackages/engine/src/recovery-reconciler.tspackages/engine/src/self-healing.tspackages/engine/src/surfacing-sweeps.ts
…rfacing family (PR #2487 review) CodeRabbit is right and this is a fix, not a reply-and-resolve. Each of the three surfacing sweeps issued its own unfiltered non-slim `listTasks({ slim: false })` AND re-resolved every task's workflow from a fresh `Map` — `irCache` was never passed to `runSurfacingSweep`, so `resolveTaskThreshold` built a new one per call. All three run back-to-back in BOTH `runStartupRecovery` and the maintenance batch, so the family cost three full board reads and three IR resolution passes per cycle where one of each suffices. Measured on a 3-card board spanning one workflow: `listTasks` 3 -> 1 and `getWorkflowDefinition` 3 -> 1. This is the ordinary consequence of the migration rather than a new defect: consolidating three copies onto one runner is what made the redundancy visible for the first time. It was there before, spread across three bodies where nobody could see it. WHY SHARING A SNAPSHOT IS SAFE, as a PROPERTY rather than a hope — the three sweeps PARTITION the task space, so no card is visible to two of them and none can observe staleness another introduced: surfaceStalePausedTodos hold column AND paused === true surfaceStalePausedReviews review column AND paused === true surfaceInReviewStalled review column AND paused !== true A card sits in exactly one column, so hold and review are exclusive; within review, `paused` splits the other two. NEW tests assert the partition directly — each of the three card shapes is claimed by EXACTLY ONE sweep, and a card in a non-role column by none — so a future sweep that widens its eligibility into another's territory fails here instead of silently sharing a stale snapshot. Distinct `logPrefix` behavior is untouched: each sweep still writes and dedups under its own prefix, and the at-most-once check matches on that prefix, so one sweep's entry is invisible to another's suppression. The cycle is opened LAZILY through a once-only memo, so a batch where every surfacing sweep is skipped (global pause, or all thresholds disabled) still reads the board zero times rather than eagerly once. Each sweep keeps its own registry entry and remains individually callable — a standalone call opens its own cycle, which is what keeps the existing per-sweep tests and startup ordering intact. Verified: 45 family tests green (4 new), 510 passed across five engine suites with the single known PRE-EXISTING `archiveStaleDoneTasks` failure, tsc clean, lint clean, merge gate green (299+10+71). Fusion-Task-Id: KB-U4
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #2486. Base is
feature/u4-safeguard-scoped-to-mutation— do not merge before it.Retires the surfacing family —
surfaceStalePausedTodos,surfaceStalePausedReviews,surfaceInReviewStalled— onto one policy-driven runner. Migrated together, because they were three copies of one skeleton and a fix applied to one had to be remembered for the other two.#2484's characterization suite is the regression floor and passes unchanged.
Two unconverted sites found in core
Without these the migration would have been cosmetic for two of the three:
getStalePausedReviewSignalandgetInReviewStalledSignalboth hardcodedtask.column !== "in-review".getStalePausedTodoSignalgained the equivalentholdColumnparameter back in B1 — its two siblings were missed, so they silently stopped matching for any workflow that renames its review column. Both now takereviewColumn, defaulting to the legacy id.Three bugs this work introduced, and the tests caught
Each was silent in the diff:
task.columnas the role column, making the signal's own column check compare a value against itself — tautologically true, so the check was silently deleted. The resolved role column is now passed.The shared test is a table
One row per sweep; every invariant asserted for all three — threshold inheritance, declared-policy override, renamed role column, the negative case outside the role column, at-most-once, activation floor, both pause gates, non-positive-threshold disable, soft-delete. Adding a fourth surfacing sweep means adding a row.
Its log mock appends to the card's log, because the at-most-once dedup reads that log — a call-recording mock cannot observe suppression at all.
Measured line delta — this corrects the survey estimate downward
self-healing.tsThree sweeps migrated increased total lines by 135 while shrinking
self-healing.tsby 97. Per sweep: −32 inself-healing.ts. Break-even is 6.5 sweeps.Extrapolated to all 34 POLICY sweeps: −1,099 in
self-healing.ts, −890 net once the runner is amortized.My survey estimated −2,612 for that bucket. The measured figure is 34% of it. The reconciler's size was never the issue — the estimate assumed per-sweep bodies collapse to almost nothing, and they do not: each keeps a real eligibility predicate, signal call, and operator message.
Verification
archiveStaleDoneTasksfailureNo changeset:
@fusion/coreand@fusion/engineare private.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests