feat(dispatcher): minimal dispatcher — Phase 1 (Epic #6)#73
Conversation
thejustinwalsh
left a comment
thejustinwalsh
left a comment
There was a problem hiding this comment.
Decisions-log highlights distilled into per-line comments. Full reasoning in planning/issues/6/decisions.md.
| for (const migration of pending) { | ||
| db.transaction(() => { | ||
| db.exec(migration.sql); | ||
| db.run("INSERT OR IGNORE INTO schema_version (version) VALUES (?)", [migration.version]); |
There was a problem hiding this comment.
Decision: the runner records the schema version, the migration SQL may too. 001_initial.sql keeps the spec's verbatim INSERT INTO schema_version VALUES (1), but relying on every future migration author to remember that self-insert is fragile. INSERT OR IGNORE here makes version tracking a property of the runner — a harmless no-op for 001, a backstop for any later migration that omits it. Verified by the idempotency test (run twice → stays at 1, no throw).
| */ | ||
| export function openDb(path: string): Database { | ||
| const db = new Database(path, { create: true }); | ||
| db.exec("PRAGMA journal_mode = WAL;"); |
There was a problem hiding this comment.
Note for reviewers/testers: SQLite silently keeps :memory: databases in "memory" journal mode — so the WAL path can only be verified against a real file. The db test suite opens databases under a mkdtemp temp dir for exactly this reason; a :memory: test would assert the wrong mode.
| * files are tolerated: an absent global file falls back to documented defaults, | ||
| * an absent per-repo file leaves the per-repo sections undefined. | ||
| */ | ||
| export function loadConfig(opts: LoadConfigOptions): MiddleConfig { |
There was a problem hiding this comment.
Decision: generic deep merge, not a field-by-field override list. The spec says "per-repo overrides global" but the two files have almost disjoint sections. A generic deep merge (per-repo over global; arrays/scalars replaced wholesale) means a per-repo file can override any global key for free, and the common disjoint case still works. Per-repo sections are typed T | undefined and populated only when the per-repo file loads — there is no honest default for repo.owner, so a global-only load leaves them undefined rather than inventing values.
| * exit between turns, so this — not an exit code — is the signal the workflow | ||
| * reacts to. Order matters: an open question outranks everything else. | ||
| * | ||
| * Phase 1 detects `done`/`failed` via `.middle/done.json` / `.middle/failed.json` |
There was a problem hiding this comment.
Decision: done/failed are detected via .middle/done.json / .middle/failed.json sentinels in Phase 1. The spec describes classifyStop as "reads PR state for done", but the fixed interface ({ payload, transcriptPath, sentinelPresent }) gives the adapter no PR handle, and Phase 1 ships no skill enforcement. Sentinels — parallel to the .middle/blocked.json question sentinel — keep every classifyStop branch deterministically classifiable and unit-testable. Phase 4's mechanically-enforced PR-ready hook gate replaces the done.json path with the real "agent ran gh pr ready" signal.
| * than depending on the dispatcher's richer helper module — entering auto mode | ||
| * is intrinsically a per-CLI keystroke concern the adapter owns. | ||
| */ | ||
| async function enterAutoMode(opts: { sessionName: string }): Promise<void> { |
There was a problem hiding this comment.
Decision: enterAutoMode shells out to tmux directly, not via the dispatcher's tmux.ts helper. @middle/adapter-claude depends on @middle/core only; the tmux helpers live in @middle/dispatcher, and an adapter→dispatcher edge would invert the layering. Entering auto mode is intrinsically a per-CLI keystroke concern the adapter owns. The S-Tab S-Tab keystroke path is the guaranteed fallback the spec calls for; whether a launch flag also works is the spec's open empirical question, resolved when the real-Claude end-to-end is exercised. Not unit-tested (needs a live tmux session).
| }; | ||
|
|
||
| /** | ||
| * Phase 1 minimal hook receiver. It handles only the two load-bearing events — |
There was a problem hiding this comment.
Decision: the "minimal SessionStart receiver" handles two events, not one. Build-sequence item 10 names it the SessionStart receiver, but #12's launch-and-drive must react to the Stop boundary, and the Phase 1 acceptance gate is "the agent hits a Stop; classifyStop runs". A SessionStart-only receiver could not drive the 3-step workflow to its gate. "Minimal" still holds relative to Phase 2: no HMAC auth, no events-table persistence, no full taxonomy — just the two load-bearing events. Early-arriving payloads are stashed so a fast hook cannot race the workflow step.
| * Built as a factory so the dispatcher injects real collaborators and tests | ||
| * inject stubs. The workflow's `executionId` doubles as the `workflows.id`. | ||
| */ | ||
| export function createImplementationWorkflow( |
There was a problem hiding this comment.
Decision: workflow factory + structural deps; compensated ≠ failed. bunqueue's StepContext carries only input/steps/signals, so ambient collaborators come via closure — a factory keeps the workflow a pure builder the dispatcher and tests configure identically. Structural TmuxOps/WorktreeOps let the end-to-end test stub tmux while using the real worktree helpers, so "no worktree leak" is genuinely verified. launch-and-drive catch-kills its session on any throw because bunqueue runs compensation only for completed steps. Terminal states: a step that throws → compensated (workflow rolled back); a clean run whose classifyStop returns failed → failed (agent failed). Keeping them distinct keeps the terminal state honest about what failed.
| await git(scratch, ["init", "repo"]); | ||
| await git(repoPath, ["commit", "--allow-empty", "-m", "init"]); | ||
| db = openAndMigrate(join(scratch, "db.sqlite3")); | ||
| // No dataPath → bunqueue's queue + workflow store are in-memory: isolated per |
There was a problem hiding this comment.
Decision: bunqueue runs in-memory in tests (no dataPath). With a file-backed dataPath under a mkdtemp dir on macOS, bunqueue's write-buffer flushes during the retry path hit SQLITE_IOERR_VNODE. Omitting dataPath makes the queue + workflow store in-memory — isolated per Engine, no vnode churn. The production dispatcher path also runs in-memory in Phase 1; durable queue persistence + crash recovery arrive with the Phase 2 watchdog/reconciler. Note: bunqueue retries a failing step by default, so the leak check asserts every distinct created session was killed (not an exact call count).
| * workflow to settle, then tear everything down. Self-contained — the caller | ||
| * (`mm dispatch`) just supplies validated inputs and an adapter registry. | ||
| */ | ||
| export async function dispatchEpic(opts: DispatchEpicOptions): Promise<DispatchEpicResult> { |
There was a problem hiding this comment.
Decision: mm dispatch is self-contained; dispatchEpic takes an injected adapter registry. It does not talk to the long-running mm start process — that needs an IPC/HTTP control plane, which is Phase 8. dispatchEpic stands up its own hook server + engine for the run. It lives in @middle/dispatcher (not the CLI) so bunqueue/tmux/worktree coupling stays contained, and takes getAdapter as a parameter so @middle/dispatcher has no concrete-adapter dependency — the dependency graph stays cli → {dispatcher, adapter-claude} with no dispatcher → adapter-* edge. Phase 8 adds the HTTP trigger that lets mm dispatch enqueue into the running process instead.
|
Ready for review — Epic #6 · PR #73 Build-spec Phase 1, the minimal dispatcher. All 7 sub-issues (#7–#13) delivered, full suite 100 pass, How to run itWhat to verify
How to review thisRead in dependency order, bottom-up: Needs extra eyes
|
|
@greptileai reveiw pls |
|
| Filename | Overview |
|---|---|
| packages/adapters/claude/src/hooks.ts | installHooks now registers both SessionStart and Stop events; prior issues about relative hookScriptPath in settings.json and dead ` |
| packages/adapters/claude/src/index.ts | ClaudeAdapter implementation — enterAutoMode uses fire-and-forget with .catch(), which swallows auth errors; the previous concern about stray keystrokes post-SessionStart remains open. |
| packages/adapters/claude/src/classify.ts | classifyStop now correctly anchors all three sentinels to opts.worktree, not payload.cwd; readTail reads the full file before slicing to 8 KB. |
| packages/dispatcher/src/hook-server.ts | Duplicate-arrival guard (keeps first) is in place; the server still binds to 0.0.0.0 without a hostname option, which is the open security concern from the previous review. |
| packages/dispatcher/src/dispatch.ts | cleanups-stack pattern handles db/hookServer teardown correctly; waitForSettle outer deadline added; unhandledRejection swallower for the bunqueue lock-token race is in place. |
| packages/dispatcher/src/workflows/implementation.ts | Three-step workflow is well-structured; sessionNameFor still produces middle-${epicNumber} with no repo component, leaving the concurrent same-issue-number cross-repo collision open. |
| packages/dispatcher/src/worktree.ts | Idempotent create/destroy logic is sound; the git branch -D exit code is still silently discarded, which can leave the branch on disk and cause a confusing WorktreeError on re-dispatch. |
| packages/cli/src/commands/start.ts | proc.unref() is still called before the pid file is written; an I/O failure on the pid write leaves an orphaned, untracked dispatcher process. |
| packages/dispatcher/src/db.ts | WAL-mode open, migration runner with per-migration transactions, and idempotent INSERT OR IGNORE schema_version tracking are all correct. |
| packages/core/src/config.ts | loadConfig deep-merges global defaults + global file + per-repo file; tilde expansion and missing-file tolerance are correctly handled. |
| packages/cli/src/commands/dispatch.ts | dispatchEpic is now wrapped in try/catch with friendly error framing; input validation, adapter registry, and deriveRepoSlug are all correct. |
| packages/dispatcher/src/main.ts | Dispatcher entrypoint with graceful SIGTERM/SIGINT shutdown; announces readiness on stdout for the main.test.ts lifecycle check. |
Sequence Diagram
sequenceDiagram
participant CLI as mm dispatch
participant DE as dispatchEpic
participant ENG as bunqueue Engine
participant WF as implementation workflow
participant HS as HookServer
participant TMUX as tmux
participant CL as Claude (agent)
CLI->>DE: dispatchEpic(repoPath, epicNumber, ...)
DE->>HS: hookServer.start(port)
DE->>ENG: engine.register(implementationWorkflow)
DE->>ENG: engine.start("implementation", input)
ENG->>WF: step: prepare-worktree
WF->>WF: createWorkflowRecord(db, id)
WF->>TMUX: createWorktree(repoPath, repo, issueNumber)
WF-->>ENG: "{handle}"
ENG->>WF: step: launch-and-drive
WF->>WF: installHooks(worktree, hookScriptPath, ...)
WF->>TMUX: newSession(sessionName, argv, env)
TMUX-->>CL: claude --dangerously-skip-permissions
WF->>WF: enterAutoMode (fire-and-forget .catch)
WF->>HS: awaitSessionStart(sessionName, 90s)
CL-->>HS: POST /hooks/session.started
HS-->>WF: "{session_id, transcript_path}"
WF->>TMUX: sendText + sendEnter (prompt)
WF->>HS: awaitStop(sessionName, 4h)
CL-->>HS: POST /hooks/agent.stopped
HS-->>WF: stopPayload
WF->>WF: classifyStop(payload, transcriptPath, worktree)
WF-->>ENG: "{classification, sessionName}"
ENG->>WF: step: cleanup
WF->>TMUX: killSession(sessionName)
WF->>TMUX: destroyWorktree(handle)
WF->>WF: updateWorkflow(db, id, finalState)
DE->>ENG: waitForSettle(executionId, 5h deadline)
ENG-->>DE: execution (completed/failed/compensated)
DE->>ENG: engine.close(false) [10s drain]
DE-->>CLI: "{workflowId, state}"
Reviews (15): Last reviewed commit: "feat(core): shared TUI primitives; claud..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
| Filename | Overview |
|---|---|
| packages/adapters/claude/src/hooks.ts | Phase 1 stub that only installs SessionStart in .claude/settings.json; the Stop hook needed by awaitStop in the workflow is absent, making real end-to-end dispatch unreachable without a timeout. |
| packages/adapters/claude/src/classify.ts | classifyStop uses payload.cwd for done.json/failed.json but blocked.json is anchored to handle.path in the caller; mismatched path roots cause missed done/failed detections when the agent changes directory. |
| packages/dispatcher/src/dispatch.ts | Self-contained dispatchEpic runner; db opened before the try/finally block so a hookServer.start() failure leaks the SQLite connection. |
| packages/dispatcher/src/workflows/implementation.ts | Clean 3-step factory workflow with proper compensation; blocked.json is correctly anchored to handle.path but classifyStop checks done/failed at a different root. |
| packages/adapters/claude/src/index.ts | Assembles the ClaudeAdapter; enterAutoMode silently swallows the tmux send-keys exit code, making auto-mode failures invisible to the workflow. |
| packages/dispatcher/src/db.ts | WAL-mode SQLite wrapper with numbered migration runner; clean implementation with idempotent re-run support. |
| packages/dispatcher/src/hook-server.ts | Minimal Bun HTTP hook receiver with early-arrival stash and per-session keying; well-structured with proper waiter cleanup on stop. |
| packages/dispatcher/src/worktree.ts | Git worktree create/list/destroy helpers with idempotent semantics and real-path normalization; porcelain parser correctly handles detached-HEAD worktrees. |
| packages/dispatcher/src/tmux.ts | Typed tmux session helpers with TmuxError surfacing and idempotent killSession. |
| packages/core/src/config.ts | TOML config loader with deep-merge, tilde expansion, and documented defaults; well-typed with safe fallback for missing files. |
Comments Outside Diff (1)
-
packages/dispatcher/src/dispatch.ts, line 214-220 (link)Database connection leaked when
hookServer.start()throwsdbis opened before thetry/finallyblock. IfhookServer.start()throws (most likely: port already in use because the dispatcher process started withmm startholds port 8822), execution jumps past thefinallyanddb.close()is never called. MovehookServer.start()beforeopenAndMigrate, or wrap the database open inside the try block so the finally always covers it.
Reviews (2): Last reviewed commit: "feat(cli): mm start/stop/status + dispat..." | Re-trigger Greptile
…eanup, exit codes
|
All Greptile findings addressed in 57dfe1b. 104 pass, The summary called out four issues; the inline thread also flagged two related edge cases I fixed at the same time. Per-thread replies have the specifics; the gist:
Regression coverage: subdir-cwd sentinel test, |
|
Update — substantial iteration since the original ready-for-review brief (2026-05-14) 13 commits between Greptile review — all 6 findings fixed (
|
|
@coderabbitai review this PR against the Phase 1 (Epic #6) requirements as well as code quality. |
🧩 Analysis chainI'll gather the code from all the key files before writing my review. 🏁 Script executed: Length of output: 79610 🏁 Script executed: Length of output: 32311 Let me grab the CLI command files and a few remaining pieces. 🏁 Script executed: Length of output: 47445
Phase 1 Requirements — All 7 Sub-IssuesAll seven sub-issues (
The post-review additions ( Issues Found🔴 P1 —
|
|
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 (1)
📝 Walkthrough<review_stack_artifact_start /> Comprehensive implementation of the dispatcher workflow engine, Claude adapter, and supporting infrastructure for executing and observing long-running AI agent tasks in isolated git worktrees.Public contracts, event types, TOML-backed config loader, and public re-exports forming the core API surface.range_546e568b2d25 range_bee2114bfa9c range_77118ea5637e range_53137103bbd5 range_dffe472c874d range_fa1f334a1dc4 range_3344654f067c range_265ddf2faf6c range_0ce2b75a2b9b range_2c45b9b82fdc range_47a9b77ecaf3 range_d2831e317a01 range_b98c48543303 range_368655898804 range_546e568b2d25Composable tmux helpers: runTmux wrapper, capturePane, sendText/sendKeys, and pollPaneFor with diagnostics and tests.range_64b6e854450c range_eb3e3d1aa347 range_9561f1705dfe range_0c79b9a8c30d range_8ff1fd3091c2 range_e70ca79601a9 range_7b8ccbafe47c range_3a60928fea4a range_aeb76d89b163 range_f35323e202fe range_499d33f96179 range_64acf004cfc8 range_7cee1e4a0bca range_f99b197443c0 range_76419614e692Claude adapter implementation: prompt builder, transcript parsing, classifyStop, installHooks, bypass/login detection, enterAutoMode, and adapter wiring with tests.range_09593db63ea1 range_31b100915c54 range_57687c77b226 range_406f9fbca551 range_359aaadf276e range_d2fff66facd4 range_8fc42898ffdf range_e4c80cdfc628 range_3115812e0a51 range_2fe62452f04b range_14fd7128165d range_694adc937f87 range_db435952a78f range_11382de9f62d range_e66b2badaad4 range_20bf5bb92bce range_ddeaea2ea19c range_8a92362853c8 range_2e555915b77a range_00890c99898f range_fcbc2f6b0e60 range_de126cecb027Runnable CLI (mm) with start/stop/status/doctor/dispatch commands, path helpers, and command tests for validation and lifecycle handling.range_7356d5f430ff range_f43dce891c1b range_6e6f45eb96f6 range_bf5093b1163c range_f143248d6b92 range_9600d446f5e9 range_aff6681daaf2 range_6016c04c0309 range_dcface5771d3 range_ceadf83714b3 range_56e7f6c42ff3 range_2c0ef5ceca33 range_20e618ace925 range_d108df491e1f range_270e0c9b0a0a range_85f4347022db range_a3380b8329f5 range_03e0a6d9b239 range_736c7f55e27f range_d905fa289382 range_31cb04b42086 range_541113f48fae range_2f7e16a8626f range_ab89214563f9DB open/migrate helpers, migration loader/runner, and initial schema SQL with tests for migrations and constraints.range_409b04c796ec range_41e19bae4ffa range_00c9e1382c9e range_84563c484e1c range_51b20b98c7fb range_ac9219f1b32f range_fb6f6c0ab4e4 range_1bf5390b2bce range_5d51ab7bbda8 range_16123e4cf184 range_664b046be0bf range_fe35b9aa0295 range_288f9c93d592 range_d32f27d8f166Typed tmux wrapper (TmuxError, newSession, sendText/capturePane, version parsing) and git worktree management (create/destroy/list) with safety checks and tests.range_d5daa5753792 range_2cf4cb47c475 range_72625d09a98a range_bc8dc20a7e98 range_adde82f06326 range_ad3108836c1a range_d94ac11b6051 range_5d2c6e044672 range_7e1162b44ed7 range_12cb43acd1e6 range_691b8881c09c range_018260d363dd range_534c155272bb range_993592e3fcf2 range_7c6d46798096HookServer bound to localhost with stash/wait delivery semantics and typed DB accessors for workflow records (create/update/get) with tests.range_a07965c5e66b range_8a6201c7b5b5 range_7da888c3b7b0 range_9439cf73c4b2 range_b567d20c1d60 range_78c1a013adae range_38499cd2987f range_18163c2f3d21 range_49a3c58a6dba range_968d6445143c range_0227abef6628 range_fbede278cffc range_422805351f31 range_8d6ab212c298 range_5a7eb9cb1c8c range_24a2c5f34db5 range_a6247f8b6132 range_c97cd447ee17 range_64acf004cfc8 range_7cee1e4a0bca range_f99b197443c0 range_76419614e692Bunqueue workflow factory orchestrating prepare-worktree, launch-and-drive (tmux/hooks/adapter), and cleanup with compensation, state mapping, and integration tests.range_194707087ae0 range_281850c5d58b range_fddf4168b5ed range_e6bb698429cd range_80496b3845ad range_11d985ab3ec5 range_8720b8ba62ec range_3b484ccd5966 range_664b046be0bf range_fb6f6c0ab4e4dispatchEpic composes DB/hook-server/engine, registers workflow, polls to settle, and performs ordered teardown; main() bootstraps dispatcher and handles graceful shutdown on signals.range_257b04c9d42e range_eed900124407 range_9d798827f206 range_c7df46b490c4 range_281850c5d58b range_2e97b0fe72da range_d89b038ae709 range_6abb012dbe99 range_c611aa27702a range_f4e7a07121f3 range_fb6884cfc42b range_281850c5d58bAll test suites, planning docs, decisions log, and dev.sh used to run the dispatcher locally; included as reviewable support material.range_e3b8644083c1 range_e70ca79601a9 range_7b8ccbafe47c range_e3b8644083c1 range_4ff2ca7e96ca range_175a80465f13 range_726c2474be4e range_04f7e37e4361 range_6dcf43058498 range_04f7e37e4361 range_2f11b2260214 range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_2f11b2260214 range_c403b1d32c7d range_175a80465f13 range_726c2474be4e range_04f7e37e4361 range_6dcf43058498 range_04f7e37e4361 range_2f11b2260214 range_c403b1d32c7d range_4ff2ca7e96ca range_c403b1d32c7d range_726c2474be4e range_04f7e37e4361 range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_c403b1d32c7d range_5cb1d60b8f00 range_265ddf2faf6c range_0ce2b75a2b9b range_2c45b9b82fdc range_47a9b77ecaf3 range_d2831e317a01 range_b98c48543303 range_368655898804 range_64acf004cfc8 range_7cee1e4a0bca range_f99b197443c0 range_76419614e692 range_9bd81b720946 range_e66b2badaad4 range_20bf5bb92bce range_18a83b51a9d1 range_943f884ab8df range_061b00fce83b range_d4c8de93a89c range_3627faa452cc range_824dd7e043f7 range_a7caa9c43824 range_221912e87413 range_0f5ca7917828 range_aa7a7c071760 range_a66c41591527 range_e41bdd4dff5e range_536aa0fa2412 range_050d4ccb03e3 range_888c06746ae1 range_484ef81e1335 range_d86d8046fdad range_78d8bf694a12 range_f1ca584eca19 range_84929a48605b range_746232f43f25 range_20b74a97b148 range_b68d8e059148 range_085f517c307d range_d5a2cd2b3f77 |
…r, localhost bind, branch-delete check, repo-scoped session names
|
Second Greptile review round — all 6 findings fixed in
Per-thread replies have the specifics. Full reasoning in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (7)
packages/dispatcher/src/db.ts (1)
37-44: ⚡ Quick winSort migrations by parsed version, not filename text.
Current ordering relies on lexicographic filename sort. A non-zero-padded file (for example
2_...and10_...) will execute out of order even though the numeric prefix is parsed.Proposed fix
export function loadMigrations(dir: string = MIGRATIONS_DIR): Migration[] { - return readdirSync(dir) + const migrations = readdirSync(dir) .filter((f) => f.endsWith(".sql")) - .sort() .map((name) => { const match = /^(\d+)_/.exec(name); if (!match) throw new Error(`migration filename missing numeric prefix: ${name}`); return { version: Number(match[1]), name, sql: readFileSync(join(dir, name), "utf8") }; }); + migrations.sort((a, b) => a.version - b.version || a.name.localeCompare(b.name)); + return migrations; }🤖 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/db.ts` around lines 37 - 44, The migration list is currently sorted lexicographically by filename which breaks numeric ordering (e.g., "10_" after "2_"); change the pipeline to first map filenames to objects with parsed numeric version (using the existing regex/match logic that produces version) and then sort the resulting array by the numeric version property before returning so executions run in numeric order; update the chain around readdirSync/filter/map/sort (or replace the current sort() call) so sorting uses the parsed Number(match[1]) rather than the raw filename string.packages/dispatcher/test/workflow-record.test.ts (1)
81-91: ⚡ Quick winAssert the no-op patch contract for
updatedAtas well.Line 81’s test name says “no-op patch,” but it only checks state. Since
updateWorkflowguaranteesupdated_atalways changes, this path should assert the timestamp bump to prevent regressions.Suggested test addition
test("a no-op patch leaves the row intact", () => { createWorkflowRecord(db, { id: "exec-1", kind: "implementation", repo: "o/r", epicNumber: 6, adapter: "claude", }); + const before = getWorkflow(db, "exec-1")!.updatedAt; updateWorkflow(db, "exec-1", {}); - expect(getWorkflow(db, "exec-1")!.state).toBe("pending"); + const after = getWorkflow(db, "exec-1")!; + expect(after.state).toBe("pending"); + expect(after.updatedAt).toBeGreaterThanOrEqual(before); });🤖 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/test/workflow-record.test.ts` around lines 81 - 91, The test currently only asserts state after calling updateWorkflow with an empty patch; also capture the initial timestamp from getWorkflow (e.g., const before = getWorkflow(db, "exec-1")!.updated_at or updatedAt), call updateWorkflow(db, "exec-1", {}), then read the record again and assert the new updated_at is greater than the previous value to enforce the "updatedAt always changes" contract while keeping the existing state assertion; use the same helpers (createWorkflowRecord, updateWorkflow, getWorkflow) and preserve the test name.packages/cli/src/commands/dispatch.ts (1)
6-6: ⚡ Quick winUse dispatcher public exports instead of
/src/*deep import.Importing
dispatchEpicfrom an internal file tightly couples CLI to dispatcher internals. Prefer a public barrel export from@middle/dispatcherto keep package boundaries stable.🤖 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/cli/src/commands/dispatch.ts` at line 6, The import in dispatch.ts uses a deep path into `@middle/dispatcher` internals; replace it with the package's public export and update the dispatcher package if needed. Export dispatchEpic from the public barrel (e.g., add/export dispatchEpic in `@middle/dispatcher` index barrel) and change the import in packages/cli/src/commands/dispatch.ts to import { dispatchEpic } from "`@middle/dispatcher`"; ensure the exported symbol name matches dispatchEpic in the dispatcher package.packages/cli/src/commands/doctor.ts (1)
1-5: ⚡ Quick winAvoid deep import from dispatcher internals in doctor command.
@middle/dispatcher/src/tmux.tsis an internal path; re-export these helpers from the dispatcher package entrypoint (or move shared capability to@middle/core) and import from that stable surface.🤖 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/cli/src/commands/doctor.ts` around lines 1 - 5, The doctor command currently deep-imports dispatcher internals; update imports of getTmuxVersion, MIN_TMUX_VERSION, and tmuxVersionAtLeast to come from a stable package entrypoint (or from `@middle/core` if you move the helpers there). Modify the dispatcher package to re-export these symbols from its public index (or relocate the tmux helpers to `@middle/core`) and then change the import in doctor.ts to import { getTmuxVersion, MIN_TMUX_VERSION, tmuxVersionAtLeast } from the dispatcher/core package entrypoint instead of "`@middle/dispatcher/src/tmux.ts`".packages/cli/src/commands/status.ts (1)
3-3: ⚡ Quick winAvoid deep-importing dispatcher internals from the CLI.
Importing
@middle/dispatcher/src/db.tsbypasses the package boundary and can break on internal refactors. Re-exportopenDbfrom the dispatcher public entrypoint and consume that instead.🤖 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/cli/src/commands/status.ts` at line 3, The CLI is deep-importing openDb from dispatcher internals; instead re-export openDb from the dispatcher package public entrypoint and import that public symbol from the package. In the dispatcher package add a public export (e.g. export { openDb } from './src/db') to its entrypoint module, then change the CLI import in status.ts to import { openDb } from '`@middle/dispatcher`' so the CLI consumes the public API rather than the internal path.packages/cli/test/start-stop.test.ts (1)
64-70: ⚡ Quick winReplace fixed sleeps with bounded polling to reduce flakiness.
Line [64], Line [69], and Line [80] rely on timing assumptions that can fail on slower CI runners. A small
waitUntil(() => isAlive(pid), timeout)/ inverse check will make lifecycle assertions deterministic.Also applies to: 80-80
🤖 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/cli/test/start-stop.test.ts` around lines 64 - 70, Replace the fixed Bun.sleep waits around the lifecycle assertions with bounded polling so tests don't flake on slow CI: instead of awaiting Bun.sleep(150) then asserting isAlive(pid) or !isAlive(pid), call a waitUntil-style helper that repeatedly checks isAlive(pid) (or its inverse) until a timeout and then assert the expected boolean; update the assertions around runStop({ pidFile }), existsSync(pidFile) and the before/after liveness checks to use this polling helper (referencing isAlive, runStop, existsSync, and pidFile) so the test waits deterministically for the process state change.packages/cli/test/doctor.test.ts (1)
4-8: ⚡ Quick winMake this test hermetic instead of host-toolchain dependent.
Line [4]–Line [7] explicitly tie success to locally installed binaries, which makes this unit test fragile across contributors/CI images. Prefer stubbing the command-exec/check layer and asserting formatting + exit-code logic here; keep real-binary verification as an opt-in smoke test.
Also applies to: 10-27
🤖 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/cli/test/doctor.test.ts` around lines 4 - 8, The test in doctor.test.ts currently shells out to real binaries via runDoctor and thus depends on the host toolchain; change it to be hermetic by stubbing the command-exec/check layer (the function(s) that run commands or check versions used by runDoctor) rather than invoking real binaries. Locate the command wrapper used by runDoctor (e.g., the exec/which/spawn helper or a wrapper like runCommand/checkBinary used in the doctor code), replace calls in the test with a mock (jest.spyOn or DI-injected stub) that returns controlled success/failure outputs, then assert the formatting of output and exit-code behavior from runDoctor; keep any real-binary verification as a separate opt-in smoke test.
🤖 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/src/hooks.ts`:
- Around line 53-55: The generated hook command strings are interpolating the
unquoted scriptPath into the hooks in hooks.ts (the objects with keys Stop and
the earlier hook array), which breaks when opts.worktree contains spaces or
shell metacharacters; change the command generation to wrap scriptPath in quotes
(e.g., produce "scriptPath session.started" and "scriptPath agent.stopped") so
the entire path is treated as one token, and update the adapter tests in
packages/adapters/claude/test/adapter.test.ts to expect the quoted substrings
(including the surrounding quotes) instead of the current unquoted ones.
In `@packages/cli/src/commands/start.ts`:
- Around line 36-38: The PID read/used needs to be validated as a positive
integer before invoking process.kill; update the checks so the value parsed into
existing (in start flow) is tested with Number.isInteger(existing) && existing >
0 before calling isAlive(existing) (or adjust isAlive to early-return false for
non-positive pids), and in the stop flow ensure the parsed pid is > 0 before
calling process.kill(pid, "SIGTERM"); this prevents pid === 0 or negative values
from triggering process group signals.
In `@packages/cli/src/commands/status.ts`:
- Around line 44-47: The catch block that currently logs “database has no
workflows table yet…” must only handle the specific "no such table" error for
the workflows table; update the catch to examine the caught error (e) and if
e.message (or e.code) indicates "no such table" for "workflows" then keep the
original console.log and return 0, otherwise write the full error to
console.error (or process.stderr) and return 1 so real SQLite failures
(corruption, permission, lock, etc.) surface; locate and modify the catch in
packages/cli/src/commands/status.ts (the catch handling the workflows query) to
implement this conditional error handling.
In `@packages/cli/src/commands/stop.ts`:
- Around line 25-31: The PID validation currently only checks
Number.isInteger(pid) and may allow 0 or negative PIDs; update the check in the
stop command to require pid to be a positive integer (e.g., if
(!Number.isInteger(pid) || pid <= 0) { console.error(...); return 1; }) before
calling process.kill(pid, "SIGTERM") so you never call process.kill with 0 or
negative values; update the error message if desired and keep the same
early-return behavior.
- Around line 30-35: The current runStop flow clears the pid file early and
treats any process.kill error as “not running”; instead, in runStop ensure you
only treat error.code === "ESRCH" as the process-not-running case (clear the pid
file and return 0), but for EPERM/other errors do NOT clear the pid file and
return a non-zero exit (or rethrow) so callers know signaling failed; adjust the
try/catch around process.kill(pid, "SIGTERM") to catch the error object, inspect
error.code, and only remove or log-and-return-success on ESRCH, otherwise
propagate or return failure, and only remove the pid file after a confirmed
successful kill or ESRCH.
In `@packages/core/src/config.ts`:
- Around line 141-143: The expandTilde function currently expands any string
starting with "~", incorrectly mapping "~username/..." to the current user's
home; change the condition to only handle the bare "~" and the "~/..." form
(i.e. value === "~" or value.startsWith("~/")). When value === "~" return
homedir(), when it startsWith("~/") return join(homedir(), value.slice(2)),
otherwise return the original value; keep using the existing expandTilde
function name and the homedir()/join utilities.
In `@packages/dispatcher/src/db.ts`:
- Around line 66-69: openAndMigrate currently opens the Database via openDb and
then calls runMigrations but does not close the Database if runMigrations
throws, leaving the sqlite handle open; update openAndMigrate to wrap
runMigrations(db, dir) in a try/catch, call db.close() (or the appropriate close
method on the Database returned by openDb) inside the catch before rethrowing
the error, ensuring the Database handle is always closed on migration failure
(and consider a finally block to guarantee closure only on error-free or error
paths as appropriate).
In `@packages/dispatcher/src/dispatch.ts`:
- Around line 118-124: dispatchEpic can throw after constructing const engine =
new Engine({ embedded: true }) leaving the Engine running because
engine.close(...) is only invoked in the finally via runCleanups; ensure engine
is always closed on error by adding a try/catch/finally around the
post-construction work in dispatchEpic so that engine.close(...) is called in
the finally (or register engine.close as an early cleanup before actions like
engine.register/engine.start/waitForSettle), referencing the Engine instance and
its close method (engine.close) and the existing runCleanups/cleanup mechanism
so Engine shutdown occurs even if engine.register, engine.start, or
waitForSettle throws.
In `@packages/dispatcher/src/hook-server.ts`:
- Around line 64-80: The handler currently accepts any event name and allows
empty session identities; update the HTTP boundary to validate the parsed event
and session before delivering: after extracting event from match (the variable
event) and computing sessionName (the variable sessionName from req.headers or
payload.sessionName), check that event is one of the supported lifecycle events
(use a whitelist/enum of allowed event names) and that sessionName is a
non-empty string; if either check fails, log the rejection and return an
appropriate 4xx Response (do not call this.#deliver or stash payloads),
otherwise proceed to call this.#deliver(`${event}:${sessionName}`, payload) as
before. Ensure the validation is applied prior to any side effects and uses the
same HookPayload parsing logic currently in place.
In `@packages/dispatcher/src/main.ts`:
- Around line 32-40: The shutdown function (shutdown) should guard teardown
calls so failures don't prevent process.exit; wrap hookServer.stop(), await
engine.close(true) and db.close() in a try/catch (or two: one for each resource
group) and log any thrown errors (use console.error or the module logger) and
then call process.exit(1) on failure; on success call process.exit(0). Ensure
shuttingDown is still set at the top of shutdown and that any thrown/rejected
promises from engine.close or db.close are awaited inside the try block so they
cannot become unhandled.
In `@packages/dispatcher/src/worktree.ts`:
- Around line 114-120: The code builds the worktree path using unvalidated
opts.repo causing possible path traversal; in createWorktree (and the
corresponding cleanup/remove function that deletes the worktree)
validate/sanitize opts.repo before using it: ensure it is a single safe segment
(e.g., reject if contains ../ or path.sep, or use path.basename) or normalize
and then resolve the candidate path and assert the resolved path startsWith the
resolved worktreeRoot; if validation fails, throw an error. Update references
where path = join(root, opts.repo, unit) and where the deletion uses that path
to rely only on a validated/sanitized repo value.
In `@packages/dispatcher/test/main.test.ts`:
- Around line 42-57: The readiness loop can block on reader.read() so the
Date.now() check doesn't enforce a timeout and the spawned proc may leak on
assertion failures; change the loop to race each await reader.read() against a
timeout Promise (e.g., Promise.race([reader.read(), timeoutPromise])) so the
loop actually stops when the deadline passes and mark/throw on timeout, and wrap
the entire wait + assertion in try/finally that always calls
proc.kill("SIGTERM") and awaits proc.exited and calls reader.releaseLock();
update references to reader.read(), deadline, proc, reader.releaseLock(), and
the expect(...) assertion accordingly.
In `@packages/dispatcher/test/worktree.test.ts`:
- Around line 29-31: The tests call git(repoPath, ["commit", "--allow-empty",
"-m", "init"]) relying on global git config; make the test setup deterministic
by configuring a local git identity before that commit: call git(repoPath,
["config", "user.name", "Test User"]) and git(repoPath, ["config", "user.email",
"test@example.com"]) (or similar) immediately after initializing the repo and
before the git(...) commit call (in the same test setup function used in
worktree.test.ts and the other mentioned tests) so commits never fail due to
missing user config.
---
Nitpick comments:
In `@packages/cli/src/commands/dispatch.ts`:
- Line 6: The import in dispatch.ts uses a deep path into `@middle/dispatcher`
internals; replace it with the package's public export and update the dispatcher
package if needed. Export dispatchEpic from the public barrel (e.g., add/export
dispatchEpic in `@middle/dispatcher` index barrel) and change the import in
packages/cli/src/commands/dispatch.ts to import { dispatchEpic } from
"`@middle/dispatcher`"; ensure the exported symbol name matches dispatchEpic in
the dispatcher package.
In `@packages/cli/src/commands/doctor.ts`:
- Around line 1-5: The doctor command currently deep-imports dispatcher
internals; update imports of getTmuxVersion, MIN_TMUX_VERSION, and
tmuxVersionAtLeast to come from a stable package entrypoint (or from
`@middle/core` if you move the helpers there). Modify the dispatcher package to
re-export these symbols from its public index (or relocate the tmux helpers to
`@middle/core`) and then change the import in doctor.ts to import {
getTmuxVersion, MIN_TMUX_VERSION, tmuxVersionAtLeast } from the dispatcher/core
package entrypoint instead of "`@middle/dispatcher/src/tmux.ts`".
In `@packages/cli/src/commands/status.ts`:
- Line 3: The CLI is deep-importing openDb from dispatcher internals; instead
re-export openDb from the dispatcher package public entrypoint and import that
public symbol from the package. In the dispatcher package add a public export
(e.g. export { openDb } from './src/db') to its entrypoint module, then change
the CLI import in status.ts to import { openDb } from '`@middle/dispatcher`' so
the CLI consumes the public API rather than the internal path.
In `@packages/cli/test/doctor.test.ts`:
- Around line 4-8: The test in doctor.test.ts currently shells out to real
binaries via runDoctor and thus depends on the host toolchain; change it to be
hermetic by stubbing the command-exec/check layer (the function(s) that run
commands or check versions used by runDoctor) rather than invoking real
binaries. Locate the command wrapper used by runDoctor (e.g., the
exec/which/spawn helper or a wrapper like runCommand/checkBinary used in the
doctor code), replace calls in the test with a mock (jest.spyOn or DI-injected
stub) that returns controlled success/failure outputs, then assert the
formatting of output and exit-code behavior from runDoctor; keep any real-binary
verification as a separate opt-in smoke test.
In `@packages/cli/test/start-stop.test.ts`:
- Around line 64-70: Replace the fixed Bun.sleep waits around the lifecycle
assertions with bounded polling so tests don't flake on slow CI: instead of
awaiting Bun.sleep(150) then asserting isAlive(pid) or !isAlive(pid), call a
waitUntil-style helper that repeatedly checks isAlive(pid) (or its inverse)
until a timeout and then assert the expected boolean; update the assertions
around runStop({ pidFile }), existsSync(pidFile) and the before/after liveness
checks to use this polling helper (referencing isAlive, runStop, existsSync, and
pidFile) so the test waits deterministically for the process state change.
In `@packages/dispatcher/src/db.ts`:
- Around line 37-44: The migration list is currently sorted lexicographically by
filename which breaks numeric ordering (e.g., "10_" after "2_"); change the
pipeline to first map filenames to objects with parsed numeric version (using
the existing regex/match logic that produces version) and then sort the
resulting array by the numeric version property before returning so executions
run in numeric order; update the chain around readdirSync/filter/map/sort (or
replace the current sort() call) so sorting uses the parsed Number(match[1])
rather than the raw filename string.
In `@packages/dispatcher/test/workflow-record.test.ts`:
- Around line 81-91: The test currently only asserts state after calling
updateWorkflow with an empty patch; also capture the initial timestamp from
getWorkflow (e.g., const before = getWorkflow(db, "exec-1")!.updated_at or
updatedAt), call updateWorkflow(db, "exec-1", {}), then read the record again
and assert the new updated_at is greater than the previous value to enforce the
"updatedAt always changes" contract while keeping the existing state assertion;
use the same helpers (createWorkflowRecord, updateWorkflow, getWorkflow) and
preserve the test name.
🪄 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: 5bdc8695-5084-42ee-ad8c-e629a9ddb3d6
⛔ Files ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (44)
packages/adapters/claude/src/classify.tspackages/adapters/claude/src/hooks.tspackages/adapters/claude/src/index.tspackages/adapters/claude/src/prompt.tspackages/adapters/claude/src/transcript.tspackages/adapters/claude/test/adapter.test.tspackages/cli/package.jsonpackages/cli/src/commands/dispatch.tspackages/cli/src/commands/doctor.tspackages/cli/src/commands/start.tspackages/cli/src/commands/status.tspackages/cli/src/commands/stop.tspackages/cli/src/index.tspackages/cli/src/paths.tspackages/cli/test/dispatch.test.tspackages/cli/test/doctor.test.tspackages/cli/test/start-stop.test.tspackages/cli/test/status.test.tspackages/core/src/adapter.tspackages/core/src/config.tspackages/core/src/events.tspackages/core/src/index.tspackages/core/src/tmux-tui.tspackages/core/test/config.test.tspackages/core/test/tmux-tui.test.tspackages/dispatcher/src/db.tspackages/dispatcher/src/db/migrations/001_initial.sqlpackages/dispatcher/src/dispatch.tspackages/dispatcher/src/hook-server.tspackages/dispatcher/src/main.tspackages/dispatcher/src/tmux.tspackages/dispatcher/src/workflow-record.tspackages/dispatcher/src/workflows/implementation.tspackages/dispatcher/src/worktree.tspackages/dispatcher/test/db.test.tspackages/dispatcher/test/hook-server.test.tspackages/dispatcher/test/implementation-workflow.test.tspackages/dispatcher/test/main.test.tspackages/dispatcher/test/tmux.test.tspackages/dispatcher/test/workflow-record.test.tspackages/dispatcher/test/worktree.test.tsplanning/issues/6/decisions.mdplanning/issues/6/plan.mdscripts/dev.sh
…h traversal, hook quoting, shutdown safety, test hermeticity
|
CodeRabbit review — all 13 inline findings fixed in
Per-thread replies have the specifics; full reasoning in |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/adapters/claude/src/hooks.ts (1)
50-57:⚠️ Potential issue | 🟠 Major | ⚡ Quick winEscape embedded quotes in Claude hook command generation
Claude runs
type: "command"in shell form (noargs), so wrappingscriptPathwith just"fails ifscriptPathcontains embedded"and the hook command won’t run correctly.Suggested patch
export async function installHooks(opts: InstallHookOpts): Promise<void> { const scriptPath = join(opts.worktree, opts.hookScriptPath); + const quotedScriptPath = shellQuote(scriptPath); @@ const settings = { hooks: { SessionStart: [ - { hooks: [{ type: "command", command: `"${scriptPath}" session.started` }] }, + { hooks: [{ type: "command", command: `${quotedScriptPath} session.started` }] }, ], - Stop: [{ hooks: [{ type: "command", command: `"${scriptPath}" agent.stopped` }] }], + Stop: [{ hooks: [{ type: "command", command: `${quotedScriptPath} agent.stopped` }] }], }, }; @@ } + +function shellQuote(value: string): string { + return `'${value.replace(/'/g, `'\\''`)}'`; +}🤖 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/adapters/claude/src/hooks.ts` around lines 50 - 57, The generated hook command wraps scriptPath in double-quotes but doesn’t escape any embedded quotes, so commands like `"${scriptPath}" session.started` will break if scriptPath contains " characters; update the hook generation in hooks.ts to escape embedded double-quotes in scriptPath (e.g., compute an escapedScriptPath = scriptPath.replace(/"/g, '\\"') or equivalent) and use that escaped value when constructing the command for the SessionStart and Stop entries so the command string becomes `"<escapedScriptPath>" session.started` / `"<escapedScriptPath>" agent.stopped`.
🤖 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/cli/test/dispatch.test.ts`:
- Around line 71-77: The test currently ignores the exit codes of the git setup
commands (init and commit), so assert they succeeded before proceeding: after
awaiting init.exited check init.exitCode (or the spawn result status) and throw
or fail the test if it is non-zero, and do the same for the commit spawn that
uses gitEnv; this ensures the repository setup succeeded and prevents setup
failures from being mistaken for the EADDRINUSE failure-path under test.
---
Duplicate comments:
In `@packages/adapters/claude/src/hooks.ts`:
- Around line 50-57: The generated hook command wraps scriptPath in
double-quotes but doesn’t escape any embedded quotes, so commands like
`"${scriptPath}" session.started` will break if scriptPath contains "
characters; update the hook generation in hooks.ts to escape embedded
double-quotes in scriptPath (e.g., compute an escapedScriptPath =
scriptPath.replace(/"/g, '\\"') or equivalent) and use that escaped value when
constructing the command for the SessionStart and Stop entries so the command
string becomes `"<escapedScriptPath>" session.started` / `"<escapedScriptPath>"
agent.stopped`.
🪄 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: 434d154a-bb12-4e9d-beb9-34cdb2e46b68
📒 Files selected for processing (16)
packages/adapters/claude/src/hooks.tspackages/adapters/claude/test/adapter.test.tspackages/cli/src/commands/start.tspackages/cli/src/commands/status.tspackages/cli/src/commands/stop.tspackages/cli/test/dispatch.test.tspackages/core/src/config.tspackages/dispatcher/src/db.tspackages/dispatcher/src/dispatch.tspackages/dispatcher/src/hook-server.tspackages/dispatcher/src/main.tspackages/dispatcher/src/worktree.tspackages/dispatcher/test/implementation-workflow.test.tspackages/dispatcher/test/main.test.tspackages/dispatcher/test/worktree.test.tsplanning/issues/6/decisions.md
✅ Files skipped from review due to trivial changes (1)
- planning/issues/6/decisions.md
1 participant
Summary
Closes #6
Build-spec Phase 1 — the minimal dispatcher. middle can now persist operational state, load layered config, abstract a CLI agent behind one adapter interface (Claude), supervise tmux sessions, isolate work in git worktrees, run a 3-step
implementationworkflow (prepare-worktree → launch-and-drive → cleanup), drive it from ammCLI, and dismiss Claude's bypass-mode confirmation autonomously on launch.All 7 sub-issues (#7–#13) are delivered, each with
bun testcoverage. Full suite: 123 pass,tsc --noEmitclean.What changed (current state)
Phase 1 core (the original 7 sub-issues):
packages/dispatcher/src/db.ts+db/migrations/001_initial.sql— WAL-mode SQLite wrapper, numbered-migration runner, full Phase 1 schema.packages/core/src/config.ts—loadConfigparses + deep-merges global + per-repo TOML.packages/core/src/{adapter,events}.ts—AgentAdapterinterface, normalized event vocabulary.packages/adapters/claude/—ClaudeAdapterwith bypass-prompt-awareenterAutoMode, login detection, classify with worktree-anchored sentinels, fullinstallHooks(SessionStart + Stop +.middle/hooks/hook.sh).packages/dispatcher/src/tmux.ts— session helpers, typedTmuxError, version-aware preflight.packages/dispatcher/src/worktree.ts— git worktree create/destroy/list, idempotent.packages/dispatcher/src/{hook-server,workflow-record}.ts+workflows/implementation.ts— minimal hook receiver (SessionGate),workflowsrow CRUD, the 3-stepimplementationworkflow factory.packages/dispatcher/src/{main,dispatch}.ts— dispatcher process entrypoint and the self-containeddispatchEpicrunner with bunqueue-lifecycle-aware draining.packages/cli/src/—commanderwiring +mm start/mm stop/mm status/mm dispatch/mm doctor/mm version.scripts/dev.sh— foreground dev launcher.Post-original-review additions:
packages/core/src/tmux-tui.ts— shared composable TUI primitives (capturePane,sendText,sendKeys,pollPaneFor<T>).packages/cli/src/commands/doctor.ts—mm doctorpreflight check.ensurePromptFileinlaunchAndDrive— writes a plan-style.middle/prompt.mdif missing.Acceptance gate (Epic #6) — REVISED
mm dispatchend-to-end with real Claude: tmux launch → SessionStart → auto mode → prompt → Stop → cleanupmm dispatchruns against~/Developer/middle: bypass warning dismissed viaDown+Enter,SessionStartfires, prompt delivered,Stopreceived, classifyStop returns, workflow finalizes, worktree cleaned up. The agent itself has only received empty / placeholder prompts so far — substantive autonomous work is what dispatching Epic #14 (Phase 2) post-merge will exercise.Verification (current)
bun test— 123 pass across 18 files.bun run typecheck— clean.Per-phase breakdown (each phase's sub-issue close comment links the proving tests). The post-original-review work added:
detectNeedsLoginregex unit tests,pollPaneFortests (match / timeout / session-gone /tag-logs-stderr),capturePane/sendText/sendKeyslive tmux tests,mm doctorsmoke test.Why these changes
The central design pressure was the interactive-tmux dispatch model (no headless mode, no exit code): the workflow's
launch-and-drivestep is launch → drive → observe, reacting to aStophook viaclassifyStoprather than a process exit. That shaped three load-bearing decisions, each posted as a per-line review comment: the hook receiver handles two events (SessionStart + Stop); the workflow is a factory over structural deps so the end-to-end test stubs tmux while exercising the real worktree helpers (genuine leak verification); andcompensated(a workflow step threw, rolled back) is kept distinct fromfailed(the agent itself failed but the run completed). Layering is strict —cli → {dispatcher, adapter-claude},adapters/* → core, nodispatcher → adapter-*edge.Live debugging surfaced a second class of decisions, captured in the update addendum: bunqueue's executor sets
exec.state='failed'before awaiting compensation (drove the inlineengine.close(false)drain pattern); Claude'sSessionStartis gated on the bypass-warning being dismissed (drove the parallel-pollerenterAutoMode); Claude's bypass-mode confirmation needsDown+Enterwith a 100ms delay between (drove thesendKeysdelayBetweenMsprimitive).Stumbling points
exec.state='failed'precedes compensation —waitForSettlereturned mid-flight;finallytorn downhookServer/dbwhile compensation still needed them; surfaced asInvalid or expired lock tokenand "step failed: hook server stopped". Fix: drain engine inline afterwaitForSettle, before thefinally. Plusretry: 1(bunqueue'sretry=maxAttempts), plus anunhandledRejectionswallower scoped todispatchEpicfor the lock-token message specifically.SessionStartis gated on the bypass-warning —enterAutoModehad to run in parallel withawaitSessionStart, not after. Chicken and egg.tmux send-keysarrives too fast; menu doesn't advance selection. Split keys with 100ms delay.--dangerously-skip-permissionsruntime-bypasses but still pops a UI prompt —--permission-mode bypassPermissionsis equivalent at runtime but has the same UI gate; we dismiss it viaenterAutoMode's poll-then-act.dataPath+ macOS temp dirs —SQLITE_IOERR_VNODEon retry writes. In-memory engine in both tests and production for Phase 1.classifyStop"reads PR state" — the spec's prose says so, but the fixed interface gives the adapter no PR handle. Resolved with.middle/done.json/.middle/failed.jsonsentinels for Phase 1 +worktree-anchored paths (added to the interface during review); Phase 4's PR-ready hook gate replaces thedonepath.extended-keys-format csi-ulands in 3.5; older tmuxes produce a.tmux.conferror and degraded key passthrough.mm doctorchecks this.Suggested CLAUDE.md updates
dataPathunder temp dirs hitsSQLITE_IOERR_VNODEon macOS.exec.state='failed'BEFORE compensation runs — any caller polling for terminal state must drain the engine inline before tearing down dependencies.cli → {dispatcher, adapter-*},adapters/* → core, nodispatcher → adapter-*edge.extended-keys-format).Architectural forks
None — every decision was resolved by the build spec, the adapter-interface contract, the dependency-layering rule, or empirical debugging.
Follow-up issues
None filed. Everything out of scope is already an explicitly-spec'd later phase (full hook taxonomy + watchdog → Phase 2; skill enforcement → Phase 4;
mm init/fullmm doctor→ Phase 3/11; CodexAdapter → Phase 10; auto-dispatch + concurrent-dispatch coordination + the HTTP trigger into the running process → Phase 8; durable bunqueue persistence + recovery → Phase 2). No genuine discoveries the phase plan didn't anticipate.Out of scope
Full hook taxonomy / HMAC / events-table population / watchdog / reconciler cron (Phase 2);
installHookswriting the full event set (Phase 2 — Phase 1 ships SessionStart + Stop); skill-enforcement gates (Phase 4); CodexAdapter (Phase 10); recommender workflow (Phase 7);mm init/uninitand the dashboard (Phase 3/9); fullmm doctorwith schema validation + retention reporting (Phase 11); retention crons (Phase 11); routingmm dispatchthrough the running dispatcher process via HTTP (Phase 8); concurrent-dispatch deduplication on (repo, epic) (Phase 8).Summary by CodeRabbit
New Features
mmCLI (start/stop/status/dispatch/doctor) and long‑running dispatcher runtimeTests
Documentation