From e4e89db554edfa93e685c798435cd68e6f01b5e7 Mon Sep 17 00:00:00 2001 From: TimMikeladze Date: Tue, 1 Sep 2026 12:52:56 -0700 Subject: [PATCH] fix(dispatch): close the report out on the run's outcome MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A report moved to "dispatched" when its run started and never moved again, so one that had been handled and one that had been ignored looked the same, and dispatchAll skipped both forever. A clean exit is not the same as the work being done. Under the default permission "plan" the agent can only propose: it writes a plan, asks whether to proceed, and exits 0 having touched nothing, with no one there to answer. That was recorded as a plain success on a stuck report, which reads as devbar ignoring what you sent while the run still costs money. A finished run now resolves the report when the working tree changed, and reopens it as "new" otherwise, carrying a note that names plan mode as the reason when that is what happened. Reopening is not a retry: the finished task still guards its report, so nothing re-runs on its own. Deciding that from `git status --porcelain` alone was wrong, and wrong in the common case. Porcelain names which paths are dirty, not their content, so a file already modified before a run and edited again during it produced byte-identical output — a real edit read as a no-op, and the report was reopened after being handled. gitSnapshot now stamps each dirty path with its size and mtime and the comparison is per file. That also fixes changedFiles, which shared the blind spot and never reported an edit to an already-dirty file. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01Ph2Rn26we5JcGUhtQckrdQ --- docs/LOCAL-AGENT.md | 39 +++++++++++ src/server/dispatcher.ts | 81 +++++++++++++++++++++- src/server/local.ts | 33 +++++++-- test/dispatcher.test.ts | 143 +++++++++++++++++++++++++++++++++++++++ 4 files changed, 290 insertions(+), 6 deletions(-) diff --git a/docs/LOCAL-AGENT.md b/docs/LOCAL-AGENT.md index 0e4c4ea..a88afd1 100644 --- a/docs/LOCAL-AGENT.md +++ b/docs/LOCAL-AGENT.md @@ -107,6 +107,40 @@ cannot see. On disk, the agent reads the PNG. The agent is then run in the project directory with the prompt on stdin. +### What a finished run does to the report + +The report moves to `dispatched` when the run starts, and the run's outcome +decides where it lands: + +| Outcome | Report | +| ------------------------------------------ | ---------- | +| Exited 0 and the working tree changed | `resolved` | +| Exited 0 and the working tree is unchanged | `new` | +| Failed, cancelled or timed out | `new` | + +A clean exit is not the same as the work being done. Under the default +`permission: "plan"` the agent can only propose: it writes a plan, asks whether +to proceed, and exits 0 having touched nothing — with no one there to answer. +That used to be recorded as a plain success on a report stuck at `dispatched`, +which reads as devbar ignoring the report while the run still costs money. Now +the run carries a `note` saying what happened, and the report goes back to +`new`, where it is visibly still waiting: + +``` +[devbar] the agent changed nothing: this project dispatches with permission +"plan", which can only propose. Set `permission: "auto"` in devbar.config.ts to +let a dispatch apply its own fix. +``` + +The no-op is only claimed when git says so — a working tree naming exactly the +files it named before the run. An agent that commits its work leaves a tree that +differs, and a project directory that is not a git repository cannot be judged +at all; neither is reported as a no-op. + +Reopening is not a retry. The finished task still guards its report, so +`dispatchAll` will not pick it up again on its own — `devbar dispatch ` +still will. + ### Supported agents | | `claude` | `codex` | `opencode` | @@ -244,6 +278,11 @@ after Submit offers **Dispatch** for that one report; otherwise turn it on, use `devbar dispatch`, open the toolbar's Agent tab, or let an agent pull with `claim_report`. +**The run went green but nothing changed.** `permission` is `plan`, the +default, which lets the agent propose but not edit. The run's `note` says so and +the report returns to `new`. Set `permission: "auto"` to let a dispatch apply its +own fix — that lets an agent write to the project directory unattended. + **"No project matched this report".** The page's origin is not in any project's `origins`, and more than one project is registered. Add the origin, or pass `project`. diff --git a/src/server/dispatcher.ts b/src/server/dispatcher.ts index ff079ce..4e26069 100644 --- a/src/server/dispatcher.ts +++ b/src/server/dispatcher.ts @@ -20,6 +20,17 @@ export type Task = { result?: DispatchResult; }; +/** + * The dirty paths in a working tree, each mapped to a stamp that moves when the + * file's content does. + * + * A bare path list cannot answer the only question that matters here. Porcelain + * names *which* files are dirty, so a file already modified before a run and + * edited again during it produces byte-identical output — the run reads as + * having touched nothing. + */ +export type GitSnapshot = Record; + export type DispatchResult = { taskId: string; exitCode: number; @@ -31,6 +42,12 @@ export type DispatchResult = { /** Files the agent changed, when the project directory is a git repo. */ changedFiles?: string[]; interrupted?: boolean; + /** + * Why a run that exited cleanly still left the report open — almost always + * plan mode, where the agent proposes and waits for an answer nobody is + * there to give. Absent when the run changed something. + */ + note?: string; }; /** What subscribers (the SSE bus, the CLI) see as a run unfolds. */ @@ -47,7 +64,7 @@ export type DispatcherOptions = { /** Overrides every project's agent command. Tests pass "echo". */ command?: string; /** Captures git state around a run. Injectable for tests. */ - gitSnapshot?: (dir: string) => Promise; + gitSnapshot?: (dir: string) => Promise; now?: () => number; }; @@ -76,6 +93,19 @@ function formatDuration(ms: number): string { return `${Math.floor(s / 60)}m${s % 60}s`; } +/** Paths whose stamp moved between two snapshots, plus any that went clean. */ +function changedBetween(before: GitSnapshot, after: GitSnapshot): string[] { + const changed = new Set(); + for (const [path, stamp] of Object.entries(after)) { + if (before[path] !== stamp) changed.add(path); + } + // A file the agent reverted leaves the dirty set; that is a change too. + for (const path of Object.keys(before)) { + if (!(path in after)) changed.add(path); + } + return [...changed].sort(); +} + function normalizePermission(project: ProjectConfig): AgentPermission { const raw = project.permission ?? project.permissionMode; switch (raw) { @@ -330,7 +360,9 @@ export function createDispatcher(options: DispatcherOptions): Dispatcher { const afterGit = options.gitSnapshot ? await options.gitSnapshot(project.dir) : undefined; const changedFiles = - beforeGit && afterGit ? afterGit.filter((f) => !beforeGit.includes(f)) : afterGit; + beforeGit && afterGit + ? changedBetween(beforeGit, afterGit) + : afterGit && Object.keys(afterGit).sort(); const completedAt = now(); let output = chunks.join(""); @@ -347,6 +379,30 @@ export function createDispatcher(options: DispatcherOptions): Dispatcher { ? "completed" : "failed"; + // A clean exit is not the same as the work being done. In plan mode the + // agent can only propose — it writes a plan, asks "shall I proceed?", and + // exits 0 having touched nothing. That was reported as a plain success, + // which reads as devbar ignoring the report, and the run still costs money. + // + // Only claim a no-op when git actually said so: not one dirty file's + // content moved. Without git we cannot tell, and that is not a no-op + // either — it is an unknown, and the report should not be reopened on it. + const touchedNothing = + beforeGit !== undefined && afterGit !== undefined && changedFiles?.length === 0; + const applied = status === "completed" && !touchedNothing; + const note = + status === "completed" && touchedNothing + ? normalizePermission(project) === "plan" + ? 'the agent changed nothing: this project dispatches with permission "plan", which can only propose. Set `permission: "auto"` in devbar.config.ts to let a dispatch apply its own fix.' + : "the agent changed nothing." + : undefined; + + if (note) { + console.log(`[dispatch] ${note}`); + recordEvent(task.id, { type: "stdout", text: `[devbar] ${note}\n` }); + output = `${output}[devbar] ${note}\n`; + } + const result: DispatchResult = { taskId: task.id, exitCode, @@ -356,10 +412,31 @@ export function createDispatcher(options: DispatcherOptions): Dispatcher { ...(costUsd !== undefined ? { costUsd } : {}), ...(sessionId ? { sessionId } : {}), ...(changedFiles && changedFiles.length > 0 ? { changedFiles } : {}), + ...(note ? { note } : {}), }; update(task, { status, completedAt, result, ...(sessionId ? { sessionId } : {}) }); + // Close the report out on the way past. It was moved to "dispatched" when + // the run started; leaving it there forever claims someone dealt with it + // and hides it from `dispatchAll`, so only a run that actually changed + // files resolves it. Everything else goes back to "new", where it is + // visibly still waiting and a later dispatch will pick it up again. + if (applied) { + const changed = changedFiles?.length + ? `Changed ${changedFiles.length} file(s): ${changedFiles.join(", ")}` + : "The project directory is not a git repository, so what it changed is unverified"; + await options.store + .resolve(report.id, { + summary: `Dispatched to ${preset.name} (${project.model}). ${changed}.`, + resolvedAt: completedAt, + by: `dispatch:${task.id}`, + }) + .catch(() => undefined); + } else { + await options.store.setStatus(report.id, "new").catch(() => undefined); + } + console.log( `[dispatch] task ${task.id.slice(0, 8)} ${status} in ${formatDuration(result.durationMs)}` + (errorMessage ? ` — ${errorMessage}` : ""), diff --git a/src/server/local.ts b/src/server/local.ts index bc9c780..29fcf63 100644 --- a/src/server/local.ts +++ b/src/server/local.ts @@ -3,7 +3,7 @@ import { mkdir, readFile } from "node:fs/promises"; import { join } from "node:path"; import { homedir } from "node:os"; import { createRegistry, type Registry, type ProjectConfig } from "./registry"; -import { createDispatcher, type Dispatcher } from "./dispatcher"; +import { createDispatcher, type Dispatcher, type GitSnapshot } from "./dispatcher"; import { createReportStore, type ReportStore } from "./report-store"; import { createPageBus, PageRpcError, type PageBus } from "./page-bus"; import { createMcpSessions, type McpSessions } from "./mcp-sessions"; @@ -777,10 +777,17 @@ export async function createLocalServer(options: LocalServerOptions = {}): Promi }; } -/** Files with uncommitted changes, so a run can report what it touched. */ -async function gitSnapshot(dir: string): Promise { +/** + * Files with uncommitted changes, each stamped with size and mtime, so a run + * can report what it touched. + * + * The stamp is what makes an already-dirty file legible: porcelain names the + * same path before and after a run that edited it again, so comparing path + * lists alone would call that run a no-op. + */ +async function gitSnapshot(dir: string): Promise { const { spawn } = await import("node:child_process"); - return new Promise((resolve) => { + const paths = await new Promise((resolve) => { try { const child = spawn("git", ["status", "--porcelain"], { cwd: dir, @@ -804,4 +811,22 @@ async function gitSnapshot(dir: string): Promise { resolve(undefined); } }); + if (!paths) return undefined; + + const { stat } = await import("node:fs/promises"); + const { join } = await import("node:path"); + const snapshot: GitSnapshot = {}; + await Promise.all( + paths.map(async (path) => { + try { + const info = await stat(join(dir, path)); + snapshot[path] = `${info.size}:${info.mtimeMs}`; + } catch { + // Deleted, or a path porcelain rendered in a form we cannot stat + // (a rename arrow, a quoted name). Its presence is still the signal. + snapshot[path] = "absent"; + } + }), + ); + return snapshot; } diff --git a/test/dispatcher.test.ts b/test/dispatcher.test.ts index da3b7da..533aae3 100644 --- a/test/dispatcher.test.ts +++ b/test/dispatcher.test.ts @@ -4,6 +4,7 @@ import { buildPrompt, adoptPersistedTask, type Dispatcher, + type GitSnapshot, type Task, } from "../src/server/dispatcher"; import { createReportStore, type ReportStore } from "../src/server/report-store"; @@ -199,6 +200,148 @@ describe("dispatcher", () => { }); }); +describe("dispatcher closes the report out", () => { + let resultsDir: string; + let tasksDir: string; + let reportsDir: string; + let store: ReportStore; + + /** A working tree where each named file carries a content stamp. */ + function tree(...entries: [string, string][]): GitSnapshot { + return Object.fromEntries(entries); + } + + /** A dispatcher whose git snapshots are scripted, one call per invocation. */ + function withGit(snapshots: (GitSnapshot | undefined)[], project = PROJECT): Dispatcher { + let call = 0; + return createDispatcher({ + store, + resultsDir, + tasksDir, + getProject: (slug) => (slug === "test-app" ? project : undefined), + command: "echo", + gitSnapshot: async () => snapshots[call++], + }); + } + + beforeEach(async () => { + resultsDir = tmpDir(); + tasksDir = tmpDir(); + reportsDir = tmpDir(); + await mkdir(resultsDir, { recursive: true }); + await mkdir(tasksDir, { recursive: true }); + await mkdir(reportsDir, { recursive: true }); + store = createReportStore(reportsDir); + }); + + afterEach(async () => { + await Promise.all( + [resultsDir, tasksDir, reportsDir].map((d) => rm(d, { recursive: true, force: true })), + ); + }); + + test("a run that changed files resolves it, with what changed", async () => { + const dispatcher = withGit([tree(), tree(["src/hero.tsx", "12:100"])]); + const reportId = (await store.save({ prompt: "fix it" }, "test-app")).id; + + dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect((await store.get(reportId))?.status).toBe("resolved"); + const resolution = JSON.parse( + await readFile(join((await store.get(reportId))!.dir, "resolution.json"), "utf-8"), + ); + expect(resolution.summary).toContain("src/hero.tsx"); + }); + + test("a plan-mode run that changed nothing reopens it and says why", async () => { + // Identical snapshots: the agent proposed and waited for an answer. + const dispatcher = withGit([ + tree(["src/other.tsx", "40:100"]), + tree(["src/other.tsx", "40:100"]), + ]); + const reportId = (await store.save({ prompt: "fix it" }, "test-app")).id; + + const taskId = dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect(dispatcher.getTask(taskId)?.status).toBe("completed"); + expect(dispatcher.getTask(taskId)?.result?.note).toContain('permission: "auto"'); + // Back to "new", not left claiming someone dealt with it. + expect((await store.get(reportId))?.status).toBe("new"); + }); + + test("an auto-permission run that changed nothing says so without blaming plan mode", async () => { + const dispatcher = withGit([tree(), tree()], { ...PROJECT, permission: "auto" }); + const reportId = (await store.save({ prompt: "fix it" }, "test-app")).id; + + const taskId = dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect(dispatcher.getTask(taskId)?.result?.note).toBe("the agent changed nothing."); + expect((await store.get(reportId))?.status).toBe("new"); + }); + + test("editing an already-dirty file counts as a change", async () => { + // Porcelain names the same path before and after, so a path-list + // comparison calls this a no-op and reopens a report that was handled. + const dispatcher = withGit([ + tree(["src/hero.tsx", "40:100"]), + tree(["src/hero.tsx", "62:900"]), + ]); + const reportId = (await store.save({ prompt: "fix it" }, "test-app")).id; + + const taskId = dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect(dispatcher.getTask(taskId)?.result?.note).toBeUndefined(); + expect(dispatcher.getTask(taskId)?.result?.changedFiles).toEqual(["src/hero.tsx"]); + expect((await store.get(reportId))?.status).toBe("resolved"); + }); + + test("a file the agent reverted to clean counts as a change", async () => { + const dispatcher = withGit([tree(["src/hero.tsx", "40:100"]), tree()]); + const reportId = (await store.save({ prompt: "revert it" }, "test-app")).id; + + const taskId = dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect(dispatcher.getTask(taskId)?.result?.changedFiles).toEqual(["src/hero.tsx"]); + expect((await store.get(reportId))?.status).toBe("resolved"); + }); + + test("without git it resolves rather than stranding the report", async () => { + const dispatcher = withGit([undefined, undefined]); + const reportId = (await store.save({ prompt: "fix it" }, "test-app")).id; + + const taskId = dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect(dispatcher.getTask(taskId)?.result?.note).toBeUndefined(); + expect((await store.get(reportId))?.status).toBe("resolved"); + }); + + test("reopening does not re-run the report on its own", async () => { + // Reopening is so the report is visibly still waiting, not a retry loop: + // the completed task still guards it against another automatic dispatch. + const dispatcher = withGit([tree(["a", "1:1"]), tree(["a", "1:1"])]); + const reportId = (await store.save({ prompt: "fix it" }, "test-app")).id; + + dispatcher.enqueue(reportId, "test-app"); + await dispatcher.process(); + await dispatcher.drain(); + + expect((await store.get(reportId))?.status).toBe("new"); + expect(await dispatcher.dispatchAll("test-app")).toHaveLength(0); + }); +}); + describe("buildPrompt", () => { const report = { id: "r1",