fleet: cli task.ts 8 → 0 + dashboard.ts 8 → 0 — and converting the retry classifier alone would have turned a silent no-op into a crash - #2736
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (6)
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 SummaryThis PR makes CLI lifecycle handling workflow-aware for renamed columns.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains.
|
| Filename | Overview |
|---|---|
| packages/cli/src/commands/task.ts | Replaces lifecycle-column literals with per-task workflow-role resolution across retry and related CLI behavior. |
| packages/cli/src/commands/dashboard.ts | Centralizes workflow-aware active-task counting and resolves scoped tasks against their originating project store. |
| packages/engine/src/restart-recovery-coordinator.ts | Allows the missing-worktree classifier to consume a resolved review-lane answer while preserving existing caller behavior. |
| packages/cli/src/tests/task-retry-renamed-review.pg.test.ts | Adds real-store coverage for retry behavior under default and renamed workflow vocabularies. |
| .changeset/cli-renamed-column-lanes.md | Records the published CLI fixes as a patch release using the required labeled format. |
| scripts/lib/lifecycle-column-census-baseline.json | Updates the literal-column census after converting the affected CLI comparisons. |
Reviews (3): Last reviewed commit: "fix(cli): keep both retry classifiers on..." | Re-trigger Greptile
#2736 review) greptile P1: converting only the GENERIC retry classifier split it from `isInReviewMissingWorktreeSessionStartFailure`, which is still keyed on the literal. On a renamed lane the generic branch fires and the specialized one does not. THE SPLIT IS REAL. THE REPORTED CONSEQUENCE IS NOT — measured, not assumed. The claim was that the generic branch leaves `worktree`/`branch` intact so the next run repeats the missing-worktree failure. It does not: BOTH branches end with them cleared, because the backward move to the hold column clears them anyway. I instrumented the after-state under both vocabularies with the fix reverted and got `{"col":"todo"}` / `{"col":"drafting"}` — no worktree, no branch, either way. So the two new cases do NOT fail with the fix reverted, and I am not claiming they do. They pin the outcome that matters (a missing-worktree failure in either vocabulary comes back retryable with no stale session metadata), and the classifiers are unified because two definitions of "in review" inside one function is a latent split worth closing on its own terms — not because I found a failing case. `isReviewColumn` is OPTIONAL on the engine predicate: `extension.ts` asks both questions with the literal and is internally consistent that way, so the default preserves its meaning exactly while the converted caller passes the resolved answer. greptile P2: adds the missing patch changeset for the published CLI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The split is real and is now closed. The stated consequence is not — and I checked rather than assuming, because the fix and the claim come apart here. What is real: converting only the generic classifier left What did not reproduce: the claim that the generic branch leaves No So the two new test cases do not fail with the fix reverted, and I am not claiming they do — the test's own comment says so, and so does the commit message. Reporting a proof I don't have would be worse than the bug. I fixed it anyway, because a latent split is worth closing on its own terms and the fix is one optional parameter. The two new cases still earn their place — they pin that a missing-worktree failure in either vocabulary comes back retryable with no stale session metadata, which nothing covered before. Verification: new suite 4 passed; |
|
Fixed — added The |
…rget the tests exposed THE FINDING: converting the retry CLASSIFIER turned a silent no-op into a hard error, and that is how the second bug surfaced. `fn task retry` decided (correctly, post-fix) to retry a card in a renamed review lane, then moved it to the literal "todo" and threw `Invalid transition: 'checking' -> 'todo'. Unknown column for this workflow.` Both the classifier and the target are converted here; shipping only the first would have been strictly worse than shipping neither. Caught only because the test drives the whole command rather than the predicate. dashboard.ts: the same `in-progress || in-review` predicate was copied into FOUR places — TUI refresh-stats, periodic refresh, shutdown summary, initial stats. On a renamed board all four reported active=0 while the per-column counts printed beside them showed the work plainly. Extracted to one resolved helper: de-duplication of an existing four-fold copy, not a new abstraction. One of the four listed from a board-SCOPED store, so the resolution is threaded to the same store it listed from rather than the outer one. task.ts: the archived-lineage label, the near-duplicate candidate filter (a renamed board never excluded completed work, so finished tasks stayed duplicate candidates), both node-override in-progress guards, and the in-review retry classifier. The near-duplicate filter reorders recency BEFORE the completion filter. Both filters still precede slice(200), so the resulting set and order are identical; the reorder bounds the per-task workflow resolution by the 7-day window instead of the whole board. Every conversion reverted individually: classifier -> 1 failed, target -> 1 failed, both restored -> 2 passed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
#2736 review) greptile P1: converting only the GENERIC retry classifier split it from `isInReviewMissingWorktreeSessionStartFailure`, which is still keyed on the literal. On a renamed lane the generic branch fires and the specialized one does not. THE SPLIT IS REAL. THE REPORTED CONSEQUENCE IS NOT — measured, not assumed. The claim was that the generic branch leaves `worktree`/`branch` intact so the next run repeats the missing-worktree failure. It does not: BOTH branches end with them cleared, because the backward move to the hold column clears them anyway. I instrumented the after-state under both vocabularies with the fix reverted and got `{"col":"todo"}` / `{"col":"drafting"}` — no worktree, no branch, either way. So the two new cases do NOT fail with the fix reverted, and I am not claiming they do. They pin the outcome that matters (a missing-worktree failure in either vocabulary comes back retryable with no stale session metadata), and the classifiers are unified because two definitions of "in review" inside one function is a latent split worth closing on its own terms — not because I found a failing case. `isReviewColumn` is OPTIONAL on the engine predicate: `extension.ts` asks both questions with the literal and is internally consistent that way, so the default preserves its meaning exactly while the converted caller passes the resolved answer. greptile P2: adds the missing patch changeset for the published CLI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
09dab59 to
93781f5
Compare
|
Both findings are already addressed on this branch — I verified rather than taking the commit messages' word — and there is a third issue neither review caught: this PR and #2728 collide. Changeset (finding 2): present. Classifier (finding 1): fixed, via The collision, which is the thing worth acting on#2728 widened the same function with a different signature, and both PRs are open:
Both typecheck and both are green on their own branch. Whichever merges second will not conflict textually in a way git flags — it will simply overwrite the other's signature and break the other's call site. My recommendation is to keep this PR's boolean, and I say that against my own change: the caller here has already resolved the lane set to answer its neighbouring guard, so passing the boolean avoids recomputing membership and removes the possibility of the classifier and the gate disagreeing — which is the precise defect both PRs were opened to fix. The set form re-opens that door for the sake of a default nobody needs once the caller resolves. If #2736 merges first, #2728's engine hunk should be dropped and its call site changed to pass the boolean; its CLI-side lane set stays as is. I will do that on #2728 rather than leave it for whoever hits the breakage. Rebased onto current main and verified: CLI + engine |
|
Correction to my comment above — the shell ate the two signatures out of that table, which is exactly the comparison the comment exists to make. Restating it properly:
Both typecheck and both are green on their own branch. Whichever merges second will not produce a conflict git flags — it will overwrite the other's signature and break the other's call site. Recommendation unchanged, and still against my own PR: keep this one's boolean. The caller has already resolved the lane set to answer its neighbouring guard, so passing the boolean avoids recomputing membership and removes any chance of the classifier and the gate disagreeing — the precise defect both PRs were opened to fix. The set form re-opens that door for a default that stops mattering once the caller resolves. If this merges first, #2728's engine hunk should be dropped and its call site switched to the boolean; its CLI-side lane set is unaffected. I will do that on #2728 rather than leave it for whoever hits the breakage. |
…annot break each other #2736 and this PR widened `isInReviewMissingWorktreeSessionStartFailure` with DIFFERENT signatures — `isReviewColumn?: boolean` there, `reviewColumns: Iterable<string>` here. Both typechecked, both were green, and neither produces a conflict git flags: whichever merged second would simply overwrite the other's signature and break its call site. This file is now BYTE-IDENTICAL to #2736's version (verified by diff), so the second merge is a no-op here rather than a silent breakage. The three call sites pass `retryReviewColumns.has(task.column)` — the same resolved set, reduced to the answer the classifier actually needs. I recommended #2736's shape against my own on that PR and am following through here: the caller has already resolved the lane to answer its neighbouring guard, so passing the boolean removes the possibility of the classifier and the gate disagreeing — the precise defect both PRs exist to fix. THE TESTS HAD TO BE REWRITTEN, and this is the part worth recording. They were written against the SET form, so after the switch `["signoff"]` was merely a TRUTHY value: two of them passed for the wrong reason. Engine tsconfig excludes `src/__tests__`, so tsc could not see the mismatch — nothing but reading them would have caught it. They now assert the resolved answer in both directions, including that `false` beats the legacy id. Mutation: ignoring the parameter fails 2 of 10. CENSUS RISE, accepted deliberately: 4 -> 5 in this file. The boolean default keeps `task.column === "in-review"` as its legacy fallback, which my set form had removed. That literal arrives with #2736 regardless; re-recording here makes this branch agree with what main will look like rather than hiding a difference. Verified: 10 classifier tests green, engine + CLI + dashboard tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
…annot break each other #2736 and this PR widened `isInReviewMissingWorktreeSessionStartFailure` with DIFFERENT signatures — `isReviewColumn?: boolean` there, `reviewColumns: Iterable<string>` here. Both typechecked, both were green, and neither produces a conflict git flags: whichever merged second would simply overwrite the other's signature and break its call site. This file is now BYTE-IDENTICAL to #2736's version (verified by diff), so the second merge is a no-op here rather than a silent breakage. The three call sites pass `retryReviewColumns.has(task.column)` — the same resolved set, reduced to the answer the classifier actually needs. I recommended #2736's shape against my own on that PR and am following through here: the caller has already resolved the lane to answer its neighbouring guard, so passing the boolean removes the possibility of the classifier and the gate disagreeing — the precise defect both PRs exist to fix. THE TESTS HAD TO BE REWRITTEN, and this is the part worth recording. They were written against the SET form, so after the switch `["signoff"]` was merely a TRUTHY value: two of them passed for the wrong reason. Engine tsconfig excludes `src/__tests__`, so tsc could not see the mismatch — nothing but reading them would have caught it. They now assert the resolved answer in both directions, including that `false` beats the legacy id. Mutation: ignoring the parameter fails 2 of 10. CENSUS RISE, accepted deliberately: 4 -> 5 in this file. The boolean default keeps `task.column === "in-review"` as its legacy fallback, which my set form had removed. That literal arrives with #2736 regardless; re-recording here makes this branch agree with what main will look like rather than hiding a difference. Verified: 10 classifier tests green, engine + CLI + dashboard tsc clean, lint clean (0 errors), gate green (487 + 158 + 10 + 71), census --strict exit 0.
|
Collision note for the coordinator — #2728 and #2736 overlap, and they are complementary rather than duplicate. #2728 was opened first; I did not see it before starting. Both touch
Suggested order: land #2728 first, then I rebase #2736 onto it and drop everything already covered, keeping the retry classifier + target, the near-duplicate filter, the lineage label, the node-override guards, and the changeset. I am happy to do that rebase — just say the word. One finding from #2736 that matters to #2728 regardless of order. Converting the retry classifier without also converting the retry target does not just leave a bug — it creates a worse one:
Also worth flagging on the shared engine predicate: our two PRs give it different signatures ( Finally, on the P1 raised here about the two classifiers disagreeing: I measured it, and the stated consequence (stale |
|
Superseded by #2752, which is the part of this that still matters. #2728 landed first and covered the active-task count, the near-duplicate filter, the archived-lineage label, the node-override guards, and the missing-worktree classifier — so re-litigating those here would be conflict for no gain. What it did not carry is now #2752: it converted the retry classifier and left the three re-queue targets on the literal #2752 carries the target fix, the PG suite that reproduces main's crash, and the changeset for the published CLI that #2728 omitted. |
…e re-queue target LIVE REGRESSION ON MAIN, introduced by #2728 and predicted in my note on that PR before it landed. #2728 converted the retry CLASSIFIER (`retryReviewColumns.has(task.column)`) and left all three re-queue targets on the literal `"todo"`. That pairing is strictly WORSE than the bug it fixed. Before: `fn task retry` silently did nothing on a renamed board. After: it correctly decides to retry, then throws TransitionRejectionError: Invalid transition: 'checking' -> 'todo'. Unknown column for this workflow. Reproduced against main's exact code: reverting this fix fails 2 of 4 cases with that error. WHY THE CENSUS DID NOT CATCH IT. The census counts COMPARISONS. A move TARGET contains no comparison, so all three sites are invisible to it — `task.ts` reads 0 guards on main while the crash is live. This is the clearest case yet that the census measures conversion progress, not correctness, and that a classifier and its target must move together. The target is resolved from the task's own workflow (`resolveTaskLifecycleColumns(...).hold`), failing soft to `"todo"` when the workflow cannot be resolved, matching every other fallback in this file. The suite is the one from #2736, rebuilt on top of #2728: real store, real persisted workflow (the BUILTIN coding IR with only its column ids renamed, so the sole difference between the two runs is vocabulary), driven through the real `runTaskRetry`. #2736's other contents are now redundant with #2728 and are dropped; only this fix, its test, and the missing changeset for the published CLI survive. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…nsumers must NOT migrate onto it (#2750) ## A flaw in the helper I merged in #2730 Found by trying to do the migration I had been advocating for three rounds. One **name** was answering two questions: | | question | answer | |---|---|---| | **broad** | "is this card in a lane where review happens?" | every `mergeOrchestration` lane + every `mergeBlocker`/`humanReview` lane — **this function** | | **narrow** | "is this card in *the* review lane the engine acts on?" | `resolveLifecycleColumns().review` = `columnsWithFlag(ir, "mergeOrchestration")[0]` — **one** lane | The narrow answer is what the executor, the scheduler and `project-engine` act on. **A caller that admits on the broad set and then MOVES the card moves cards the engine does not consider in review.** ## The correction I owe I have been arguing across #2722, #2723 and #2728 that the inline review unions should converge on this helper. For the notifier that is right — over-admission there just means an extra notification. For `register-task-workflow-routes.ts` it is **wrong**. That resolver is deliberately narrower (#2723): its re-engagement *moves* the card, so admitting a second merge lane is a state change the engine will not agree with. Its local copy is **not drift from this helper — it is the other question.** Migrating it would reintroduce precisely the over-admission that PR's review round reasoned away. I was about to make that change. Reading both implementations side by side is the only thing that stopped me, and "consolidate the duplicates" would have looked like an obvious cleanup to the next person too. ## What this PR does Nothing to behaviour. It writes the distinction down **at the helper**, where a consumer reaching for "the review columns" will see it, and pins the difference with a test. The test needs a board declaring `mergeOrchestration` **twice** — no default lineage does, which is exactly why the two answers look identical everywhere else and why the conflation survived review. **Mutation: narrowing this helper to the first merge lane — the consolidation someone would reasonably attempt — fails the test.** ## Verification 30 trait tests green · core `tsc` clean · lint clean (0 errors) · gate green (487 + 158 + 10 + 71). No census movement. ## Not done here Migrating the notifier and the CLI copies onto this helper. Those genuinely should converge, but both live in open PRs (#2722, #2728/#2736) with live review threads; switching them under their authors mid-flight is worse than letting them adopt it once this distinction is documented. <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Clarified the distinction between broad review-capable lanes and the workflow’s primary review lane. * Documented how review lane selection affects workflow state handling. * **Tests** * Added coverage confirming that review detection includes all matching merge lanes. * Verified lifecycle review selection continues to use only the primary merge lane. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
…ted the classifier and left the target (#2752) ## This is a live regression on `main`, not a conversion `#2728` converted the retry **classifier** and left all three re-queue **targets** on the literal `"todo"`. That pairing is **strictly worse than the bug it fixed**: - **Before:** `fn task retry` silently did nothing on a renamed board. - **After (main today):** it correctly decides to retry, then throws. ``` TransitionRejectionError: Invalid transition: 'checking' → 'todo'. Unknown column for this workflow. ``` `todo` is not a column that board declares. Reproduced against main's exact code — reverting this fix fails **2 of 4** cases with that error. I flagged this on #2728 before it landed; posting it as a fix rather than a comment now that it is merged. ## Why the census did not catch it The census counts **comparisons**. A move **target** contains no comparison, so all three sites are invisible to it — `packages/cli/src/commands/task.ts` reads **0 guards** on main while the crash is live. That is the clearest case in this program so far that **the census measures conversion progress, not correctness**. A classifier and the target it feeds have to move together, and no automated signal will say so. ## The fix The target resolves from the task's own workflow: ```ts const retryHoldColumn = (await resolveTaskLifecycleColumns(context.store, id))?.hold ?? "todo"; ``` Failing soft to `"todo"` when the workflow cannot be resolved, matching every other fallback in this file. Three call sites, all three converted. ## Revert proof | state | result | |---|---| | main today (classifier converted, target literal) | **2 failed** / 2 passed — `Invalid transition: 'checking' → 'todo'` | | with this fix | **4 passed** | ## The fixture is derived, not hand-built The renamed workflow is `BUILTIN_CODING_WORKFLOW_IR` with **only its column ids renamed**, so the sole difference between the two runs is vocabulary. Hand-building a graph tested the fixture's shape as much as the code — the IR validator rejects an undeclared back-edge, and once declared as `kind: "rework"` the transition table still did not match the default board's. The suite also asserts the rename landed (`checking` present, `in-review` absent), so a surviving literal cannot pass by accident. Real store, real persisted workflow, driven through the real `runTaskRetry` — not the predicate. A unit test of the classifier goes green on the half-fix; only driving the whole command surfaces the crash. ## Relationship to #2736 This replaces it. #2736's other contents (active-task count, near-duplicate filter, archived-lineage label, node-override guards, the missing-worktree classifier) are now redundant with #2728, so they are dropped rather than re-litigated. What survives is this fix, its test, and the **changeset for the published CLI** that #2728 did not include. I will close #2736 once this is reviewed. ## Verification - new PG suite **4 passed** · `task-retry.test.ts` **7 passed** across 2 files - `pnpm test:gate` — **10 / 158 / 487 / 71** · `pnpm lint` clean · CLI `tsc --noEmit` clean · `check:changesets` passes · `--strict` exits 0 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Census
packages/cli/src/commands/task.tspackages/cli/src/commands/dashboard.tsBaseline re-recorded;
--strictexits 0.The finding: half this conversion would have been worse than none
Converting the retry classifier — so
fn task retryrecognises a renamed review lane at all — turned a silent no-op into a hard error:The command now correctly decided to retry, and then moved the card to the literal
"todo", which does not exist on that board. ThreemoveTask(id, "todo")targets, none of which the census counts — it counts comparisons, and a move target contains no comparison at all.So the classifier and the target are both converted here. Shipping only the first would have replaced "retry quietly does nothing" with "retry throws", which is a worse operator experience than the bug it fixed. This was caught only because the test drives the whole command rather than the predicate — a unit test of the classifier would have gone green on the half-fix.
What was broken
dashboard.ts— one rule, four copies.column === "in-progress" || column === "in-review"was pasted into the TUI refresh-stats callback, the periodic TUI refresh, the shutdown log summary, and the initial stats population. On a renamed board all four reportedactive=0while the per-column counts printed immediately beside them showed the work plainly — a dashboard insisting nothing is running while its own board disagrees.Extracted to one resolved helper. I am calling this de-duplication of an existing four-fold copy rather than a new abstraction: one rule, one implementation, so the four cannot drift apart again.
A real bug found while wiring it: one of the four lists from a board-scoped project store (
taskStore), not the outerstore. My first pass resolved workflows against the outer store for all four — which would have resolved foreign workflows for a scoped board. Threaded to the same store each site listed from.task.ts— the archived-lineage label (a renamed archive silently lost its(archived)suffix), both node-override in-progress guards (the CLI would rewrite the node override of a task an agent was actively executing), the in-review retry classifier, and the near-duplicate candidate filter — where a renamed board never excluded completed work, so every finished task stayed a duplicate candidate and the guard flagged new work as a duplicate of something already shipped.The duplicate filter reorders recency before the completion filter. Both still precede
slice(200), so the resulting set and its order are identical; the reorder bounds the per-task workflow resolution by the 7-day window instead of the whole board.The fixture is derived, not hand-built
The renamed workflow is
BUILTIN_CODING_WORKFLOW_IRwith only its column ids renamed. I first hand-built a four-node graph and the failures were instructive: the IR validator rejects an undeclared back-edge, and once declared askind: "rework"the transition table still did not match the default board's. A hand-rolled fixture ends up testing the fixture's shape as much as the code. Deriving from the builtin guarantees the only difference between the two runs is the vocabulary — which is the entire differential claim.It also asserts the rename landed (
checkingpresent,in-reviewabsent), so a surviving literal cannot pass by accident.Revert proof
column === "in-review""todo"Verification
runTaskRetry)pnpm test:gate— 158 / 10 / 487 / 71 ·pnpm lintclean · CLItsc --noEmit0 errorsNot converted, deliberately
One
DELIBERATE-LITERALmark intask.ts: the board-print glyph at ~L615. Its comment already reasoned the case out fully — the loop iterates the legacyCOLUMNSconstant, socolcan never be a custom id — it was simply missing the machine-readable marker. The deeper bug it names (a card in a renamed column is not rendered at all, because the loop derives its column set from the legacy enum) is the R8/U10 surface change and is far larger than this glyph.