U4 trim: delete the dead resetStepsIfWorkLost duplicate in self-healing - #2474
Conversation
|
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 (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThe self-healing manager no longer checks task branches for missing unique commits or resets ChangesSelf-healing behavior cleanup
Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Greptile SummaryRemoves the unused private Confidence Score: 5/5The PR appears safe to merge once its declared stacked base dependency is satisfied. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/engine/src/self-healing.ts | Deletes an unreferenced private lost-work recovery duplicate; no reachable self-healing caller or resulting orphaned dependency was identified. |
Reviews (3): Last reviewed commit: "refactor(engine): delete the dead resetS..." | Re-trigger Greptile
fc6edad to
fede53c
Compare
9936611 to
ba6e64b
Compare
…ealing (U4 trim) Provably-dead code, removed with reachability evidence rather than a redundancy argument. `SelfHealingManager.resetStepsIfWorkLost` was `private` with ZERO in-file references beyond its own declaration — TypeScript already reported it as "declared but its value is never read", so the compiler had been flagging this for some time. The live implementation is `executor.ts:20080`, an independent copy that executor.ts actually calls (12667, 14664). The self-healing copy is an orphaned duplicate of it. 50 lines including its docblock and the section header that had no other member. Safe against every U4 constraint: it enforces none of user pause, autoMerge:false, dependency, capacity, merge-proof, or an at-most-once safeguard. It is not in either sweep registry, has no test of its own, and no caller anywhere in engine/dashboard/cli. This is the first of two deletions approved from the U4 reshape survey. The SECOND (`surfaceDependencyBlockedTodos`) is deliberately NOT in this commit: on close inspection it is not a 28-line orphan but the tip of an entire unreachable feature (~450 production lines across three files plus 684 lines of tests), and deleting only its tip would orphan more code than it removes. Escalated for a decision rather than resolved here. Verified: tsc clean, lint clean, self-healing suite 415 passed with the single known PRE-EXISTING `archiveStaleDoneTasks` failure (identical before this change). Fusion-Task-Id: KB-U4
ba6e64b to
eecfccc
Compare
…roduction) and revert 5a2de7d (#2477) Stacked on #2474. Base is `feature/workflow-vocabulary-u4-dead-code` — do not merge before it. Deletes an **entire feature that has never executed in production**, and reverts `5a2de7d`, which only threaded resolved lifecycle columns through it. ## Reachability evidence — the whole basis for this ``` surfaceDependencyBlockedTodos ← in NEITHER sweep registry; no caller in └─ getDependencyBlockedTodoReporter() engine/dashboard/cli — only tests └─ engine/dependency-blocked-todo-reporter.ts ← sole caller of ↓ └─ core/computeDependencyBlockedTodoReport ``` self-healing owns two name-based sweep registries (`runStartupRecovery`, 58 entries; `runMaintenance`, 76). `surfaceDependencyBlockedTodos` is in **neither**, so nothing ever invoked the chain below it. Its four tests passed while proving nothing about production. ## Why delete rather than wire it up Wiring was the tempting option and is the riskier one. Switching on a 450-line path that has never run — whose tests therefore establish nothing about its behavior against real data — is a **behavior change with unquantified blast radius**. This program already refused exactly that move for the **pool-id sentinel**, a one-line change that would switch on dormant enforcement across every project. This is the same class of move at ~450× the size. Deleting is also the recoverable direction: git keeps the feature, and it can be resurrected deliberately — with tests that prove it *runs* — if dependency-blocked reporting is actually wanted. ## The settings keys go with it `dependencyBlockedTodoReportEnabled` defaulted `true` while driving nothing. A schema/API-visible switch that lies about what the system does is worse than no switch. (It had no dashboard UI field — the dashboard test allowlist already recorded it as *"no UI field"*.) Four sibling tuning keys are removed with it. ## Against my own earlier work `5a2de7d` threaded resolved lifecycle roles into `computeDependencyBlockedTodoReport` and its reporter, answering a review finding I confirmed as real. **The code was correct; the impact claim was not**, because the path never executes. Neither the reviewer nor I checked *reachability* before agreeing the defect mattered — only correctness. A correction is posted on that thread in #2470. **Scope limit on that admission:** the same finding also described *incorrect scheduler ordering*. That half runs through `buildUnblockWeightMap` in `task-priority.ts`, which is **live** and was already threading `terminalColumns` (B1, `434b385`). Scheduler ordering was never affected, before or after. ## What survives `blocker-fanout.ts` **stays** — it is live via `task-priority.ts`. Only the plural `holdColumns` option added by `5a2de7d` is reverted, since the deleted report was its sole consumer. `holdColumn` (singular, from B1) remains. ## Net **1,244 deletions / 5 insertions across 15 files** — ~450 production lines, ~684 test lines, 5 settings keys. ## Verification - `tsc --noEmit` clean in **core, engine, and dashboard-app**; `pnpm lint` clean - merge gate green (299 + 10 + 71) - self-healing suite: 411 passed, 1 **pre-existing** failure (`archiveStaleDoneTasks`) - dashboard settings-descriptions suite green - `settings-parity.test.ts` has one **pre-existing** failure (`agentToolOutputMaxChars` overlap) that fails identically with these changes stashed — unrelated to this deletion No changeset: `@fusion/core` and `@fusion/engine` are private. 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **New Features** * Added quiet-window backlog health diagnostics for stalled items in review, with repeat-alert suppression. * Added default thresholds for backlog-pressure alerts. * **Changes** * Removed dependency-blocked todo reporting and related alerts. * Removed the dependency-blocked todo enable/disable setting; remaining tuning options are no longer active. * Updated the workflow hold classification to use a single todo column. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
Stacked on #2472 (Phase B slice B3.1). Base is
feature/workflow-vocabulary-b3-stranded-todo— do not merge before it.First output of the U4 reshape survey: dead code removed with reachability evidence, not a redundancy argument.
What is deleted
SelfHealingManager.resetStepsIfWorkLost— 50 lines including its docblock and a section header left with no other member.Evidence:
private, with zero in-file references beyond its own declaration.declared but its value is never read— the compiler has been flagging this.executor.ts:20080, an independent copy thatexecutor.tsactually calls (12667, 14664). The self-healing copy is an orphaned duplicate of it.Safe against every U4 constraint: it enforces none of user pause,
autoMerge:false, dependency, capacity, merge-proof, or an at-most-once safeguard.Why the second approved deletion is NOT here
surfaceDependencyBlockedTodoswas approved alongside this one as a 28-line orphan. It is not. On inspection it is the tip of an entire unreachable feature:≈ 450 production lines across three files, plus 684 lines of tests in four files. The operator-visible setting
dependencyBlockedTodoReportEnableddefaultstrueinsettings-schema.tsand drives nothing.Deleting only the approved 28-line tip would be strictly worse than leaving it — it orphans the getter and field and strands 407 lines of module with no remaining reference to explain why. Escalated for a decision (delete the subtree / wire the feature up / leave it) rather than resolved unilaterally.
Verification
tsc --noEmitclean,pnpm lintcleanarchiveStaleDoneTasks— fails identically before this change)No changeset:
@fusion/engineis private.🤖 Generated with Claude Code
Summary by CodeRabbit