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",