feat(dispatcher): human-in-the-loop and review-driven resume flow#90
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 (2)
📝 WalkthroughWalkthroughAdds a durable park→external-signal→resume spine: parse .middle/blocked.json sentinels, add fired_at for idempotent wait signals, implement a GitHub poller (gh gateway + cron + classifiers), refactor implementation workflow to park/resume with continuations and round caps, and wire poller/continuation startup and tests. ChangesPark → Signal → Resume Workflow with GitHub Poller
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 |
Restructure createImplementationWorkflow into a park -> external-signal -> resume spine: launch-and-drive -> branch(park|terminal) -> top-level waitFor -> resume-or-finalize. asked-question and done stops arm a durable waitfor_signals row (epic-<n>-answered / epic-<n>-review-resolved), end the session, set waiting-human, and park; terminal stops pre-seed the signal so the single top-level waitFor falls through. Resume consumes the row and re-drives a fresh session re-primed per reason. bunqueue 2.7.12 filters branch/loop bodies to steps only and signal() targets one execution, so waitFor is a single top-level node (the spec's nested graph isn't expressible); additional review rounds loop via re-enqueue (#36). Adds getWaitForSignal/consumeWaitForSignal to workflow-record. Closes #33
…fyStop classifyStop now reads the .middle/blocked.json question sentinel and carries its parsed contents on the asked-question classification (new BlockedSentinel type in core). Parsing is tolerant — a missing/malformed/contentless sentinel yields sentinel: null while still classifying asked-question (presence is the signal). The workflow's postQuestion seam receives the question + context so the parked Epic can show the human what's being asked. Closes #34
Adds runPoller: a pure pass over parked workflows (waiting-human + armed waitfor_signals) behind an injected GitHubPollGateway. For epic-<n>-answered it fires on a new non-bot reply; for epic-<n>-review-resolved it classifies the PR verdict — CHANGES_REQUESTED (review or changes-requested label) -> changes-requested, APPROVED or a fresh 0-actionable re-review -> resolved (so a bot reviewer that never flips its verdict doesn't hang the loop). Detect-and-fire only; #36 interprets the payload. Idempotent via a new waitfor_signals.fired_at column (migration 002): a fired wait is skipped until the workflow resumes and re-parks. Per-workflow failures (rate limits, transient errors) are isolated and retried next pass. Production gh gateway in poller-gateway.ts; 60s cron wired into main.ts alongside the watchdog (full cross-process resume hosting is Phase 8). Closes #35
…n and review-changes The Phase 5 spine (#33) parked and did a single inline resume drive. #36 replaces that with the re-enqueue continuation model the bunqueue reality forces: a park happens once per execution (no loop-back; loop bodies can't hold a waitFor), so every resume is a fresh execution re-primed from a ResumeInput that reuses the prior round's worktree — same branch, same PR. - resume-or-finalize interprets the fired signal: terminal stop / review resolved (APPROVED or clean re-review) -> finalize; answered-question or CHANGES_REQUESTED under the cap -> enqueueContinuation; a CHANGES_REQUESTED that would exceed the round cap (default 5) -> park waiting-human, no re-arm, no continuation (bounded loop). - prepare-worktree reuses input.resume.worktree and writes the reason-specific resume brief to .middle/prompt.md (answer injected, or the address-review brief pointing at the skill procedure). launch-and-drive picks answer/resume framing from the reason. - enqueueContinuation injected; wired in dispatch.ts to engine.start (the long-lived-engine hosting of parked execs is the Phase 8 integration). - Tests: asked-question e2e (reply injected into continuation brief), review e2e (address-review brief + APPROVED ends the loop), round-cap boundary (waiting-human at the cap, no further round).
…nting-github-issues Adds Phase 11 (the review-changes loop the #36 resume brief points at) so the autonomous daemon loop and a hand-driven agent follow the same per-round procedure: batch the whole pass -> resolve class-wide with a test per fix -> internal clean-eyes review loop over the batched diff -> push once -> reply in-thread -> re-request review -> stop and wait for the next verdict. APPROVED ends the loop; the agent never merges. Also expands 'You may be resumed mid-workstream' under the headless-dispatch section: fresh session in the same worktree, the two resume reasons (answer injected / address-review brief), and the bounded review loop (round cap 5). Synced to the bootstrap-assets mirror and the dogfood .claude/.codex copies.
thejustinwalsh
left a comment
thejustinwalsh
left a comment
There was a problem hiding this comment.
Decision-log distillation (Phase 8 of implementing-github-issues). Each inline comment is the why behind a load-bearing choice; full entries live in planning/issues/32/decisions.md.
| // Single top-level `waitFor`: parks park-worthy stops until the poller | ||
| // fires RESUME_EVENT; terminal stops pre-seeded the signal and fall | ||
| // through. Same timeout budget as the drive step. | ||
| .waitFor(RESUME_EVENT, { timeout: WAITFOR_TIMEOUT_MS }) |
There was a problem hiding this comment.
Why a single top-level waitFor, not the spec's nested graph. bunqueue@2.7.12's Workflow builder filters branch .path() and loop bodies to type === 'step' only — a waitFor nested inside is silently dropped (workflow.js:39-51). The executor also has no goto/loop-back. So park/resume is expressible only as a top-level waitFor node, and multi-round looping is re-enqueue (which the spec itself annotates // loop back via re-enqueue).
| }); | ||
| } else if (finalState === "completed") { | ||
| } | ||
| (ctx.signals as Record<string, unknown>)[RESUME_EVENT] = { terminal: true }; |
There was a problem hiding this comment.
Conditional parking via pre-seeding ctx.signals. A top-level waitFor always executes (no skip primitive). buildContext returns signals: exec.signals by reference (runner.js:178) and runWaitFor advances when exec.signals[event] !== undefined (executor.js:150). So terminal stops mutate ctx.signals[RESUME_EVENT] = { terminal: true } here to fall straight through, while park-worthy stops leave it unset to genuinely park. resume-or-finalize reads the same terminal marker to tell a pre-seeded fall-through from a poller-fired verdict.
| * the epic-scoped, reason-scoped name lives in the durable `waitfor_signals` | ||
| * row (see `signalNameFor`), which is what the poller and dashboard read. | ||
| */ | ||
| export const RESUME_EVENT = "resume"; |
There was a problem hiding this comment.
One static engine event name; epic-scoped naming lives in waitfor_signals. waitFor(event) takes a static string in this bunqueue version, and engine.signal(execId, event, payload) already targets one execution — so the event name needn't be parameterized to avoid cross-execution collisions. The durable, poller-facing name (epic-<n>-answered / epic-<n>-review-resolved, via signalNameFor) is the waitfor_signals.signal_name; the reason + data ride in the payload.
| -- re-fire the same wait before the workflow has resumed and consumed the row. | ||
| -- A fresh park (next review round) deletes-and-reinserts the row, clearing it. | ||
|
|
||
| ALTER TABLE waitfor_signals ADD COLUMN fired_at INTEGER; |
There was a problem hiding this comment.
Poller idempotency via fired_at. The poller is a pure detect-and-fire pass; it marks a wait fired so a later pass won't re-fire before the workflow resumes and consumes the row. A fresh park (next round) deletes-and-reinserts the row, clearing fired_at. Note the row is keyed by signal_name (epic-scoped) — rounds of the same Epic never overlap because each round consumes its wait before the next round arms (sequential handoff), so the shared key doesn't collide.
| // parked execution. NOTE: routing dispatches through this long-lived engine | ||
| // (so parked executions live here to be resumed) is the Phase 8 auto-dispatch | ||
| // integration; the poller + signal seam are in place ahead of it. | ||
| const stopPoller = await startPoller({ |
There was a problem hiding this comment.
Poller + signal seam wired; cross-process resume hosting is Phase 8. startPoller runs as a 60s cron on the long-lived engine, with fireSignal = engine.signal(id, RESUME_EVENT, …). Today dispatches run on dispatchEpic's throwaway engine, which drains when the workflow parks (waitForSettle returns on waiting), so a parked execution doesn't yet live on this engine to be resumed. Routing dispatches through the persistent engine is the Phase 8 auto-dispatch integration (explicitly out of scope for the Epic). The seam is in place ahead of it; a fire for a not-yet-hosted execution is caught by the poller's per-workflow guard and retried.
| // RATE_LIMITED before handing off. | ||
| markAvailableOnSuccess(deps.db, ctx.input.adapter); | ||
| // Hand control to a fresh continuation that reuses this worktree. | ||
| await deps.enqueueContinuation({ |
There was a problem hiding this comment.
Multi-round resume = re-enqueue (one round = one execution). A single execution can park only once. The review loop needs up to cap real parks (each frees the session for a reviewer who may take days), so the only expressible loop is re-enqueue. waitfor_signals.workflow_id must equal the bunqueue execution id for engine.signal to target the parked execution, so each round is necessarily a fresh execution (and workflows row), keyed by the same epic_number; the live one is the latest non-terminal row. The continuation reuses the prior round's worktree via input.resume.worktree — same branch, same PR.
| * This unconditionally overwrites (unlike `ensurePromptFile`, which preserves an | ||
| * operator brief on the *initial* dispatch) — a resume's brief is the live one. | ||
| */ | ||
| function writeResumeBrief( |
There was a problem hiding this comment.
The agent fetches review threads; the dispatcher only writes the brief. On a review-changes continuation this overwrites .middle/prompt.md with an "address review" brief (round, decision, the skill's per-round procedure) and the agent pulls the PR's live threads itself via gh. Having a full Claude session fetch live threads is more robust than embedding a stale snapshot, and keeps the dispatcher GitHub-read-light. Codifying the procedure in the skill's new "Addressing review feedback" (Phase 11) section is what makes the autonomous daemon loop and a hand-driven agent behave identically — the #36 acceptance's explicit requirement.
| }); | ||
| // This round handed off — terminal in the bunqueue sense. The worktree is | ||
| // NOT torn down; the continuation reuses it. | ||
| updateWorkflow(deps.db, ctx.executionId, { state: "completed" }); |
There was a problem hiding this comment.
A handed-off round ends completed, not a new superseded state. The row must be terminal so findActiveWorkflowBySession / loadPollableWaits ignore it in favour of the live continuation. Adding superseded means rebuilding the table (SQLite can't ALTER a CHECK constraint) — disproportionate for a today-cosmetic distinction whose only consumer is the Phase 9 dashboard (out of scope). markAvailableOnSuccess firing on a handoff is correct (the round's drive ran a working adapter). If the dashboard later needs honest per-round accounting, Phase 8 (which revisits the workflows lifecycle) is the place to add superseded.
thejustinwalsh
added
the
ready-for-review
Reviewer's brief — Epic #32 (PR #90)What this is. A park → external-signal → resume spine for the How to run itbun run typecheck # tsc --noEmit — clean
bun test # 237 pass / 0 fail across 27 files
# the load-bearing suites for this PR:
bun test packages/dispatcher/test/implementation-workflow.test.ts # 8 pass — the park/resume spine + re-enqueue loop
bun test packages/dispatcher/test/poller.test.ts # 15 pass — the GitHub poller / review classifier
bun test packages/adapters/claude/test/adapter.test.ts # classifyStop sentinel surfacingWhat to verify (and what "correct" looks like)
How to review it
Fragile / needs extra eyes
Terminal state: all 4 sub-issues closed, all gates green, PR marked ready. The human reviews and merges — the workflow does not. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
packages/dispatcher/src/workflows/implementation.ts (2)
662-665: 💤 Low valueConsider reducing timeout for
resume-or-finalizestep.The step only performs quick operations (finalize worktree destruction or enqueueContinuation), but it inherits the same ~4-hour timeout budget as
launch-and-drive. A much shorter timeout (e.g., 60 seconds) would be more appropriate and would surface hangs faster..step("resume-or-finalize", resumeOrFinalize, { retry: 1, - timeout: launchTimeout + stopTimeout + 60_000, + timeout: 60_000, })🤖 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/dispatcher/src/workflows/implementation.ts` around lines 662 - 665, The "resume-or-finalize" workflow step currently inherits the large timeout computed as launchTimeout + stopTimeout + 60_000 (same as "launch-and-drive") but it only does quick operations; change the timeout for the .step("resume-or-finalize", resumeOrFinalize, { timeout: ... }) call to a much shorter value (e.g., 60_000 ms) so hangs surface faster while leaving retry behavior unchanged; update the timeout expression in the step configuration where "resume-or-finalize" and the variables launchTimeout/stopTimeout are referenced.
586-606: ⚡ Quick winReassess null-deref risk: bunqueue timeout should not reach
resumeOrFinalize
resumeOrFinalizeassumessignalis present (const payload = signal as ResumeSignalPayload;thenpayload.reason), but this step is only entered after the top-level.waitFor(RESUME_EVENT, { timeout: WAITFOR_TIMEOUT_MS })resolves; when the timeout elapses without the signal, bunqueue fails the execution (emitssignal:timeout), so it shouldn’t proceed withsignal === undefined.- Optional: add an explicit invariant guard before casting/accessing
payload.reason(e.g., throw for!signal) to make the assumption resilient to future engine/flow changes.🤖 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/dispatcher/src/workflows/implementation.ts` around lines 586 - 606, The code assumes a non-null signal in resumeOrFinalize when it does "const payload = signal as ResumeSignalPayload" and then accesses payload.reason; add an explicit invariant guard just after computing signal (e.g., if (!signal) throw new Error(`Missing resume signal after WAITFOR_TIMEOUT_MS for execution ${ctx.executionId}`)) so the function never proceeds with a null/undefined payload; keep the existing calls to consumeWaitForSignal(deps.db, ctx.executionId) and finalize(ctx, handle, ...) intact and only cast/access payload after the guard, referencing RESUME_EVENT, WAITFOR_TIMEOUT_MS, resumeOrFinalize, consumeWaitForSignal, and finalize to locate the change.
🤖 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/poller-gateway.ts`:
- Around line 28-33: The gh CLI output with --paginate can emit one JSON value
per page, so JSON.parse(out) in listIssueComments (and where reviewsOut is
parsed in findPrForEpic) will fail for multi-page results; update both places to
either call gh without --paginate and use --slurp, or (preferably) treat out as
newline-separated JSON pages: split out into separate JSON blobs, parse each
blob, and concatenate array results into a single array before proceeding (look
for function listIssueComments and variable reviewsOut / function findPrForEpic
to locate the two spots).
In `@packages/dispatcher/src/poller.ts`:
- Around line 130-136: The fallback that treats snapshot.reviewDecision ===
"CHANGES_REQUESTED" as a fresh trigger is firing on stale standing decisions;
change the logic in the poller (where snapshot.reviewDecision and
snapshot.labels are evaluated) to only emit "changes-requested" when the
standing decision is recent enough — i.e. check the review decision timestamp
(e.g. snapshot.reviewDecisionTimestamp or snapshot.reviewUpdatedAt) against the
arming boundary (sinceMs) before returning { outcome: "changes-requested", ...
}; keep the existing labels check but also ensure it respects the same
sinceMs/timebound so stale states do not re-trigger.
- Around line 121-128: The current logic in poller.ts uses ACTIONABLE_RE on
latest.body and may return resolved when the regex shows "0" even if
latest.state === "CHANGES_REQUESTED"; change the branching in the function that
evaluates latest (uses ACTIONABLE_RE, latest.body, latest.state,
snapshot.reviewDecision) so that you first check for an explicit latest.state
=== "CHANGES_REQUESTED" and return outcome "changes-requested" (with decision
"CHANGES_REQUESTED") before treating the ACTIONABLE_RE === 0 case as resolved;
ensure the check for Number(m[1]) === 0 only runs if latest.state is not
"CHANGES_REQUESTED" so explicit state wins over regex-derived actionable count.
In `@packages/dispatcher/test/implementation-workflow.test.ts`:
- Around line 451-456: The loop waiting for the cap-path execution to settle
(using engine.getExecution(id2)?.state and Bun.sleep) can time out silently;
after the waiting loop in implementation-workflow.test.ts, assert/fail fast if
the execution did not reach "completed" or "failed": retrieve the final state
via engine.getExecution(id2)?.state and throw an explicit error or call expect
to fail (including id2 and the observed state) so the test stops immediately and
reports the unsettled execution instead of continuing to subsequent DB
assertions.
In `@planning/issues/32/plan.md`:
- Line 10: Update the sentence that currently reads as a hard limit ("5") to
state the cap is configurable with a default of 5 and that the review loop still
terminates early on the "APPROVED" state; mention the configuration setting that
controls this (e.g. max_review_rounds / review_round_cap) so readers know where
to change it and keep the doc aligned with runtime behavior.
---
Nitpick comments:
In `@packages/dispatcher/src/workflows/implementation.ts`:
- Around line 662-665: The "resume-or-finalize" workflow step currently inherits
the large timeout computed as launchTimeout + stopTimeout + 60_000 (same as
"launch-and-drive") but it only does quick operations; change the timeout for
the .step("resume-or-finalize", resumeOrFinalize, { timeout: ... }) call to a
much shorter value (e.g., 60_000 ms) so hangs surface faster while leaving retry
behavior unchanged; update the timeout expression in the step configuration
where "resume-or-finalize" and the variables launchTimeout/stopTimeout are
referenced.
- Around line 586-606: The code assumes a non-null signal in resumeOrFinalize
when it does "const payload = signal as ResumeSignalPayload" and then accesses
payload.reason; add an explicit invariant guard just after computing signal
(e.g., if (!signal) throw new Error(`Missing resume signal after
WAITFOR_TIMEOUT_MS for execution ${ctx.executionId}`)) so the function never
proceeds with a null/undefined payload; keep the existing calls to
consumeWaitForSignal(deps.db, ctx.executionId) and finalize(ctx, handle, ...)
intact and only cast/access payload after the guard, referencing RESUME_EVENT,
WAITFOR_TIMEOUT_MS, resumeOrFinalize, consumeWaitForSignal, and finalize to
locate the change.
🪄 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: 61c9c87a-df01-4360-8ab6-f50d03a153da
📒 Files selected for processing (21)
.claude/skills/implementing-github-issues/SKILL.md.codex/skills/implementing-github-issues/SKILL.mdpackages/adapters/claude/src/classify.tspackages/adapters/claude/test/adapter.test.tspackages/cli/src/bootstrap-assets/skills/implementing-github-issues/SKILL.mdpackages/core/src/adapter.tspackages/core/src/index.tspackages/dispatcher/src/db/migrations/002_waitfor_fired.sqlpackages/dispatcher/src/dispatch.tspackages/dispatcher/src/main.tspackages/dispatcher/src/poller-cron.tspackages/dispatcher/src/poller-gateway.tspackages/dispatcher/src/poller.tspackages/dispatcher/src/workflow-record.tspackages/dispatcher/src/workflows/implementation.tspackages/dispatcher/test/db.test.tspackages/dispatcher/test/implementation-workflow.test.tspackages/dispatcher/test/poller.test.tspackages/skills/implementing-github-issues/SKILL.mdplanning/issues/32/decisions.mdplanning/issues/32/plan.md
…hase 4 gates + #80 Phase 5 (#32) was built against a pre-Phase-4 base; main since landed the skill-enforcement gates (#86), the positive-done-signal nudge loop (#80), the plan-comment guard, and the review-handling skill codification (#87/#88/#89). Integration (Phase 5 as the base, main's additions layered on): - driveOnce resolves a bare-stop through the #80 nudge loop (session stays alive during nudges); a ready, non-draft Epic PR resolves to `done`, which Phase 5 then parks on review-resolved. nudge-exhausted parks in waiting-human. - The plan-comment guard runs in driveOnce: an unposted `done` is demoted to `failed` so it never enters the review park. - DriveResult carries a DriveOutcome (StopClassification | nudge-exhausted); finalStateFor handles nudge-exhausted; the drive-step timeout widened for nudges. - Skill (4 copies) resolved to main's codified version; the #80 nudge + plan-comment tests rewritten park-aware. Verified: typecheck clean, full suite 289 pass / 0 fail, skills mirror in sync.
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/workflows/implementation.ts`:
- Around line 658-660: The code always calls
deps.worktree.destroyWorktree(handle) even when the job resolves to a human
handoff; change the teardown to skip destroying the worktree when the workflow
final state is "waiting-human" by checking finalStateFor(settled) (or the
computed finalState variable) and only calling
deps.worktree.destroyWorktree(handle) when finalState !== "waiting-human"; apply
the same conditional logic to the other destroyWorktree call in the nearby block
around the second occurrence (the block handling settled kinds around lines
667-673) so handoff context is preserved for waiting-human outcomes.
🪄 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: 61b72c65-9b8a-477f-aa84-03933e982666
📒 Files selected for processing (5)
packages/adapters/claude/test/adapter.test.tspackages/core/src/index.tspackages/dispatcher/src/dispatch.tspackages/dispatcher/src/workflows/implementation.tspackages/dispatcher/test/implementation-workflow.test.ts
… handoff teardown Address CodeRabbit review on PR #90: - poller-gateway: slurp + flatten paginated `gh api --paginate` output so multi-page issue-comment / PR-review reads don't break JSON.parse past page 1. - poller: drop the stale standing `reviewDecision === CHANGES_REQUESTED` resume fallback. A bot reviewer never flips its standing verdict, so it would re-dispatch the agent every pass with no new feedback; only a fresh review or an explicit `changes-requested` label is a trustworthy resume signal. The 0-actionable body still wins over an un-flipped state for a fresh review. - implementation: preserve the worktree on a `waiting-human` finalize so the human can inspect / resume the in-progress state. - test: fail fast if the cap-path execution never settles; add a regression test for the stale-standing-decision fix. - plan: note the review-round cap is configurable (default 5), not fixed.
|
@coderabbitai review Addressed the previous round in 0e48630: fixed paginated |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 127168
1.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/dispatcher/src/workflows/implementation.ts (1)
659-664: ⚡ Quick winAdd a regression assertion for the
nudge-exhaustedkeep-worktree path.This branch is only exercised when
finalize()seeswaiting-human(for example, the bare-stop →nudge-exhaustedflow). The current tests only assert worktree preservation on the review-cap path, which bypassesfinalize(). Please add alistWorktrees(...)assertion to thenudge-exhaustedtest so this fix cannot regress silently.🤖 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/dispatcher/src/workflows/implementation.ts` around lines 659 - 664, Add an assertion in the existing nudge-exhausted test to ensure the worktree is preserved when finalize() leaves the job in the "waiting-human" path: after driving the scenario that produces nudge-exhausted (the bare-stop → nudge-exhausted flow) and after any call that triggers finalize(), call deps.worktree.listWorktrees(...) and assert the returned list still contains the worktree handle used in the test; this complements the existing review-cap assertion and prevents regressions to the finalize() branch which calls deps.worktree.destroyWorktree(handle).
🤖 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/dispatcher/src/workflows/implementation.ts`:
- Around line 659-664: Add an assertion in the existing nudge-exhausted test to
ensure the worktree is preserved when finalize() leaves the job in the
"waiting-human" path: after driving the scenario that produces nudge-exhausted
(the bare-stop → nudge-exhausted flow) and after any call that triggers
finalize(), call deps.worktree.listWorktrees(...) and assert the returned list
still contains the worktree handle used in the test; this complements the
existing review-cap assertion and prevents regressions to the finalize() branch
which calls deps.worktree.destroyWorktree(handle).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f8f98424-d87e-4cf6-9377-c1ed21f0d8c8
📒 Files selected for processing (6)
packages/dispatcher/src/poller-gateway.tspackages/dispatcher/src/poller.tspackages/dispatcher/src/workflows/implementation.tspackages/dispatcher/test/implementation-workflow.test.tspackages/dispatcher/test/poller.test.tsplanning/issues/32/plan.md
✅ Files skipped from review due to trivial changes (1)
- planning/issues/32/plan.md
…eeping Address CodeRabbit's post-approval batch on PR #90: - poller-gateway: the `in:body Closes #N` search is a prefix match, so `#3` surfaced `#30`/`#300`. Re-confirm the exact closing reference client-side with a non-digit-boundary regex over the returned PR bodies, making epic→PR resolution deterministic as the issue space grows. - implementation: drop the duplicate `setRateLimited` in `recordTerminal`; `finalize` is the single authoritative terminal handler and always runs after the pre-seed falls the `waitFor` through. - implementation: in `resumeOrFinalize`, enqueue the continuation before clearing RATE_LIMITED so a throwing enqueue leaves both the rate-limit state and the row untouched for a clean poller retry.
|
@coderabbitai Addressed all three in fb2e47e.
Typecheck clean, 290 pass / 0 fail. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 4926
290 pass / 0 fail is a healthy signal too. The PR looks good to merge. 🐇✨ |
1 participant
Summary
Closes #32
Gives the
implementationworkflow a park → external-signal → resume spine so an agent can hand control back to a human (asked a question) or a reviewer (PR-ready), and later resume — as a fresh session in the same worktree, branch, and PR — with the answer or the review threads in context.APPROVED(or a clean re-review) ends the loop; a never-satisfied review loop is bounded to a configurable round cap (default 5), after which it parks for a human. middle never merges — the human does.The Epic's 4 sub-issues were the phases; all land on this one branch / PR.
What changed
packages/dispatcher/src/workflows/implementation.ts— the park/resume spine: a single top-levelwaitFor, an outcome branch (park vs terminal), andresume-or-finalizethat interprets the fired verdict and either finalizes or re-enqueues a continuation execution that reuses the prior round's worktree, re-primed per reason.packages/adapters/claude/src/classify.ts—classifyStopsurfaces the.middle/blocked.jsonquestion/context for the asked-question park.packages/dispatcher/src/poller.ts+poller-gateway.ts+poller-cron.ts— the GitHub poller: firesepic-<n>-answeredon a new human reply andepic-<n>-review-resolvedon a review transition (CHANGES_REQUESTED/label → resume;APPROVEDor 0-actionable re-review → resolved). Idempotent + rate-limit resilient.packages/dispatcher/src/db/migrations/002_waitfor_fired.sql—fired_atfor poller idempotency.packages/dispatcher/src/main.ts/dispatch.ts— poller +enqueueContinuationseams wired.packages/skills/implementing-github-issues/SKILL.md(+ synced mirrors) — new Phase 11 — Addressing review feedback procedure the resume brief points at.Why these changes
The installed
bunqueue@2.7.12WorkflowDSL silently drops awaitFornested inside a branch.path()or loop body (filters to steps only), and the executor has no goto/loop-back. So the spec's idealized nested park/resume graph isn't expressible: park/resume must be a single top-levelwaitFor, and multi-round looping must be re-enqueue (which the spec itself annotates// loop back via re-enqueue). That one constraint drives the whole shape — each park/resume cycle is one execution; a continuation is a fresh execution carryingresume = { reason, round, worktree, payload }that reuses the worktree (same branch/PR) and re-drives from a rewritten.middle/prompt.md. The full rationale chain is in the 8 inline review comments andplanning/issues/32/decisions.md.Status
Acceptance evidence (Epic #32)
blocked.json→ shown "asked question", session ends to free the slot, workflow parks on a wait signal; a human reply resumes it as a fresh session with the answer in contextasked-question(adapter.test.ts); park armsepic-<n>-answered+ ends session (implementation-workflow.test.ts "asked-question park → answer → resume (e2e)"); poller fires on a new non-bot reply (poller.test.ts); resume injects the reply into.middle/prompt.md(same e2e test asserts the brief contains the reply body + author)CHANGES_REQUESTEDreview (CodeRabbit, human, orchanges-requestedlabel) resumes the agent with the PR review threads in context — addresses them, pushes, replies in-thread, re-requests reviewepic-<n>-review-resolved+ pollerclassifyReviewOutcome(poller.test.ts); resume writes the "address review" brief that instructs the agent to pull the PR threads viaghand follow the procedure (implementation-workflow.test.ts "done park → review-changes → resume (e2e)" asserts the brief); the per-round procedure (batch → resolve class-wide → internal review loop → push once → reply in-thread → re-request → re-park) is codified in SKILL.md Phase 11APPROVEDends the loop (terminal — middle never merges); a never-satisfied review loop is bounded to 5 rounds, after which it parks inwaiting-humanAPPROVED→ terminalcompleted, no further round (implementation-workflow.test.ts review e2e); round counter increments perCHANGES_REQUESTEDpass and parkswaiting-humanat the cap with no re-arm / no continuation (same file, "review-round cap" test, run with cap=2)Verification
All gates green at
c3c86a3:bun run typecheckclean;bun test→ 237 pass / 0 fail across 27 files.implementation-workflow.test.ts— terminal fall-through, park/arm, compensation.adapter.test.ts—classifyStopsurfaces blocked.jsonquestion/context; tolerant of malformed sentinel.poller.test.ts— 15 tests: newest-non-bot-reply, review classifier, idempotent fire, per-workflow error isolation.implementation-workflow.test.ts— asked-question e2e (reply injected into continuation brief), review-changes e2e (address-review brief +APPROVEDends loop), round-cap boundary (waiting-humanat the cap, no further round). 8 workflow tests pass.Stumbling points
waitFornested in a branch/loop body compiles without error and is silently dropped at runtime. Discovered by a spike against the real embedded engine (Integrate waitFor signals into the implementation workflow #33) before building the production workflow; it reshaped the whole design (top-levelwaitFor+ re-enqueue).waiting-human. A test initially raced — it asserted the cap outcome while the workflow was still in its initial park (same row state). Fixed by waiting on the bunqueue execution settling (the definitive barrier), not the row state.ALTERa CHECK constraint in place, so a dedicatedsupersededstate would need a table rebuild — disproportionate today. Reusedcompleted; documented as a Phase 8/9 refinement (decision log + inline review comment).Suggested CLAUDE.md updates
The repo's
state-issuecontract section is solid. One candidate addition for the dispatcher area: a short note that the installed bunqueueWorkflowDSL filters branch/loop bodies to steps only —waitFor/branchnested inside are dropped; park/resume is a top-levelwaitFor+ re-enqueue. It cost a spike to discover and will bite the next workflow author. Deferred to the maintainer's judgment rather than edited unilaterally.Architectural forks
None — the bunqueue DSL constraint (verified by spike) decided the design; no genuinely-tied options to A/B.
Follow-up issues
None filed. The one discovery — a dedicated
supersededworkflows.statefor handed-off continuation rounds (vs reusingcompleted) — is naturally Phase 8 work (which routes dispatches through the persistent engine and revisits the row lifecycle) and is captured indecisions.md+ an inline review comment, not as a standalone issue.Out of scope (per the Epic)
engine.signal+enqueueContinuationseams are wired, and the resume logic is fully built and tested against a live embedded engine. ButdispatchEpic's engine drains when a workflow parks (waitForSettlereturns onwaiting), so a parked execution is not yet hosted on the long-livedmain.tsengine to be resumed in production. Routing dispatches through the persistent engine + durable bunqueue +recover()is the Phase 8 auto-dispatch integration — explicitly out of scope for Phase 5.Summary by CodeRabbit
New Features
Tests
Chores
Documentation