fix(dispatcher): make dispatcher the sole writer of In-flight/Rate-limits/Slot-usage#186
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR surfaces auto-dispatch parse failures (deduped) to state issues, threads workflow heartbeats into recommender context, and adds a post-agent reapply step that overwrites dispatcher-owned sections (In-flight, Rate limits, Slot usage) with canonical, heartbeat-inclusive content. Tests and planning docs accompany the changes. ChangesDispatcher Issue
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
…ery and recommender context
… agent stops authoring them The recommender agent reconstructed the In-flight line from prompt JSON that carries no heartbeat, producing the malformed 4-field line that silently broke auto-dispatch (#180). Wire applyDispatcherSections as the sole writer: a new reapply-dispatcher-sections step overwrites In-flight/Rate-limits/Slot-usage with canonical content (heartbeat included) right after the agent runs, and the prompt tells the agent to emit the In-flight empty placeholder instead of authoring it. Reapply is best-effort; verify-state-issue-parses stays the single surfacing gate for an unparseable agent body.
…h on the issue The read-only auto-dispatch loop threw `… does not parse …` only to stderr, so a malformed body silently stalled the auto-loop until a human noticed (#180). Add a deduped ParseFailureSurfacer that comments the failure on the state issue once per distinct message and re-arms after a healthy read; wire it into runAutoDispatch.
…omment; drop dead StateIssueReader A failed gh comment must be retried next tick, not suppressed by a prematurely recorded dedup entry. Also removes the now-unused StateIssueReader type (the recommender dep is the full StateIssueGateway).
thejustinwalsh
force-pushed
the
middle-issue-180
branch
from
487be64 to
5582fbb
Compare
| * comment). Re-gathers context here (not from build-prompt) so the In-flight | ||
| * snapshot reflects state *after* the agent's run. | ||
| */ | ||
| async function reapplyDispatcherSections(ctx: StepContext<RecommenderInput>): Promise<void> { |
There was a problem hiding this comment.
Decision — the dispatcher's in-place section seam was never wired. grep showed applyDispatcherSections/updateDispatcherSections were defined and unit-tested but had zero production callers. So In-flight was authored only by the recommender agent, reconstructing the line from the in_flight prompt JSON — which carried no heartbeat (InFlightSummary and listActiveImplementationWorkflows both lacked one). The renderer requires last heartbeat <rel>, so the agent literally could not produce the canonical 5-field line. This step makes the dispatcher reapply its three owned sections from its own state right after the agent runs. Relaxing the parser (issue option 2) was rejected as hiding the symptom.
| async function reapplyDispatcherSections(ctx: StepContext<RecommenderInput>): Promise<void> { | ||
| const before = await deps.stateIssue.readBody(ctx.input.repo, ctx.input.stateIssue); | ||
| const parsed = parseStateIssue(before); | ||
| if (isParseError(parsed)) { |
There was a problem hiding this comment.
Decision — reapply is best-effort; verify-state-issue-parses is the single parse gate. A surgical section overwrite needs a parseable body. On the happy path the agent emits the canonical empty In-flight placeholder (per the new prompt), so this parses and overwrites. If a disobedient agent leaves an unparseable body, this step skips (logs) rather than surfacing, so the downstream verify step is the only place a parse failure is announced — no double comment on the state issue.
| const problem = `⚠️ auto-dispatch halted: state issue #${stateIssue} does not parse, so the ranked dispatch plan can't be read and no Epics will dispatch until this is fixed.\n\n\`${error.message}\``; | ||
| if (lastSurfaced.get(repo) === problem) return false; | ||
| // Record only AFTER a successful comment — a failed `gh` comment (throws) | ||
| // must be retried next tick, not silently suppressed by a recorded dedup. |
There was a problem hiding this comment.
Decision — record the dedup key only after a successful comment. A failed gh comment throws; if we recorded the key first, a transient failure would suppress the surface permanently (until a healthy read resets it). Recording after the await means the next tick retries. Self-review caught this; covered by the retry-after-failed-comment test in auto-dispatch.test.ts.
thejustinwalsh
added
the
ready-for-review
Reviewer's brief — PR #186 (fixes #180)What this fixes: the recurring How to run itbun install
bun run format && bun run lint && bun run typecheck # all clean
bun test # 1082 pass, 0 fail
# focused:
bun test packages/dispatcher/test/recommender-workflow.test.ts \
packages/dispatcher/test/auto-dispatch.test.ts \
packages/dispatcher/test/workflow-record.test.tsWhat to verify (and what "correct" looks like)
How to reviewStart at the Fragile bits needing extra eyes
Branch is rebased onto current |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/dispatcher/src/main.ts`:
- Around line 341-342: The parse-failure dedupe reset is being called
unconditionally; move the parseFailureSurfacer.reset(repo) call so it only runs
after a successful state read instead of whenever autoDispatch returns
"disabled". Concretely, update the logic in the auto-dispatch branch that uses
autoDispatch(...) and the result object: only invoke
parseFailureSurfacer.reset(repo) after you have performed and confirmed a
healthy read (the code path that produces the successful `result`), not
before/when autoDispatch returns "disabled" — e.g., relocate the call into the
block that handles successful reads (the same place where you check
`result.enqueued.length` or otherwise confirm the read was healthy).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ff1795f6-7b0c-4812-9d13-d0b02106d84a
📒 Files selected for processing (11)
packages/dispatcher/src/auto-dispatch.tspackages/dispatcher/src/main.tspackages/dispatcher/src/recommender-run.tspackages/dispatcher/src/workflow-record.tspackages/dispatcher/src/workflows/recommender.tspackages/dispatcher/test/auto-dispatch.test.tspackages/dispatcher/test/recommender-run.test.tspackages/dispatcher/test/recommender-workflow.test.tspackages/dispatcher/test/workflow-record.test.tsplanning/issues/180/decisions.mdplanning/issues/180/plan.md
…state read A "disabled" auto-dispatch pass returns before readState(), so it never performs a healthy read; resetting the parse-failure dedup there would let an unfixed parse failure re-surface a duplicate comment without an intervening read. Gate the reset on a new didReadState() predicate that centralizes the no-read contract (#180).
- migration filenames: 008→009 (workflows.epic_ref) to clear collision with #181's 007_retention.sql - db.test.ts + db-scripts.test.ts: bump schema-version assertions to 9 (final post-008+009 state) - re-apply rename codemod to references introduced by #185/#186 after the original rename codemod landed
- migration filenames: 008→009 (workflows.epic_ref) to clear collision with #181's 007_retention.sql - db.test.ts + db-scripts.test.ts: bump schema-version assertions to 9 (final post-008+009 state) - re-apply rename codemod to references introduced by #185/#186 after the original rename codemod landed
1 participant
Summary
Closes #180
The recurring
malformed "In-flight" itemparse failure that silently stalled auto-dispatch is fixed at its root. The dispatcher's in-place section seam (applyDispatcherSections) existed but was never called in production — so the three dispatcher-owned sections (In-flight / Rate limits / Slot usage) were authored only by the recommender agent, which reconstructed the In-flight line from prompt JSON that carries no heartbeat. The renderer needs a 5-field line (… · last heartbeat <rel> · …); the agent could only produce 4 fields → a malformed body every time, after which every auto-dispatch tick died with a parse error in stderr until a human noticed.This wires the dispatcher as the sole writer of those three sections and makes parse failures self-announcing.
What changed
packages/dispatcher/src/workflow-record.ts—listActiveImplementationWorkflowsnow returnslastHeartbeat(the canonical In-flight line needs it; the agent never had it).packages/dispatcher/src/workflows/recommender.ts— newreapply-dispatcher-sectionsstep overwrites the three owned sections with canonical content (dispatcherSectionsFromContext+heartbeatRel) right after the agent runs; the prompt now tells the agent to emit the In-flight empty placeholder and treat the context blocks as ranking input only.packages/dispatcher/src/auto-dispatch.ts—createParseFailureSurfacer: comments a parse failure on the state issue, deduped per repo, re-armed after a healthy read.packages/dispatcher/src/main.ts—runAutoDispatchsurfaces parse failures through it instead of dying in stderr.Why these changes
The schema names the dispatcher the owner of In-flight / Rate limits / Slot usage, but nothing enforced it — the agent was authoring them and had no heartbeat to render. Making the dispatcher reapply those sections from its own state (heartbeat included) after every recommender run closes the bug at the source; the agent emitting the empty placeholder removes the temptation to reconstruct a malformed line. Reapply is best-effort — if a disobedient agent leaves an unparseable body,
verify-state-issue-parsesstays the single surfacing point (no double comment). Relaxing the parser (issue option 2) was rejected as hiding the symptom.Verification
All four mechanical gates pass on the rebased branch:
bun run format,bun run lint,bun run typecheck(0 errors),bun test(1082 pass, 0 fail).packages/dispatcher/test/workflow-record.test.ts—listActiveImplementationWorkflows (#180)returns the touched epoch / null.recommender-workflow.test.ts—dispatcherSectionsFromContextmapping (null-issue dropped, null-session→pending),heartbeatRelformatting table, the 8-step order, and the self-heal test asserting the canonical 5-field line plus arenderStateIssue(parseStateIssue(body)) === bodyround-trip assertion.#44 build-prompttest asserts the agent is toldDISPATCHER-OWNED/ emit- _no agents in flight_/decision INPUT.auto-dispatch.test.ts—createParseFailureSurfacer (#180)dedup / reset / per-repo / retry-after-failed-comment / ignore-non-parse-errors.Acceptance criteria
applyDispatcherSectionsis the only writer (reapply-dispatcher-sections step, prompt change verified by the#44 build-prompttest, and theself-heal+exact bug shapetests inrecommender-workflow.test.ts).createParseFailureSurfacertests for the auto-dispatch surfacing path — see fix(dispatcher): recurring "In-flight" parse failure blocks auto-dispatch (heartbeat field missing) #180).renderStateIssue(parseStateIssue(body)) === bodyround-trip (explicit assertion in the self-heal test; parser/renderer untouched,@middle/state-issuefuzz + fixture suites still green — see fix(dispatcher): recurring "In-flight" parse failure blocks auto-dispatch (heartbeat field missing) #180).Stumbling points
updateDispatcherSections/applyDispatcherSectionsseam looked wired but agrepshowed it had zero production callers — the real gap. The heartbeat simply never flowed into a rendered body becauselistActiveImplementationWorkflowsdidn't carry it.origin/main; rebased onto current main (rerere on) — two isolated conflicts (a test import list, and main.ts imports restructured by the Persist parked executions across daemon restart (durable bunqueue store) #116 durable-engine work), resolved new-work-as-base. Re-verified all gates after.Suggested CLAUDE.md updates
None. The dispatcher CLAUDE.md's "ownership" framing already matches; this PR just makes the code honor it.
Follow-up issues
None. One nit surfaced (the In-flight
progressfield can carry non-runningworkflow states likelaunching/waiting-human, which deviate from the schema's documented closed set) — it parses and validates fine and showing the real state is arguably more useful, so it's left as-is rather than filed.Out of scope
Summary by CodeRabbit
New Features
Tests
Documentation