diff --git a/packages/adapters/claude/src/classify.ts b/packages/adapters/claude/src/classify.ts index 96c49dd9..abd0abc6 100644 --- a/packages/adapters/claude/src/classify.ts +++ b/packages/adapters/claude/src/classify.ts @@ -1,6 +1,11 @@ import { existsSync, readFileSync } from "node:fs"; import { join } from "node:path"; -import type { HookPayload, RateLimitDetection, StopClassification } from "@middle/core"; +import type { + BlockedSentinel, + HookPayload, + RateLimitDetection, + StopClassification, +} from "@middle/core"; const USAGE_LIMIT_RE = /You've hit your usage limit\. Resets at (.+?)\./; @@ -27,7 +32,8 @@ export function classifyStop(opts: { const middleDir = join(opts.worktree, ".middle"); if (opts.sentinelPresent) { - return { kind: "asked-question", sentinelPath: join(middleDir, "blocked.json") }; + const sentinelPath = join(middleDir, "blocked.json"); + return { kind: "asked-question", sentinelPath, sentinel: readBlockedSentinel(sentinelPath) }; } const match = USAGE_LIMIT_RE.exec(readTail(opts.transcriptPath)); @@ -68,6 +74,27 @@ function readTail(path: string): string { } } +/** + * Read and tolerantly parse the `.middle/blocked.json` question sentinel so the + * workflow can surface the agent's question (and any context) to the human — + * e.g. posted on the Epic when it parks on `asked-question`. Returns `null` when + * the file is missing, unreadable, not JSON, or carries no string `question`: + * the Stop is still classified `asked-question` (the sentinel's *presence* is + * the signal), the contents are just best-effort. + */ +function readBlockedSentinel(path: string): BlockedSentinel | null { + try { + const parsed = JSON.parse(readFileSync(path, "utf8")) as Record; + if (typeof parsed.question !== "string" || parsed.question.length === 0) return null; + const context = typeof parsed.context === "string" ? parsed.context : undefined; + return context === undefined + ? { question: parsed.question } + : { question: parsed.question, context }; + } catch { + return null; + } +} + function readFailedReason(path: string): string { try { const parsed = JSON.parse(readFileSync(path, "utf8")) as { reason?: unknown }; diff --git a/packages/adapters/claude/test/adapter.test.ts b/packages/adapters/claude/test/adapter.test.ts index c147798c..7ad21e63 100644 --- a/packages/adapters/claude/test/adapter.test.ts +++ b/packages/adapters/claude/test/adapter.test.ts @@ -171,8 +171,12 @@ function writeMiddleDir(): { cwd: string; middle: string; transcript: string } { } describe("classifyStop", () => { - test("sentinelPresent → asked-question, with the worktree-anchored blocked.json path", () => { - const { cwd, transcript } = writeMiddleDir(); + test("sentinelPresent → asked-question, surfacing the blocked.json path + question/context", () => { + const { cwd, middle, transcript } = writeMiddleDir(); + writeFileSync( + join(middle, "blocked.json"), + JSON.stringify({ question: "Use option A or B?", context: "Both pass typecheck." }), + ); const result = claudeAdapter.classifyStop({ payload: { cwd }, transcriptPath: transcript, @@ -182,6 +186,25 @@ describe("classifyStop", () => { expect(result.kind).toBe("asked-question"); if (result.kind === "asked-question") { expect(result.sentinelPath).toBe(join(cwd, ".middle", "blocked.json")); + expect(result.sentinel).toEqual({ + question: "Use option A or B?", + context: "Both pass typecheck.", + }); + } + }); + + test("asked-question tolerates a malformed/contentless blocked.json (sentinel → null)", () => { + const { cwd, middle, transcript } = writeMiddleDir(); + writeFileSync(join(middle, "blocked.json"), "{ not valid json"); + const result = claudeAdapter.classifyStop({ + payload: { cwd }, + transcriptPath: transcript, + sentinelPresent: true, + worktree: cwd, + }); + expect(result.kind).toBe("asked-question"); + if (result.kind === "asked-question") { + expect(result.sentinel).toBeNull(); } }); diff --git a/packages/core/src/adapter.ts b/packages/core/src/adapter.ts index d37bc3b2..06b9e825 100644 --- a/packages/core/src/adapter.ts +++ b/packages/core/src/adapter.ts @@ -86,9 +86,21 @@ export type TranscriptState = { lastToolUse: string | null; }; +/** + * The contents of a `.middle/blocked.json` question sentinel: the question the + * agent needs answered to proceed, plus optional supporting context the human + * needs to answer it. The skill writes this when it parks on `asked-question`. + * Parsed tolerantly — a sentinel that is missing or malformed yields `null` on + * the classification rather than failing the Stop. + */ +export type BlockedSentinel = { + question: string; + context?: string; +}; + export type StopClassification = | { kind: "done" } // agent marked the PR ready - | { kind: "asked-question"; sentinelPath: string } + | { kind: "asked-question"; sentinelPath: string; sentinel: BlockedSentinel | null } | { kind: "rate-limited"; resetAt: string /* ISO */ } | { kind: "bare-stop" } // stopped, no sentinel, not done | { kind: "failed"; reason: string }; diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 9f4fb99b..6643e631 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -25,6 +25,7 @@ export type { LaunchOpts, TranscriptState, StopClassification, + BlockedSentinel, RateLimitDetection, } from "./adapter.ts"; diff --git a/packages/dispatcher/src/db/migrations/002_waitfor_fired.sql b/packages/dispatcher/src/db/migrations/002_waitfor_fired.sql new file mode 100644 index 00000000..a2b57059 --- /dev/null +++ b/packages/dispatcher/src/db/migrations/002_waitfor_fired.sql @@ -0,0 +1,9 @@ +-- 002_waitfor_fired.sql +-- The GitHub poller (Phase 5) fires a workflow's resume signal once per park. +-- `fired_at` records when a signal was fired so a subsequent poll pass does not +-- re-fire the same wait before the workflow has resumed and consumed the row. +-- A fresh park (next review round) deletes-and-reinserts the row, clearing it. + +ALTER TABLE waitfor_signals ADD COLUMN fired_at INTEGER; + +INSERT OR IGNORE INTO schema_version VALUES (2); diff --git a/packages/dispatcher/src/dispatch.ts b/packages/dispatcher/src/dispatch.ts index 58f3ec56..21966973 100644 --- a/packages/dispatcher/src/dispatch.ts +++ b/packages/dispatcher/src/dispatch.ts @@ -168,6 +168,11 @@ export async function dispatchEpic(opts: DispatchEpicOptions): Promise opts.repoPath, worktreeRoot: opts.worktreeRoot, dispatcherUrl: `http://127.0.0.1:${hookServer.port}`, + // Resume hand-off: a continuation round re-enters the same workflow on + // this engine (Phase 8 hosts parked executions on a long-lived engine). + enqueueContinuation: async (input) => { + await engine.start("implementation", input); + }, planCommentReader: ghGitHub, agentLogin, // Positive done-signal (#80): a bare-stop only completes if the Epic diff --git a/packages/dispatcher/src/main.ts b/packages/dispatcher/src/main.ts index b83d2214..3a89a02c 100644 --- a/packages/dispatcher/src/main.ts +++ b/packages/dispatcher/src/main.ts @@ -13,8 +13,11 @@ import { Engine } from "bunqueue/workflow"; import { openAndMigrate } from "./db.ts"; import { HookServer } from "./hook-server.ts"; import { DbHookStore } from "./hook-store.ts"; +import { ghPollGateway } from "./poller-gateway.ts"; +import { startPoller } from "./poller-cron.ts"; import { killSession, status } from "./tmux.ts"; import { startWatchdog } from "./watchdog-cron.ts"; +import { RESUME_EVENT } from "./workflows/implementation.ts"; /** Phase 2 adapter registry — only `claude` is implemented. */ function getAdapter(name: string): AgentAdapter { @@ -44,6 +47,18 @@ async function main(): Promise { getAdapter, }); + // GitHub poller: every 60s, for each parked workflow with an armed wait, fire + // its resume signal when the unblocking event appears (a human reply, or a PR + // review verdict). `fireSignal` delivers it to the engine that hosts the + // parked execution. NOTE: routing dispatches through this long-lived engine + // (so parked executions live here to be resumed) is the Phase 8 auto-dispatch + // integration; the poller + signal seam are in place ahead of it. + const stopPoller = await startPoller({ + db, + github: ghPollGateway, + fireSignal: (workflowId, payload) => engine.signal(workflowId, RESUME_EVENT, payload), + }); + console.log( `middle dispatcher up — hooks on :${hookServer.port}, db ${config.global.dbPath}`, ); @@ -59,6 +74,11 @@ async function main(): Promise { } catch (error) { console.error(`shutdown: stopWatchdog failed — ${(error as Error).message}`); } + try { + await stopPoller(); + } catch (error) { + console.error(`shutdown: stopPoller failed — ${(error as Error).message}`); + } try { hookServer.stop(); } catch (error) { diff --git a/packages/dispatcher/src/poller-cron.ts b/packages/dispatcher/src/poller-cron.ts new file mode 100644 index 00000000..4c3b7dd7 --- /dev/null +++ b/packages/dispatcher/src/poller-cron.ts @@ -0,0 +1,34 @@ +import { Bunqueue } from "bunqueue/client"; +import { runPoller, type PollerDeps } from "./poller.ts"; + +/** + * How often the poller checks GitHub for resume triggers. Slower than the + * watchdog (30s) — a human reply or a review verdict is not latency-sensitive, + * and a gentler cadence is kinder to GitHub rate limits. + */ +export const POLLER_INTERVAL_MS = 60_000; + +/** + * Stand up the GitHub poller as a bunqueue cron: every {@link POLLER_INTERVAL_MS} + * it runs one {@link runPoller} pass over parked workflows with an armed wait, + * firing the resume signal when the unblocking event appears. Returns a stop + * function that tears the cron down. The pass is resilient on its own (per- + * workflow failures are isolated); this wrapper guards the whole pass too so a + * thrown pass never crashes the cron worker. + */ +export async function startPoller(deps: PollerDeps): Promise<() => Promise> { + const queue = new Bunqueue("middle-poller", { + embedded: true, + processor: async () => { + try { + await runPoller(deps); + } catch (error) { + console.error(`[poller] pass failed: ${(error as Error).message}`); + } + }, + }); + await queue.every("poller-tick", POLLER_INTERVAL_MS); + return async () => { + await queue.close(true); + }; +} diff --git a/packages/dispatcher/src/poller-gateway.ts b/packages/dispatcher/src/poller-gateway.ts new file mode 100644 index 00000000..4cfe4840 --- /dev/null +++ b/packages/dispatcher/src/poller-gateway.ts @@ -0,0 +1,123 @@ +import type { GitHubPollGateway, IssueComment, PrReview, PrSnapshot } from "./poller.ts"; + +/** + * The production {@link GitHubPollGateway} — reads issue comments and PR review + * state through the `gh` CLI. The poller's logic is unit-tested against an + * injected stub gateway; this is the thin subprocess glue that backs it in the + * dispatcher. Read-only: the poller never writes to GitHub. + */ + +async function gh(argv: string[]): Promise { + const proc = Bun.spawn(["gh", ...argv], { stdout: "pipe", stderr: "pipe", stdin: "ignore" }); + const [stdout, stderr] = await Promise.all([ + new Response(proc.stdout).text(), + new Response(proc.stderr).text(), + ]); + if ((await proc.exited) !== 0) { + throw new Error(`gh ${argv.join(" ")} failed: ${stderr.trim()}`); + } + return stdout; +} + +function isBotLogin(login: string, type: string | undefined): boolean { + return type === "Bot" || login.endsWith("[bot]"); +} + +export const ghPollGateway: GitHubPollGateway = { + async listIssueComments(repo: string, issueNumber: number): Promise { + // `--slurp` wraps the per-page arrays into one outer array; `gh` without it + // emits one JSON array *per page*, which `JSON.parse` chokes on past page 1. + const out = await gh([ + "api", + "--paginate", + "--slurp", + `repos/${repo}/issues/${issueNumber}/comments`, + ]); + const rows = ( + JSON.parse(out) as Array< + Array<{ + id: number; + body: string; + created_at: string; + user: { login: string; type?: string } | null; + }> + > + ).flat(); + return rows.map((r) => ({ + id: r.id, + body: r.body ?? "", + createdAt: Date.parse(r.created_at), + authorLogin: r.user?.login ?? "", + authorIsBot: isBotLogin(r.user?.login ?? "", r.user?.type), + })); + }, + + async findPrForEpic(repo: string, epicNumber: number): Promise { + // The Epic's one PR closes the Epic — find the open PR referencing it. + // The server-side search is a prefix match, so `Closes #3` also surfaces + // `Closes #30`/`#300`; re-confirm the exact closing reference client-side on + // the returned bodies, anchoring the number with a non-digit boundary. + const listOut = await gh([ + "pr", + "list", + "--repo", + repo, + "--state", + "open", + "--search", + `in:body Closes #${epicNumber}`, + "--json", + "number,body", + ]); + const closesRe = new RegExp(`\\bcloses\\s+#${epicNumber}(?!\\d)`, "i"); + const prs = JSON.parse(listOut) as Array<{ number: number; body: string | null }>; + const prNumber = prs.find((pr) => closesRe.test(pr.body ?? ""))?.number; + if (prNumber === undefined) return null; + + const viewOut = await gh([ + "pr", + "view", + String(prNumber), + "--repo", + repo, + "--json", + "reviewDecision,labels", + ]); + const view = JSON.parse(viewOut) as { + reviewDecision: string | null; + labels: Array<{ name: string }>; + }; + + const reviewsOut = await gh([ + "api", + "--paginate", + "--slurp", + `repos/${repo}/pulls/${prNumber}/reviews`, + ]); + const reviewRows = ( + JSON.parse(reviewsOut) as Array< + Array<{ + id: number; + state: string; + body: string; + submitted_at: string | null; + user: { login: string } | null; + }> + > + ).flat(); + const reviews: PrReview[] = reviewRows.map((r) => ({ + id: r.id, + state: r.state, + body: r.body ?? "", + submittedAt: r.submitted_at ? Date.parse(r.submitted_at) : 0, + authorLogin: r.user?.login ?? "", + })); + + return { + number: prNumber, + reviewDecision: view.reviewDecision ?? null, + reviews, + labels: view.labels.map((l) => l.name), + }; + }, +}; diff --git a/packages/dispatcher/src/poller.ts b/packages/dispatcher/src/poller.ts new file mode 100644 index 00000000..edd5ee4a --- /dev/null +++ b/packages/dispatcher/src/poller.ts @@ -0,0 +1,190 @@ +import type { Database } from "bun:sqlite"; +import type { ResumeReason } from "./workflows/implementation.ts"; +import { loadPollableWaits, markSignalFired } from "./workflow-record.ts"; + +/** + * The GitHub poller fires a parked workflow's resume signal when its unblocking + * event appears on GitHub — for both pause kinds, which share the + * park → external-signal → resume spine: + * + * - `answered-question` — a new human (non-bot) reply on the Epic resumes it. + * - `review-changes` — a PR review verdict. `CHANGES_REQUESTED` (a review or + * the `changes-requested` label) resumes the agent to address feedback; + * **resolved** — `APPROVED`, or a fresh re-review reporting **0 actionable + * comments** — ends the loop. The 0-actionable case matters because a bot + * reviewer (CodeRabbit) often won't flip `CHANGES_REQUESTED → APPROVED` on + * its own, so a clean re-review must count as resolved or the loop hangs on + * an approval that never comes. + * + * The poller only *detects and fires*; the resume step (sub-issue #36) + * interprets the payload (re-prime with the answer / review threads, the round + * cap, terminating on resolved). Firing is idempotent: a fired wait is marked + * (`fired_at`) and skipped until the workflow resumes and a fresh park rearms. + * + * Source of truth: build spec → "Build sequence" → "Phase 5". + */ + +/** One issue comment, normalized for bot detection + recency. */ +export type IssueComment = { + id: number; + authorLogin: string; + authorIsBot: boolean; + createdAt: number; // unix ms + body: string; +}; + +/** One PR review, normalized. `state` is GitHub's review state verb. */ +export type PrReview = { + id: number; + state: string; // 'APPROVED' | 'CHANGES_REQUESTED' | 'COMMENTED' | 'DISMISSED' | ... + authorLogin: string; + submittedAt: number; // unix ms + body: string; +}; + +/** A PR's review-relevant snapshot. */ +export type PrSnapshot = { + number: number; + reviewDecision: string | null; // 'APPROVED' | 'CHANGES_REQUESTED' | 'REVIEW_REQUIRED' | null + reviews: PrReview[]; + labels: string[]; +}; + +/** The read-only GitHub surface the poller needs — injectable so tests need no `gh`. */ +export type GitHubPollGateway = { + listIssueComments(repo: string, issueNumber: number): Promise; + /** The Epic's one open PR, or null if it hasn't been opened yet. */ + findPrForEpic(repo: string, epicNumber: number): Promise; +}; + +/** What the poller fires into the workflow's resume signal for #36 to interpret. */ +export type ResumeSignalPayload = + | { + reason: "answered-question"; + reply: { commentId: number; authorLogin: string; body: string }; + } + | { + reason: "review-changes"; + outcome: ReviewOutcome; + reviewId: number | null; + decision: string | null; + }; + +export type ReviewOutcome = "changes-requested" | "resolved"; + +export type PollerDeps = { + db: Database; + github: GitHubPollGateway; + /** Deliver the resume signal to the parked workflow (engine.signal in prod). */ + fireSignal: (workflowId: string, payload: ResumeSignalPayload) => Promise; + now?: () => number; +}; + +const ACTIONABLE_RE = /actionable comments posted:\s*(\d+)/i; + +/** The resume reason a durable signal name encodes, or null if not poller-driven. */ +export function reasonFromSignalName(name: string): ResumeReason | null { + if (name.endsWith("-review-resolved")) return "review-changes"; + // `epic--answered` (workflow) and `blocked:` (watchdog re-arm fallback) + // are both the question-sentinel pause. + if (name.endsWith("-answered") || name.startsWith("blocked:")) return "answered-question"; + return null; +} + +/** The newest non-bot reply posted after the wait armed, or null. */ +export function classifyNewHumanReply(comments: IssueComment[], sinceMs: number): IssueComment | null { + const fresh = comments + .filter((c) => !c.authorIsBot && c.createdAt > sinceMs) + .sort((a, b) => b.createdAt - a.createdAt); + return fresh[0] ?? null; +} + +/** + * Classify the PR's review state into a resume verdict, or null when nothing + * actionable has changed since the wait armed. The newest review submitted this + * round is authoritative; a 0-actionable re-review counts as **resolved** even + * while the PR's `reviewDecision` still reads `CHANGES_REQUESTED`. Falls back to + * the standing decision / `changes-requested` label when no fresh review exists. + */ +export function classifyReviewOutcome( + snapshot: PrSnapshot, + sinceMs: number, +): { outcome: ReviewOutcome; reviewId: number | null; decision: string | null } | null { + const fresh = snapshot.reviews + .filter((r) => r.submittedAt > sinceMs) + .sort((a, b) => b.submittedAt - a.submittedAt); + const latest = fresh[0]; + if (latest) { + if (latest.state === "APPROVED") { + return { outcome: "resolved", reviewId: latest.id, decision: "APPROVED" }; + } + const m = ACTIONABLE_RE.exec(latest.body); + if (m && Number(m[1]) === 0) { + // Clean re-review — resolved even if the decision hasn't flipped. + return { outcome: "resolved", reviewId: latest.id, decision: snapshot.reviewDecision }; + } + if (latest.state === "CHANGES_REQUESTED" || (m && Number(m[1]) > 0)) { + return { outcome: "changes-requested", reviewId: latest.id, decision: "CHANGES_REQUESTED" }; + } + } + // No fresh verdict from a review this round — fall back to standing state. + if (snapshot.reviewDecision === "APPROVED") { + return { outcome: "resolved", reviewId: null, decision: "APPROVED" }; + } + // Deliberately NOT a `reviewDecision === "CHANGES_REQUESTED"` fallback: a bot + // reviewer leaves the PR's standing decision at CHANGES_REQUESTED even after a + // clean re-review, so re-firing off it would re-dispatch the agent every pass + // with no new feedback (and burn a round). A fresh review (handled above) or an + // explicit human `changes-requested` label is the only trustworthy resume signal. + if (snapshot.labels.includes("changes-requested")) { + return { outcome: "changes-requested", reviewId: null, decision: "CHANGES_REQUESTED" }; + } + return null; +} + +/** + * One poll pass over every parked workflow with an armed, not-yet-fired wait. + * Fires the resume signal when the unblocking event appears, then marks the + * wait fired (idempotent). Per-workflow failures (GitHub rate limits, transient + * errors) are isolated and logged — they skip that workflow this pass and are + * retried next pass; they never abort the pass for the others. Returns the + * number of signals fired (for logging/tests). + */ +export async function runPoller(deps: PollerDeps): Promise { + const now = (deps.now ?? Date.now)(); + let fired = 0; + for (const wait of loadPollableWaits(deps.db)) { + if (wait.firedAt !== null || wait.epicNumber === null) continue; + const reason = reasonFromSignalName(wait.signalName); + if (!reason) continue; + try { + if (reason === "answered-question") { + const comments = await deps.github.listIssueComments(wait.repo, wait.epicNumber); + const reply = classifyNewHumanReply(comments, wait.createdAt); + if (!reply) continue; + await deps.fireSignal(wait.workflowId, { + reason, + reply: { commentId: reply.id, authorLogin: reply.authorLogin, body: reply.body }, + }); + } else { + const pr = await deps.github.findPrForEpic(wait.repo, wait.epicNumber); + if (!pr) continue; + const verdict = classifyReviewOutcome(pr, wait.createdAt); + if (!verdict) continue; + await deps.fireSignal(wait.workflowId, { + reason, + outcome: verdict.outcome, + reviewId: verdict.reviewId, + decision: verdict.decision, + }); + } + markSignalFired(deps.db, wait.workflowId, now); + fired++; + } catch (error) { + console.error( + `[poller] poll failed for workflow ${wait.workflowId} (${wait.signalName}): ${(error as Error).message}`, + ); + } + } + return fired; +} diff --git a/packages/dispatcher/src/workflow-record.ts b/packages/dispatcher/src/workflow-record.ts index ad4b5aae..761286da 100644 --- a/packages/dispatcher/src/workflow-record.ts +++ b/packages/dispatcher/src/workflow-record.ts @@ -188,6 +188,86 @@ export function armWaitForSignal( ); } +export type ArmedSignal = { signalName: string; payloadJson: string | null }; + +/** A parked workflow the poller is watching: its armed wait joined to repo/epic. */ +export type PollableWait = { + workflowId: string; + repo: string; + epicNumber: number | null; + signalName: string; + createdAt: number; + firedAt: number | null; +}; + +/** + * Every armed wait on a parked (`waiting-human`) workflow, joined to its + * repo/epic — the poller's working set. Already-fired waits are included so the + * poller can decide idempotently; the poller filters on `firedAt`. + */ +export function loadPollableWaits(db: Database): PollableWait[] { + return db + .query( + `SELECT s.workflow_id, s.signal_name, s.created_at, s.fired_at, + w.repo, w.epic_number + FROM waitfor_signals s + JOIN workflows w ON w.id = s.workflow_id + WHERE w.state = 'waiting-human'`, + ) + .all() + .map((r) => { + const row = r as { + workflow_id: string; + signal_name: string; + created_at: number; + fired_at: number | null; + repo: string; + epic_number: number | null; + }; + return { + workflowId: row.workflow_id, + repo: row.repo, + epicNumber: row.epic_number, + signalName: row.signal_name, + createdAt: row.created_at, + firedAt: row.fired_at, + }; + }); +} + +/** Mark a workflow's armed wait as fired so the poller won't re-fire it. */ +export function markSignalFired(db: Database, workflowId: string, ts: number = Date.now()): void { + db.run("UPDATE waitfor_signals SET fired_at = ? WHERE workflow_id = ?", [ts, workflowId]); +} + +/** + * The signal armed for this workflow, or null. The poller reads this to learn + * what an Epic is waiting on (the epic-scoped, reason-scoped `signal_name`) + * without consuming it; only a successful resume consumes the row. + */ +export function getWaitForSignal(db: Database, workflowId: string): ArmedSignal | null { + const row = db + .query( + "SELECT signal_name, payload_json FROM waitfor_signals WHERE workflow_id = ? LIMIT 1", + ) + .get(workflowId) as { signal_name: string; payload_json: string | null } | null; + if (!row) return null; + return { signalName: row.signal_name, payloadJson: row.payload_json }; +} + +/** + * Consume (delete) the armed signal for a workflow on resume, returning what it + * was. The durable `waitfor_signals` row is middle's own record that the + * workflow is parked — distinct from bunqueue's in-memory `exec.signals`. It is + * armed when the workflow parks and consumed exactly once when it resumes, so a + * resumed workflow no longer reads as waiting and the poller stops watching it. + */ +export function consumeWaitForSignal(db: Database, workflowId: string): ArmedSignal | null { + const armed = getWaitForSignal(db, workflowId); + if (armed) db.run("DELETE FROM waitfor_signals WHERE workflow_id = ?", [workflowId]); + return armed; +} + type WorkflowRow = { id: string; kind: string; diff --git a/packages/dispatcher/src/workflows/implementation.ts b/packages/dispatcher/src/workflows/implementation.ts index bd85a04b..90f78ebe 100644 --- a/packages/dispatcher/src/workflows/implementation.ts +++ b/packages/dispatcher/src/workflows/implementation.ts @@ -6,21 +6,73 @@ import { Workflow } from "bunqueue/workflow"; import type { StepContext } from "bunqueue/workflow"; import { type PlanCommentReader, verifyPlanComment } from "../gates/plan-comment.ts"; import type { SessionGate } from "../hook-server.ts"; +import type { ResumeSignalPayload } from "../poller.ts"; import { markAvailableOnSuccess, parseResetAt, setRateLimited } from "../rate-limits.ts"; import type { CreateWorktreeOpts, WorktreeHandle } from "../worktree.ts"; import { + armWaitForSignal, + consumeWaitForSignal, createWorkflowRecord, updateWorkflow, type WorkflowState, } from "../workflow-record.ts"; +/** + * The handoff carried by a continuation execution. A park can only happen once + * per bunqueue execution (no loop-back; loop bodies can't hold a `waitFor`), so + * every resume is a *fresh* execution re-primed from this — reusing the prior + * round's worktree (no new branch / PR) and re-driving from the resume brief. + */ +export type ResumeInput = { + reason: ResumeReason; + /** Review-pass counter; one round = one whole `CHANGES_REQUESTED` pass. */ + round: number; + /** The worktree handle from the prior round — reused verbatim. */ + worktree: WorktreeHandle; + /** What the poller fired: the human's reply, or the review verdict. */ + payload: ResumeSignalPayload; +}; + /** A dispatch unit: an Epic (or standalone issue) pointed at one adapter. */ export type ImplementationInput = { repo: string; epicNumber: number; adapter: string; + /** + * Present only on a continuation execution (a resume). Absent on the initial + * dispatch. When set, `prepare-worktree` reuses `resume.worktree` instead of + * creating one, and writes the reason-specific resume brief to + * `.middle/prompt.md` before the drive. + */ + resume?: ResumeInput; }; +/** + * Which pause kind the workflow parked on. The two pause kinds share one + * park → external-signal → resume spine; the reason is what the resume step + * uses to pick its re-priming framing (`answer` vs `resume`/review-changes). + */ +export type ResumeReason = "answered-question" | "review-changes"; + +/** + * The single bunqueue signal event the workflow's top-level `waitFor` listens + * on. bunqueue's `waitFor(event)` takes a *static* string and `engine.signal` + * targets a specific execution by id, so one constant event name suffices — + * the epic-scoped, reason-scoped name lives in the durable `waitfor_signals` + * row (see `signalNameFor`), which is what the poller and dashboard read. + */ +export const RESUME_EVENT = "resume"; + +/** The durable, poller-facing signal name for a workflow's armed wait. */ +export function signalNameFor(epicNumber: number, reason: ResumeReason): string { + return reason === "review-changes" + ? `epic-${epicNumber}-review-resolved` + : `epic-${epicNumber}-answered`; +} + +/** The `waitFor` timeout — a parked workflow waits up to a week for its signal. */ +const WAITFOR_TIMEOUT_MS = 7 * 24 * 3600 * 1000; + /** The tmux surface the workflow drives — structural so tests can stub it. */ export type TmuxOps = { newSession(opts: { @@ -52,12 +104,37 @@ export type ImplementationDeps = { dispatcherUrl: string; launchTimeoutMs?: number; stopTimeoutMs?: number; + /** + * Post the agent's open question on the Epic for human visibility when it + * parks on `asked-question`. Receives the sentinel contents `classifyStop` + * surfaced (`question` + optional `context`). Optional + injectable so tests + * need no `gh`; the default (wired by the dispatcher) comments on the issue. + */ + postQuestion?: (opts: { + repo: string; + epicNumber: number; + question: string; + context?: string; + }) => Promise; + /** + * Enqueue a continuation execution for the next round (a resume). Injected so + * the workflow stays free of the engine: in prod the dispatcher wires this to + * `engine.start("implementation", input)` on the long-lived engine that hosts + * parked executions; tests wire it to their embedded engine. The continuation + * reuses the prior round's worktree via `input.resume.worktree`. + */ + enqueueContinuation: (input: ImplementationInput) => Promise; + /** + * The review-round ceiling: after this many `CHANGES_REQUESTED` passes without + * an `APPROVED`, the workflow parks in `waiting-human` and stops auto-resuming + * (a never-satisfied loop must not run forever). Defaults to 5. + */ + reviewRoundCap?: number; /** * Positive done-signal (skill enforcement #80): the only thing that turns a - * `bare-stop` into completion is a ready, non-draft Epic PR. When this seam is - * wired, a `bare-stop` without that signal nudges the agent (bounded) instead - * of finalizing as `completed`. Left optional so callers that haven't opted in - * keep the legacy "bare-stop → completed" behavior. + * `bare-stop` into completion is a ready, non-draft Epic PR. When wired, a + * `bare-stop` without that signal nudges the agent (bounded) instead of + * finalizing; without it, the legacy "bare-stop → completed" mapping holds. */ epicPrReadiness?: (repo: string, epicNumber: number) => Promise<{ exists: boolean; isDraft: boolean }>; /** Max "continue" nudges on a bare-stop before parking in waiting-human. */ @@ -66,9 +143,8 @@ export type ImplementationDeps = { nudgeStopTimeoutMs?: number; /** * Plan-comment guard (skill enforcement #1): when wired, a `done` dispatch only - * truly completes if a comment on the Epic carries the plan body. Left optional - * so the gate-free unit tests (and any caller that hasn't opted in) keep their - * unguarded completion behavior. + * truly completes if a comment on the Epic carries the plan body. Optional so + * gate-free unit tests keep their unguarded completion behavior. */ planCommentReader?: PlanCommentReader; /** The agent's gh account — restricts the plan-comment match to its comments. */ @@ -77,6 +153,7 @@ export type ImplementationDeps = { const DEFAULT_LAUNCH_TIMEOUT_MS = 90_000; const DEFAULT_STOP_TIMEOUT_MS = 4 * 60 * 60 * 1000; +const DEFAULT_REVIEW_ROUND_CAP = 5; const DEFAULT_MAX_NUDGES = 3; const DEFAULT_NUDGE_STOP_TIMEOUT_MS = 30 * 60 * 1000; @@ -126,11 +203,108 @@ time. Operating rules for this dispatch: } /** - * Read the workstream's committed plan from the worktree. The implementer skill - * writes it to `planning/issues//plan.md` and posts the same body as the - * Epic comment the plan-comment guard checks for. A missing file yields "" — the - * guard treats that as "no plan", which is the correct outcome. + * Overwrite `.middle/prompt.md` with the reason-specific resume brief for a + * continuation execution. The agent re-reads this on its `@`-referenced resume + * drive (`buildPromptText` kind `answer` / `resume`): + * + * - `answered-question` — inlines the human's reply so the agent reads the + * answer and continues the workstream. + * - `review-changes` — an "address review" brief. The agent pulls the PR's + * review threads itself (`gh`) and follows the `implementing-github-issues` + * skill's "Addressing review feedback" procedure (batch → internal review + * loop → push once → reply in-thread → re-request review → re-park). Carries + * the round and cap so a bounded loop is visible to the agent. + * + * This unconditionally overwrites (unlike `ensurePromptFile`, which preserves an + * operator brief on the *initial* dispatch) — a resume's brief is the live one. */ +function writeResumeBrief( + worktreePath: string, + epicNumber: number, + resume: ResumeInput, + reviewRoundCap: number, +): void { + const middleDir = join(worktreePath, ".middle"); + mkdirSync(middleDir, { recursive: true }); + const promptPath = join(middleDir, "prompt.md"); + const operatingRules = `## Operating rules for this dispatch + +- You are running autonomously under middle. There is no human watching in real + time. Continue the workstream — do not restart it. The branch, draft PR, + \`plan.md\`, and \`decisions.md\` are all intact. +- Work continuously; pause only if you are genuinely blocked (write + \`.middle/blocked.json\` and exit). The terminal state is the PR marked ready. +`; + + if (resume.reason === "answered-question") { + const reply = resume.payload.reason === "answered-question" ? resume.payload.reply : undefined; + const answer = reply + ? `> ${reply.body.replace(/\n/g, "\n> ")}\n\n— @${reply.authorLogin}` + : "(the human's reply text was unavailable — check the Epic thread on GitHub)"; + writeFileSync( + promptPath, + `# middle dispatch brief — Epic #${epicNumber} (resumed: a human answered) + +A human answered the open question you parked on. Their reply: + +${answer} + +Read this answer, fold it into your plan / decisions log, and continue the +workstream from where you left off. + +${operatingRules}`, + ); + return; + } + + // review-changes + const decision = resume.payload.reason === "review-changes" ? resume.payload.decision : null; + writeFileSync( + promptPath, + `# middle dispatch brief — Epic #${epicNumber} (resumed: address review — round ${resume.round} of ${reviewRoundCap}) + +A reviewer requested changes on the PR${decision ? ` (decision: ${decision})` : ""}. Address this +review pass now, following the \`implementing-github-issues\` skill's +**"Addressing review feedback"** procedure: + +1. Pull **every** open review thread on the PR yourself via \`gh\` (the review + comments and the review bodies). Read the whole pass before changing anything. +2. **Batch** the findings and resolve each **class-wide** — a fix plus a test per + fix, not one comment at a time. +3. Run the **internal clean-eyes review loop** over the batched diff (a review + subagent), looping until it surfaces nothing new, to catch adjacent edges + before re-review. +4. **Push once** — one push for the whole pass, not per fix. +5. Reply in-thread to each addressed comment, **re-request review**, then stop. + The workflow re-parks for the next verdict. + +This is review round ${resume.round} of ${reviewRoundCap}. After ${reviewRoundCap} rounds without an +\`APPROVED\` the workflow parks for a human and stops auto-resuming. + +${operatingRules}`, + ); +} + +/** + * The drive loop's resolved outcome: a `StopClassification` plus one + * dispatcher-only terminal `nudge-exhausted` (#80) — a `bare-stop` that never + * produced a positive done-signal within the nudge budget. Kept out of the core + * `StopClassification` union: a single Stop is never "nudge-exhausted"; only the + * loop is. + */ +type DriveOutcome = StopClassification | { kind: "nudge-exhausted" }; + +/** A park-worthy stop ends the session and waits for a human/reviewer signal. */ +function isParkKind(kind: DriveOutcome["kind"]): boolean { + return kind === "asked-question" || kind === "done"; +} + +/** The resume reason a park-worthy outcome maps to. */ +function reasonFor(kind: DriveOutcome["kind"]): ResumeReason { + return kind === "done" ? "review-changes" : "answered-question"; +} + +/** Read the workstream's committed plan from the worktree (for the plan-comment guard). */ function readPlanBody(worktreePath: string, epicNumber: number): string { try { return readFileSync(join(worktreePath, "planning", "issues", String(epicNumber), "plan.md"), "utf8"); @@ -139,16 +313,8 @@ function readPlanBody(worktreePath: string, epicNumber: number): string { } } -/** - * The drive loop's resolved outcome. It is a `StopClassification` plus one - * dispatcher-only terminal: `nudge-exhausted`, when a `bare-stop` never produced - * a positive done-signal within the nudge budget. Keeping this out of the core - * `StopClassification` union keeps the adapter's per-Stop classifier honest — a - * single Stop is never "nudge-exhausted"; only the loop is. - */ -type DriveOutcome = StopClassification | { kind: "nudge-exhausted" }; - -function finalStateForOutcome(outcome: DriveOutcome): WorkflowState { +/** The terminal `workflows.state` a settled outcome resolves to. */ +function finalStateFor(outcome: DriveOutcome): WorkflowState { switch (outcome.kind) { case "done": return "completed"; @@ -157,12 +323,14 @@ function finalStateForOutcome(outcome: DriveOutcome): WorkflowState { case "rate-limited": return "rate-limited"; case "asked-question": + // Defensive only: park kinds (`asked-question`, `done`) route to + // `parkForResume`, not here — a resume re-enqueues a continuation. return "waiting-human"; case "nudge-exhausted": - // bounded nudges produced no positive done-signal — park for a human + // #80: bounded nudges produced no positive done-signal — park for a human. return "waiting-human"; case "bare-stop": - // legacy path: no positive-done-signal seam wired, so a clean stop completes + // legacy: no positive-done-signal seam wired, so a clean stop completes. return "completed"; } } @@ -171,11 +339,25 @@ type PrepareResult = { handle: WorktreeHandle }; type DriveResult = { outcome: DriveOutcome; sessionName: string }; /** - * The Phase 1 `implementation` workflow — deliberately just three steps: - * prepare-worktree → launch-and-drive → cleanup. No skill enforcement, no - * sub-issue plan resolution, no hook-driven heartbeats; those land in Phases - * 2 and 4. `launch-and-drive` runs the launch → drive → observe loop and reacts - * to the `Stop` boundary via the adapter's `classifyStop`. + * The `implementation` workflow with the Phase 5 park → external-signal → + * resume spine: + * + * prepare-worktree → launch-and-drive → branch(park | terminal) + * → waitFor(RESUME_EVENT) → resume-or-finalize + * + * `launch-and-drive` runs the launch → drive → observe loop and ends the + * session at the `Stop` boundary (every classify outcome frees the slot). The + * branch arms a durable `waitfor_signals` row and parks the workflow in + * `waiting-human` for park-worthy stops (`asked-question`, `done`), or — for + * terminal stops — pre-seeds the signal so the single top-level `waitFor` falls + * through without parking. `resume-or-finalize` consumes the signal and + * re-drives a fresh session on resume, then finalizes (worktree teardown + + * terminal state). + * + * bunqueue's branch `.path()` bodies and loop bodies are *steps only* — a + * `waitFor` nested inside is silently dropped — and `engine.signal(id, event)` + * targets one execution, so the `waitFor` is a single top-level node and the + * loop-back for additional review rounds is re-enqueue (sub-issue #36). * * Built as a factory so the dispatcher injects real collaborators and tests * inject stubs. The workflow's `executionId` doubles as the `workflows.id`. @@ -185,45 +367,15 @@ export function createImplementationWorkflow( ): Workflow { const launchTimeout = deps.launchTimeoutMs ?? DEFAULT_LAUNCH_TIMEOUT_MS; const stopTimeout = deps.stopTimeoutMs ?? DEFAULT_STOP_TIMEOUT_MS; - - async function prepareWorktree(ctx: StepContext): Promise { - createWorkflowRecord(deps.db, { - id: ctx.executionId, - kind: "implementation", - repo: ctx.input.repo, - epicNumber: ctx.input.epicNumber, - adapter: ctx.input.adapter, - }); - const handle = await deps.worktree.createWorktree({ - repoPath: deps.resolveRepoPath(ctx.input.repo), - repo: ctx.input.repo, - issueNumber: ctx.input.epicNumber, - worktreeRoot: deps.worktreeRoot, - }); - updateWorkflow(deps.db, ctx.executionId, { worktreePath: handle.path }); - return { handle }; - } - - /** Compensation for prepare-worktree: roll the worktree back, free the session. */ - async function cleanupWorktree(ctx: StepContext): Promise { - const prepared = ctx.steps["prepare-worktree"] as PrepareResult | undefined; - if (prepared?.handle) { - await deps.tmux.killSession(sessionNameFor(ctx.input)); - await deps.worktree.destroyWorktree(prepared.handle); - } - updateWorkflow(deps.db, ctx.executionId, { state: "compensated" }); - } - + const reviewRoundCap = deps.reviewRoundCap ?? DEFAULT_REVIEW_ROUND_CAP; const maxNudges = deps.maxNudges ?? DEFAULT_MAX_NUDGES; const nudgeStopTimeout = deps.nudgeStopTimeoutMs ?? DEFAULT_NUDGE_STOP_TIMEOUT_MS; /** - * Resolve a `bare-stop` into a terminal outcome. Completion requires a - * positive done-signal — a ready, non-draft Epic PR. Without it, send a cheap - * same-session "continue" nudge and re-await the Stop, up to `maxNudges`; a - * nudge that produces a definitive classification (done, question, failure, - * rate-limit) short-circuits. Exhausting the budget parks in waiting-human - * rather than silently completing. + * Resolve a `bare-stop` into a terminal outcome (#80). Completion requires a + * ready, non-draft Epic PR; without it, send a same-session "continue" nudge + * and re-await the Stop, up to `maxNudges`. A nudge that yields a definitive + * classification short-circuits; exhausting the budget parks for a human. */ async function resolveBareStop(args: { tag: string; @@ -252,8 +404,54 @@ export function createImplementationWorkflow( } } - async function launchAndDrive(ctx: StepContext): Promise { - const { handle } = ctx.steps["prepare-worktree"] as PrepareResult; + async function prepareWorktree(ctx: StepContext): Promise { + createWorkflowRecord(deps.db, { + id: ctx.executionId, + kind: "implementation", + repo: ctx.input.repo, + epicNumber: ctx.input.epicNumber, + adapter: ctx.input.adapter, + }); + const resume = ctx.input.resume; + if (resume) { + // Continuation: reuse the prior round's worktree (same branch, same PR — + // no new branch, no new PR) and re-prime the brief for this resume reason. + const handle = resume.worktree; + updateWorkflow(deps.db, ctx.executionId, { worktreePath: handle.path }); + writeResumeBrief(handle.path, ctx.input.epicNumber, resume, reviewRoundCap); + return { handle }; + } + const handle = await deps.worktree.createWorktree({ + repoPath: deps.resolveRepoPath(ctx.input.repo), + repo: ctx.input.repo, + issueNumber: ctx.input.epicNumber, + worktreeRoot: deps.worktreeRoot, + }); + updateWorkflow(deps.db, ctx.executionId, { worktreePath: handle.path }); + return { handle }; + } + + /** Compensation for prepare-worktree: roll the worktree back, free the session. */ + async function cleanupWorktree(ctx: StepContext): Promise { + const prepared = ctx.steps["prepare-worktree"] as PrepareResult | undefined; + if (prepared?.handle) { + await deps.tmux.killSession(sessionNameFor(ctx.input)); + await deps.worktree.destroyWorktree(prepared.handle); + } + updateWorkflow(deps.db, ctx.executionId, { state: "compensated" }); + } + + /** + * Launch (or resume) one interactive session in the worktree, drive one turn, + * and classify the `Stop`. Ends the session before returning — at `Stop` the + * turn is over and the slot frees regardless of outcome ("END SESSION" in the + * dispatch lifecycle). Shared by the initial drive and the resume drive. + */ + async function driveOnce( + ctx: StepContext, + handle: WorktreeHandle, + promptKind: "initial" | "resume" | "answer", + ): Promise { const adapter = deps.getAdapter(ctx.input.adapter); const sessionName = sessionNameFor(ctx.input); const sessionToken = crypto.randomUUID(); @@ -285,9 +483,9 @@ export function createImplementationWorkflow( }, }); // Clear any orphaned session of the same name left by a prior dispatch - // that was interrupted (Ctrl-C / crash) before its cleanup ran — - // otherwise newSession fails with "duplicate session". killSession is a - // no-op when nothing's there. + // (or this workflow's own prior drive) before its cleanup ran — otherwise + // newSession fails with "duplicate session". killSession is a no-op when + // nothing's there. await deps.tmux.killSession(sessionName); console.error(`${tag} launching tmux session: ${argv.join(" ")} (cwd=${handle.path})`); await deps.tmux.newSession({ sessionName, command: argv, cwd: handle.path, env }); @@ -308,8 +506,6 @@ export function createImplementationWorkflow( console.error( `${tag} SessionStart received — session_id=${startPayload.session_id ?? ""}`, ); - // dismissPromise will resolve on its own (answered the prompt, or never - // saw it within the polling window). No further enterAutoMode call. void dismissPromise; const transcriptPath = adapter.resolveTranscriptPath(startPayload); @@ -322,10 +518,10 @@ export function createImplementationWorkflow( const promptText = adapter.buildPromptText({ promptFile: ".middle/prompt.md", - kind: "initial", + kind: promptKind, epicNumber: ctx.input.epicNumber, }); - console.error(`${tag} sending prompt: "${promptText}"`); + console.error(`${tag} sending prompt (${promptKind}): "${promptText}"`); await deps.tmux.sendText(sessionName, promptText); await deps.tmux.sendEnter(sessionName); @@ -341,96 +537,241 @@ export function createImplementationWorkflow( }); const classification = classifyAt(stopPayload); console.error(`${tag} Stop received — classification=${classification.kind}`); - - // Positive done-signal (#80): a bare-stop is NOT completion on its own. - // Only a ready, non-draft Epic PR completes it; otherwise nudge (bounded), - // then park. When no readiness seam is wired, fall through to the legacy - // "bare-stop → completed" mapping. + // Positive done-signal (#80): a bare-stop is NOT completion on its own — + // only a ready, non-draft Epic PR is. Otherwise nudge (session still + // alive) up to maxNudges, then park. No readiness seam → legacy mapping. + let outcome: DriveOutcome = classification; if (classification.kind === "bare-stop" && deps.epicPrReadiness) { - const outcome = await resolveBareStop({ + outcome = await resolveBareStop({ tag, sessionName, repo: ctx.input.repo, epicNumber: ctx.input.epicNumber, classifyAt, }); - return { outcome, sessionName }; } - return { outcome: classification, sessionName }; + // Plan-comment guard (skill enforcement #1): a `done` only truly completes + // if the agent posted its plan as an Epic comment. Demote an unposted + // `done` to `failed` here so it never enters the review-resolve park. + if (outcome.kind === "done" && deps.planCommentReader) { + const planBody = readPlanBody(handle.path, ctx.input.epicNumber); + const guard = await verifyPlanComment({ + gh: deps.planCommentReader, + repo: ctx.input.repo, + epicNumber: ctx.input.epicNumber, + planBody, + agentLogin: deps.agentLogin, + }); + if (!guard.ok) { + console.error(`${tag} plan-comment guard: ${guard.reason}`); + outcome = { kind: "failed", reason: guard.reason }; + } + } + // END SESSION — the turn is over; free the slot before parking/finalizing. + await deps.tmux.killSession(sessionName); + return { outcome, sessionName }; } catch (error) { // never leak a tmux session on the failure path; the compensation rolls // back the worktree - console.error(`${tag} step failed: ${(error as Error).message}`); + console.error(`${tag} drive failed: ${(error as Error).message}`); await deps.tmux.killSession(sessionName); throw error; } } - async function cleanup(ctx: StepContext): Promise { + async function launchAndDrive(ctx: StepContext): Promise { const { handle } = ctx.steps["prepare-worktree"] as PrepareResult; - const { outcome, sessionName } = ctx.steps["launch-and-drive"] as DriveResult; - await deps.tmux.killSession(sessionName); - - let finalState = finalStateForOutcome(outcome); - - // Plan-comment guard: a dispatch only truly completes if the agent posted - // its plan as a comment on the Epic. Run it BEFORE destroying the worktree — - // the plan body is read from the worktree's committed plan.md. - if (finalState === "completed" && deps.planCommentReader) { - const planBody = readPlanBody(handle.path, ctx.input.epicNumber); - const guard = await verifyPlanComment({ - gh: deps.planCommentReader, - repo: ctx.input.repo, - epicNumber: ctx.input.epicNumber, - planBody, - agentLogin: deps.agentLogin, - }); - if (!guard.ok) { - console.error(`[workflow:${sessionName}] ${guard.reason}`); - finalState = "failed"; + const resume = ctx.input.resume; + const promptKind = !resume + ? "initial" + : resume.reason === "answered-question" + ? "answer" + : "resume"; + return driveOnce(ctx, handle, promptKind); + } + + /** + * Park-worthy stop: arm the durable `waitfor_signals` row under the + * epic-scoped, reason-scoped name the poller watches, set `waiting-human`, + * and (for `asked-question`) post the question for human visibility. The + * session already ended in `driveOnce`. The top-level `waitFor` that follows + * then parks the execution because RESUME_EVENT is unset. + */ + async function parkForResume(ctx: StepContext): Promise { + const { outcome } = ctx.steps["launch-and-drive"] as DriveResult; + const reason = reasonFor(outcome.kind); + armWaitForSignal( + deps.db, + signalNameFor(ctx.input.epicNumber, reason), + ctx.executionId, + JSON.stringify({ reason }), + ); + updateWorkflow(deps.db, ctx.executionId, { state: "waiting-human" }); + if (outcome.kind === "asked-question" && deps.postQuestion) { + try { + await deps.postQuestion({ + repo: ctx.input.repo, + epicNumber: ctx.input.epicNumber, + question: outcome.sentinel?.question ?? "(question text unavailable)", + context: outcome.sentinel?.context, + }); + } catch (error) { + // Visibility is best-effort — the wait is already armed and durable, so + // a failed comment must not abort the park. + console.error(`[workflow] postQuestion failed: ${(error as Error).message}`); } } + } - await deps.worktree.destroyWorktree(handle); + /** + * Terminal stop: pre-seed RESUME_EVENT so the single top-level `waitFor` + * falls through without parking. Rate-limit bookkeeping and the final + * `workflows.state` are set in `resume-or-finalize` (alongside worktree + * teardown), so all terminal handling lives in one place. `ctx.signals` is + * the live `exec.signals` (passed by reference), which is exactly what the + * downstream `waitFor` reads. + */ + async function recordTerminal(ctx: StepContext): Promise { + // Rate-limit bookkeeping lives solely in `finalize` (the authoritative terminal + // handler), which always runs after this pre-seed falls the `waitFor` through. + (ctx.signals as Record)[RESUME_EVENT] = { terminal: true }; + } - if (outcome.kind === "rate-limited") { - // Reactive rate-limit: record the durable signal the auto-dispatch loop - // (Phase 8) reads to delay re-enqueue until reset_at. resetAt is the raw - // text the transcript carried after "Resets at "; parse it to unix ms, - // null when unrecognized (RATE_LIMITED with an unknown reset). + /** + * Tear the worktree down and resolve the terminal `workflows.state` for a + * settled classification. Called for genuinely-terminal stops and for a + * review-resolved (`APPROVED` / clean re-review) `done`. middle never merges — + * the human merges; this just records the terminal state and frees the worktree. + */ + async function finalize( + ctx: StepContext, + handle: WorktreeHandle, + settled: DriveOutcome, + ): Promise { + const finalState = finalStateFor(settled); + // A `waiting-human` handoff (round cap exhausted, or nudge-exhausted mid-work) + // keeps the worktree so the human can inspect / resume the in-progress state. + // Every other terminal state frees it — the work is in the PR or abandoned. + if (finalState !== "waiting-human") { + await deps.worktree.destroyWorktree(handle); + } + if (settled.kind === "rate-limited") { setRateLimited(deps.db, { adapter: ctx.input.adapter, - resetAt: parseResetAt(outcome.resetAt), + resetAt: parseResetAt(settled.resetAt), source: "transcript", - detail: outcome.resetAt, + detail: settled.resetAt, }); } else if (finalState === "completed") { // Probe-via-real-work: a completed dispatch proves the adapter is serving // again, so a previously RATE_LIMITED adapter reverts to AVAILABLE. markAvailableOnSuccess(deps.db, ctx.input.adapter); } - updateWorkflow(deps.db, ctx.executionId, { state: finalState }); } - return new Workflow("implementation") - .step("prepare-worktree", prepareWorktree, { compensate: cleanupWorktree }) - // timeout: must exceed the step's OWN internal waits (launchTimeout for - // SessionStart + stopTimeout for Stop), or bunqueue's default 30s step - // timeout fires mid-work and kills the live session. The internal - // awaitSessionStart/awaitStop timeouts stay the controlling ones (they give - // specific errors); this is a backstop just above them. - // retry: 1 — bunqueue's `retry` is `maxAttempts` (loop runs `attempt = 1 - // … <= retry`), not "retries after the first attempt". `1` means exactly - // one attempt, no retries. Phase 1 fails fast and compensates: retrying a - // launch piles up tmux/branch state and aggravates bunqueue's - // job-lifecycle race on the failure path. The full workflow's retry - // budgets (spec) live on `plan` / `implement-loop`. - .step("launch-and-drive", launchAndDrive, { - retry: 1, - // backstop above the step's own internal waits: SessionStart + the first - // Stop + up to maxNudges further Stop-awaits (the bare-stop nudge loop). - timeout: launchTimeout + stopTimeout + maxNudges * nudgeStopTimeout + 60_000, - }) - .step("cleanup", cleanup); + /** + * Reached after the `waitFor` resolves. Three outcomes: + * + * - **Terminal stop** — `record-terminal` pre-seeded `{ terminal: true }`, so + * this drive's own classification is final; `finalize` ends it. + * - **Review resolved** — the poller fired `outcome: "resolved"` (`APPROVED` + * or a clean re-review). The loop ends (terminal); the human merges. + * - **A continuing resume** — an answered question, or a `CHANGES_REQUESTED` + * pass under the round cap. Hand off to a fresh continuation execution that + * reuses this worktree (re-primed per reason); this round ends `completed` + * and the continuation becomes the Epic's live (latest non-terminal) row. + * + * The review-round counter increments **per pass**. Once a `CHANGES_REQUESTED` + * verdict would exceed `reviewRoundCap`, the workflow parks in `waiting-human` + * with no re-arm and no continuation — a never-satisfied loop is bounded. + */ + async function resumeOrFinalize(ctx: StepContext): Promise { + const { handle } = ctx.steps["prepare-worktree"] as PrepareResult; + const initial = ctx.steps["launch-and-drive"] as DriveResult; + const signal = (ctx.signals as Record)[RESUME_EVENT] as + | { terminal?: boolean } + | ResumeSignalPayload + | undefined; + + // Terminal stop: the branch pre-seeded the signal; this drive is final. + if (signal && (signal as { terminal?: boolean }).terminal) { + await finalize(ctx, handle, initial.outcome); + return; + } + + // We genuinely parked, and the poller fired a resume verdict. Consume the + // durable wait record so the workflow no longer reads as parked. + const payload = signal as ResumeSignalPayload; + consumeWaitForSignal(deps.db, ctx.executionId); + + // A resolved review (APPROVED, or a 0-actionable re-review) ends the loop. + if (payload.reason === "review-changes" && payload.outcome === "resolved") { + await finalize(ctx, handle, { kind: "done" }); + return; + } + + // A continuing resume. Only a `CHANGES_REQUESTED` pass advances the review + // counter; an answered question carries the round through unchanged. + const currentRound = ctx.input.resume?.round ?? 0; + let nextRound = currentRound; + if (payload.reason === "review-changes") { + nextRound = currentRound + 1; + if (nextRound > reviewRoundCap) { + // Bounded: stop auto-resuming and park for a human. Keep the worktree; + // do not re-arm a wait (the poller stops watching) and do not re-enqueue. + // Everything the agent has pushed stays on the branch / PR. + updateWorkflow(deps.db, ctx.executionId, { state: "waiting-human" }); + return; + } + } + + // Hand control to a fresh continuation that reuses this worktree. Enqueue + // FIRST: if it throws, neither the rate-limit state nor the row state has + // changed, so the poller retries cleanly on its next pass. + await deps.enqueueContinuation({ + repo: ctx.input.repo, + epicNumber: ctx.input.epicNumber, + adapter: ctx.input.adapter, + resume: { reason: payload.reason, round: nextRound, worktree: handle, payload }, + }); + // The drive that just parked ran a working adapter; revert any stale + // RATE_LIMITED now that the hand-off is committed. + markAvailableOnSuccess(deps.db, ctx.input.adapter); + // This round handed off — terminal in the bunqueue sense. The worktree is + // NOT torn down; the continuation reuses it. + updateWorkflow(deps.db, ctx.executionId, { state: "completed" }); + } + + return ( + new Workflow("implementation") + .step("prepare-worktree", prepareWorktree, { compensate: cleanupWorktree }) + // timeout: must exceed the step's OWN internal waits (launchTimeout for + // SessionStart + stopTimeout for Stop), or bunqueue's default 30s step + // timeout fires mid-work and kills the live session. The internal + // awaitSessionStart/awaitStop timeouts stay the controlling ones (they + // give specific errors); this is a backstop just above them. retry: 1 — + // bunqueue's `retry` is `maxAttempts`; `1` means one attempt, no retries. + .step("launch-and-drive", launchAndDrive, { + retry: 1, + // Backstop above the internal waits, widened for the bare-stop nudge loop + // (up to maxNudges further Stop-awaits) so it can't fire mid-nudge. + timeout: launchTimeout + stopTimeout + maxNudges * nudgeStopTimeout + 60_000, + }) + .branch((ctx) => + isParkKind((ctx.steps["launch-and-drive"] as DriveResult).outcome.kind) + ? "park" + : "terminal", + ) + .path("park", (w) => w.step("park-for-resume", parkForResume)) + .path("terminal", (w) => w.step("record-terminal", recordTerminal)) + // Single top-level `waitFor`: parks park-worthy stops until the poller + // fires RESUME_EVENT; terminal stops pre-seeded the signal and fall + // through. Same timeout budget as the drive step. + .waitFor(RESUME_EVENT, { timeout: WAITFOR_TIMEOUT_MS }) + .step("resume-or-finalize", resumeOrFinalize, { + retry: 1, + timeout: launchTimeout + stopTimeout + 60_000, + }) + ); } diff --git a/packages/dispatcher/test/db.test.ts b/packages/dispatcher/test/db.test.ts index bfe0a470..dc756860 100644 --- a/packages/dispatcher/test/db.test.ts +++ b/packages/dispatcher/test/db.test.ts @@ -58,10 +58,10 @@ describe("runMigrations", () => { db.close(); }); - test("applies 001_initial and reports version 1", () => { + test("applies every migration and reports the latest version", () => { const db = openDb(dbPath); - expect(runMigrations(db)).toBe(1); - expect(currentSchemaVersion(db)).toBe(1); + expect(runMigrations(db)).toBe(2); + expect(currentSchemaVersion(db)).toBe(2); db.close(); }); @@ -81,11 +81,20 @@ describe("runMigrations", () => { db.close(); }); - test("is idempotent — running twice leaves version at 1 and does not throw", () => { + test("is idempotent — running twice leaves version at the latest and does not throw", () => { const db = openDb(dbPath); runMigrations(db); - expect(runMigrations(db)).toBe(1); - expect(currentSchemaVersion(db)).toBe(1); + expect(runMigrations(db)).toBe(2); + expect(currentSchemaVersion(db)).toBe(2); + db.close(); + }); + + test("002 adds the waitfor_signals.fired_at column", () => { + const db = openAndMigrate(dbPath); + const cols = (db.query("PRAGMA table_info(waitfor_signals)").all() as { name: string }[]).map( + (c) => c.name, + ); + expect(cols).toContain("fired_at"); db.close(); }); @@ -117,7 +126,7 @@ describe("runMigrations", () => { describe("openAndMigrate", () => { test("opens, migrates, and returns a ready database", () => { const db = openAndMigrate(dbPath); - expect(currentSchemaVersion(db)).toBe(1); + expect(currentSchemaVersion(db)).toBe(2); db.close(); }); }); diff --git a/packages/dispatcher/test/implementation-workflow.test.ts b/packages/dispatcher/test/implementation-workflow.test.ts index 1ccfe018..b227aad4 100644 --- a/packages/dispatcher/test/implementation-workflow.test.ts +++ b/packages/dispatcher/test/implementation-workflow.test.ts @@ -1,6 +1,6 @@ import { afterEach, beforeEach, describe, expect, test } from "bun:test"; import type { Database } from "bun:sqlite"; -import { mkdirSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from "node:fs"; +import { mkdirSync, mkdtempSync, readFileSync, realpathSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import type { AgentAdapter, HookPayload, StopClassification } from "@middle/core"; @@ -9,9 +9,11 @@ import { Engine } from "bunqueue/workflow"; import { openAndMigrate } from "../src/db.ts"; import type { SessionGate } from "../src/hook-server.ts"; import { getRateLimitState, setRateLimited } from "../src/rate-limits.ts"; -import { getWorkflow } from "../src/workflow-record.ts"; +import { getWaitForSignal, getWorkflow } from "../src/workflow-record.ts"; import { createImplementationWorkflow, + RESUME_EVENT, + signalNameFor, type ImplementationDeps, } from "../src/workflows/implementation.ts"; import { createWorktree, destroyWorktree, listWorktrees } from "../src/worktree.ts"; @@ -92,14 +94,28 @@ const readyGate: SessionGate = { awaitStop: async () => ({ reason: "turn-end" }) as HookPayload, }; -/** A minimal AgentAdapter stub with a configurable classifyStop outcome. */ -function makeAdapterStub(classification: StopClassification): AgentAdapter { +/** + * A minimal AgentAdapter stub. `classifyStop` returns each supplied + * classification in turn (one per drive); the last value repeats — so a single + * value behaves as a constant, and a `[asked-question, done]` pair models a + * park that resumes to completion. `prompts` records every `buildPromptText` + * kind so tests can assert resume framing. + */ +function makeAdapterStub( + classifications: StopClassification | StopClassification[], + prompts: string[] = [], +): AgentAdapter { + const seq = Array.isArray(classifications) ? [...classifications] : [classifications]; + let i = 0; return { name: "stub", readyEvent: "session.started", async installHooks() {}, buildLaunchCommand: () => ({ argv: ["true"], env: {} }), - buildPromptText: () => "@.middle/prompt.md", + buildPromptText: (opts) => { + prompts.push(opts.kind); + return `@.middle/prompt.md (${opts.kind})`; + }, async enterAutoMode() {}, resolveTranscriptPath: (payload) => payload.transcript_path as string, readTranscriptState: () => ({ @@ -108,7 +124,7 @@ function makeAdapterStub(classification: StopClassification): AgentAdapter { turnCount: 0, lastToolUse: null, }), - classifyStop: () => classification, + classifyStop: () => seq[Math.min(i++, seq.length - 1)]!, }; } @@ -124,10 +140,59 @@ function makeDeps(overrides: Partial): ImplementationDeps { dispatcherUrl: "http://127.0.0.1:8822", launchTimeoutMs: 2000, stopTimeoutMs: 2000, + // Default: no continuation expected. Tests exercising the re-enqueue loop + // override this with the engine-backed harness below. + enqueueContinuation: async () => { + throw new Error("unexpected continuation enqueue"); + }, ...overrides, }; } +/** + * Wire `enqueueContinuation` to the test engine so a resume actually starts the + * next round as a fresh execution, recording each continuation's id. This is + * the production seam (`engine.start("implementation", input)`) under test — + * the re-enqueue loop the spec annotates `// loop back via re-enqueue`. + */ +function withContinuations(overrides: Partial): { + deps: ImplementationDeps; + continuationIds: string[]; +} { + const continuationIds: string[] = []; + const deps = makeDeps({ + ...overrides, + enqueueContinuation: async (input) => { + const handle = await engine.start("implementation", input); + continuationIds.push(handle.id); + }, + }); + return { deps, continuationIds }; +} + +/** Wait until the indexed continuation has been enqueued, returning its id. */ +async function awaitContinuation(ids: string[], index: number, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (ids[index]) return ids[index]!; + await Bun.sleep(15); + } + throw new Error(`continuation #${index} was not enqueued within ${timeoutMs}ms`); +} + +const CHANGES_REQUESTED = { + reason: "review-changes" as const, + outcome: "changes-requested" as const, + reviewId: 1, + decision: "CHANGES_REQUESTED", +}; +const APPROVED = { + reason: "review-changes" as const, + outcome: "resolved" as const, + reviewId: 2, + decision: "APPROVED", +}; + /** No session leak: every tmux session that was created was also killed. */ function expectNoSessionLeak(tmux: { created: string[]; killed: string[] }): void { expect(tmux.created.length).toBeGreaterThanOrEqual(1); @@ -136,61 +201,92 @@ function expectNoSessionLeak(tmux: { created: string[]; killed: string[] }): voi } } -async function runToEnd(deps: ImplementationDeps): Promise { +const EPIC = 6; +const INPUT = { repo: "thejustinwalsh/middle", epicNumber: EPIC, adapter: "stub" }; + +async function start(deps: ImplementationDeps): Promise { engine.register(createImplementationWorkflow(deps)); - const handle = await engine.start("implementation", { - repo: "thejustinwalsh/middle", - epicNumber: 6, - adapter: "stub", - }); - const deadline = Date.now() + 5000; + const handle = await engine.start("implementation", INPUT); + return handle.id; +} + +/** + * Wait until the execution is genuinely parked on the `waitFor` node — bunqueue + * `exec.state === 'waiting'`. Signalling before the branch has advanced to the + * `waitFor` would race the park; in production the poller only fires after a + * real reply, long after parking. Asserts the `workflows` row reads + * `waiting-human` once parked. + */ +async function awaitParked(id: string, timeoutMs = 5000): Promise { + const deadline = Date.now() + timeoutMs; while (Date.now() < deadline) { - const execution = engine.getExecution(handle.id); - if (execution && execution.state !== "running" && execution.state !== "compensating") { - return handle.id; + if (engine.getExecution(id)?.state === "waiting") { + expect(getWorkflow(db, id)?.state).toBe("waiting-human"); + return; } await Bun.sleep(15); } - throw new Error("workflow did not settle within 5s"); + throw new Error( + `workflow ${id} did not park within ${timeoutMs}ms (exec '${engine.getExecution(id)?.state}', row '${getWorkflow(db, id)?.state}')`, + ); } -describe("implementation workflow — happy path", () => { - test("runs prepare → drive → cleanup, ends 'completed', leaks nothing", async () => { - const tmux = makeTmuxStub(); - const deps = makeDeps({ - tmux: tmux.ops, - getAdapter: () => makeAdapterStub({ kind: "done" }), - }); - const id = await runToEnd(deps); +/** + * Wait for the `workflows` row to reach `state`, regardless of bunqueue exec + * state. Unlike `awaitParked` (which requires a live `waiting` execution), this + * also catches `waiting-human` reached via the terminal path — e.g. + * `nudge-exhausted`, which finalizes the execution but parks the row. + */ +async function awaitRow(id: string, state: string, timeoutMs = 6000): Promise { + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + if (getWorkflow(db, id)?.state === state) return; + await Bun.sleep(15); + } + throw new Error(`workflow ${id} did not reach '${state}' (was '${getWorkflow(db, id)?.state}')`); +} - const record = getWorkflow(db, id)!; - expect(record.state).toBe("completed"); - expect(record.epicNumber).toBe(6); - expect(record.sessionName).toBe("middle-thejustinwalsh-middle-6"); - expect(record.sessionId).toBe("stub-session"); - expect(record.transcriptPath).toBe("/tmp/stub.jsonl"); +/** Run the engine until the workflow row reaches a terminal-ish state. */ +async function awaitSettled(id: string, timeoutMs = 5000): Promise { + const terminal = new Set(["completed", "failed", "rate-limited", "compensated", "cancelled"]); + const deadline = Date.now() + timeoutMs; + while (Date.now() < deadline) { + const s = getWorkflow(db, id)?.state; + if (s && terminal.has(s)) return s; + await Bun.sleep(15); + } + throw new Error(`workflow ${id} did not settle within ${timeoutMs}ms (was '${getWorkflow(db, id)?.state}')`); +} - // no worktree leak - expect(await listWorktrees({ repoPath, worktreeRoot })).toEqual([]); - // no session leak — every created session was killed - expectNoSessionLeak(tmux); - }); +/** Start a dispatch and wait for it to settle; returns the workflow id. */ +async function runToEnd(deps: ImplementationDeps): Promise { + const id = await start(deps); + await awaitSettled(id); + return id; +} - test("a 'failed' classifyStop ends the workflow 'failed' but still cleans up", async () => { +describe("implementation workflow — terminal stops fall through the waitFor", () => { + test("a 'failed' classifyStop ends 'failed', destroys the worktree, leaks no session", async () => { const tmux = makeTmuxStub(); const deps = makeDeps({ tmux: tmux.ops, getAdapter: () => makeAdapterStub({ kind: "failed", reason: "stub failure" }), }); - const id = await runToEnd(deps); + const id = await start(deps); - expect(getWorkflow(db, id)!.state).toBe("failed"); + expect(await awaitSettled(id)).toBe("failed"); + expect(getWaitForSignal(db, id)).toBeNull(); // never armed a wait expect(await listWorktrees({ repoPath, worktreeRoot })).toEqual([]); expectNoSessionLeak(tmux); }); -}); -describe("implementation workflow — rate-limit state", () => { + test("a 'bare-stop' ends 'completed' without parking", async () => { + const deps = makeDeps({ getAdapter: () => makeAdapterStub({ kind: "bare-stop" }) }); + const id = await start(deps); + expect(await awaitSettled(id)).toBe("completed"); + expect(getWaitForSignal(db, id)).toBeNull(); + }); + test("a rate-limited classifyStop ends 'rate-limited' and records rate_limit_state", async () => { const tmux = makeTmuxStub(); const resetAt = "2026-05-23T18:00:00Z"; @@ -198,28 +294,207 @@ describe("implementation workflow — rate-limit state", () => { tmux: tmux.ops, getAdapter: () => makeAdapterStub({ kind: "rate-limited", resetAt }), }); - const id = await runToEnd(deps); + const id = await start(deps); - expect(getWorkflow(db, id)!.state).toBe("rate-limited"); + expect(await awaitSettled(id)).toBe("rate-limited"); const state = getRateLimitState(db, "stub")!; expect(state.status).toBe("RATE_LIMITED"); expect(state.resetAt).toBe(Date.parse(resetAt)); - expect(state.source).toBe("transcript"); - // worktree + session still cleaned up expect(await listWorktrees({ repoPath, worktreeRoot })).toEqual([]); expectNoSessionLeak(tmux); }); +}); - test("a completed dispatch reverts a previously RATE_LIMITED adapter to AVAILABLE", async () => { - setRateLimited(db, { adapter: "stub", resetAt: Date.parse("2026-05-23T18:00:00Z"), source: "transcript" }); - const deps = makeDeps({ getAdapter: () => makeAdapterStub({ kind: "done" }) }); - const id = await runToEnd(deps); +/** The `.middle/prompt.md` written into the (shared) worktree, by workflow id. */ +function readPromptBrief(workflowId: string): string { + const path = getWorkflow(db, workflowId)?.worktreePath; + if (!path) throw new Error(`workflow ${workflowId} has no worktree path`); + return readFileSync(join(path, ".middle", "prompt.md"), "utf8"); +} - expect(getWorkflow(db, id)!.state).toBe("completed"); +describe("implementation workflow — asked-question park → answer → resume (e2e)", () => { + test("parks on asked-question, a human reply resumes a fresh continuation with the answer injected", async () => { + const tmux = makeTmuxStub(); + const prompts: string[] = []; + const postQuestionCalls: Array<{ epicNumber: number; question: string; context?: string }> = []; + // One shared stub instance so its classification sequence advances across + // both executions: initial → asked-question, the continuation → done. + const adapter = makeAdapterStub( + [ + { + kind: "asked-question", + sentinelPath: "/x/.middle/blocked.json", + sentinel: { question: "Option A or B?", context: "Both compile." }, + }, + { kind: "done" }, + ], + prompts, + ); + const { deps, continuationIds } = withContinuations({ + tmux: tmux.ops, + getAdapter: () => adapter, + postQuestion: async (opts) => { + postQuestionCalls.push({ + epicNumber: opts.epicNumber, + question: opts.question, + context: opts.context, + }); + }, + }); + const id0 = await start(deps); + + // Parked: waiting-human, the epic-scoped 'answered' signal armed, worktree kept. + await awaitParked(id0); + expect(getWaitForSignal(db, id0)).toEqual({ + signalName: signalNameFor(EPIC, "answered-question"), + payloadJson: JSON.stringify({ reason: "answered-question" }), + }); + expect(postQuestionCalls).toEqual([ + { epicNumber: EPIC, question: "Option A or B?", context: "Both compile." }, + ]); + expect((await listWorktrees({ repoPath, worktreeRoot })).length).toBe(1); + expect(prompts).toEqual(["initial"]); // continuation not yet driven + + // The poller fires the human's reply → a fresh continuation execution. + await engine.signal(id0, RESUME_EVENT, { + reason: "answered-question", + reply: { commentId: 7, authorLogin: "alice", body: "Use option B." }, + }); + // The original execution hands off and ends; its wait is consumed. + expect(await awaitSettled(id0)).toBe("completed"); + expect(getWaitForSignal(db, id0)).toBeNull(); + + // The continuation re-drives with the 'answer' prompt, reusing the worktree, + // and the human's reply is injected into its brief. + const id1 = await awaitContinuation(continuationIds, 0); + await awaitParked(id1); // the answered continuation reaches done → parks on review + expect(prompts).toEqual(["initial", "answer"]); + expect(getWorkflow(db, id1)?.worktreePath).toBe(getWorkflow(db, id0)?.worktreePath); + const brief = readPromptBrief(id1); + expect(brief).toContain("a human answered"); + expect(brief).toContain("Use option B."); + expect(brief).toContain("@alice"); + // An answered question does not advance the review counter; it parks on review. + expect(getWaitForSignal(db, id1)).toEqual({ + signalName: signalNameFor(EPIC, "review-changes"), + payloadJson: JSON.stringify({ reason: "review-changes" }), + }); + + // Approve to end the loop cleanly and prove the worktree is torn down once. + await engine.signal(id1, RESUME_EVENT, APPROVED); + expect(await awaitSettled(id1)).toBe("completed"); + expect(await listWorktrees({ repoPath, worktreeRoot })).toEqual([]); + expectNoSessionLeak(tmux); + }); +}); + +describe("implementation workflow — done park → review-changes → resume (e2e)", () => { + test("a CHANGES_REQUESTED pass resumes a continuation with the address-review brief; APPROVED ends the loop", async () => { + const tmux = makeTmuxStub(); + const prompts: string[] = []; + const adapter = makeAdapterStub({ kind: "done" }, prompts); + const { deps, continuationIds } = withContinuations({ tmux: tmux.ops, getAdapter: () => adapter }); + const id0 = await start(deps); + + await awaitParked(id0); + expect(getWaitForSignal(db, id0)).toEqual({ + signalName: signalNameFor(EPIC, "review-changes"), + payloadJson: JSON.stringify({ reason: "review-changes" }), + }); + expect((await listWorktrees({ repoPath, worktreeRoot })).length).toBe(1); + + // A reviewer requests changes → resume a continuation to address them. + await engine.signal(id0, RESUME_EVENT, CHANGES_REQUESTED); + expect(await awaitSettled(id0)).toBe("completed"); + + const id1 = await awaitContinuation(continuationIds, 0); + await awaitParked(id1); + // Resumes with the 'resume' framing; the brief is the address-review brief + // (round 1 of the default cap 5) that points at the skill's procedure. + expect(prompts).toEqual(["initial", "resume"]); + const brief = readPromptBrief(id1); + expect(brief).toContain("address review — round 1 of 5"); + expect(brief).toContain("Addressing review feedback"); + expect(brief).toContain("Push once"); + expect(brief).toContain("CHANGES_REQUESTED"); + + // The agent re-requested review; an APPROVED verdict ends the loop (terminal). + await engine.signal(id1, RESUME_EVENT, APPROVED); + expect(await awaitSettled(id1)).toBe("completed"); + expect(continuationIds).toHaveLength(1); // no further round after APPROVED + expect(getWaitForSignal(db, id1)).toBeNull(); + expect(await listWorktrees({ repoPath, worktreeRoot })).toEqual([]); + expectNoSessionLeak(tmux); + }); + + test("a resolved review reverts a previously RATE_LIMITED adapter to AVAILABLE", async () => { + setRateLimited(db, { + adapter: "stub", + resetAt: Date.parse("2026-05-23T18:00:00Z"), + source: "transcript", + }); + const { deps } = withContinuations({ getAdapter: () => makeAdapterStub({ kind: "done" }) }); + const id = await start(deps); + await awaitParked(id); + await engine.signal(id, RESUME_EVENT, APPROVED); + expect(await awaitSettled(id)).toBe("completed"); expect(getRateLimitState(db, "stub")!.status).toBe("AVAILABLE"); }); }); +describe("implementation workflow — review-round cap", () => { + test("after the configured cap of CHANGES_REQUESTED passes without APPROVED, it parks in waiting-human and stops auto-resuming", async () => { + const tmux = makeTmuxStub(); + const adapter = makeAdapterStub({ kind: "done" }); + // Cap of 2: rounds 1 and 2 re-enqueue; the 3rd CHANGES_REQUESTED caps. + const { deps, continuationIds } = withContinuations({ + tmux: tmux.ops, + getAdapter: () => adapter, + reviewRoundCap: 2, + }); + const id0 = await start(deps); + + // Round 0 (initial) parks; request changes → round 1. + await awaitParked(id0); + await engine.signal(id0, RESUME_EVENT, CHANGES_REQUESTED); + expect(await awaitSettled(id0)).toBe("completed"); + + // Round 1 parks; request changes → round 2. + const id1 = await awaitContinuation(continuationIds, 0); + await awaitParked(id1); + expect(readPromptBrief(id1)).toContain("round 1 of 2"); + await engine.signal(id1, RESUME_EVENT, CHANGES_REQUESTED); + expect(await awaitSettled(id1)).toBe("completed"); + + // Round 2 parks; request changes again → would be round 3 > cap → capped. + const id2 = await awaitContinuation(continuationIds, 1); + await awaitParked(id2); + expect(readPromptBrief(id2)).toContain("round 2 of 2"); + await engine.signal(id2, RESUME_EVENT, CHANGES_REQUESTED); + + // Both "parked" and "capped" read as `waiting-human`, so wait on the + // definitive barrier: the bunqueue execution fully settling (the cap path + // runs `resume-or-finalize` to completion, which consumes id2's armed wait). + const deadline = Date.now() + 5000; + let settledState: string | undefined; + while (Date.now() < deadline) { + settledState = engine.getExecution(id2)?.state; + if (settledState === "completed" || settledState === "failed") break; + await Bun.sleep(15); + } + // Fail fast: if the cap path never ran resume-or-finalize to settle, the + // assertions below could still pass off the park-time `waiting-human` state + // and mask the regression. Require the execution to have actually settled. + expect(settledState === "completed" || settledState === "failed").toBe(true); + // Capped: parks in waiting-human, no continuation enqueued, no armed wait + // (poller stops watching), worktree preserved for the human. + expect(getWorkflow(db, id2)?.state).toBe("waiting-human"); + expect(continuationIds).toHaveLength(2); // id1, id2 — no third round + expect(getWaitForSignal(db, id2)).toBeNull(); // consumed, not re-armed + expect((await listWorktrees({ repoPath, worktreeRoot })).length).toBe(1); + }); +}); + /** A worktree stub that materializes a temp dir and (optionally) writes a plan.md. */ function makeWorktreeStub(planBody: string | null) { const handles: { path: string }[] = []; @@ -274,7 +549,7 @@ describe("implementation workflow — plan-comment completion gate", () => { expectNoSessionLeak(tmux); }); - test("a 'done' drive with a matching plan comment completes", async () => { + test("a 'done' with a matching plan comment passes the guard and parks for review", async () => { const tmux = makeTmuxStub(); const wt = makeWorktreeStub(PLAN); const deps = makeDeps({ @@ -284,16 +559,17 @@ describe("implementation workflow — plan-comment completion gate", () => { planCommentReader: makePlanReader([{ authorLogin: "agentbot", body: PLAN, url: "u" }]), agentLogin: "agentbot", }); - const id = await runToEnd(deps); + const id = await start(deps); - expect(getWorkflow(db, id)!.state).toBe("completed"); + // Guard passed (not demoted to failed) → `done` parks on review-resolved. + await awaitRow(id, "waiting-human"); expectNoSessionLeak(tmux); }); - test("without a planCommentReader wired, completion is unguarded (back-compat)", async () => { + test("without a planCommentReader wired, a 'done' parks unguarded (back-compat)", async () => { const deps = makeDeps({ getAdapter: () => makeAdapterStub({ kind: "done" }) }); - const id = await runToEnd(deps); - expect(getWorkflow(db, id)!.state).toBe("completed"); + const id = await start(deps); + await awaitRow(id, "waiting-human"); }); }); @@ -306,15 +582,16 @@ describe("implementation workflow — positive done-signal (bare-stop nudge loop epicPrReadiness: async () => ({ exists: false, isDraft: false }), maxNudges: 2, }); - const id = await runToEnd(deps); + const id = await start(deps); - expect(getWorkflow(db, id)!.state).toBe("waiting-human"); + // nudge-exhausted parks the row in waiting-human (via the terminal path). + await awaitRow(id, "waiting-human"); // nudged exactly maxNudges times before giving up expect(tmux.sent.filter((t) => t === "continue").length).toBe(2); expectNoSessionLeak(tmux); }); - test("a bare-stop completes once a ready, non-draft Epic PR exists (no nudge)", async () => { + test("a ready, non-draft Epic PR is the positive done-signal — done (no nudge), parks for review", async () => { const tmux = makeTmuxStub(); const deps = makeDeps({ tmux: tmux.ops, @@ -322,9 +599,9 @@ describe("implementation workflow — positive done-signal (bare-stop nudge loop epicPrReadiness: async () => ({ exists: true, isDraft: false }), maxNudges: 2, }); - const id = await runToEnd(deps); + const id = await start(deps); - expect(getWorkflow(db, id)!.state).toBe("completed"); + await awaitRow(id, "waiting-human"); expect(tmux.sent.filter((t) => t === "continue").length).toBe(0); }); @@ -336,9 +613,9 @@ describe("implementation workflow — positive done-signal (bare-stop nudge loop epicPrReadiness: async () => ({ exists: true, isDraft: true }), maxNudges: 1, }); - const id = await runToEnd(deps); + const id = await start(deps); - expect(getWorkflow(db, id)!.state).toBe("waiting-human"); + await awaitRow(id, "waiting-human"); expect(tmux.sent.filter((t) => t === "continue").length).toBe(1); }); @@ -366,20 +643,8 @@ describe("implementation workflow — compensation", () => { }; const deps = makeDeps({ tmux: tmux.ops, sessionGate: failingGate }); - engine.register(createImplementationWorkflow(deps)); - const handle = await engine.start("implementation", { - repo: "thejustinwalsh/middle", - epicNumber: 6, - adapter: "stub", - }); - const deadline = Date.now() + 5000; - while (Date.now() < deadline) { - const execution = engine.getExecution(handle.id); - if (execution && execution.state !== "running" && execution.state !== "compensating") break; - await Bun.sleep(15); - } - - expect(getWorkflow(db, handle.id)!.state).toBe("compensated"); + const id = await start(deps); + expect(await awaitSettled(id)).toBe("compensated"); expect(await listWorktrees({ repoPath, worktreeRoot })).toEqual([]); expectNoSessionLeak(tmux); }); diff --git a/packages/dispatcher/test/poller.test.ts b/packages/dispatcher/test/poller.test.ts new file mode 100644 index 00000000..70363841 --- /dev/null +++ b/packages/dispatcher/test/poller.test.ts @@ -0,0 +1,309 @@ +import { afterEach, beforeEach, describe, expect, test } from "bun:test"; +import type { Database } from "bun:sqlite"; +import { mkdtempSync, rmSync } from "node:fs"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; +import { openAndMigrate } from "../src/db.ts"; +import { + classifyNewHumanReply, + classifyReviewOutcome, + reasonFromSignalName, + runPoller, + type GitHubPollGateway, + type IssueComment, + type PrSnapshot, + type ResumeSignalPayload, +} from "../src/poller.ts"; +import { + armWaitForSignal, + createWorkflowRecord, + getWaitForSignal, + updateWorkflow, +} from "../src/workflow-record.ts"; +import { signalNameFor, type ResumeReason } from "../src/workflows/implementation.ts"; + +let scratch: string; +let db: Database; + +beforeEach(() => { + scratch = mkdtempSync(join(tmpdir(), "middle-poll-")); + db = openAndMigrate(join(scratch, "db.sqlite3")); +}); + +afterEach(() => { + db.close(); + rmSync(scratch, { recursive: true, force: true }); +}); + +const REPO = "thejustinwalsh/middle"; +const EPIC = 32; +const ARMED_AT = 1_000_000; + +/** Seed a parked workflow with an armed wait for `reason`, armed at ARMED_AT. */ +function seedParked(reason: ResumeReason, epic = EPIC): string { + const id = crypto.randomUUID(); + createWorkflowRecord(db, { id, kind: "implementation", repo: REPO, epicNumber: epic, adapter: "claude" }); + updateWorkflow(db, id, { state: "waiting-human" }); + // armWaitForSignal stamps created_at = Date.now(); normalize it to ARMED_AT so + // recency comparisons in the poller are deterministic. + armWaitForSignal(db, signalNameFor(epic, reason), id, JSON.stringify({ reason })); + db.run("UPDATE waitfor_signals SET created_at = ? WHERE workflow_id = ?", [ARMED_AT, id]); + return id; +} + +function comment(over: Partial): IssueComment { + return { + id: 1, + authorLogin: "octocat", + authorIsBot: false, + createdAt: ARMED_AT + 1000, + body: "hi", + ...over, + }; +} + +function prSnapshot(over: Partial): PrSnapshot { + return { number: 90, reviewDecision: null, reviews: [], labels: [], ...over }; +} + +/** A gateway stub returning fixed comments / PR snapshot, recording calls. */ +function makeGateway(opts: { + comments?: IssueComment[]; + pr?: PrSnapshot | null; +}): GitHubPollGateway & { commentCalls: number; prCalls: number } { + const g = { + commentCalls: 0, + prCalls: 0, + async listIssueComments() { + g.commentCalls++; + return opts.comments ?? []; + }, + async findPrForEpic() { + g.prCalls++; + return opts.pr ?? null; + }, + }; + return g; +} + +function captureFires(): { + fired: Array<{ workflowId: string; payload: ResumeSignalPayload }>; + fireSignal: (id: string, p: ResumeSignalPayload) => Promise; +} { + const fired: Array<{ workflowId: string; payload: ResumeSignalPayload }> = []; + return { + fired, + fireSignal: async (workflowId, payload) => { + fired.push({ workflowId, payload }); + }, + }; +} + +describe("reasonFromSignalName", () => { + test("maps the durable signal names to resume reasons", () => { + expect(reasonFromSignalName("epic-32-answered")).toBe("answered-question"); + expect(reasonFromSignalName("epic-32-review-resolved")).toBe("review-changes"); + expect(reasonFromSignalName("blocked:wf_123")).toBe("answered-question"); + expect(reasonFromSignalName("something-else")).toBeNull(); + }); +}); + +describe("classifyNewHumanReply", () => { + test("returns the newest non-bot reply posted after the wait armed", () => { + const reply = classifyNewHumanReply( + [ + comment({ id: 1, createdAt: ARMED_AT + 100, body: "first" }), + comment({ id: 2, createdAt: ARMED_AT + 500, body: "newest" }), + comment({ id: 3, authorIsBot: true, createdAt: ARMED_AT + 900, body: "bot noise" }), + comment({ id: 4, createdAt: ARMED_AT - 100, body: "stale (pre-armed)" }), + ], + ARMED_AT, + ); + expect(reply?.id).toBe(2); + expect(reply?.body).toBe("newest"); + }); + + test("returns null when only bot/stale comments exist", () => { + expect( + classifyNewHumanReply( + [comment({ authorIsBot: true, createdAt: ARMED_AT + 100 }), comment({ createdAt: ARMED_AT - 1 })], + ARMED_AT, + ), + ).toBeNull(); + }); +}); + +describe("classifyReviewOutcome", () => { + test("a fresh CHANGES_REQUESTED review → changes-requested", () => { + const v = classifyReviewOutcome( + prSnapshot({ + reviewDecision: "CHANGES_REQUESTED", + reviews: [{ id: 7, state: "CHANGES_REQUESTED", authorLogin: "coderabbitai[bot]", submittedAt: ARMED_AT + 10, body: "Actionable comments posted: 3" }], + }), + ARMED_AT, + ); + expect(v).toEqual({ outcome: "changes-requested", reviewId: 7, decision: "CHANGES_REQUESTED" }); + }); + + test("a fresh APPROVED review → resolved", () => { + const v = classifyReviewOutcome( + prSnapshot({ + reviewDecision: "APPROVED", + reviews: [{ id: 8, state: "APPROVED", authorLogin: "human", submittedAt: ARMED_AT + 10, body: "lgtm" }], + }), + ARMED_AT, + ); + expect(v).toEqual({ outcome: "resolved", reviewId: 8, decision: "APPROVED" }); + }); + + test("a fresh 0-actionable re-review → resolved even while decision stays CHANGES_REQUESTED", () => { + const v = classifyReviewOutcome( + prSnapshot({ + reviewDecision: "CHANGES_REQUESTED", // bot didn't flip its standing verdict + reviews: [{ id: 9, state: "COMMENTED", authorLogin: "coderabbitai[bot]", submittedAt: ARMED_AT + 10, body: "**Actionable comments posted: 0**\n\nLooks good." }], + }), + ARMED_AT, + ); + expect(v).toEqual({ outcome: "resolved", reviewId: 9, decision: "CHANGES_REQUESTED" }); + }); + + test("the `changes-requested` label alone (no fresh review) → changes-requested", () => { + const v = classifyReviewOutcome(prSnapshot({ labels: ["changes-requested"] }), ARMED_AT); + expect(v).toEqual({ outcome: "changes-requested", reviewId: null, decision: "CHANGES_REQUESTED" }); + }); + + test("only stale reviews and no actionable label → null (nothing changed)", () => { + const v = classifyReviewOutcome( + prSnapshot({ + reviews: [{ id: 1, state: "CHANGES_REQUESTED", authorLogin: "x", submittedAt: ARMED_AT - 5, body: "old" }], + }), + ARMED_AT, + ); + expect(v).toBeNull(); + }); + + test("a stale standing CHANGES_REQUESTED decision (no fresh review, no label) → null", () => { + // A bot reviewer leaves the PR's standing decision at CHANGES_REQUESTED even + // after the agent addressed it, so the standing decision alone must NOT + // re-fire a resume every pass — only a fresh review or an explicit label does. + const v = classifyReviewOutcome(prSnapshot({ reviewDecision: "CHANGES_REQUESTED" }), ARMED_AT); + expect(v).toBeNull(); + }); +}); + +describe("runPoller — answered-question", () => { + test("a new human reply fires epic--answered exactly once (idempotent across passes)", async () => { + const id = seedParked("answered-question"); + const github = makeGateway({ + comments: [comment({ id: 42, authorLogin: "maintainer", body: "Go with option B." })], + }); + const { fired, fireSignal } = captureFires(); + + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(1); + expect(fired).toEqual([ + { + workflowId: id, + payload: { + reason: "answered-question", + reply: { commentId: 42, authorLogin: "maintainer", body: "Go with option B." }, + }, + }, + ]); + + // Second pass must NOT re-fire (fired_at guards it). + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 9000 })).toBe(0); + expect(fired.length).toBe(1); + }); + + test("a bot-only reply does not fire", async () => { + seedParked("answered-question"); + const github = makeGateway({ + comments: [comment({ id: 1, authorLogin: "coderabbitai[bot]", authorIsBot: true })], + }); + const { fired, fireSignal } = captureFires(); + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(0); + expect(fired).toEqual([]); + }); +}); + +describe("runPoller — review-changes", () => { + test("CHANGES_REQUESTED fires review-resolved with outcome 'changes-requested'", async () => { + const id = seedParked("review-changes"); + const github = makeGateway({ + pr: prSnapshot({ + reviewDecision: "CHANGES_REQUESTED", + reviews: [{ id: 7, state: "CHANGES_REQUESTED", authorLogin: "coderabbitai[bot]", submittedAt: ARMED_AT + 10, body: "Actionable comments posted: 2" }], + }), + }); + const { fired, fireSignal } = captureFires(); + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(1); + expect(fired[0]).toEqual({ + workflowId: id, + payload: { reason: "review-changes", outcome: "changes-requested", reviewId: 7, decision: "CHANGES_REQUESTED" }, + }); + }); + + test("APPROVED fires review-resolved as resolved", async () => { + seedParked("review-changes"); + const github = makeGateway({ + pr: prSnapshot({ + reviewDecision: "APPROVED", + reviews: [{ id: 8, state: "APPROVED", authorLogin: "human", submittedAt: ARMED_AT + 10, body: "ship it" }], + }), + }); + const { fired, fireSignal } = captureFires(); + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(1); + expect(fired[0]!.payload).toEqual({ + reason: "review-changes", + outcome: "resolved", + reviewId: 8, + decision: "APPROVED", + }); + }); + + test("a 0-actionable re-review fires review-resolved as resolved", async () => { + seedParked("review-changes"); + const github = makeGateway({ + pr: prSnapshot({ + reviewDecision: "CHANGES_REQUESTED", + reviews: [{ id: 9, state: "COMMENTED", authorLogin: "coderabbitai[bot]", submittedAt: ARMED_AT + 10, body: "**Actionable comments posted: 0**" }], + }), + }); + const { fired, fireSignal } = captureFires(); + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(1); + expect(fired[0]!.payload).toMatchObject({ reason: "review-changes", outcome: "resolved" }); + }); + + test("no PR yet → no fire", async () => { + seedParked("review-changes"); + const github = makeGateway({ pr: null }); + const { fired, fireSignal } = captureFires(); + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(0); + expect(fired).toEqual([]); + }); +}); + +describe("runPoller — resilience", () => { + test("a gateway error for one workflow is isolated; others still fire", async () => { + const good = seedParked("answered-question", 100); + seedParked("answered-question", 200); // this one's gateway throws + + let n = 0; + const github: GitHubPollGateway = { + async listIssueComments(_repo, epicNumber) { + n++; + if (epicNumber === 200) throw new Error("API rate limit exceeded"); + return [comment({ id: 1, authorLogin: "human", body: "answer" })]; + }, + async findPrForEpic() { + return null; + }, + }; + const { fired, fireSignal } = captureFires(); + // One fires, one throws-and-is-skipped — the pass still completes. + expect(await runPoller({ db, github, fireSignal, now: () => ARMED_AT + 5000 })).toBe(1); + expect(n).toBe(2); + expect(fired.map((f) => f.workflowId)).toEqual([good]); + expect(getWaitForSignal(db, good)).not.toBeNull(); // row still present until resume consumes it + }); +}); diff --git a/planning/issues/32/decisions.md b/planning/issues/32/decisions.md new file mode 100644 index 00000000..8bf68556 --- /dev/null +++ b/planning/issues/32/decisions.md @@ -0,0 +1,168 @@ +# Decisions — Issue #32 (Human-in-the-loop + review-driven resume) + +## bunqueue cannot express the spec's nested waitFor graph; use a top-level waitFor spine +**File(s):** `packages/dispatcher/src/workflows/implementation.ts` +**Date:** 2026-05-23 + +**Decision:** Model park/resume as a **top-level `waitFor` node** reached after a branch, with +additional review rounds achieved by re-enqueue — not as `.path((w) => w.step().waitFor().step())` +as the build spec's idealized example shows. + +**Why:** The installed `bunqueue@2.7.12` `Workflow` builder filters branch `.path()` bodies and +loop (`doUntil`/`doWhile`/`forEach`) bodies to `type === 'step'` only (`workflow.js:46-48, 83-85`). +A `waitFor` nested inside a path or loop is **silently dropped** — it never executes. The executor +also has no goto/loop-back: `advance()` only moves forward or completes. So a `waitFor` only works +as a top-level node in the workflow's `nodes` array. The spec's annotation `// and loop back via +re-enqueue` confirms re-enqueue was always the intended looping mechanism. + +**Evidence:** `node_modules/.bun/bunqueue@2.7.12*/dist/client/workflow/workflow.js:39-51` (path +filters to steps), `executor.js:129-138` (runBranch runs path steps inline then advances), +`executor.js:149-181` (runWaitFor), `types.d.ts:51-54` (BranchDefinition.paths is `StepDefinition[]`). + +## Conditional parking via pre-seeding `ctx.signals` (by-reference) +**File(s):** `packages/dispatcher/src/workflows/implementation.ts` +**Date:** 2026-05-23 + +**Decision:** A single top-level `waitFor` follows the outcome branch. Park-worthy outcomes +(asked-question, done) leave the signal unset so the `waitFor` genuinely parks; terminal outcomes +(bare-stop, failed, rate-limited) **pre-seed `ctx.signals[RESUME_EVENT]`** in their branch step so +the same `waitFor` falls through immediately and the workflow finalizes without waiting. + +**Why:** A top-level `waitFor` always executes (no skip primitive). `buildContext` returns +`signals: exec.signals` by reference (`runner.js:169-181`), and `runWaitFor` advances when +`exec.signals[node.event] !== undefined` (`executor.js:150`). Mutating `ctx.signals` in a step +therefore satisfies the wait for terminal paths. Validated by a spike against the real embedded +engine before building the production workflow (build-to-learn). + +**Evidence:** spike test (see commit); `runner.js:178`, `executor.js:150`. + +## One generic engine event name; epic-specific naming lives in `waitfor_signals` +**File(s):** `packages/dispatcher/src/workflows/implementation.ts`, `workflow-record.ts` +**Date:** 2026-05-23 + +**Decision:** The bunqueue `waitFor` uses a single constant event string (`"resume"`). The +durable, poller-facing name (`epic--answered` / `epic--review-resolved`) is the +`waitfor_signals.signal_name`. The poller looks up the workflow by its armed row and calls +`engine.signal(workflowId, "resume", payload)` regardless of reason; the reason + data ride in the +payload and the DB row. + +**Why:** `waitFor(event)` takes a **static string** in this bunqueue version (not the spec's +`(ctx) => ...`), and `engine.signal` already targets a specific execution by id, so the event name +need not be parameterized to avoid cross-execution signal collisions. This keeps the workflow +definition static while preserving the epic-scoped, reason-scoped naming the poller and dashboard need. + +**Evidence:** `workflow.d.ts:24` (`waitFor(event: string, ...)`), `executor.js:83-97` (signal +targets one execution), spec §"implementation workflow". + +## Poller idempotency via a `fired_at` column; detect-only, interpret in #36 +**File(s):** `packages/dispatcher/src/poller.ts`, `db/migrations/002_waitfor_fired.sql` +**Date:** 2026-05-24 + +**Decision:** The poller is a pure pass over parked workflows (`waiting-human` + an +armed `waitfor_signals` row) behind an injected `GitHubPollGateway`, mirroring the +`watchdog.ts` / `state-issue.ts` gateway pattern. It *detects and fires* only — it +classifies the trigger (new non-bot reply; review verdict) and calls `fireSignal`; +the resume step (#36) interprets the payload. Idempotency is a `fired_at` column on +`waitfor_signals`: a fired wait is skipped until the workflow resumes and a fresh +park (next round) deletes-and-reinserts the row. + +**Why:** Keeps the poller unit-testable without `gh` and keeps "what to do on resume" +(round cap, threads into the prompt, terminate-on-resolved) in one place (#36). The +0-actionable-re-review-counts-as-resolved rule lives in the classifier because the +poller must decide *whether* to fire and *what outcome* to report — a bot reviewer +often won't flip `CHANGES_REQUESTED → APPROVED`, so without it the loop would hang. + +**Evidence:** `poller.test.ts` (15 tests); acceptance §#35. + +## Poller wired into `main.ts`; cross-process resume hosting is Phase 8 +**File(s):** `packages/dispatcher/src/main.ts`, `poller-cron.ts` +**Date:** 2026-05-24 + +**Decision:** `startPoller` runs as a 60s bunqueue cron in the long-running dispatcher +alongside the watchdog, with `fireSignal = (id, p) => engine.signal(id, RESUME_EVENT, p)`. + +**Why:** The poller and the signal-delivery seam belong in the persistent process. But +today dispatches run through `dispatchEpic`'s throwaway engine (which drains when the +workflow parks — `waitForSettle` returns on `waiting`), so a parked execution does not +yet live on `main.ts`'s engine to be resumed. Routing dispatches through the persistent +engine + durable bunqueue + `recover()` is the **Phase 8 auto-dispatch** integration +(explicitly out of scope for Phase 5). Wiring the seam now keeps it ready; until Phase 8, +`fireSignal` for a not-yet-hosted execution is caught by the poller's per-workflow guard +and retried — it never crashes the pass. + +**Evidence:** `dispatch.ts:42-55` (`waitForSettle` returns on non-running/non-compensating, +i.e. `waiting`); spec §"Phase 8 — Auto-dispatch + limits". + +## Multi-round resume = re-enqueue a continuation execution (one round = one execution) +**File(s):** `packages/dispatcher/src/workflows/implementation.ts` +**Date:** 2026-05-24 + +**Decision:** Each park/resume cycle is one bunqueue execution. `resume-or-finalize` +**interprets** the fired verdict and either finalizes (terminal / review *resolved*) +or **re-enqueues a continuation execution** (via an injected `enqueueContinuation` +dep) that carries `resume = { reason, round, worktree, payload }` in its input. The +continuation reuses the same worktree (its `prepare-worktree` skips `createWorktree` +and reuses the handle from `input.resume.worktree`) and drives the resume prompt in +its own `launch-and-drive`. The addressing drive therefore happens in the continuation, +not inline in `resume-or-finalize`. + +**Why:** A single execution can park only once (bunqueue has one top-level `waitFor` +per linear graph and no loop-back; loop bodies can't hold a `waitFor`). The review +loop needs up to `cap` real parks (each frees the session for a reviewer who may take +days), so the only expressible loop is re-enqueue — which the spec annotates twice +(`// loop back via re-enqueue`). The `waitfor_signals.workflow_id` must equal the +bunqueue execution id for `engine.signal` to target the parked execution, so each +round is necessarily a fresh execution (and a fresh `workflows` row, keyed by the same +`epic_number`); the live one is the latest non-terminal row. The round counter rides in +`input.resume.round`; `resume-or-finalize` increments per pass and parks in +`waiting-human` (no re-arm, no re-enqueue) once it would exceed the cap (default 5). + +**Evidence:** `#36` tests (asked-question e2e, review-changes single-round, cap boundary); +`executor.js` (no loop-back); spec §"implementation workflow". + +## The agent fetches review threads; the dispatcher writes the "address review" brief +**File(s):** `packages/dispatcher/src/workflows/implementation.ts`, +`packages/skills/implementing-github-issues/SKILL.md` +**Date:** 2026-05-24 + +**Decision:** On a `review-changes` continuation, the dispatcher overwrites +`.middle/prompt.md` with an "address review" brief (round, decision, the skill's +per-round procedure) and the agent pulls the PR's review threads itself via `gh`, +following the new **"Addressing review feedback"** section of the +`implementing-github-issues` skill (batch → internal clean-eyes review loop → push +once → reply in-thread → re-request review → re-park). + +**Why:** The agent is a full Claude session with `gh`; having it fetch live threads is +more robust than the dispatcher embedding a stale snapshot, and it keeps the dispatcher +GitHub-read-light. Codifying the procedure in the skill is what makes the autonomous +daemon loop and a hand-driven agent behave identically (the #36 acceptance's explicit +requirement). The brief in `.middle/prompt.md` is the "address-review brief" the threads +are pulled behind. + +**Evidence:** skill "Addressing review feedback" section; `prompt.ts` resume framing. + +## A handed-off continuation round terminates as `completed`, not a new `superseded` state +**File(s):** `packages/dispatcher/src/workflows/implementation.ts` +**Date:** 2026-05-24 + +**Decision:** When `resume-or-finalize` re-enqueues a continuation, the round that +handed off is marked `state = 'completed'` (the continuation becomes the Epic's +latest non-terminal row). I did **not** add a dedicated `superseded` state. + +**Why:** The handed-off row must be terminal so `findActiveWorkflowBySession` +(hook correlation) and `loadPollableWaits` ignore it — otherwise a stale round +would compete with the live continuation for the deterministic session name. +Adding a `superseded` state means modifying the `workflows.state` CHECK +constraint, which SQLite can't `ALTER` — it needs a full table rebuild (create ++ copy + drop + rename, with the `events` FK in tow). That's disproportionate +for what is, today, a cosmetic distinction: the only consumer that would tell +`completed` from `superseded` apart is the Phase 9 dashboard (out of scope), and +`markAvailableOnSuccess` firing on a handoff is *correct* (the round's drive ran +a working adapter). The honest accounting (one Epic can have several `completed` +rows, one per round) is a Phase 8/9 concern — Phase 8 routes dispatches through +the persistent engine and revisits the `workflows` row lifecycle, which is the +natural place to introduce `superseded` if the dashboard needs it. + +**Evidence:** `001_initial.sql` (state CHECK; no in-place ALTER for CHECK in +SQLite); `workflow-record.ts` `TERMINAL_STATES` / `findActiveWorkflowBySession`; +`rate-limits.ts:88` (`markAvailableOnSuccess` no-ops unless RATE_LIMITED). diff --git a/planning/issues/32/plan.md b/planning/issues/32/plan.md new file mode 100644 index 00000000..1834f9d8 --- /dev/null +++ b/planning/issues/32/plan.md @@ -0,0 +1,63 @@ +# Issue #32: Human-in-the-loop and review-driven resume flow + +**Link:** https://github.com/thejustinwalsh/middle/issues/32 +**Branch:** middle-issue-32 + +## Goal +Give the `implementation` workflow a **park → external-signal → resume** spine so an agent +can hand control back to a human (asked a question) or to a reviewer (PR-ready), and later +resume a fresh session in the same worktree with the answer / review threads in context. +`APPROVED` ends the loop; a never-satisfied review loop is bounded by a configurable round cap (default 5). + +## Approach +- The Epic's 4 open sub-issues are the phases. Build down them on one branch / one PR. +- **bunqueue reality check (load-bearing):** the installed `bunqueue@2.7.12` `Workflow` DSL + filters `.path()` / loop bodies to **steps only** — a `waitFor` nested in a branch path is + silently dropped (`workflow.js:46`). `waitFor` must be a **top-level node**. `engine.signal(execId, event, payload)` + targets a specific execution and sets `exec.signals[event]`; the matching top-level `waitFor` + then advances. `buildContext` passes `signals` **by reference** (`runner.js:178`), so a step can + pre-seed `ctx.signals[event]` to make a downstream top-level `waitFor` fall through without parking. + There is no goto/loop-back. The spec's idealized nested graph (§"implementation workflow") is + therefore expressed as: **a top-level `waitFor` spine + re-enqueue for additional rounds**, which + matches the spec's own `// loop back via re-enqueue` annotation. +- Reuse the existing `waitfor_signals` table + `armWaitForSignal`/`isWaitForArmed` (built in + Phase 2 for the watchdog sentinel re-arm). Add a `consumeWaitForSignal` (delete on resume) and a + per-workflow round counter (`meta_json` or a column). +- Poller talks to GitHub via the `gh` CLI subprocess pattern already used in `state-issue.ts`. +- Tests follow the existing `implementation-workflow.test.ts` / adapter test style: stub tmux + + SessionGate + adapter, drive the real embedded engine, assert DB state + signal flow. + +## Phases +1. **#33 waitFor signal spine** — branch on `classifyStop` outcome; asked-question + done paths arm + a `waitfor_signals` row, end the session (keep the worktree), set state `waiting-human`, park on a + top-level `waitFor`; resume re-enters carrying the resume reason; row consumed on resume. +2. **#34 classifyStop sentinel** — `.middle/blocked.json` → `{kind:'asked-question', sentinelPath}` + with the question/context surfaced to the workflow; no sentinel → `done`/`bare-stop`. +3. **#35 GitHub poller** — for Epics with an armed wait, fire `epic--answered` on a new human + reply, and `epic--review-resolved` on a review transition (CHANGES_REQUESTED/label → resume; + APPROVED **or** 0-actionable re-review → resolved). Idempotent + rate-limit resilient. +4. **#36 resume logic** — fresh session re-primed per reason; review-changes follows the skill's + "Addressing review feedback" per-round procedure (batch → internal review loop → single push → + reply in-thread → re-request → re-park); round counter per pass; cap (default 5) → `waiting-human`; + APPROVED ends the loop. + +## Files likely to change +- `packages/dispatcher/src/workflows/implementation.ts` — the park/resume spine (#33, #36) +- `packages/dispatcher/src/workflow-record.ts` — `consumeWaitForSignal`, round-counter helpers (#33, #36) +- `packages/dispatcher/src/db/migrations/00X_*.sql` — round counter / signal metadata if a column is needed +- `packages/adapters/claude/src/classify.ts` + `prompt.ts` — sentinel contents, resume prompt framing (#34, #36) +- `packages/core/src/adapter.ts` — `StopClassification` enrichment if contents are surfaced via the type (#34) +- `packages/dispatcher/src/poller.ts` (new) + wiring in `main.ts` — the GitHub poller (#35) +- `packages/dispatcher/test/*` + `packages/adapters/claude/test/*` — tests per phase + +## Out of scope +- Mechanical verification gates (Phase 6) — the poller fires on review state; it does not run gates. +- Auto-dispatch / slot enforcement (Phase 8) — parking frees the slot conceptually; the auto-dispatch + loop that consumes freed slots is Phase 8. +- Dashboard surfaces (Phase 9) — "asked question" / "waiting review" rendering. +- middle never merges — APPROVED is terminal; the human merges. + +## Open questions +- None blocking. The round-counter storage (new column vs `meta_json`) and the exact re-enqueue shape + for multi-round loops will be resolved during #36 by building (the spike in #33 validates the core + park/signal mechanic against the real engine first).