Skip to content

feat(dispatcher): auto-dispatch + limits (Phase 8)#132

Merged
thejustinwalsh merged 14 commits into
mainfrom
middle-issue-48
May 25, 2026
Merged

feat(dispatcher): auto-dispatch + limits (Phase 8)#132
thejustinwalsh merged 14 commits into
mainfrom
middle-issue-48

Conversation

@thejustinwalsh

@thejustinwalsh thejustinwalsh commented May 25, 2026

Copy link
Copy Markdown
Owner

Summary

Closes #48

Phase 8 makes dispatch autonomous within limits. Slot accounting gates the enqueue path (per-adapter / per-repo / global), an auto-dispatch loop walks the recommender's ranked plan and dispatches ready Epics — triggered on all four events (recommender-run completes, workflow terminal transition, rate-limit change, manual mm dispatch) behind one debounced scheduler. Auto-dispatch is opt-in per repo ([recommender] auto_dispatch, default off) and pausable (mm pause/mm resume). There is no pre-dispatch complexity gate: a complexity overrun is a runtime pause on a sub-issue that routes the workflow to waiting-human and surfaces under the state issue's complexity pause label — unless the Epic carries the approved label, which the dispatch brief uses to authorize a best-judgment call.

Status

Acceptance criteria (Epic #48)

What changed

  • packages/dispatcher/src/slots.ts (new) — getSlotState/hasFreeSlot/reserveSlot: the slot authority over the three gating dimensions, derived from live workflows rows + merged config (recommender row excluded).
  • packages/dispatcher/src/auto-dispatch.ts (new) — the loop: walk readyToDispatch, skip rate-limited adapters + exhausted per-adapter slots, stop on a full repo/global, decrement a local view as it enqueues. No complexity gate.
  • packages/dispatcher/src/main.ts — wires the four triggers behind a 250 ms debounced, re-entrancy-guarded scheduler; startDispatchImpl records source; slotAvailable gates manual dispatch; resolveComplexityCeiling feeds the brief.
  • packages/dispatcher/src/repo-config.ts (new) — paused_until helpers (mm pause/mm resume); future-dated pause auto-expires.
  • packages/dispatcher/src/rate-limits.ts — a rate-limit-change observer (a reset re-runs auto-dispatch for every known repo).
  • packages/dispatcher/src/hook-server.tsControlPlane.slotAvailable (manual dispatch → 429 when full, distinct from the 409 collision) + afterDispatch (manual-dispatch trigger).
  • packages/dispatcher/src/workflows/implementation.ts — complexity-pause routing (a kind-tagged asked-question), complexity_ceiling + approved injected into the dispatch brief, source tracking; brief-context resolution is failure-safe.
  • packages/dispatcher/src/build-deps.ts — default postQuestion (gh comment, complexity pause framing) + default isEpicApproved (reads the approved label) + formatPauseComment.
  • packages/dispatcher/src/github.tsgetIssueLabels. workflow-record.tssource in meta_json + getWorkflowSource.
  • packages/core/src/adapter.tsBlockedSentinel.kind. packages/adapters/claude/src/classify.ts — parse it.
  • packages/cli/mm pause / mm resume (DB-keyed by the git-remote slug) and mm config <repo> <key> <value> (formatting-preserving, section-scoped TOML edit; v1 auto_dispatch); shared deriveRepoSlug in paths.ts.

Why these changes

The slot guard is a pure function of (SlotState, adapter) so the loop and manual dispatch share one consultation and the cases are unit-testable without the engine — built on the existing countActiveImplementationSlots. The loop enqueues through the daemon's collision-guarded startDispatchImpl directly (not the HTTP route), so its own enqueues never re-fire the manual-dispatch trigger — no recursion. A complexity pause reuses the existing asked-question park spine (distinguished by a sentinel kind), so the Phase-5 waitFor/resume plumbing needs no change; since the dispatcher doesn't own the state issue's "Needs human input" section, it surfaces the pause as a recognizable Epic comment the recommender classifies. approved rides in the dispatch brief alongside complexity_ceiling — one place the agent reads its fork policy. Full rationale: planning/issues/48/decisions.md, distilled into inline review comments on this PR.

Verification

bun run typecheck, bun run lint, and bun test packages all green (540 tests). Per phase:

Stumbling points

  • The state issue's "Needs human input" section is recommender-owned, not dispatcher-owned — so a complexity pause can't be written there directly; it's surfaced as an Epic comment the recommender classifies on its next run.
  • A parked (waiting-human/rate-limited) workflow still counts as a used slot under the existing countActiveImplementationSlots semantics; I kept that convention rather than redefine it, so slot-freeing triggers fire only on the four terminal states.
  • The daemon has no startup registry of managed repos (repoPaths is populated lazily), which is why a true recommender cron is a follow-up (Periodic recommender scheduling (cron) + daemon repo registry #135) rather than a small addition here.

Suggested CLAUDE.md updates

  • The dispatcher package CLAUDE.md could note the new module-level setRateLimitObserver (mirrors setUpdateWorkflowObserver: process-scoped, reset to null on shutdown) so a future change preserves the shutdown reset.

Follow-up issues

Known limitations (for reviewer attention)

  • Manual mm dispatch slot-limiting is a soft cap checked just before enqueue; two concurrent manual force-dispatches of different Epics could over-subscribe by one (the collision guard only de-dupes the same Epic). Acceptable for an explicit force-dispatch; flagged rather than fixed with heavier atomic reservation.
  • inFlightEpics releases the dispatch reservation via the first workflow broadcast; if createWorkflowRecord itself threw after engine.start succeeded (a narrow DB-insert failure), the reservation would leak until restart. Pre-existing behavior, not introduced here.

Out of scope

  • Slot pills + auto-dispatch toggle UI in the dashboard (Phase 9).
  • The agent-side fork mechanic / pause decision (the implementing-github-issues skill) — Route sub-issue complexity overruns to waiting-human #52 defines only the sentinel kind contract the dispatcher reads, delivered to the agent via the brief.

Mergeability

Rebased onto main (clean, rerere-assisted) and re-verified; mergeable reads MERGEABLE.

Summary by CodeRabbit

  • New Features

    • Added mm pause/resume and mm config CLI commands; introduced auto-dispatch that enqueues ready work within slot and rate limits.
    • Slot-based concurrency accounting (global, per-repo, per-adapter) and manual-dispatch slot gating (HTTP 429 when full).
    • Distinct "complexity" pause kind with specialized pause comments and approval-aware behavior; dispatch records now track source ("manual"|"auto").
  • Tests

    • Expanded comprehensive test coverage across auto-dispatch, CLI config/pause, slot accounting, pause kinds, and dispatch source.
  • Documentation

    • Planning/decision docs updated describing auto-dispatch, slots, and complexity pause behavior.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 25, 2026

Copy link
Copy Markdown

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: a941dade-6f41-4798-837c-b54a941b6968

📥 Commits

Reviewing files that changed from the base of the PR and between 95a06bf and 81d35b3.

📒 Files selected for processing (10)
  • packages/adapters/claude/test/adapter.test.ts
  • packages/cli/src/commands/config.ts
  • packages/cli/src/commands/pause.ts
  • packages/cli/test/config.test.ts
  • packages/cli/test/pause-resume.test.ts
  • packages/dispatcher/src/build-deps.ts
  • packages/dispatcher/src/hook-server.ts
  • packages/dispatcher/src/main.ts
  • packages/dispatcher/test/control-routes.test.ts
  • planning/issues/48/plan.md
✅ Files skipped from review due to trivial changes (1)
  • planning/issues/48/plan.md

📝 Walkthrough

Walkthrough

Adds Phase 8: slot accounting, a debounced per-repo auto-dispatch loop, per-repo pause/resume and mm config CLI, complexity pause propagation from adapters to dispatcher with GitHub comment formatting, rate-limit observer, dispatch source metadata, implementation workflow prompts, and tests/docs.

Changes

Phase 8 Auto-dispatch, Limits, and Pause/Resume

Layer / File(s) Summary
Complexity pause classification in core types and adapters
packages/core/src/adapter.ts, packages/adapters/claude/src/classify.ts, packages/adapters/claude/test/adapter.test.ts
BlockedSentinel gains optional kind; Claude adapter parsing now preserves kind: "complexity" when present and tests validate recognized vs unrecognized kinds.
Slot accounting and concurrency limits
packages/dispatcher/src/slots.ts, packages/dispatcher/test/slots.test.ts
Introduces SlotLimits/SlotState, getSlotState deriving usage from DB, hasFreeSlot gating, and pure reserveSlot semantics with tests.
Per-repo pause/resume state repository
packages/dispatcher/src/repo-config.ts, packages/dispatcher/test/repo-config.test.ts
Adds INDEFINITE_PAUSE constant and repo-config functions: setPausedUntil (upsert), clearPaused, getPausedUntil, isPaused, with tests for semantics and expiry.
CLI pause, resume, and config commands
packages/cli/src/paths.ts, packages/cli/src/commands/pause.ts, packages/cli/src/commands/config.ts, packages/cli/src/index.ts, packages/cli/test/config.test.ts, packages/cli/test/pause-resume.test.ts
Adds deriveRepoSlug, mm pause/mm resume handlers wired into CLI, mm config with setTomlKey performing scoped TOML edits, and tests covering command behaviors and edge cases.
Rate-limit observer pattern and notifications
packages/dispatcher/src/rate-limits.ts
Introduces RateLimitObserver and setRateLimitObserver(); setRateLimited/markAvailable emit observer notifications safely.
GitHub label and comment operations
packages/dispatcher/src/github.ts, packages/dispatcher/src/build-deps.ts, packages/dispatcher/test/build-deps.test.ts
Adds getIssueLabels, updates formatPauseComment to differentiate question vs complexity comment bodies, wires default postQuestion to github.postComment, and tests the behavior.
Auto-dispatch loop orchestration
packages/dispatcher/src/auto-dispatch.ts, packages/dispatcher/test/auto-dispatch.test.ts
Implements autoDispatch with dependency/result types; iterates ready rows, skips malformed/rate-limited/adapters without slots, enqueues eligible workflows, and returns enqueued plus reason.
Manual dispatch slot gating and callbacks
packages/dispatcher/src/hook-server.ts, packages/dispatcher/test/control-routes.test.ts
Control plane adds slotAvailable check to return HTTP 429 when no slot is free, and invokes optional afterDispatch after successful manual dispatch; tests assert 429/200 behaviors and callback exceptions.
Recommender auto-dispatch trigger integration
packages/dispatcher/src/recommender-run.ts, packages/dispatcher/test/recommender-run.test.ts
Adds optional triggerAutoDispatch to recommender options and wires it into recommender workflow; tests validate invocation respects autoDispatch config.
Dispatch source tracking and metadata
packages/dispatcher/src/workflow-record.ts, packages/dispatcher/test/workflow-record.test.ts
Adds optional source to CreateWorkflowRecordInput, persists meta_json.source on create, and provides getWorkflowSource to read it safely; tests added.
Implementation workflow complexity ceiling and approval handling
packages/dispatcher/src/workflows/implementation.ts, packages/dispatcher/test/implementation-workflow.test.ts
Adds resolveComplexityCeiling and isEpicApproved deps, parameterizes prompt brief with ceiling/approval, classifies/parks complexity pauses with kind, threads source across continuations, and adds tests validating behavior and fallbacks.
Daemon main loop integration and orchestration
packages/dispatcher/src/main.ts
Integrates auto-dispatch into daemon with debounced per-repo scheduler, collision-guarded enqueue/reservation semantics, rate-limit observer scheduling, recommender registration, manual gating, complexity-ceiling wiring, and shutdown cleanup.
Public API exports and documentation
packages/dispatcher/src/index.ts
Re-exports autoDispatch, slot helpers/types, and pause-state functions; updates module doc comments to reference new modules.
Planning and decision documentation
planning/issues/48/plan.md, planning/issues/48/decisions.md
Adds Phase 8 planning and decisions documenting slot authority, auto-dispatch triggers, CLI/keying semantics, complexity pause surfacing, and approval override rules.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~75 minutes

Possibly related issues

Possibly related PRs

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The PR title 'feat(dispatcher): auto-dispatch + limits (Phase 8)' accurately and concisely summarizes the main change: implementing autonomous dispatch within limits as the primary feature of Phase 8.
Linked Issues check ✅ Passed The PR comprehensively implements all Phase 8 objectives from issue #48: slot tracking across three dimensions [slots.ts], auto-dispatch loop with four triggers behind one debounced scheduler [main.ts, auto-dispatch.ts], per-repo auto_dispatch toggle and pause/resume controls [repo-config.ts, config.ts, pause.ts], runtime complexity-pause routing [workflows/implementation.ts, build-deps.ts], and approved-label handling [build-deps.ts, implementation.ts].
Out of Scope Changes check ✅ Passed All changes are scoped to Phase 8 objectives and the five sub-issues (#49#53): slot accounting, auto-dispatch loop, CLI pause/resume/config, complexity-pause handling, and approved-label routing. The periodic recommender cron scheduling is appropriately tracked separately as #135.
Docstring Coverage ✅ Passed Docstring coverage is 83.64% which is sufficient. The required threshold is 80.00%.

✏️ 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

If the error stems from missing dependencies, add them to the package.json file. For unrecoverable errors (e.g., due to private dependencies), disable the tool in the CodeRabbit configuration.

ESLint skipped: no ESLint configuration detected in root package.json. To enable, add eslint to devDependencies.


Comment @coderabbitai help to get the list of available commands and usage tips.

Add packages/dispatcher/src/slots.ts: getSlotState derives the three
gating dimensions (per-adapter, per-repo, global) from live workflows
rows + merged config; hasFreeSlot is the enqueue guard; reserveSlot is
the auto-dispatch loop's pure local decrement. The recommender's row is
excluded from slot counts (its dedicated slot). Covers at-capacity,
free-slot, and per-adapter-vs-global-vs-repo cases.
Add packages/dispatcher/src/auto-dispatch.ts: walk readyToDispatch,
skip rate-limited adapters and exhausted per-adapter slots, stop on a
full repo/global, decrementing a local SlotState as it enqueues. No
pre-dispatch complexity gate. Wire the four triggers in main.ts behind
a debounced, re-entrancy-guarded scheduler: workflow terminal-state
transitions (broadcastWorkflow), rate-limit changes (new rate-limit
observer), recommender-run completion (threaded triggerAutoDispatch),
and manual mm dispatch (ControlPlane.afterDispatch). The loop's enqueue
goes through the daemon's collision-guarded startDispatch directly, so
its own enqueues never re-trigger the loop.
repo-config.ts: setPausedUntil/clearPaused/isPaused/getPausedUntil over
the repo_config.paused_until column (lazy row create, future-dated pause
auto-expires). The auto-dispatch loop's enable-check now composes the
[recommender] auto_dispatch toggle (default false) with !isPaused.

CLI: mm pause / mm resume (DB-keyed by the git-remote slug, shared
deriveRepoSlug in paths.ts) and mm config <repo> <key> <value> (a
formatting-preserving, section-scoped TOML edit; v1 supports
auto_dispatch). Tests cover toggle off/on, paused-until-in-future,
comment/sibling preservation, and unknown-key/invalid-value rejection.
A complexity pause is a kind-tagged asked-question: BlockedSentinel
gains kind ('question'|'complexity'), the claude adapter parses it, and
the implementation workflow passes the kind to postQuestion on park
(already routing to waiting-human). The default gh-backed poster frames
a complexity pause with the literal 'complexity pause' label so the
recommender classifies it under Needs human input. The dispatch brief
now carries the repo's complexity_ceiling (resolved per repo, default 3)
as the agent's fork budget and instructs it to tag a complexity overrun.
No pre-dispatch ceiling gate in the loop (pinned by a test).
#53)

isEpicApproved defaults to reading the Epic's 'approved' GitHub label
(new github.getIssueLabels); when present the dispatch brief authorizes
proceeding past a complexity overrun with a best-judgment call instead
of pausing. Manual mm dispatch now respects slot limits via a route-level
ControlPlane.slotAvailable gate (429 when full), distinct from the 409
collision guard; the loop bypasses the route so its accounting is
unaffected. Dispatch origin recorded as meta_json.source ('manual' vs
'auto'), threaded through ImplementationInput and carried by
continuations. Tests: approved-proceeds brief, manual 429/200 slot gate,
source round-trip.
A flaky gh label read (isEpicApproved) or per-repo config load
(resolveComplexityCeiling) must not fail the whole dispatch — fall back
to safe defaults (ceiling 3, not approved) and log. Also gate the
resolution on the brief not already existing, so a resume (which writes
its own brief) skips the gh round-trip entirely.
…nfig key

Self-review follow-ups:
- slotAvailable now receives the full ControlDispatchInput and resolves
  slot caps from the request's own repoPath, so manual mm dispatch
  respects max_concurrent even for a repo the daemon hasn't dispatched
  yet this lifetime (previously a cold repoPaths returned 'slot free').
- setTomlKey escapes the key before building its regex (SETTABLE is the
  extension point; a future key with regex metacharacters must match
  literally).

@thejustinwalsh thejustinwalsh left a comment

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decision-log rationale, distilled inline (the why behind the load-bearing choices). Source: planning/issues/48/decisions.md.

* distinct (a repo's per-repo cap must not be charged for another repo's agents).
* The recommender's row is excluded by {@link countActiveImplementationSlots}.
*/
export function getSlotState(db: Database, repo: string, limits: SlotLimits): SlotState {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a separate authority over re-counting: countActiveImplementationSlots already counts non-terminal implementation rows (recommender excluded) per-repo and globally; slots.ts only adds the limit/availability layer on top. Keeping the guard a pure function of (SlotState, adapter) makes at-capacity / free-slot / per-adapter-vs-global unit-testable without the engine, and lets the loop and manual dispatch share one consultation. I deliberately did not overload the daemon's startDispatch string|null (whose null means the 409 collision reservation) with a slots-full signal — that would conflate "already running" with "queue full".

const autoDispatchTimers = new Map<string, ReturnType<typeof setTimeout>>();
const autoDispatchRunning = new Set<string>();
const autoDispatchRerun = new Set<string>();
function scheduleAutoDispatch(repo: string): void {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a debounced, re-entrancy-guarded scheduler: all four trigger events (recommender-complete, terminal transition, rate-limit change, manual dispatch) funnel here so a burst coalesces into one pass per repo, and a trigger arriving mid-pass re-runs exactly once after. Crucially the loop's own enqueue calls startDispatchImpl directly (not the HTTP route), so it never fires afterDispatch and can't recursively re-trigger itself — afterDispatch is manual-dispatch-only.

* hasn't dispatched yet this lifetime (cold `repoPaths`). Conservative: an
* unreadable config reports a free slot rather than blocking a manual dispatch.
*/
function slotAvailable(input: ControlDispatchInput): boolean {

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why the slot gate is route-level (not inside startDispatchImpl): the auto-dispatch loop already does its own local slot accounting and enqueues through startDispatchImpl directly; a DB-slot check inside the shared path would double-gate the loop inconsistently (rows are created async, so a mid-pass DB read disagrees with the loop's local view). Resolving caps from the request's own repoPath keeps the gate effective even for a repo the daemon hasn't dispatched yet this lifetime. A distinct 429 (vs the collision 409) tells mm dispatch why it was refused.

* timestamp are touched on conflict, so a later config sync isn't clobbered).
* `until` is unix-ms; omit it to pause indefinitely (`mm pause`).
*/
export function setPausedUntil(

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why keyed by the git-remote slug + indefinite default: the auto-dispatch loop reads pause state by repo = the git-remote owner/name (the same key a dispatch/recommender resolves), so mm pause must derive the slug identically or it writes a row the loop never reads. mm pause with no duration is indefinite (MAX_SAFE_INTEGER); isPaused honors the timestamp so a future-dated pause auto-expires with no cleanup.

if (outcome.kind === "asked-question" && deps.postQuestion) {
// The sentinel's `kind` distinguishes a complexity pause (surfaced under the
// `complexity pause` state-issue label) from a plain question.
const kind = outcome.sentinel?.kind === "complexity" ? "complexity" : "question";

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why a complexity pause reuses the asked-question spine: distinguishing it by a kind field on the sentinel (rather than a new park kind) means the waitFor/resume plumbing (Phase 5) needs no change — a complexity pause resumes the same way once the human clarifies or applies approved. The dispatcher can't write the state issue's "Needs human input" section (recommender-owned), so the surface is a GitHub comment the recommender keys off for the complexity pause label.

}),
resolveComplexityCeiling: args.resolveComplexityCeiling,
// Default: the Epic is approved iff it carries the `approved` label (#53).
isEpicApproved:

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why approved is delivered via the brief: it lands alongside complexity_ceiling — one place the agent reads its fork policy — rather than a separate channel. The default reads the Epic's labels via gh; a flaky read is made failure-safe in driveOnce (falls back to not-approved, never fails the dispatch).

@thejustinwalsh

Copy link
Copy Markdown
Owner Author

Reviewer's brief — Phase 8 (auto-dispatch + limits), PR #132

What shipped: all five sub-issues (#49#53). Slot accounting + an auto-dispatch loop (four triggers behind one debounced scheduler), per-repo auto_dispatch toggle + mm pause/mm resume, runtime complexity-pause routing to waiting-human, and the approved-label override — all delivered to the agent through the dispatch brief.

How to run it

bun install
bun run typecheck   # tsc --noEmit
bun run lint        # oxlint --fix --deny-warnings
bun test packages   # 540 pass

What to verify (and what "correct" looks like)

  • Slot math (packages/dispatcher/src/slots.ts): hasFreeSlot is true only when global and repo and (if capped) the adapter dimension all have headroom; reserveSlot is pure and decrements all three. getSlotState excludes the recommender's row.
  • The loop (auto-dispatch.ts): skips a rate-limited adapter / exhausted per-adapter slot (continue), stops on a full repo/global (break), decrements a local view as it enqueues, and never gates on complexity/sub-issue-count. A refused (collision) enqueue must not charge a slot.
  • No recursion (main.ts): the loop enqueues via startDispatchImpl directly; only the HTTP route fires afterDispatch. Confirm the loop can't re-trigger itself.
  • Complexity pause (workflows/implementation.ts + build-deps.ts): a .middle/blocked.json with "kind":"complexity"waiting-human + an Epic comment carrying the literal complexity pause (the recommender's label key). The brief carries the repo's complexity_ceiling; with the approved label it instead authorizes a best-judgment call. Brief-context resolution is failure-safe (a flaky gh read falls back to defaults, never fails the dispatch).
  • Manual dispatch (hook-server.ts): 429 when no slot (distinct from the 409 collision); source: 'manual' recorded in meta_json.

Fragile / extra eyes

  • mm config does a section-scoped, formatting-preserving TOML line edit (config.ts) — confirm it leaves comments/siblings byte-identical.
  • The slot cap on manual dispatch is a soft check just before enqueue (see "Known limitations" in the PR body): concurrent force-dispatches of different Epics could over-subscribe by one.
  • The "recommender runs on cron" half of Epic criterion 2 is not in this PR — it needs a daemon repo registry; tracked in Periodic recommender scheduling (cron) + daemon repo registry #135. Everything else for that criterion (auto-dispatch within slot limits, triggered on recommender completion) is delivered.

How to review

Decision rationale is distilled into inline review comments on the PR (source: planning/issues/48/decisions.md). The PR body's "What changed" / "Why" / "Verification" cover the rest.

@thejustinwalsh thejustinwalsh mentioned this pull request May 25, 2026
3 tasks
@thejustinwalsh thejustinwalsh marked this pull request as ready for review May 25, 2026 04:34
@thejustinwalsh thejustinwalsh added the ready-for-review All phases done and verified — PR ready for final human review and merge label May 25, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🧹 Nitpick comments (1)
packages/dispatcher/src/build-deps.ts (1)

22-24: ⚡ Quick win

Use APPROVED_LABEL in pause text to avoid label drift.

The complexity comment text hardcodes approved while approval checks use APPROVED_LABEL; keeping one source of truth prevents mismatch later.

Proposed fix
- A human resolves this by **scope reduction or clarification** — or applies the \`approved\` label to authorize a best-judgment call within the ceiling on resume.`;
+ A human resolves this by **scope reduction or clarification** — or applies the \`${APPROVED_LABEL}\` label to authorize a best-judgment call within the ceiling on resume.`;

Also applies to: 40-45

🤖 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/build-deps.ts` around lines 22 - 24, Replace the
hardcoded "approved" string in the complexity pause/comment text with the
APPROVED_LABEL constant to keep a single source of truth; update every
occurrence in this file (the pause text variable(s) around the complexity pause
and the other instance noted between lines 40-45) so they reference
APPROVED_LABEL instead of the literal "approved".
🤖 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/adapters/claude/test/adapter.test.ts`:
- Around line 252-267: The test that verifies fallback for an unrecognized kind
should assert the classification explicitly instead of guarding with an if;
update the test around claudeAdapter.classifyStop to assert that result.kind ===
"asked-question" (e.g., using expect(result.kind).toBe("asked-question")) and
then assert the sentinel content (result.sentinel) equals { question: "Q" }, so
the test fails if classification regresses rather than silently passing when the
guarded block is skipped.

In `@packages/cli/src/commands/config.ts`:
- Around line 4-7: Add a top-level TSDoc comment for the exported ConfigOptions
type describing its purpose and fields; update the file by adding a JSDoc/TSDoc
block immediately above the exported type ConfigOptions explaining that it
represents CLI configuration overrides and documenting the optional configFile
property (what it overrides and the default path behavior). Ensure the comment
uses proper TSDoc tags/format (brief summary and param/field description for
configFile) so the export complies with the project's “every public export must
carry a TSDoc/JSDoc comment” guideline.
- Around line 30-48: The header detection currently uses headerRe which only
matches bare “[section]” and thus misses headers with trailing comments; update
headerRe (and the similar use around lines 50-55) to accept optional trailing
comments/whitespace (e.g., allow a "#" and anything after the closing bracket)
so headers like "[recommender] # note" are recognized; keep the captured group
as the section name (m[1]) unchanged, leave keyRe, sectionStart, assignment,
lines and source logic intact, and run the same adjusted regex wherever headerRe
is used to avoid appending duplicate sections.

In `@packages/cli/src/commands/pause.ts`:
- Around line 45-47: The resolve(...) and openAndMigrate(...) calls in the pause
command run outside the try/catch so thrown errors can bypass the intended
exit-code handling; move or duplicate those calls inside the existing try block
(or wrap each call in its own try/catch) so any exception from resolve("pause",
repoPath, opts) or openAndMigrate(resolved.dbPath) is caught and the function
returns the documented exit code (1 on failure, 0 on success), and ensure the
same pattern is applied to the other occurrence around lines 66–69.
- Around line 8-15: Add a top-level TSDoc/JSDoc block immediately above the
exported type declaration export type PauseResumeOptions that briefly describes
the purpose of this options object (e.g., options for pause/resume CLI
commands), noting that it overrides config and db paths and can supply a
resolveSlug function; keep it concise (one summary sentence and optional
`@remarks` or `@example` if useful) so the public export carries documentation per
guidelines while keeping the existing per-field comments intact.

In `@packages/dispatcher/src/hook-server.ts`:
- Around line 398-400: The best-effort post-dispatch callback
control.afterDispatch(normalizedRepo) can throw and turn a successful manual
dispatch into a 500; wrap the call in a try/catch to isolate errors so they
don't affect the HTTP response. Locate the invocation of control.afterDispatch
in hook-server.ts (the lines calling afterDispatch with normalizedRepo) and
surround it with a try block, catch any error, and log it (or swallow it)
without rethrowing so startDispatch's success path still returns a successful
response.

In `@packages/dispatcher/src/main.ts`:
- Around line 254-256: scheduleAutoDispatch reads the variable shuttingDown
before it's declared, risking a TDZ ReferenceError when hookServer.start
triggers afterDispatch; move the declaration/initialization of shuttingDown (and
any related state like autoDispatchTimers if applicable) above any
functions/calls that may reference it (specifically ensure let shuttingDown =
false; is defined before scheduleAutoDispatch, hookServer.start, and any
registration of afterDispatch handlers), or alternatively convert shuttingDown
to a const/var initialized earlier so scheduleAutoDispatch and afterDispatch can
safely read it at runtime.

In `@planning/issues/48/plan.md`:
- Line 7: Update the scoped statement on line containing the phrase "the
recommender runs on cron" to remove the claim that cron scheduling is delivered
in this Epic; reword it to indicate the recommender can be scheduled later or
that cron scheduling is out of scope (referencing follow-up `#135`) — keep the
rest of the sentence about ready Epics auto-dispatching within slot limits and
the approved-label override unchanged.

---

Nitpick comments:
In `@packages/dispatcher/src/build-deps.ts`:
- Around line 22-24: Replace the hardcoded "approved" string in the complexity
pause/comment text with the APPROVED_LABEL constant to keep a single source of
truth; update every occurrence in this file (the pause text variable(s) around
the complexity pause and the other instance noted between lines 40-45) so they
reference APPROVED_LABEL instead of the literal "approved".
🪄 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: 7566d9b1-7b1d-41a2-b235-4938a3a4bdd7

📥 Commits

Reviewing files that changed from the base of the PR and between 4cf69ca and 95a06bf.

📒 Files selected for processing (31)
  • packages/adapters/claude/src/classify.ts
  • packages/adapters/claude/test/adapter.test.ts
  • packages/cli/src/commands/config.ts
  • packages/cli/src/commands/pause.ts
  • packages/cli/src/index.ts
  • packages/cli/src/paths.ts
  • packages/cli/test/config.test.ts
  • packages/cli/test/pause-resume.test.ts
  • packages/core/src/adapter.ts
  • packages/dispatcher/src/auto-dispatch.ts
  • packages/dispatcher/src/build-deps.ts
  • packages/dispatcher/src/github.ts
  • packages/dispatcher/src/hook-server.ts
  • packages/dispatcher/src/index.ts
  • packages/dispatcher/src/main.ts
  • packages/dispatcher/src/rate-limits.ts
  • packages/dispatcher/src/recommender-run.ts
  • packages/dispatcher/src/repo-config.ts
  • packages/dispatcher/src/slots.ts
  • packages/dispatcher/src/workflow-record.ts
  • packages/dispatcher/src/workflows/implementation.ts
  • packages/dispatcher/test/auto-dispatch.test.ts
  • packages/dispatcher/test/build-deps.test.ts
  • packages/dispatcher/test/control-routes.test.ts
  • packages/dispatcher/test/implementation-workflow.test.ts
  • packages/dispatcher/test/recommender-run.test.ts
  • packages/dispatcher/test/repo-config.test.ts
  • packages/dispatcher/test/slots.test.ts
  • packages/dispatcher/test/workflow-record.test.ts
  • planning/issues/48/decisions.md
  • planning/issues/48/plan.md

Comment thread packages/adapters/claude/test/adapter.test.ts
Comment thread packages/cli/src/commands/config.ts
Comment thread packages/cli/src/commands/config.ts Outdated
Comment thread packages/cli/src/commands/pause.ts
Comment thread packages/cli/src/commands/pause.ts Outdated
Comment thread packages/dispatcher/src/hook-server.ts Outdated
Comment thread packages/dispatcher/src/main.ts
Comment thread planning/issues/48/plan.md Outdated
A bare `[section]` header match missed valid TOML like `[recommender] # note`
and `[ recommender ]`, so `mm config` appended a duplicate section instead of
editing in place. Match via a `headerName` helper (trailing-comment- and
whitespace-tolerant, trimmed name) used by both the section-find loop and the
key-scan break. Also adds the missing TSDoc on the exported `ConfigOptions`.
`resolve()` and `openAndMigrate()` ran before the try/catch, so a throw (e.g. a
failed git-remote slug derivation) rejected the promise instead of returning the
documented exit code 1. Move both inside the try; `db` is now nullable with
`db?.close()` in finally. Also adds the missing TSDoc on `PauseResumeOptions`.
…response

A throwing `afterDispatch` turned a manual dispatch that already succeeded into a
500. Wrap the callback so a failure logs and is swallowed; the route still
returns the workflow id.
…atch

`scheduleAutoDispatch` reads `shuttingDown`, but the `let` was declared after
`hookServer.start`; a slot-freeing broadcast firing that path early threw a TDZ
ReferenceError. Declare it up front, before any reader.
The pause comment hardcoded `approved` while approval checks use APPROVED_LABEL;
interpolate the constant so the label has one source of truth.
Without asserting `result.kind`, the test passed even if classification
regressed and the guarded block never ran. Also reword plan.md so it no longer
claims cron scheduling shipped in this Epic (out of scope, tracked as #135).
@thejustinwalsh

Copy link
Copy Markdown
Owner Author

Review round 1 — addressed

Batched all 8 actionable findings + the nitpick, one fix (with a test) per class, pushed once (81d35b3):

Finding Resolution
main.ts TDZ on shuttingDown (🔴 critical) Hoisted the declaration above its first reader.
config.ts header match too loose (🟠) Resolved the classheaderName helper now tolerates trailing # comment and [ inner whitespace ]; both edges tested.
pause.ts pre-try failures bypass exit codes (🟠) Moved resolve()/openAndMigrate() inside the try in both commands; nullable db + db?.close(); tested.
hook-server.ts afterDispatch can 500 a success (🟠) Wrapped best-effort, logs+swallows; tested via a throwing afterDispatch.
config.ts / pause.ts missing TSDoc (🟠) Added top-level blocks on ConfigOptions and PauseResumeOptions.
adapter.test.ts silent-pass fallback test Now asserts result.kind explicitly.
plan.md over-claims cron delivery (🟡) Reworded; cron is out of scope (follow-up #135).
build-deps.ts hardcoded approved (nitpick) Interpolates APPROVED_LABEL.

Verification: bun run typecheck + bun run lint clean; full bun test green (544 pass). Ran an internal clean-eyes review pass over the batched diff — converged with no new findings.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ready-for-review All phases done and verified — PR ready for final human review and merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Auto-dispatch and limits

1 participant