From 662c37bcc1c25b5d1415fe09fdd1bdf5b7ee0529 Mon Sep 17 00:00:00 2001 From: Joshua Gilbert <54961107+Joshua-Gilbert@users.noreply.github.com> Date: Fri, 3 Jul 2026 10:58:26 -0700 Subject: [PATCH 1/3] feat(bob2): capture full review findings on the 2.0 in-process driver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A review dispatched by the 2.0 driver landed only a one-line summary on the board — no structured bob-review note — because two steps the 1.x worker does were missing on the 2.0 path: - finalize() never called persistReviewFindings, so the bob-review note was never written, regardless of the result text. - readResultText returns only Bob's LAST assistant message, which for a review is often the closing summary rather than the findings (they sit in an earlier message). Added readReviewText to join the task's assistant messages so the parser sees the whole transcript. The 2.0 driver now parses the full review text into reviewFindings for review mode, and finalize() persists them via the same path the 1.x worker uses. --- src/bob2-driver.test.ts | 4 ++++ src/bob2-driver.ts | 18 ++++++++++++++---- src/bob2-taskstore.test.ts | 26 ++++++++++++++++++++++++++ src/bob2-taskstore.ts | 24 ++++++++++++++++++++++++ src/driver-loop.ts | 3 ++- src/worker.ts | 5 +++-- 6 files changed, 73 insertions(+), 7 deletions(-) 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/driver-loop.ts b/src/driver-loop.ts index ee6284e..33fe9ab 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"; @@ -176,6 +176,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)`); diff --git a/src/worker.ts b/src/worker.ts index 403bd5d..4d7c8c7 100644 --- a/src/worker.ts +++ b/src/worker.ts @@ -787,8 +787,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 From 27b45942bc5c98976a1641f6af559a7537f1f054 Mon Sep 17 00:00:00 2001 From: Joshua Gilbert <54961107+Joshua-Gilbert@users.noreply.github.com> Date: Fri, 3 Jul 2026 11:06:49 -0700 Subject: [PATCH 2/3] feat(board): surface each drainer's last dispatch outcome in board_status MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A logged-out Bob still heartbeats (the 2.0 in-process loop keeps running), so worker_draining reads healthy while every dispatch aborts on a provider auth/network error — the foreman can't tell "draining" from "draining but failing" and dispatches into a guaranteed abort. Record each dispatch's outcome onto the worker's heartbeat row and surface the freshest one among live drainers as worker_draining.last_dispatch ({status, detail, seconds_ago}). A status of "aborted" now flags "alive but not completing work" up front. Proactive login state can't be probed (Bob exposes no auth API), so this is the reactive signal the driver already has. - db: three heartbeat columns + recordDispatchOutcome; getWorkerLiveness picks the freshest outcome among live rows (a dead worker's stale one doesn't leak). - driver-loop: finalize() stamps every dispatch outcome via the loop's worker id. --- src/db.ts | 44 +++++++++++++++++++++++++++++++++++- src/driver-loop.ts | 10 +++++++- src/server.ts | 5 +++- src/worker-heartbeat.test.ts | 29 ++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 3 deletions(-) diff --git a/src/db.ts b/src/db.ts index e9995fe..e3d064a 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,20 @@ 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 and truncated for compact display. */ +export function recordDispatchOutcome(workerId: string, status: string, detail?: string | null): void { + const now = nowIso(); + const d = 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 +821,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 +856,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.ts b/src/driver-loop.ts index 33fe9ab..19ab6e1 100644 --- a/src/driver-loop.ts +++ b/src/driver-loop.ts @@ -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. @@ -246,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 @@ -287,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/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/worker-heartbeat.test.ts b/src/worker-heartbeat.test.ts index 879e5ca..f5e12b1 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,31 @@ 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("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); + }); }); From 1b6d5ad872917c6f48b2ade02725ddd7046e0190 Mon Sep 17 00:00:00 2001 From: Joshua Gilbert <54961107+Joshua-Gilbert@users.noreply.github.com> Date: Thu, 9 Jul 2026 12:45:42 -0700 Subject: [PATCH 3/3] fix: worker last_dispatch coverage, review-parse noise, webhook hardening - The standalone CLI worker never stamped last_dispatch, so the new board_status health signal stayed null on the documented dispatch path. runOne now stamps every outcome (and main's catch stamps error), mirroring the 2.0 loop's finalize(). - recordDispatchOutcome stores detail only for a non-completed status: on success the caller's text is Bob's last assistant message, and task content must not leak into board_status. - parseReviewFindings skips a ### section with no severity and no Location/Category field: the 2.0 path feeds the whole transcript, where a reasoning turn's headed markdown became phantom info findings on the board note. - Webhook: the overflow warning re-arms when capacity returns (below cap, not zero, which a busy-but-alive endpoint may never reach); the payload carries worker.run (the heartbeat id) so seq scopes per process run and a restart isn't read as data loss; post() survives a synchronously-throwing fetch; --webhook-secret without --webhook fails loud instead of silently not signing. - CHANGELOG: --webhook moved out of the published 2.1.0 notes (it merged after the npm publish and is not in that package) into a new Unreleased section, alongside the last_dispatch and 2.0 review-capture entries. --- CHANGELOG.md | 35 ++++++++++++++++++++++++++++------- README.md | 3 ++- src/db.ts | 5 +++-- src/driver-loop.test.ts | 17 +++++++++++++++++ src/review-findings.test.ts | 20 ++++++++++++++++++++ src/review-findings.ts | 11 +++++++++-- src/webhook.test.ts | 31 +++++++++++++++++++++++++++++-- src/webhook.ts | 23 ++++++++++++++++++----- src/worker-heartbeat.test.ts | 9 +++++++++ src/worker.ts | 29 ++++++++++++++++++++++++----- 10 files changed, 159 insertions(+), 24 deletions(-) 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/db.ts b/src/db.ts index e3d064a..bc06f38 100644 --- a/src/db.ts +++ b/src/db.ts @@ -667,10 +667,11 @@ export function recordWorkerHeartbeat(workerId: string, meta: HeartbeatMeta = {} /** 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 and truncated for compact display. */ + * (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 = detail ? detail.replace(/\s+/g, " ").trim().slice(0, 160) || null : null; + 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 = ?", 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/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/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 f5e12b1..5ecfb61 100644 --- a/src/worker-heartbeat.test.ts +++ b/src/worker-heartbeat.test.ts @@ -124,6 +124,15 @@ describe("worker heartbeat liveness", () => { 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" }); diff --git a/src/worker.ts b/src/worker.ts index 4d7c8c7..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); @@ -995,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. @@ -1014,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) { @@ -1317,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.