fleet: in-process-runtime 6 → 1 — a finished card's plan review could re-enter, and the mission requeue wrote a column that may not exist - #2741
Conversation
|
Warning Review limit reached
Next review available in: 35 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 (2)
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 adds renamed-terminal-lane regression coverage and documents a remaining raw-column concurrency path.
Confidence Score: 4/5The PR does not appear safe to merge until mission failure recovery can rehome tasks across non-adjacent workflow transitions. The previously reported recovery defect remains: the failure handler resolves a hold lane but invokes the ordinary guarded move API, whose adjacency check can reject that rebound and leave the task failed in WIP without reaching autopilot retry. Files Needing Attention: packages/engine/src/mission-autopilot.ts, packages/core/src/task-store/moves.ts
|
| Filename | Overview |
|---|---|
| packages/engine/src/tests/plan-approval-hold-invariant.test.ts | Adds focused coverage for renamed terminal-column membership and the corresponding non-terminal control case. |
| packages/engine/src/concurrency.ts | Adds documentation around the existing persisted-agent counting path without changing executable behavior. |
Reviews (6): Last reviewed commit: "test(engine): adapt the renamed-terminal..." | Re-trigger Greptile
8d3b4d1 to
cc22653
Compare
Scope note: one comment-only finding added, from assessing the next conversion targetPushed an FNXC block at Why it is recorded here at allI went to convert The exposure is at the callers that decline to enrich. I traced every one before writing anything:
So admission control is correct and already has a PG live-E2E pinning the exact number the scheduler compares against the cap. The residue is one repair path. The consequence, and why I did not fix itOn a renamed board, cards in a renamed wip lane are not counted, so
Two smaller notes from the same trace: VerificationGate GREEN (158 + 10 + 487 + 71) · |
cc22653 to
3f2f634
Compare
…review could re-enter CONVERTED (5): - The planning-continuation drain's TERMINAL test. It orphans a continuation whose card has reached a terminal lane; the test was `column === "archived" || === "done"`, so on a renamed board it matched nothing and a FINISHED card's continuation was still handed to the executor — re-entering plan review on completed work. Threaded through the deps object, which is the documented seam "so the wiring is testable without constructing a runtime". OPTIONAL, so every existing harness keeps the legacy-id behaviour; the runtime wires it from its TaskStore with one IR cache per drain pass, allocated per pass so it cannot outlive a workflow edit. - The mission-linked failure requeue, which had TWO literals and the target one is worse. The guard `column === "in-progress"` never matched on a renamed board, so autopilot retry policy never got to decide; and if it HAD matched, the requeue wrote the literal `"todo"` — a column that workflow may not declare. That is the exact rebound-target defect agent-heartbeat-worktree-renamed-hold.test.ts exists for, on a path it does not cover. The target now uses `resolveReboundTarget` (KTD-10 ordering: hold -> intake -> first column) rather than `lifecycle.hold`, because a workflow may declare no hold lane and that helper is where the ordering lives. FLAGGED AND LEFT COUNTED (1): the `task:moved` listener that deletes task-local planner chats on archival. It runs on EVERY move in the project and the archival test IS its gate, so converting means resolving an IR on every move to decide most moves are not archival — the same call made at github-tracking-comments.ts:232. Consequence stated: on a renamed archived lane those chats are not deleted at the cutoff. A retention miss, not a loss. The fix wants the resolved lane on the event payload, which is an event-contract change. REVERT PROOF: restoring the drain's literals fails the new renamed-terminal case with `shipped should be terminal: expected 'skip' to be 'orphan'`. None of the 25 existing cases in that suite could have caught this — `lifecycle` is optional and they all omit it, so they assert the legacy fallback. The two new cases supply it, and the second pins that the terminal test does not swallow every column on a renamed board. Census 6 -> 1. Gate 158+10+487+71 GREEN. 102 passed across the hold-invariant and mission-loop suites. Engine tsc and lint clean; --strict exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… (capacity leak-recovery) Comment only. No behaviour change; the census entry moves only because this branch's in-process-runtime conversion is re-recorded after rebase. FOUND while assessing `live-agent-count.ts` (6 census entries) as the next conversion target. It is NOT one: all six literals are genuine degraded fallbacks, and the file is correctly designed — `enrichRunningAgentTaskShape` / `...FromFlags` populate the trait fields from an IR or from board flags, and the `??` arms answer only when enrichment did not happen. The exposure is at the CALLERS that decline to enrich, so the census points at the wrong file. TRACED EVERY CALLER before writing anything: - `executor.ts` (fn_spawn_agent) and `project-engine.ts` — the live ADMISSION paths — use `computeTopLevelConcurrencyClaimedFromStore`, which enriches. Admission is correct and `workflow-agent-count-live-e2e.pg.test.ts` already pins that number end to end. - `computeTopLevelConcurrencyClaimed` (raw) has NO production caller; tests only. - `persistedTopLevelAgentSlots(tasks)` in `reconcileIdleSemaphoreLeak` is the one live path left that does not enrich. CONSEQUENCE on a renamed board: cards in a renamed wip lane are not counted, so `persistedActive` and `bound` are too low and the continuous-excess timer can see phantom excess and reclaim a semaphore slot that is legitimately held — the same harm the FN-7017 note directly below guards against for nested helper agents, arriving through the vocabulary door. NOT FIXED, deliberately. The enriching variant is ASYNC and this is a SYNCHRONOUS capacity REPAIR path whose failure mode is reclaiming live work. Adopting it changes the signature of leak recovery; the alternative is passing pre-enriched tasks in from the caller. Either is a behaviour change in the capacity subsystem, not a vocabulary conversion, and wants an owner who can decide whether recovery may await a store read at all. Recorded at the site rather than in a report, so the next reader of that line finds it. Gate 158+10+487+71 GREEN. concurrency 56/56. Engine tsc and lint clean; --strict exits 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ns membership API
3f2f634 to
ed8fae9
Compare
|
Superseded by batch-engine (#2773) per the coordinator's mega-batch directive — the CI queue is the bottleneck, not conversion speed. Every commit from this PR, including its review fixes, is cherry-picked onto that branch with the same content; nothing is dropped. |
…worker's mega-batch) (#2773) The engine mega-batch. Folds my four engine PRs and will absorb the remaining `packages/engine` guards as commits on this branch. **Superseded and closed:** #2722, #2741, #2766, #2770. ## Census — files converted so far | file | before | after | |---|---:|---:| | `notification/notification-service.ts` | 9 | **5** | | `runtimes/in-process-runtime.ts` | 6 | **1** | | `eval-followups.ts` | 2 | **0** | | `pr-comment-handler.ts` | 1 | **0** | | `task-revert.ts` | 2 | **0** | The last two are **census-invisible** (`Set.has(task.column)` membership) — the class measured in #2763, which a comparison-based scan cannot count. So the backlog number moves less than the work does, deliberately. ## What each one actually fixed — all silent, none cosmetic - **Notifications stopped entirely.** `handleTaskMovedAsync` compared `data.to` to `in-review`/`done`, so on a renamed board the two notifications operators rely on most were never sent. - **A finished card's plan review could re-enter.** The continuation drain's terminal test matched nothing, so a completed card's planning continuation was handed to the executor. - **The revert route admitted and the service refused.** The route resolved terminal lanes; the service compared to a hardcoded pair. The operator got a dead end from an affordance the UI and route both offered. - **Follow-up dedup blocked new cards forever.** A finished follow-up in a renamed complete lane read as *open*, so the dedup matched it permanently — defeating the intent the code documents in the line above it. - **The mission requeue wrote a column that may not exist**, and its guard never matched. ## Flagged, not fixed — deliberately - **`concurrency.ts` idle semaphore leak recovery** — the last live caller of the running-agent predicate that does not enrich. On a renamed board it under-counts and can reclaim a legitimately-held slot. The enriching variant is async and this is a synchronous repair path whose failure mode is reclaiming live work. - **The archival `task:moved` listener** — runs on every move with no cheap gate ahead of it; converting costs an IR resolution per move to decide most moves are not archival. ## Notes carried from the folded PRs Two conflicts resolved in main's favour because **main's version was better**: `in-process-runtime`'s seam uses `terminalColumns: ReadonlySet` (membership) where mine used `LifecycleColumns` (first-per-role), and the test is rewritten against main's API. That arity trap has now caught me four times, so membership is the default shape in everything new here. Review fixes from the folded PRs are included: the notifier's review set, the second human-review site, the second dedup copy, the workspace revert surface, and the file-content assertions. ## Verification `pnpm test:gate` **GREEN** (158 + 10 + 487 + 71) · **224 passed** across the touched engine suites · engine and dashboard `tsc` clean · `pnpm lint` clean · census `--strict` exits 0. 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
in-process-runtime.ts6 → 1. Two real defects, one flagged hot-path site, and a third file examined and deliberately left alone.The drain could re-enter plan review on finished work
resolvePlanningContinuationCandidateorphans a continuation whose card has reached a terminal lane. That test wascolumn === "archived" || column === "done"— so on a renamed board it matched nothing, and a finished card's planning continuation was still handed to the executor.Threaded through the
depsobject, which is already the documented seam for this function ("so the wiring is testable without constructing a runtime"). Optional, so every existing harness keeps the legacy-id behaviour unchanged; the runtime wires it from itsTaskStorewith one IR cache per drain pass — allocated per pass rather than on the instance, so it cannot outlive a workflow edit between passes.The mission requeue had two literals, and the target one is worse
On a renamed board the guard never matched, so a mission-linked failure was never requeued and autopilot retry policy never got to decide. And had it matched, the requeue writes a column that workflow may not declare — the exact rebound-target defect
agent-heartbeat-worktree-renamed-hold.test.tswas written for, on a path that test does not cover.The target now uses
resolveReboundTarget(the KTD-10 ordering: hold → intake → first column) rather thanlifecycle.holdalone, because a workflow may declare no hold lane at all and that helper is the one place the ordering lives.Flagged and left counted
The
task:movedlistener that deletes task-local planner chats on archival. It runs on every move in the project and the archival test is its gate, so converting means resolving a workflow IR on every move just to decide most moves are not archival — the same call I made atgithub-tracking-comments.ts:232.Consequence stated rather than hidden: on a renamed archived lane, those chats are not deleted at the cutoff. That is a retention miss, not a loss — the chats are kept. The fix wants the resolved lane on the event payload, since the emitter already knows it; that is an event-contract change.
Revert proof
Restoring the drain's literals fails the new case with
shipped should be terminal: expected 'skip' to be 'orphan'.None of the 25 existing cases in that suite could have caught this —
lifecycleis optional and they all omit it, so they assert the legacy fallback. The two new cases supply it, and the second pins that the terminal test does not swallow every column on a renamed board.A file I examined and did not convert
reliability-metrics.ts(6) readsfrom/toout of historical activity-log and run-audit metadata. Those records store the column ids as they were at the time of the move, so comparing them against the currently resolved review lane is wrong for any entry written before a rename — and the workflow as-of-the-event is not recorded anywhere.Converting it would make post-rename windows work and silently mis-handle windows spanning a rename; leaving it means renamed boards report zero review latency. Both are wrong in different directions, and picking needs a product answer about what a metric spanning a vocabulary change should say. That is a behaviour decision, not a conversion, so it stays flagged — the historical-event-metadata class is distinct from anything else in this backlog and is worth naming as such.
Verification
pnpm test:gateGREEN (158 + 10 + 487 + 71) · 102 passed across the hold-invariant and mission-loop suites · enginetscclean ·pnpm lintclean · census--strictexits 0.🤖 Generated with Claude Code