diff --git a/CHANGELOG.md b/CHANGELOG.md index 54aff57..fc4784e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,20 +3,41 @@ All notable changes to this project are documented here. Format loosely follows [Keep a Changelog](https://keepachangelog.com/); versions are [SemVer](https://semver.org/). +## [Unreleased] + +### Added + +- **`--webhook ` on the worker.** POSTs notable transitions (a task done / blocked / needs-input / + retrying, and the worker stopping / erroring) to a URL as `application/json`. One payload serves Slack + (`text`), Discord (`content`), and a generic receiver (`{ event, seq, data, worker, ts }`; `seq` is + monotonic within `worker.run`, a per-process id, so a restart reads as a new run, not data loss). + Best-effort: a POST never blocks or crashes the drain, an in-flight cap drops bursts to a slow endpoint + (re-warning on each new overload), and pending POSTs flush before every exit so the final event lands. + The URL is validated at startup (http/https only) and redacted in logs; `--webhook-secret ` HMAC-signs + the body (`X-Bob-Signature`) and fails loud when passed without `--webhook`. +- **`board_status.worker_draining.last_dispatch`** — each live drainer's most recent dispatch outcome + (status, failure detail, freshness), stamped by both the 2.0 in-process loop and the standalone worker, + so a live-but-failing drainer (e.g. Bob logged out → every dispatch aborts) surfaces up front instead of + reading as a healthy heartbeat. Detail is stored only for failures — success text never reaches the board. +- **Full review findings on the 2.0 in-process driver** — review-mode dispatches parse Bob's whole + transcript into the structured `bob-review` board note (parity with 1.x), not just the closing summary. + +### Fixed + +- **The review parser no longer invents findings from prose headings.** A `### ` section with no severity + and no `Location`/`Category` field (a reasoning turn's "Investigation plan") is skipped instead of + persisted as a phantom `info` finding. + ## [2.1.0] — 2026-07-03 — npm distribution + untracked-aware verifier +_Correction: `--webhook` was listed here but merged after the 2.1.0 npm publish; it ships in the next +release (see Unreleased)._ + ### Added - **Published to npm as `@pounceai/bob-control`.** `npx -y @pounceai/bob-control` runs the MCP server standalone via a new `bob-control` bin. The `files` allowlist ships only runtime `dist` (no tests or fixtures), and a `check:shebang` publish gate guards both bin shebangs. -- **`--webhook ` on the worker.** POSTs notable transitions (a task done / blocked / needs-input / - retrying, and the worker stopping / erroring) to a URL as `application/json`. One payload serves Slack - (`text`), Discord (`content`), and a generic receiver (`{ event, seq, data, worker, ts }`, `seq` - monotonic for reordering). Best-effort: a POST never blocks or crashes the drain, an in-flight cap drops - bursts to a slow endpoint, and pending POSTs flush before every exit so the final event lands. The URL - is validated at startup (http/https only) and redacted in logs; `--webhook-secret ` HMAC-signs the - body (`X-Bob-Signature`) for a generic receiver to verify. ### Fixed diff --git a/README.md b/README.md index 5f7ef5f..1377f83 100644 --- a/README.md +++ b/README.md @@ -338,7 +338,8 @@ system sound and terminal bell are off by default). retrying, and the worker itself stopping or erroring — to a URL as `application/json`. One payload serves three consumers: it carries `text` (a Slack incoming webhook renders it), `content` (a Discord webhook renders it), and the structured `{ event, seq, data, worker, ts }` for a generic receiver -(`seq` monotonic, so concurrent POSTs can be reordered). Delivery is +(`seq` monotonic within `worker.run`, a per-process id, so a restart reads as a new run, not dropped +deliveries; concurrent POSTs can be reordered). Delivery is best-effort: a POST never blocks or crashes the drain, bursts past an in-flight cap to a slow endpoint are dropped, and the worker flushes pending POSTs before it exits so the final event still lands. The URL is validated at startup (http/https only) and only its host is logged, since a Slack/Discord webhook diff --git a/src/bob2-driver.test.ts b/src/bob2-driver.test.ts index 0821652..22d0f21 100644 --- a/src/bob2-driver.test.ts +++ b/src/bob2-driver.test.ts @@ -135,6 +135,10 @@ test("mapOutcome: real error → aborted, settled → completed, unsettled → t assert.equal(mapOutcome(r({ last_error: "boom" }), true, { result: "x", tokensUsed: 7 }).result, ""); // not on abort assert.equal(mapOutcome(r(), false, { tokensUsed: 42 }).tokensUsed, 42); assert.equal(mapOutcome(r(), true, { maxIdleMs: 6500 }).maxIdleMs, 6500); // watchdog telemetry, any outcome + // reviewFindings pass through on completion (parity with 1.x), absent on a non-completed outcome + const finding = [{ severity: "HIGH", title: "a bug", description: "details", file: "x.ts", category: "correctness" }]; + assert.deepEqual(mapOutcome(r(), true, { reviewFindings: finding }).reviewFindings, finding); + assert.equal(mapOutcome(r(), false, { reviewFindings: finding }).reviewFindings, undefined); // not on timeout }); // ── connect / queryWorkspace ───────────────────────────────────────────────────────────────────── diff --git a/src/bob2-driver.ts b/src/bob2-driver.ts index 0ad1ca0..09fcd0c 100644 --- a/src/bob2-driver.ts +++ b/src/bob2-driver.ts @@ -12,6 +12,8 @@ import { type Bob2TaskRow, } from "./bob2-taskstore.js"; import { writeAutoApprove } from "./bob2-config.js"; +import { producesReviewFindings } from "./modes.js"; +import { parseReviewFindings, type ReviewIssue } from "./review-findings.js"; // The Bob 2.0 in-process driver. Bob 2.0 removed the node-ipc pipe, so the only way to start a task // is the extension's exported activate() API (`getExtension('IBM.bob-code').exports.startTask`), callable @@ -82,7 +84,7 @@ export interface InProcessDriverOptions { export function mapOutcome( row: Bob2TaskRow | null, settled: boolean, - extras: { result?: string; tokensUsed?: number; maxIdleMs?: number } = {}, + extras: { result?: string; reviewFindings?: ReviewIssue[]; tokensUsed?: number; maxIdleMs?: number } = {}, ): DispatchResult { const err = row ? taskError(row) : null; const detail = row ? `bob2 status=${row.status}${err ? ` error=${err}` : ""}` : ""; @@ -95,7 +97,8 @@ export function mapOutcome( maxIdleMs: extras.maxIdleMs ?? 0, }; if (err) return { ...base, status: "aborted" }; - if (settled) return { ...base, status: "completed", result: extras.result ?? "" }; + if (settled) + return { ...base, status: "completed", result: extras.result ?? "", reviewFindings: extras.reviewFindings }; return { ...base, status: "timeout" }; } @@ -282,8 +285,15 @@ export class InProcessDriver implements BobDriver { // clean completion; a timeout/error row's last message would be partial/misleading). maxGapMs is // stall-watchdog telemetry (see DispatchResult.maxIdleMs). const tokensUsed = parseCosts(row?.costs ?? null)?.output ?? 0; - const result = settled && row && !taskError(row) ? (store.readResultText(id) ?? "") : ""; - return mapOutcome(row, settled, { result, tokensUsed, maxIdleMs: maxGapMs }); + const done = settled && !!row && !taskError(row); + const result = done ? (store.readResultText(id) ?? "") : ""; + // Review mode: findings span the task's assistant messages and readResultText returns only the last + // (a summary), so parse the full transcript into structured findings for the board's bob-review note. + const reviewFindings = + done && producesReviewFindings(opts.mode ?? "") + ? parseReviewFindings(store.readReviewText(id) ?? "") + : undefined; + return mapOutcome(row, settled, { result, reviewFindings, tokensUsed, maxIdleMs: maxGapMs }); } finally { store?.close(); } diff --git a/src/bob2-taskstore.test.ts b/src/bob2-taskstore.test.ts index bb16ff4..a87fed1 100644 --- a/src/bob2-taskstore.test.ts +++ b/src/bob2-taskstore.test.ts @@ -403,6 +403,32 @@ test("readResultText flattens array content and ignores unparseable rows", () => assert.equal(store.readResultText("b"), null); // bad JSON → null, never throws }); +test("readReviewText joins all assistant messages oldest→newest (findings precede the summary)", () => { + const { db, store } = makeStore(); + insertMessage(db, "r1", "user", { content: "review it" }, 100); + insertMessage(db, "r1", "assistant", { content: "Let me look." }, 200); // reasoning + insertMessage(db, "r1", "assistant", { content: "### HIGH: a real bug\nLocation: x.ts" }, 300); // findings + insertMessage(db, "r1", "tool", { content: "read file" }, 350); // not assistant → excluded + insertMessage(db, "r1", "assistant", { content: "Review complete. 1 finding." }, 400); // summary (last) + const text = store.readReviewText("r1") ?? ""; + assert.ok(text.includes("### HIGH: a real bug"), "the findings message is included, not just the summary"); + assert.ok(text.includes("Review complete"), "the closing summary is included too"); + // Contrast with readResultText, which sees only the last (summary) message — the bug this fixes. + assert.equal(store.readResultText("r1"), "Review complete. 1 finding."); + assert.equal(store.readReviewText("none"), null); +}); + +test("readReviewText skips a malformed row but keeps the rest", () => { + const { db, store } = makeStore(); + insertMessage(db, "r2", "assistant", { content: "### LOW: keep me" }, 10); + db.prepare( + "INSERT INTO messages (id, task_id, role, data, created_at) VALUES ('bad2','r2','assistant','{not json',20)", + ).run(); + insertMessage(db, "r2", "assistant", { content: "and me" }, 30); + const text = store.readReviewText("r2") ?? ""; + assert.ok(text.includes("### LOW: keep me") && text.includes("and me")); +}); + test("open() throws a clear error when the db file is absent", () => { const missing = join(mkdtempSync(join(tmpdir(), "bob2db-")), "nope.db"); assert.throws(() => Bob2TaskStore.open(missing), /not found/); diff --git a/src/bob2-taskstore.ts b/src/bob2-taskstore.ts index ede5f98..2d56544 100644 --- a/src/bob2-taskstore.ts +++ b/src/bob2-taskstore.ts @@ -284,6 +284,30 @@ export class Bob2TaskStore { } } + /** The task's assistant messages joined oldest→newest — for review read-back. A review's findings sit in + * an EARLIER message than readResultText's last one (which is Bob's closing summary), so the structured + * parser must see the whole transcript, not just the tail. Per-row parse guard: one bad row can't drop + * the rest. Best-effort — null when there are no assistant messages (never throws). */ + readReviewText(id: string): string | null { + try { + const rows = this.q( + "SELECT data FROM messages WHERE task_id = ? AND role = 'assistant' ORDER BY created_at ASC", + ).all(id) as { data: string }[]; + const parts: string[] = []; + for (const r of rows) { + try { + const t = extractText((JSON.parse(r.data) as { content?: unknown }).content); + if (t) parts.push(t); + } catch { + /* skip a malformed row, keep the rest */ + } + } + return parts.length ? parts.join("\n\n") : null; + } catch { + return null; + } + } + close(): void { this.db.close(); } diff --git a/src/db.ts b/src/db.ts index e9995fe..bc06f38 100644 --- a/src/db.ts +++ b/src/db.ts @@ -208,6 +208,9 @@ function migrate(d: DatabaseSync): void { addColumnIfMissing(d, "tasks", "estimated_tokens", "INTEGER"); addColumnIfMissing(d, "worker_heartbeats", "worktree", "TEXT"); // T7 worktree lease addColumnIfMissing(d, "worker_heartbeats", "tag", "TEXT"); // the worker's --tag pin, surfaced in worker_draining + addColumnIfMissing(d, "worker_heartbeats", "last_dispatch_status", "TEXT"); // most recent dispatch outcome (health signal) + addColumnIfMissing(d, "worker_heartbeats", "last_dispatch_detail", "TEXT"); // short reason, e.g. a provider/auth error + addColumnIfMissing(d, "worker_heartbeats", "last_dispatch_at", "TEXT"); // when that dispatch settled (freshness) } /** @@ -633,6 +636,10 @@ export interface WorkerLiveness { /** Distinct --tag pins of the live workers (null = unfiltered, drains every tag) — so a foreman can see * why a live worker isn't pulling a task: if no entry is null or matches the task's tags, it's filtered out. */ tags: (string | null)[]; + /** The most recent dispatch outcome among the live workers — a health signal beyond "is anything beating". + * A logged-out Bob keeps beating (the 2.0 loop runs) but its dispatches abort, so `status:"aborted"` here + * flags "drainer alive but not actually working" up front. null when no live worker has dispatched yet. */ + last_dispatch: { status: string; detail: string | null; seconds_ago: number } | null; } /** Identifying metadata for a beat. `worktree` is the checkout the worker is bound to (its normalized cwd) @@ -657,6 +664,21 @@ export function recordWorkerHeartbeat(workerId: string, meta: HeartbeatMeta = {} .run(workerId, meta.assignee ?? null, meta.pid ?? null, meta.worktree ?? null, meta.tag ?? null, now, now); } +/** Stamp the worker's most recent dispatch outcome onto its heartbeat — a board_status health signal, so a + * live-but-failing drainer (e.g. Bob logged out → every dispatch aborts) shows as last_dispatch.status + * "aborted" instead of a healthy-looking "draining". Refreshes last_beat too; no-op if the row is gone + * (worker stopped). `detail` (Bob's error line) is whitespace-collapsed, truncated, and stored only for a + * non-completed status — success text is task content and must not leak into board_status. */ +export function recordDispatchOutcome(workerId: string, status: string, detail?: string | null): void { + const now = nowIso(); + const d = status !== "completed" && detail ? detail.replace(/\s+/g, " ").trim().slice(0, 160) || null : null; + getDb() + .prepare( + "UPDATE worker_heartbeats SET last_dispatch_status = ?, last_dispatch_detail = ?, last_dispatch_at = ?, last_beat = ? WHERE worker_id = ?", + ) + .run(status, d, now, now, workerId); +} + export interface WorktreeLeaseHolder { worker_id: string; pid: number | null; @@ -800,19 +822,33 @@ export function getWorkerLiveness(windowMs = WORKER_HEARTBEAT_WINDOW_MS, nowMs = getDb() .prepare("DELETE FROM worker_heartbeats WHERE last_beat < ?") .run(new Date(nowMs - windowMs * 30).toISOString()); // prune workers dead far past the window - const rows = getDb().prepare("SELECT last_beat, tag FROM worker_heartbeats").all() as { + const rows = getDb() + .prepare( + "SELECT last_beat, tag, last_dispatch_status, last_dispatch_detail, last_dispatch_at FROM worker_heartbeats", + ) + .all() as { last_beat: string; tag: string | null; + last_dispatch_status: string | null; + last_dispatch_detail: string | null; + last_dispatch_at: string | null; }[]; const liveCutoff = nowMs - windowMs; let workers = 0; let mostRecent: number | null = null; const tags = new Set(); + let lastDispatch: { status: string; detail: string | null; at: number } | null = null; for (const r of rows) { const t = Date.parse(r.last_beat); if (t >= liveCutoff) { workers++; tags.add(r.tag); // null = an unfiltered worker (drains all tags) + // The freshest dispatch outcome across live workers (a dead worker's stale outcome must not surface). + if (r.last_dispatch_status && r.last_dispatch_at) { + const dt = Date.parse(r.last_dispatch_at); + if (!lastDispatch || dt > lastDispatch.at) + lastDispatch = { status: r.last_dispatch_status, detail: r.last_dispatch_detail, at: dt }; + } } if (mostRecent === null || t > mostRecent) mostRecent = t; } @@ -821,6 +857,13 @@ export function getWorkerLiveness(windowMs = WORKER_HEARTBEAT_WINDOW_MS, nowMs = workers, last_beat_seconds_ago: mostRecent === null ? null : Math.max(0, Math.round((nowMs - mostRecent) / 1000)), tags: [...tags], + last_dispatch: lastDispatch + ? { + status: lastDispatch.status, + detail: lastDispatch.detail, + seconds_ago: Math.max(0, Math.round((nowMs - lastDispatch.at) / 1000)), + } + : null, }; } diff --git a/src/driver-loop.test.ts b/src/driver-loop.test.ts index 3729b47..8605d54 100644 --- a/src/driver-loop.test.ts +++ b/src/driver-loop.test.ts @@ -201,6 +201,23 @@ test("runDriverLoop beats a liveness heartbeat while draining, and clears it on assert.equal(getWorkerLiveness().draining, false, "heartbeat should be cleared once the loop stops"); }); +test("runDriverLoop stamps each dispatch outcome onto its own heartbeat (board_status last_dispatch)", async () => { + // Exercises the workerId threading end-to-end (runDriverLoop → driveCfg → finalize), not just the + // recordDispatchOutcome unit. The row clears on stop, so read it at the taskFail emit (fires post-stamp). + createTask({ title: "will abort", mode: "code" }); + const driver = fakeDriver({ status: "aborted", lastText: "bob2 status=error error=ProviderAuth" }); + const seen: { last: ReturnType["last_dispatch"] } = { last: null }; + await runDriverLoop({ + ...cfg(driver, ["--once"]), + emit: (t) => { + if (t === "taskFail") seen.last = getWorkerLiveness().last_dispatch; + }, + }); + assert.equal(seen.last?.status, "aborted", "the loop's own heartbeat carries the dispatch outcome"); + assert.match(seen.last?.detail ?? "", /ProviderAuth/, "the failure detail surfaces for the on-call"); + assert.equal(getWorkerLiveness().last_dispatch, null, "cleared with the heartbeat on stop"); +}); + test("runDriverLoop clears the heartbeat (and closes the driver) even if the loop throws", async () => { // Teardown is in finally, so a throw escaping the loop body must not leak the beat — an orphaned interval // would pin worker_draining true forever in the never-exiting extension host. diff --git a/src/driver-loop.ts b/src/driver-loop.ts index ee6284e..19ab6e1 100644 --- a/src/driver-loop.ts +++ b/src/driver-loop.ts @@ -1,7 +1,7 @@ import { randomUUID } from "node:crypto"; import type { BobDriver } from "./bob-driver.js"; import type { DispatchResult } from "./bob-ipc.js"; -import { type Opts, pickEligible, pidAlive } from "./worker.js"; +import { type Opts, pickEligible, pidAlive, persistReviewFindings } from "./worker.js"; import * as repo from "./db.js"; import { resolveMode, profileFor, isReadOnlyMode } from "./modes.js"; import { preserveWipToBranch } from "./checkpoint.js"; @@ -32,6 +32,9 @@ export interface DriverLoopConfig { /** Judge LLM transport overrides (fetchImpl/spawnImpl) merged into the judge deps — injected in tests * so --verify-judge runs without a real LLM call. */ judgeLlm?: Partial; + /** This loop's heartbeat id — set by runDriverLoop so finalize() can stamp each dispatch outcome onto the + * heartbeat (board_status health signal). Absent in a direct driveOnce() test → outcome recording no-ops. */ + workerId?: string; } function buildPrompt(task: Task): string { @@ -160,6 +163,9 @@ async function finalize( ): Promise { const { opts, cwd } = cfg; const log = cfg.log ?? (() => {}); + // Stamp this dispatch's outcome onto the heartbeat before branching, so board_status surfaces a + // live-but-failing drainer (every status — completed / aborted / timeout — is captured uniformly). + if (cfg.workerId) repo.recordDispatchOutcome(cfg.workerId, res.status, res.lastText); // Gate completion on the status, NOT on result text: the verify-and-continue loop returns Bob's last // (non-empty) result with status 'aborted' when it gives up, and that must block, not falsely complete. @@ -176,6 +182,7 @@ async function finalize( const usage = usageNote(res); if (usage) repo.addNote(task.id, usage, "worker"); const completed = repo.completeTask(task.id, { result, ranReadOnly, evidenceReliable: changed.gitAvailable }); + persistReviewFindings(task, mode, res); // review mode → structured bob-review note (parity with 1.x) if (task.retry_attempts > 0) repo.resetRetryAttempts(task.id); const finalStatus = completed?.status ?? "done"; log(` ✓ ${finalStatus} — ${changed.count} file(s) changed, result captured (${result.length} chars)`); @@ -245,6 +252,8 @@ export async function runDriverLoop(cfg: DriverLoopConfig, shouldStop: () => boo // Liveness beat — no worktree, so it shows under worker_draining but holds no lease (one window, one loop). // startHeartbeat is shared with the 1.x worker; stopHeartbeat() clears the timer + row. const stopHeartbeat = repo.startHeartbeat(workerId, { assignee: opts.assignee, pid: process.pid, tag: opts.tag }); + // Thread the heartbeat id so finalize() can stamp each dispatch outcome onto this worker's row. + const driveCfg: DriverLoopConfig = { ...cfg, workerId }; let deferred = false; // tracks defer state across iterations for one-shot deferred/resumed emits // Teardown in `finally` so a throw from isBoardArmed()/sleep/shouldStop/emit can't leak the heartbeat: an @@ -286,7 +295,7 @@ export async function runDriverLoop(cfg: DriverLoopConfig, shouldStop: () => boo } let processed = false; try { - processed = await driveOnce(cfg); + processed = await driveOnce(driveCfg); } catch (e) { log(`loop error: ${(e as Error).message}`); // driveOnce shouldn't throw, but never let the loop die } diff --git a/src/review-findings.test.ts b/src/review-findings.test.ts index b51c96c..99870df 100644 --- a/src/review-findings.test.ts +++ b/src/review-findings.test.ts @@ -143,6 +143,26 @@ test("formatReviewFindings preserves line 0 (round-trips, not dropped as falsy)" assert.equal(round[0].line, 0); }); +test("parseReviewFindings skips prose headings with no review marker (the 2.0 full-transcript feed)", () => { + // readReviewText hands the parser Bob's WHOLE transcript, where a reasoning turn may use headed markdown. + const md = [ + "### Investigation plan", + "First I'll read the worker, then the driver loop.", + "", + "### HIGH: Race in claim", + "**Location:** src/worker.ts:42", + "**Category:** correctness", + "Two claimers can both win.", + "", + "### Summary of what I checked", + "worker.ts, db.ts — no other issues.", + ].join("\n"); + const issues = parseReviewFindings(md); + assert.equal(issues.length, 1, "reasoning/narration headings must not become phantom findings"); + assert.equal(issues[0].title, "Race in claim"); + assert.equal(issues[0].severity, "high"); +}); + test("parseReviewFindings ignores a '### ' heading inside a fenced diff (no spurious finding, fix intact)", () => { const md = [ "### HIGH: Refactor the header", diff --git a/src/review-findings.ts b/src/review-findings.ts index d89eac6..35e397c 100644 --- a/src/review-findings.ts +++ b/src/review-findings.ts @@ -90,6 +90,9 @@ function splitH3Sections(text: string): string[] { * model emits. Splits on `### ` headings; for each, recovers severity/title from * the heading, **Location:** (file + optional first line number), **Category:**, * a fenced ```diff fixed_diff, and the remaining prose as the description. + * A section with no severity and no Location/Category field is prose, not a + * finding (the 2.0 path feeds the WHOLE transcript, where a reasoning turn's + * `### Investigation plan` must not become a phantom "info" finding). * Returns [] when no `### ` sections are present. */ export function parseReviewFindings(markdown: string): ReviewIssue[] { @@ -108,7 +111,7 @@ export function parseReviewFindings(markdown: string): ReviewIssue[] { // Recover severity + title from the heading, tolerant of the two shapes a // model emits: a leading token ("HIGH: title") or an inline suffix // ("title - Severity: HIGH"). Falls back to a Severity: field in the body. - let severity = "info"; + let severity: string | undefined; let title = heading; const sep = heading.indexOf(":"); const leading = sep !== -1 ? heading.slice(0, sep).trim().toLowerCase() : ""; @@ -126,11 +129,15 @@ export function parseReviewFindings(markdown: string): ReviewIssue[] { const sv = body.match(/severity\**:?\**\s*(critical|high|medium|low|info|warning|minor|major)\b/i); if (sv) severity = sv[1].toLowerCase(); } + // No severity and no structured field → a prose heading (a plan / narration turn), not a finding. + if (severity === undefined && field(body, "Location") === undefined && field(body, "Category") === undefined) { + continue; + } if (!title) continue; const issue: ReviewIssue = { title, - severity, + severity: severity ?? "info", category: field(body, "Category") ?? "general", description: "", }; diff --git a/src/server.ts b/src/server.ts index 1d9d0f7..34c78c9 100644 --- a/src/server.ts +++ b/src/server.ts @@ -619,7 +619,10 @@ server.registerTool( "status, whether a drainer is currently servicing the board " + "(`worker_draining` — a live heartbeat from either a 1.x worker process or the 2.0 in-process " + "loop, with `.tags` = the --tag each live worker drains, null = an unfiltered worker that drains " + - "all tags), `worker_leases` (which checkout each live worker owns), " + + "all tags, and `.last_dispatch` = the freshest dispatch outcome among live workers " + + "({status, detail, seconds_ago}, null if none yet) — a health signal beyond mere liveness: a " + + "logged-out/failing Bob keeps beating, so a `last_dispatch.status` of 'aborted' means the drainer " + + "is alive but not completing work (warn before dispatching)), `worker_leases` (which checkout each live worker owns), " + "and `open_tasks` — the non-terminal tasks (staged / " + "pending / in_progress / needs_input / blocked) as compact {id,title,status,mode,tags,priority} " + "rows for deduping before create_task (capped; see open_tasks_truncated). Check " + diff --git a/src/webhook.test.ts b/src/webhook.test.ts index 416e504..271f9fb 100644 --- a/src/webhook.test.ts +++ b/src/webhook.test.ts @@ -13,7 +13,7 @@ import { type WebhookMeta, } from "./webhook.js"; -const META: WebhookMeta = { cwd: "/repo", assignee: "bob", tag: "rpg" }; +const META: WebhookMeta = { cwd: "/repo", assignee: "bob", tag: "rpg", run: "r-1" }; // A recording fake fetch: captures calls, returns a controllable Response. function fakeFetch(response: { ok: boolean; status: number } = { ok: true, status: 200 }) { @@ -74,7 +74,7 @@ test("buildPayload: same summary in text + content, structured event, seq", () = assert.equal(p.event, "taskDone"); assert.equal(p.seq, 7); assert.equal(p.ts, "T0"); - assert.deepEqual(p.worker, { cwd: "/repo", assignee: "bob", tag: "rpg" }); + assert.deepEqual(p.worker, { cwd: "/repo", assignee: "bob", tag: "rpg", run: "r-1" }); assert.equal(p.data.filesChanged, 3); // raw event preserved for generic consumers }); @@ -165,6 +165,33 @@ test("backpressure: over the cap, excess POSTs are dropped and warned once", asy assert.equal(logs.filter((l) => l.includes("dropping")).length, 1, "warned exactly once"); }); +test("backpressure: the warning re-arms once capacity returns — a busy-but-alive endpoint can't mute it forever", async () => { + const g = gatedFetch(); + const logs: string[] = []; + const sink = createWebhookSink("http://x", META, { fetchImpl: g.impl, maxInFlight: 2, log: (m) => logs.push(m) }); + sink.post("taskDone", { id: 1, status: "done" }); // in flight + sink.post("taskDone", { id: 2, status: "done" }); // in flight (at cap) + sink.post("taskDone", { id: 3, status: "done" }); // dropped + warned (episode 1) + g.resolvers[0]({ ok: true, status: 200 } as unknown as Response); // one settles → capacity, but never zero + await new Promise((r) => setImmediate(r)); // let the settle's .finally re-arm run + sink.post("taskDone", { id: 4, status: "done" }); // back at cap + sink.post("taskDone", { id: 5, status: "done" }); // dropped again — a NEW episode must warn + assert.equal(logs.filter((l) => l.includes("dropping")).length, 2, "the second overload episode warned too"); + g.releaseAll(); + await sink.flush(); +}); + +test("post() survives a synchronously-throwing fetch (never-throws contract)", async () => { + const logs: string[] = []; + const impl = (() => { + throw new Error("sync boom"); + }) as unknown as typeof fetch; + const sink = createWebhookSink("http://x", META, { fetchImpl: impl, log: (m) => logs.push(m) }); + assert.doesNotThrow(() => sink.post("taskDone", { id: 1, status: "done" })); + await sink.flush(); // nothing made it in flight — must resolve immediately, not hang + assert.ok(logs.some((l) => l.includes("sync boom"))); +}); + test("flush awaits in-flight POSTs", async () => { let release!: (r: Response) => void; const gate = new Promise((r) => (release = r)); diff --git a/src/webhook.ts b/src/webhook.ts index 9d3f010..f3293e9 100644 --- a/src/webhook.ts +++ b/src/webhook.ts @@ -23,6 +23,9 @@ export interface WebhookMeta { cwd: string; assignee: string; tag?: string; + /** Per-process run id (the worker's heartbeat id); scopes `seq`, so a restart's seq 0 reads as a new + * run, not data loss. */ + run?: string; } export interface WebhookSink { @@ -80,9 +83,9 @@ export interface WebhookPayload { text: string; // Slack renders this content: string; // Discord renders this — intentionally identical to `text` event: WorkerEvent; - seq: number; // monotonic per-worker; concurrent POSTs can land out of order, so receivers can reorder + seq: number; // monotonic per worker.run; concurrent POSTs can land out of order, so receivers can reorder data: Record; - worker: { cwd: string; assignee: string; tag?: string }; + worker: { cwd: string; assignee: string; tag?: string; run?: string }; ts: string; } @@ -100,7 +103,7 @@ export function buildPayload( event: type, seq, data, - worker: { cwd: meta.cwd, assignee: meta.assignee, tag: meta.tag }, + worker: { cwd: meta.cwd, assignee: meta.assignee, tag: meta.tag, run: meta.run }, ts, }; } @@ -201,16 +204,26 @@ export function createWebhookSink(url: string, meta: WebhookMeta, opts: WebhookO const headers: Record = { "content-type": "application/json" }; if (opts.secret) headers["x-bob-signature"] = `sha256=${createHmac("sha256", opts.secret).update(body).digest("hex")}`; + // post() never throws: a sync throw from a broken fetchImpl is logged and dropped like any delivery failure. + let fetched: Promise; + try { + fetched = doFetch(url, { method: "POST", headers, body, signal: AbortSignal.timeout(timeoutMs) }); + } catch (e) { + log(`[bob-control] webhook ${safeUrl} POST failed for ${type}: ${(e as Error).message}`); + return; + } // p closes over itself in .finally; safe because .finally runs as a microtask, strictly after the // synchronous inFlight.add(p) below — delete never precedes add. (Don't "simplify" by splitting.) - const p = doFetch(url, { method: "POST", headers, body, signal: AbortSignal.timeout(timeoutMs) }) + const p = fetched .then((res) => { if (!res.ok) log(`[bob-control] webhook ${safeUrl} → HTTP ${res.status} for ${type}`); }) .catch((e) => log(`[bob-control] webhook ${safeUrl} POST failed for ${type}: ${(e as Error).message}`)) .finally(() => { inFlight.delete(p); - if (inFlight.size === 0) warnedOverflow = false; // re-arm the overload warning for the next episode + // Re-arm the warning when capacity returns — a busy-but-alive endpoint may never reach size 0, + // which would mute every later overload. + if (inFlight.size < maxInFlight) warnedOverflow = false; }); inFlight.add(p); }, diff --git a/src/worker-heartbeat.test.ts b/src/worker-heartbeat.test.ts index 879e5ca..5ecfb61 100644 --- a/src/worker-heartbeat.test.ts +++ b/src/worker-heartbeat.test.ts @@ -6,6 +6,7 @@ import { join } from "node:path"; import { getDb, recordWorkerHeartbeat, + recordDispatchOutcome, clearWorkerHeartbeat, getWorkerLiveness, hasLivePeer, @@ -41,6 +42,7 @@ describe("worker heartbeat liveness", () => { workers: 0, last_beat_seconds_ago: null, tags: [], + last_dispatch: null, }); }); @@ -110,4 +112,40 @@ describe("worker heartbeat liveness", () => { recordWorkerHeartbeat("reloaded", { assignee: "bob", pid: 999999 }); // fresh beat, but the process is gone assert.equal(hasLivePeer("bob", "me", onlyMe), false); // dead pid → the reload case still reclaims }); + + it("records a dispatch outcome, surfacing status + a collapsed/fresh detail; no dispatch → null", () => { + getDb().exec("DELETE FROM worker_heartbeats"); + recordWorkerHeartbeat("d1", { assignee: "bob" }); + assert.equal(getWorkerLiveness(WIN, Date.now()).last_dispatch, null, "a beat with no dispatch yet → null"); + recordDispatchOutcome("d1", "aborted", "bob2 status=error error=ProviderError\nnetwork"); + const live = getWorkerLiveness(WIN, Date.now()); + assert.equal(live.last_dispatch?.status, "aborted"); + assert.match(live.last_dispatch?.detail ?? "", /ProviderError network/); // whitespace collapsed + assert.ok((live.last_dispatch?.seconds_ago ?? 99) < 5); + }); + + it("a completed dispatch stores status but NO detail (task content must not leak into board_status)", () => { + getDb().exec("DELETE FROM worker_heartbeats"); + recordWorkerHeartbeat("d2", { assignee: "bob" }); + recordDispatchOutcome("d2", "completed", "Bob's final assistant prose — possibly sensitive"); + const live = getWorkerLiveness(WIN, Date.now()); + assert.equal(live.last_dispatch?.status, "completed"); + assert.equal(live.last_dispatch?.detail, null, "detail is the error line; success stores none"); + }); + + it("last_dispatch is the freshest among live workers, and a dead worker's outcome doesn't surface", () => { + getDb().exec("DELETE FROM worker_heartbeats"); + recordWorkerHeartbeat("older", { assignee: "bob" }); + recordWorkerHeartbeat("newer", { assignee: "bob" }); + // Fixed timestamps so the "freshest wins" pick is deterministic (both rows stay live via their beats). + const set = (id: string, status: string, at: string) => + getDb() + .prepare("UPDATE worker_heartbeats SET last_dispatch_status = ?, last_dispatch_at = ? WHERE worker_id = ?") + .run(status, at, id); + set("older", "completed", "2020-01-01T00:00:00.000Z"); + set("newer", "aborted", "2020-01-01T00:00:01.000Z"); // one second later → wins + assert.equal(getWorkerLiveness(WIN, Date.now()).last_dispatch?.status, "aborted"); + // Read past the freshness window: both workers go stale → no dispatch signal from dead rows. + assert.equal(getWorkerLiveness(WIN, Date.now() + WIN + 5_000).last_dispatch, null); + }); }); diff --git a/src/worker.ts b/src/worker.ts index 403bd5d..2c43feb 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -235,6 +235,12 @@ export function parseOpts(argv: string[]): Opts { process.exit(1); } } + // A secret with no URL is a misconfiguration — fail loud rather than silently not signing all session. + const webhookSecret = val("--webhook-secret"); + if (webhookSecret !== undefined && webhookUrl === undefined) { + console.error("--webhook-secret requires --webhook "); + process.exit(1); + } return { once: has("--once"), newTab, @@ -278,7 +284,7 @@ export function parseOpts(argv: string[]): Opts { denyCommands: csv(val("--deny-commands")), allowAllCommands: has("--allow-all-commands"), webhookUrl, - webhookSecret: val("--webhook-secret"), + webhookSecret, }; } @@ -399,7 +405,13 @@ async function parkWorkspaceMismatch(task: Task, opts: Opts, m: WorkspaceMismatc } } -async function runOne(client: BobClient, task: Task, opts: Opts, patchPresent: boolean | null): Promise { +async function runOne( + client: BobClient, + task: Task, + opts: Opts, + patchPresent: boolean | null, + workerId?: string, +): Promise { const { mode, source, profile, pressesLand } = resolveRouting(task, patchPresent); console.log(`\n▶ #${task.id} "${task.title}" → mode {${mode}} (${source}, risk:${profile.risk})`); emit(opts, "taskStart", { id: task.id, title: task.title, mode, risk: profile.risk }); @@ -428,6 +440,9 @@ async function runOne(client: BobClient, task: Task, opts: Opts, patchPresent: b const initialRes = await doDispatch(buildPrompt(task)); const res = await pollLoop(initialRes, planStopBaseline); + // Stamp the outcome onto the heartbeat before terminal handling — the 1.x analog of finalize()'s + // last_dispatch stamp, so a live-but-failing drainer surfaces on this path too. + if (workerId) repo.recordDispatchOutcome(workerId, res.status, res.lastText); persistReviewFindings(task, mode, res); // The followup gate is unregistered by the caller's finally (guaranteed on every exit path). await finalizeDispatch(client, task, opts, mode, res, getSeenAsk(), evidenceBaseline); @@ -787,8 +802,9 @@ async function prepareDispatch( return { planStopBaseline, evidenceBaseline }; } -/** Format and persist Bob's review findings (review-producing modes) as a structured board note. */ -function persistReviewFindings(task: Task, mode: string, res: DispatchResult): void { +/** Format and persist Bob's review findings (review-producing modes) as a structured board note. + * Exported so the 2.0 in-process driver's finalize() reuses the same capture the 1.x worker does. */ +export function persistReviewFindings(task: Task, mode: string, res: DispatchResult): void { // Format and persist review findings (review-producing modes: review + devsecops). // Prefer the structured submit_review_findings capture; but under headless IPC // dispatch Bob is tool-restricted and never calls that tool — it returns the review @@ -994,11 +1010,14 @@ export async function main(): Promise { const detail = reason instanceof Error ? (reason.stack ?? reason.message) : String(reason); console.error(`bob-worker: unhandled rejection (ignored): ${detail}`); }); + // One id per run: the lease heartbeat AND the webhook `run` field, so webhook events correlate to a + // board heartbeat and `seq` is scoped per run (a restart = a new id). + const workerId = randomUUID(); // Wire the webhook sink before the first emit so even a startup error (lease/connect) is delivered. if (opts.webhookUrl) { webhookSink = createWebhookSink( opts.webhookUrl, - { cwd: process.cwd(), assignee: opts.assignee, tag: opts.tag }, + { cwd: process.cwd(), assignee: opts.assignee, tag: opts.tag, run: workerId }, { secret: opts.webhookSecret }, ); // Redact the URL: a Slack/Discord webhook carries its secret in the path. @@ -1013,7 +1032,6 @@ export async function main(): Promise { // so it holds whether the board is per-project or worktree-shared. claimWorktreeLease checks-and-claims // atomically (one transaction), so two workers starting together can't both observe "no holder" and // both proceed. The claim doubles as the first liveness heartbeat board_status reads. - const workerId = randomUUID(); const worktreeKey = normalizeWorkspacePath(process.cwd()); const beatMeta = { assignee: opts.assignee, pid: process.pid, worktree: worktreeKey, tag: opts.tag }; if (!opts.dryRun) { @@ -1316,9 +1334,11 @@ export async function main(): Promise { } pollStatus.enter("active"); try { - await runOne(client, task, opts, patchPresent); + await runOne(client, task, opts, patchPresent, workerId); } catch (err) { console.error(` ! error on #${task.id}: ${(err as Error).message}`); + // A dispatch that died before runOne's stamp still marks the heartbeat as "error", not a healthy beat. + repo.recordDispatchOutcome(workerId, "error", (err as Error).message); // Only park genuinely unfinished work — don't clobber a task that already completed // (done OR analysis_done) or is legitimately parked awaiting a human answer (needs_input) // when the error is thrown after completion/parking.