Conversation
|
Warning Review limit reached
Next review available in: 6 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 (4)
📝 WalkthroughWalkthroughThe self-healing zero-commit audit now resolves review lanes by workflow and project column roles, supports renamed lanes, and preserves filtering for declared no-commit cards. Tests cover branch-ahead checks, candidate flagging, and exclusions. ChangesZero-commit audit
Estimated code review effort: 2 (Simple) | ~10 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/engine/src/self-healing.ts (1)
11341-11362: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winConsider filtering cheap conditions before resolving each candidate's workflow IR.
noCommitsLanesresolvesresolveWorkflowIrForTaskWithProvenance(and thus a workflow definition read) for every entry incandidateMap— every lane-matched task plus every board-widestatus === "failed"task — before the cheapnoCommitsExpected/stepsfilters run. Several sibling sweeps in this file (e.g.recoverStrandedCompletedTodoTasks) explicitly document running cheap rejections first so only survivors pay for IR resolution. Reordering here (filternoCommitsExpected/steps first, then resolve lanes only for the reduced set) would avoid resolving workflows for tasks that get discarded anyway, and a sharedMapcache passed as the third arg toresolveWorkflowIrForTaskWithProvenancewould also avoid redundant re-resolution when multiple candidates share a workflow (as most other sweeps in this file already do, though not universally — e.g.recoverMergedReviewTaskshas the same gap).♻️ Suggested reordering
- const noCommitsLanes = new Map<string, Set<string>>(); - for (const entry of candidateMap.values()) { - try { - const { ir, source } = await resolveWorkflowIrForTaskWithProvenance(this.store, entry.id); + const cheapCandidates = [...candidateMap.values()].filter((task) => { + if (task.noCommitsExpected === true) return false; + return task.steps.length > 0 && task.steps.every((step) => step.status === "done" || step.status === "skipped"); + }); + const noCommitsIrCache = new Map<string, WorkflowIr>(); + const noCommitsLanes = new Map<string, Set<string>>(); + for (const entry of cheapCandidates) { + try { + const { ir, source } = await resolveWorkflowIrForTaskWithProvenance(this.store, entry.id, noCommitsIrCache); noCommitsLanes.set( entry.id, source === "default" ? new Set(noCommitsColumns) : new Set(REVIEW_ROLES.flatMap((role) => [...columnsWithFlag(ir, role)])), ); } catch { noCommitsLanes.set(entry.id, new Set(noCommitsColumns)); } } - const candidates = [...candidateMap.values()].filter((task) => { - if (task.noCommitsExpected === true) return false; - if (task.steps.length === 0 || !task.steps.every((step) => step.status === "done" || step.status === "skipped")) return false; + const candidates = cheapCandidates.filter((task) => { const noCommitsError = typeof task.error === "string" && /no_commits/i.test(task.error); return (noCommitsLanes.get(task.id) ?? noCommitsColumns).has(task.column) || noCommitsError; });🤖 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/self-healing.ts` around lines 11341 - 11362, In the candidate-processing flow around noCommitsLanes, apply the cheap noCommitsExpected and steps rejection checks before calling resolveWorkflowIrForTaskWithProvenance, so only surviving tasks resolve workflow IR. Create and reuse a shared Map cache as the resolver’s third argument to prevent repeated workflow-definition reads for candidates sharing a workflow, while preserving the existing lane mapping and fallback behavior.
🤖 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.
Nitpick comments:
In `@packages/engine/src/self-healing.ts`:
- Around line 11341-11362: In the candidate-processing flow around
noCommitsLanes, apply the cheap noCommitsExpected and steps rejection checks
before calling resolveWorkflowIrForTaskWithProvenance, so only surviving tasks
resolve workflow IR. Create and reuse a shared Map cache as the resolver’s third
argument to prevent repeated workflow-definition reads for candidates sharing a
workflow, while preserving the existing lane mapping and fallback behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 453a2e15-94a8-4b79-a810-a24835debe20
📒 Files selected for processing (4)
.changeset/self-healing-no-commits-audit-query.mdpackages/engine/src/__tests__/self-healing-query-filter-blindness.test.tspackages/engine/src/self-healing.tsscripts/lib/lifecycle-column-census-baseline.json
… (twenty-eighth sweep) auditNoCommitsExpectedCandidates flags a card that finished every step and pushed NO commits — either a legitimately commit-free task nobody declared as such, or work that silently produced nothing. HALF-BLIND, NOT DEAD, which is harder to notice. It also reads all failed tasks, and that read is lane-independent, so on a renamed board the `no_commits` ERROR arm kept working while the lane arm went silent: a card sitting quietly in a renamed review lane with zero commits and no error was never flagged. A sweep that still reports something is not obviously broken. Reverts measured: with the literal read and verdict restored the new case fails — the card contributes nothing, because it has no `no_commits` error for the other arm to catch. Fusion-Task-Id: KB-SELF-HEALING-QUERIES
|
Superseded by #2944 — folded into The conversion and its revert measurements are carried over in the commit message. Nothing here is dropped. |
|
Rebase blocked on a conflict shape the earlier pattern does not cover — recording it precisely rather than forcing it.
Both are needed. The difficulty is that the conflict boundary cuts through the middle of the construct: main's I attempted that join and the file came out +2 unbalanced. Rather than iterate on a fourth variant, I restored both branches to their origin state. Where the family stands: 17 of 22 branches are current with main and green. Five are behind —
Nothing broken was pushed at any point; every branch was verified before push and reset when it could not be. The consolidation remains the right answer — the source commits fold cleanly and only this one test file resists, which is precisely why sixteen branches sharing it cannot each be rebased into a moving main. |
…olds 23 PRs) (#2944) **Consolidation of 23 open PRs into one.** Every one shared a single root cause and mostly touched a single file; 23 CI runs for that was indefensible. Folds and supersedes: #2867 #2869 #2876 #2879 #2883 #2891 #2899 #2901 #2902 #2905 #2906 #2914 #2916 #2918 #2919 #2920 #2922 #2927 #2929 #2932 #2934 #2937 #2939. (#2865, #2882, #2897, #2909, #2912 already merged and are not re-folded.) ## The root cause A self-healing sweep selects its work with `listTasks({ column: "in-review" })`. On a board whose lanes are renamed that returns **nothing**, so the sweep never runs — no error, no log line, no failed task. Several sweeps had already had their *predicates* converted to resolved lanes, which dropped a census count and changed nothing, because the query above the loop had already returned an empty list. **26 sweeps converted.** Each one: read the project's columns for the role, then decide each card against **its own** workflow, with the legacy ids unioned so a board mid-rename is never skipped. ## What each sweep stops silently failing to do | | | | --- | --- | | stale merger status | one finished card held the **merge queue** for everything behind it | | stale `blockedBy` / completed-task release | dependents stayed blocked on work that had already finished — the board stops moving | | workspace partial lands | a task left with **some repos merged and some not** | | mid-merge retry stamp | the card stalled *and* the operator's manual Retry was gated by the same stamp | | in-progress limbo / no-progress failures | dead cards held a work slot forever | | partial-progress retry | real work parked failed with its **retry budget unspent** | | orphaned-execution signal | visibility only — the one signal pointing at an orphan went silent | | zero-commit audit | went **half-blind**: the error arm kept working, the lane arm did not | Plus: ghost review cards, transient merge failures, misclassified failures, branch misbinding, missing-worktree failures, merged-but-unfinished finalization, done-metadata repair, self-owned branch conflicts, orphan-only scope violations, post-done wedges, idle assigned agents, PR-conflict worktree ownership, and orphaned workspace worktrees. ## Two defects the conversion itself introduced, both caught and fixed 1. **Missed pairs.** Widening a read without converting the guards beneath it is *worse than not converting*: the sweep starts admitting renamed-board cards and then mis-decides every one. Review caught a second guard on a re-read row; the audit that triggered found **five more**, one of which gates the `reviewProof` triple-proof — a renamed review card would have been moved backward with the safety check silently skipped. Column guards 86 → 81. 2. **Duplicate processing.** The literal reads were disjoint by construction; resolved reads are not, so a column carrying two role flags put one card in two buckets — duplicate moves, duplicate audit rows, inflated counts. Both now have ratchets. `self-healing-converted-sweeps-have-no-literal-lane-guards.test.ts` **derives** its sweep list (a sweep counts as converted when its body calls `resolveProjectColumnsForRoles`), so it cannot go stale, and it carries two positive controls because a broken regex finds no offenders and a broken derivation iterates nothing — an empty loop registers no tests and reads green. ## Deliberately unchanged - 22 `moveTask` destinations carrying `recoveryRehome: true` — `moves.ts` exempts these so a card stranded in an undeclared column stays rescuable. - One literal in `clearStaleBlockedBy`'s log-dedup closure (allowed by name in the ratchet, with the reason). - `surfaceInReviewStalls` — hot list-read path, needs a batched prefetch; that is a performance design decision, not a conversion. - `scheduler.ts` and `replan-target.ts` — built on `resolveTaskWorkflowIrSync`, which returns the default IR for every task in production. Converting there produces inert code. ## The fold itself is worth one note All 23 branches appended to the **same test file at the same anchor**, so every automatic strategy — git 3-way, `merge-file --union`, and three hand-written resolvers — interleaved them mid-block. Two attempts committed conflict markers before I caught it. The file is therefore **reconstructed**: head authored once, body assembled as the union of each branch's own intact top-level segments keyed by test title, with the nested `already-merged hard blocker` describe appended whole (flattening it orphaned its helper). Verified by *parsing after every step* rather than trusting the merge — which is how each interleaving was caught. ## Verification `pnpm test:gate` 161 + 487 + 13 + 71. Scoped suites 592 passed (self-healing, the blindness suite at 68 cases, the ratchet, and the notification suite). `tsc` engine clean; `pnpm lint`, `check:changesets`, `lifecycle-column-census --strict` and `check-sql-column-literals` all clean, each run explicitly. Each folded conversion was individually revert-proven on its original branch — the read reverted alone, and the per-card verdict reverted alone — and those measurements are recorded in the commit messages carried into this branch. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
auditNoCommitsExpectedCandidatesflags a card that finished every step and pushed no commits — either a legitimately commit-free task nobody declared as such, or work that silently produced nothing.Half-blind, not dead — which is harder to notice
This sweep has two arms. It also reads all failed tasks, and that read is lane-independent, so on a renamed board the
no_commitserror arm kept working while the lane arm went silent. A card sitting quietly in a renamed review lane with zero commits and no error was never flagged.Every other sweep in this series went fully dead on a renamed board, which at least produces a conspicuous absence. This one kept reporting, just less — the failure mode that survives longest.
Revert result
no_commitserror for the other arm to catchA non-vacuous companion (same card,
noCommitsExpected: true) rules out a sweep that flags every zero-commit card it finds — the declaration is the entire point of the flag.isBranchAheadOfBaseis mocked rather than spied: it is a static named import that shells out to git, so the ESM binding is already resolved before a spy could replace it.Verification
pnpm test:gate161 + 487 + 13 + 71, plusself-healing.test.ts412;tscengine clean;pnpm lint,check:changesets, census--strictclean, each run explicitly.Summary by CodeRabbit
Bug Fixes
Tests
Chores