U4 trim: delete the dependency-blocked-todo feature (unreachable in production) and revert 5a2de7d - #2477
Conversation
📝 WalkthroughWalkthroughThe dependency-blocked todo reporting pipeline, exports, settings, implementation, and tests are removed. Blocker fan-out now uses a single hold column, while self-healing adds quiet-window diagnostics for stalled in-review tasks. ChangesCore reporting removal and classification updates
Self-healing diagnostics replacement
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
🚥 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 unreachable dependency-blocked-todo reporting feature.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains, and the previously reported empty parity test has been removed without disturbing its neighboring tests.
|
| Filename | Overview |
|---|---|
| packages/core/src/tests/settings-parity.test.ts | Removes the empty dependency-blocked settings test while preserving the populated neighboring parity tests. |
| packages/core/src/blocker-fanout.ts | Removes the report-only plural hold-column fallback while retaining singular and per-task classification paths. |
| packages/core/src/dependency-blocked-todo-report.ts | Deletes the unreachable dependency-blocked report implementation. |
| packages/core/src/settings-schema.ts | Removes defaults for the deleted reporting feature. |
| packages/core/src/types/settings-scope.ts | Removes the deleted feature's project-setting fields. |
| packages/engine/src/dependency-blocked-todo-reporter.ts | Deletes the unreachable engine reporter. |
| packages/engine/src/self-healing.ts | Removes the dormant reporter field, factory, and surfacing method. |
Reviews (3): Last reviewed commit: "test(core): delete the emptied dependenc..." | Re-trigger Greptile
9936611 to
ba6e64b
Compare
…t (PR #2477 review, P2) Review finding, verified. When the dependency-blocked-todo feature was deleted, its five settings keys went with it — and every assertion inside "keeps dependency-blocked todo report settings project-scoped with documented defaults" went with them, leaving an `it()` with an empty body. That is worse than no test: it reports a passing case in every run while verifying nothing, so the suite's green count overstates its coverage. A test that cannot fail is cruft with a checkmark. Removed rather than repurposed — there is no longer a feature for it to assert against. Verified: parity suite 78 passed with the single PRE-EXISTING `agentToolOutputMaxChars` overlap failure, which fails identically on the unmodified branch. Fusion-Task-Id: KB-U4
…e union (PR #2479 review, P1+P2) Two findings, one fix, exactly as diagnosed. P1 — BOARD-WIDE ROLE UNIONS MISCLASSIFY TASKS. My previous fix resolved roles across every workflow on the board and UNIONED them into one holdColumns / terminalColumns pair. That breaks the moment two workflows reuse a column id for DIFFERENT roles: one workflow calling `done` its hold column and another calling `done` terminal puts `done` in BOTH sets, so dependents count as held while the blocker sitting beside them is discarded as finished — from a single ambiguous id, in either direction. The union was wrong for the reason this entire program exists: a column id means something only RELATIVE TO ITS OWN WORKFLOW. A board-wide union quietly re-assumes ids are globally meaningful, which is precisely the assumption being removed. It answered a real problem (a single `holdColumn` cannot describe a multi-workflow board) with the wrong shape. Roles are now resolved PER TASK against that task's own workflow, which makes the misclassification impossible BY CONSTRUCTION rather than detectable afterwards. `ComputeBlockerFanoutOptions` and the report context each gain an optional `classify`/`classifyTask` callback that takes precedence over the set-shaped options; the sets remain for genuinely single-vocabulary callers (task-priority's unblock weighting) and as the legacy fallback, so no existing caller changes. P2 — REPEATED WORKFLOW READS. `listTasksImpl` resolved the workflow independently for every paused row, repeating workflow-definition and prompt-override reads for a board with many paused cards on one workflow. Fixed by threading ONE caller-owned `irCache` across the list pass — which U1's resolver already accepts for exactly this — so reads scale with the number of WORKFLOWS, not cards. The reporter's classifier does the same, so the P2 disappears as a side effect of the P1 fix rather than needing its own memo layer. Both P1 directions are tested, because a one-directional fix passes half of them: a hold column mistaken for terminal (report goes silent on real blockage) and a terminal column mistaken for hold (report invents blockage that does not exist). Written first and observed FAILING against the union. Mutation-verified: dropping the classifier fails the suite. Per-task fail-soft: a card whose workflow will not resolve falls back to the legacy roles for THAT CARD only, so one bad workflow degrades a single card to today's behavior instead of breaking the report. NOTE FOR THE STACK: `dependency-blocked-todo-reporter.ts` is deleted entirely by #2477 later in this stack, having been proven unreachable in production. This fix is still correct and worth landing — #2479 merges first, so the window exists — but its lifetime is bounded. The reads.ts half is NOT deleted and is permanent. Verified: 4 new tests green; 38 core and 18 engine tests green across the related suites; 7 PG hydration tests green (the reads.ts path); tsc clean in core and engine; lint clean; merge gate green (299+10+71). Fusion-Task-Id: KB-B1
…guards, red-green) (#2479) **Stacked on #2469** → #2468 → #2467. Base is `feature/workflow-capacity-ground-truth`. This is **slice B1 of Phase B, not all of Phase B.** Sizing escalation sent separately; the census is below. ## Why this is a slice Measured census of code lines referencing a lifecycle column literal (comments excluded): | Unit | Files | Sites | |---|---|---:| | U4 | `self-healing.ts` | 203 | | U5 | `executor.ts` 171, `scheduler.ts` 55, `replan-target.ts` 20, `merger-ai.ts` 5, `hold-release.ts` 4, `mesh-lease-manager.ts` 4, `task-agent-sync.ts` 3 | 262 | | U6 | `moves.ts` 34, `default-workflow-hooks.ts` 13, `board-config.ts` 9, `blocker-fanout.ts` 6, `task-priority.ts` 5, `dependency-blocked-todo-report.ts` 2, `stale-paused-todo.ts` 1 | 70 | | | **Total** | **535** | The plan's "~207" counts the guard category only. Under the phase's non-negotiable rule — a test that **fails before** conversion, per guard — that is ~200 red-green cycles. Doing it as one sweep would reproduce exactly the failure this phase exists to prevent: converted guards nobody proved still fire. `moves.ts` and `default-workflow-hooks.ts` stay **parked** per the dispatch constraint (move-path convergence and the pool-id sentinel are on an operator decision). ## Guards converted (4), each red-green Every case below was written **first** and observed failing against the literal implementation. | Module | Guard | Before → After | |---|---|---| | `stale-paused-todo.ts` | stall detection | `column !== "todo"` → resolved **hold** column | | `blocker-fanout.ts` | active | `ACTIVE_COLUMNS.has(col)` → `!terminalColumns.has(col)` | | `blocker-fanout.ts` | hold-wait metric | `col === "todo"` → resolved **hold** column | | `task-priority.ts` | unblock active | `UNBLOCK_ACTIVE_COLUMNS` **deleted**, folded into the terminal set | Three of the seven new cases are **regression floors** that pass before and after. One of them earned its keep immediately: it failed on my own fixture (`activeCount` vs the public `totalCount`), catching a bad test rather than bad code — which is the point of asserting the default path alongside the renamed one. ### The `task-priority` finding `UNBLOCK_ACTIVE_COLUMNS` and `DONE_COLUMNS` encoded **one concept twice**, two lines apart, and disagreed for any custom column: dependency counting treated a `drafting` card as unmet (correct) while the active check treated it as inactive (wrong), zeroing the blocker's unblock weight. The enumeration wasn't just legacy-shaped — it contradicted its own neighbour. ##⚠️ Behavior change, not a pure refactor Inverting active from enumeration to exclusion means **a card in a column that is neither terminal nor in the legacy enum now counts as active where it previously did not.** That is the plan's stated intent, but it is a real change for any project already using a custom column — **Coding (Ideas)' `ideas` column is the in-tree case.** Fan-out counts and unblock weights for such cards will rise. ## Verification - Four affected suites green (45 tests), each conversion observed red→green. - `pnpm lint`, `tsc --noEmit` (core) green. **Not verified / not done, stated plainly:** - **Call sites are not wired.** These modules now *accept* resolved roles; every parameter still defaults to the legacy set, so at the call sites the vocabulary is unchanged. A caller that cannot resolve a workflow keeps literal behavior. Threading `resolveLifecycleColumns` through `reads.ts` and `self-healing.ts` is follow-on work — until then the guards are *convertible*, not *converted end-to-end*. - `dependency-blocked-todo-report.ts` and `board-config.ts` are untouched in this slice. - 19 core-suite failures exist on this branch; all confirmed **pre-existing** by stashing and re-running on a clean tree (`duplicate-guard`, `log-severity-spam-contract`, `settings-parity`, `task-delete-caller-attribution`, `settings-defaults`). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --- **Supersedes #2470**, which GitHub force-closed when its base branch was deleted by the merge of #2469 and refuses to reopen. Same head branch, same commits (rebased onto `main`), now based on `main` directly. The two P1 review threads on #2470 were resolved there — one of them with a correction noting the threading half landed in code that was subsequently deleted as a dead feature in #2477. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Bug Fixes** * Dependency and blocker reports now correctly recognize custom hold, active, and terminal workflow columns. * Blockers in renamed terminal columns are no longer incorrectly reported as active. * Stale paused-task badges and self-healing now work with workflow-specific hold columns. * Mixed boards with different workflow column names are handled consistently. * Existing default workflow behavior remains compatible, including fallback handling when workflow details cannot be resolved. * **Enhancements** * Reporting and task-priority calculations now support configurable single or multiple hold and terminal columns. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
ba6e64b to
eecfccc
Compare
… production (U4 trim) Removes an entire feature that has never executed, and reverts 5a2de7d, which only threaded resolved lifecycle columns through it. REACHABILITY EVIDENCE, which is 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 at ~450x 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 the 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: commit 5a2de7d threaded resolved lifecycle roles into `computeDependencyBlockedTodoReport` and its reporter, in response to 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 — we only checked correctness. A correction has been posted to that thread on #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. `blocker-fanout.ts` SURVIVES — it is live via `task-priority.ts`. Only the plural `holdColumns` option added by 5a2de7d is reverted, because the deleted report was its sole consumer; `holdColumn` (singular, B1) stays. Net: 1,244 deletions / 5 insertions across 15 files — ~450 production lines, ~684 test lines, 5 settings keys. Verified: tsc clean in core, engine, and dashboard-app; lint clean; merge gate green (299+10+71); self-healing suite 411 passed with the single known PRE-EXISTING `archiveStaleDoneTasks` failure; 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. Fusion-Task-Id: KB-U4
…t (PR #2477 review, P2) Review finding, verified. When the dependency-blocked-todo feature was deleted, its five settings keys went with it — and every assertion inside "keeps dependency-blocked todo report settings project-scoped with documented defaults" went with them, leaving an `it()` with an empty body. That is worse than no test: it reports a passing case in every run while verifying nothing, so the suite's green count overstates its coverage. A test that cannot fail is cruft with a checkmark. Removed rather than repurposed — there is no longer a feature for it to assert against. Verified: parity suite 78 passed with the single PRE-EXISTING `agentToolOutputMaxChars` overlap failure, which fails identically on the unmodified branch. Fusion-Task-Id: KB-U4
f43f4db to
d3b9783
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/blocker-fanout.ts (1)
49-49: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse a current FNXC timestamp.
2026-07-28-17:50is future-dated relative to July 27, 2026. Update it to the actual change/review timestamp so the requirement-history comment remains trustworthy.As per coding guidelines, dated FNXC comments must use a current
yyyy-MM-dd-hh:mmtimestamp.🤖 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/core/src/blocker-fanout.ts` at line 49, Update the dated FNXC comment in the WorkflowLifecycleColumns area to use the actual current change or review timestamp in yyyy-MM-dd-hh:mm format, replacing the future-dated 2026-07-28-17:50 value while preserving the existing requirement-history context.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@packages/core/src/blocker-fanout.ts`:
- Line 49: Update the dated FNXC comment in the WorkflowLifecycleColumns area to
use the actual current change or review timestamp in yyyy-MM-dd-hh:mm format,
replacing the future-dated 2026-07-28-17:50 value while preserving the existing
requirement-history context.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3a4154c4-53dd-449b-b5ab-98599360019c
📒 Files selected for processing (15)
packages/core/src/__tests__/dependency-blocked-todo-report-renamed-columns.test.tspackages/core/src/__tests__/dependency-blocked-todo-report.test.tspackages/core/src/__tests__/settings-parity.test.tspackages/core/src/blocker-fanout.tspackages/core/src/dependency-blocked-todo-report.tspackages/core/src/index.gate.tspackages/core/src/index.tspackages/core/src/settings-schema.tspackages/core/src/types/settings-scope.tspackages/dashboard/app/components/settings/sections/__tests__/settings-default-descriptions.test.tsxpackages/engine/src/__tests__/dependency-blocked-todo-reporter-renamed-columns.test.tspackages/engine/src/__tests__/dependency-blocked-todo-reporter.test.tspackages/engine/src/__tests__/self-healing.test.tspackages/engine/src/dependency-blocked-todo-reporter.tspackages/engine/src/self-healing.ts
💤 Files with no reviewable changes (14)
- packages/engine/src/tests/dependency-blocked-todo-reporter-renamed-columns.test.ts
- packages/core/src/tests/dependency-blocked-todo-report-renamed-columns.test.ts
- packages/core/src/tests/dependency-blocked-todo-report.test.ts
- packages/core/src/tests/settings-parity.test.ts
- packages/engine/src/tests/dependency-blocked-todo-reporter.test.ts
- packages/engine/src/dependency-blocked-todo-reporter.ts
- packages/core/src/dependency-blocked-todo-report.ts
- packages/core/src/index.ts
- packages/dashboard/app/components/settings/sections/tests/settings-default-descriptions.test.tsx
- packages/core/src/types/settings-scope.ts
- packages/core/src/index.gate.ts
- packages/engine/src/self-healing.ts
- packages/core/src/settings-schema.ts
- packages/engine/src/tests/self-healing.test.ts
…iant (measured: engine ~780, real cost is a settings migration) (#2478) 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 | | lines | what | |---|---:|---| | `WorkflowColumnRecovery` (IR) | 42 | one key — `stalenessMs` + `onStale`. Optional and omitted when unset, so existing workflows serialize byte-identically. | | `recovery-reconciler.ts` | 176 | one engine: walks live cards, resolves each card's policy from **its own** workflow (per task, shared `irCache` — 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.ts` | 156 | one-time. The **ratified invariant**. | ## Measured 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, 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. 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: 1. Does workflow policy **override** the global setting, or defer to it? 2. What happens to **existing projects** that already configured those settings? 3. 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 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: - **structural** — the policy exposes only an allow-listed key set; adding a key requires editing the test and re-stating the safety argument (the friction is the point); - **behavioral** — a policy attempting every spelling of "ignore the user pause" has no effect. **Both halves 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** - adding an unreviewed key to the policy schema → **fails** 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 `surface` action is implemented, so only its relevant safeguard is wired. `surface` mutates 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 when `rebound` lands. ## Verification - `tsc --noEmit` clean in core and engine; `pnpm lint` clean - merge gate green (299 + 10 + 71) - 23 safety tests green; `workflow-lifecycle-traits` green No changeset: `@fusion/core` and `@fusion/engine` are private. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
## Summary - remove the remaining dependency-blocked reporter test after the reporter was deleted in #2477 - prevent Vitest from failing during module collection on a deleted import ## Test plan - `pnpm --filter @fusion/engine typecheck` - `pnpm --filter @fusion/engine build`
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
self-healing owns two name-based sweep registries (
runStartupRecovery, 58 entries;runMaintenance, 76).surfaceDependencyBlockedTodosis 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
dependencyBlockedTodoReportEnableddefaultedtruewhile 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
5a2de7dthreaded resolved lifecycle roles intocomputeDependencyBlockedTodoReportand 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
buildUnblockWeightMapintask-priority.ts, which is live and was already threadingterminalColumns(B1,434b385). Scheduler ordering was never affected, before or after.What survives
blocker-fanout.tsstays — it is live viatask-priority.ts. Only the pluralholdColumnsoption added by5a2de7dis 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 --noEmitclean in core, engine, and dashboard-app;pnpm lintcleanarchiveStaleDoneTasks)settings-parity.test.tshas one pre-existing failure (agentToolOutputMaxCharsoverlap) that fails identically with these changes stashed — unrelated to this deletionNo changeset:
@fusion/coreand@fusion/engineare private.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Changes