diff --git a/src/main/services/agent-goal-tools.ts b/src/main/services/agent-goal-tools.ts index 1c6f0e41..5a984d14 100644 --- a/src/main/services/agent-goal-tools.ts +++ b/src/main/services/agent-goal-tools.ts @@ -11,7 +11,7 @@ import type { ProviderService } from "./provider-service"; import { unreadableArguments, type AgentTurnMessage, type ToolCall, type ToolDefinition } from "./provider-tool-turn"; import { MAX_TOOL_CALLS_PER_TURN, throwIfRunStopped, untilStopped } from "./run-control"; import { heldWorkTitles } from "./held-deliverables"; -import { goalAuditor, resultPointers, type GoalAuditor, type ResultPointers } from "./goal-audit-inputs"; +import { AuditBasis, goalAuditor, resultPointers, unfinishedGoalTasks, type GoalAuditor, type ResultPointers } from "./goal-audit-inputs"; import { DOCUMENT_TOOLS, listDocumentsTool, readDocumentTool } from "./agent-document-tools"; const AUDIT_MAX_ITERATIONS = 6; @@ -84,16 +84,28 @@ export async function completeGoalTool(args: GoalCompletionArgs): Promise 0) return heldGoalRefusal(deps, goal); + const unfinished = unfinishedGoalTasks(deps.db, goal.id, ctx.taskId); + if (unfinished.length > 0) return unfinishedRefusal(goal, unfinished); const pointers = resultPointers(deps.db, ctx, call.arguments); const auditor = goalAuditor(deps.db, ctx, args.providerId); + const basis = new AuditBasis(deps.db, goal.id, ctx.workspace?.localPath ?? null, pointers); - const verdict = await auditGoalCompletion(args, goal, pointers, auditor); + const verdict = await auditGoalCompletion(args, goal, pointers, auditor, basis); if (!verdict.approved) { return `The independent audit rejected closing the goal: ${verdict.reason} The goal stays active — close that gap or create the tasks that will.`; } throwIfRunStopped(); - // Work held for review while the audit ran keeps the goal open too. + // Work held for review, tasks opened, or the goal or result changed while the audit ran: the verdict no longer covers what exists. if (heldWorkTitles(deps.db, { goalId: goal.id }).length > 0) return heldGoalRefusal(deps, goal); + const openedMeanwhile = unfinishedGoalTasks(deps.db, goal.id, ctx.taskId); + if (openedMeanwhile.length > 0) return unfinishedRefusal(goal, openedMeanwhile); + const edited = basis.editedInCopy(); + if (edited.length > 0) { + return `ERROR: The auditor's commands changed ${edited.slice(0, 5).join(", ")} in its copy of the workspace, so its verdict is not about the real work; call complete_goal again.`; + } + if (basis.changed()) { + return "ERROR: The goal or the result it points to changed during the audit; call complete_goal again so the audit judges what exists now."; + } deps.db.prepare("update goals set status = 'achieved', stalled_reason = null, updated_at = ? where id = ? and org_id = ?") .run(new Date().toISOString(), goal.id, ctx.orgId); new AuditService(deps.db).record({ @@ -111,6 +123,10 @@ export async function completeGoalTool(args: GoalCompletionArgs): Promise { const workspace = args.ctx.workspace; // With the operator's file-access approval the auditor may build and test, but only in a throwaway copy. const scratch = workspace?.writeAccess ? copyWorkspace(workspace.localPath) : null; try { + if (scratch) basis.auditInCopy(scratch); const auditWorkspace = workspace ? { ...workspace, localPath: scratch ?? workspace.localPath, writeAccess: scratch !== null } : null; - return await runAudit(args, goal, pointers, auditor, auditWorkspace); + return await runAudit(args, goal, pointers, auditor, basis, auditWorkspace); } finally { - if (scratch) rmSync(scratch, { recursive: true, force: true }); + if (scratch) { + basis.closeCopy(); + rmSync(scratch, { recursive: true, force: true }); + } } } @@ -155,6 +176,7 @@ async function runAudit( goal: { title: string; description: string }, pointers: ResultPointers, auditor: GoalAuditor, + basis: AuditBasis, workspace: AgentToolContext["workspace"], ): Promise { const canRun = workspace?.writeAccess === true; @@ -175,7 +197,7 @@ async function runAudit( "The executor points to these as the result. Verify them yourself:", `Files: ${pointers.files.length > 0 ? pointers.files.join(", ") : "none"}`, `Documents: ${pointers.documents.length > 0 ? pointers.documents.map((doc) => `${doc.title} (doc_id=${doc.id})`).join(", ") : "none"}`, - `Inspect the work before deciding: list_files and read_file for the workspace, list_documents and read_document for the library. An approval counts only after you have read some of it. ${canRun ? "run_command runs in a throwaway copy of the workspace: build and run the tests yourself instead of trusting claims that they pass; nothing you do there reaches the real workspace." : "You cannot run commands here, so a claim that something was run or passes is unverified unless the workspace holds its results."} Judge outcomes, not artifacts about outcomes: plans, specs, status reports, and skeletons do not make a goal achieved; working deliverables do. If the goal promises software, look for the actual implementation, not documents describing it.`, + `Inspect the work before deciding: list_files and read_file for the workspace, list_documents and read_document for the library. An approval counts only after you have read some of it. ${canRun ? "run_command runs in a throwaway copy of the workspace: build and run the tests yourself instead of trusting claims that they pass; nothing you do there reaches the real workspace. Never edit the work there: if your commands change any file that was already in the workspace, the verdict is void." : "You cannot run commands here, so a claim that something was run or passes is unverified unless the workspace holds its results."} Judge outcomes, not artifacts about outcomes: plans, specs, status reports, and skeletons do not make a goal achieved; working deliverables do. If the goal promises software, look for the actual implementation, not documents describing it.`, "Keep your reasoning apart from your decision. End your final message with the decision alone on its last line: by itself if the goal is genuinely achieved, or followed by one short sentence naming the decisive gap. Use no marker anywhere else.", ].join("\n\n"), }]; @@ -207,7 +229,11 @@ async function runAudit( const output = index >= MAX_TOOL_CALLS_PER_TURN ? `ERROR: not run: this turn asked for ${turn.toolCalls.length} tool calls; the limit is ${MAX_TOOL_CALLS_PER_TURN}.` : toolCall.argumentsError ? unreadableArguments(toolCall) : await runAuditTool(args.deps, auditCtx, toolCall); - if (readsEvidence(toolCall) && !output.startsWith("ERROR")) evidenceRead = true; + if (readsEvidence(toolCall) && !output.startsWith("ERROR")) { + evidenceRead = true; + if (toolCall.name === "read_file") basis.noteFile(String(toolCall.arguments.path)); + else basis.noteDocument(String(toolCall.arguments.doc_id)); + } results.push({ toolCallId: toolCall.id, toolName: toolCall.name, output }); } messages.push({ role: "tool", results }); diff --git a/src/main/services/agent-workspace-tools.ts b/src/main/services/agent-workspace-tools.ts index 949e25f0..bb874d3a 100644 --- a/src/main/services/agent-workspace-tools.ts +++ b/src/main/services/agent-workspace-tools.ts @@ -9,7 +9,7 @@ import { PAGE_TOOL_PROPERTIES, pageText, readPageArgs, type PageRequest } from " const LIST_FILES_LIMIT = 400; /** Dependency and VCS internals: huge, machine-managed, never what an agent is looking for. */ -const LIST_SKIPPED_DIRECTORIES = new Set([".git", "node_modules", ".pnpm-store", "out", "dist"]); +export const LIST_SKIPPED_DIRECTORIES = new Set([".git", "node_modules", ".pnpm-store", "out", "dist"]); /** * The workspace tools turn agents from document writers into workers: they can diff --git a/src/main/services/goal-audit-inputs.ts b/src/main/services/goal-audit-inputs.ts index 02d07308..63a406cc 100644 --- a/src/main/services/goal-audit-inputs.ts +++ b/src/main/services/goal-audit-inputs.ts @@ -1,6 +1,9 @@ -import { existsSync } from "node:fs"; +import { createHash } from "node:crypto"; +import { existsSync, readdirSync, readFileSync, realpathSync } from "node:fs"; +import { join, relative, sep } from "node:path"; import type { DatabaseSync } from "node:sqlite"; import { optionalStringArray } from "./agent-tool-args"; +import { LIST_SKIPPED_DIRECTORIES } from "./agent-workspace-tools"; import { inputBudgetTokens } from "./context-budget"; import type { AgentToolContext } from "./agent-tools"; import { resolveWorkspacePath } from "./files/workspace-path"; @@ -9,6 +12,7 @@ import { currentRunId } from "./run-control"; import { effectiveModel } from "./task-run-preparation"; const AUDITOR_SETTING = "Settings > Advanced > Goal audit"; +const UNFINISHED_SHOWN = 5; export interface GoalAuditor { providerId: string; @@ -53,6 +57,116 @@ export function setGoalAuditor(db: DatabaseSync, input: { orgId: string; provide console.info(`[goal-audit] org ${input.orgId} audits goals with ${model} on ${input.providerId}`); } +/** Unfinished work of a goal besides the task asking to close it; bookkeeping tasks (chat wakes, reviews) do not count. */ +export function unfinishedGoalTasks(db: DatabaseSync, goalId: string, exceptTaskId: string | null): string[] { + const rows = db.prepare(` + select title from tasks where goal_id = ? and status <> 'done' and origin is null and (? is null or id <> ?) + order by rowid limit ? + `).all(goalId, exceptTaskId, exceptTaskId, UNFINISHED_SHOWN) as Array<{ title: string }>; + return rows.map((row) => row.title); +} + +/** What a verdict rests on, each stamped as it entered the audit: the goal, the pointers, and what the auditor read in the real workspace. */ +export class AuditBasis { + private readonly stamps = new Map(); + /** The audit's copy and its file hashes taken before any command ran, i.e. the workspace as the audit found it. */ + private copy: { root: string; atStart: Map } | null = null; + /** Files that existed at the start and that the auditor's own commands changed or removed in the copy. */ + private editedFiles: string[] = []; + + constructor(private readonly db: DatabaseSync, private readonly goalId: string, private readonly workspaceRoot: string | null, pointers: ResultPointers) { + this.note("goal:"); + for (const file of pointers.files) this.noteFile(file); + for (const document of pointers.documents) this.noteDocument(document.id); + } + + /** When the auditor works in a copy, a read file is stamped as the copy held it at the start: what the auditor saw. */ + auditInCopy(copyRoot: string): void { + this.copy = { root: copyRoot, atStart: hashTree(copyRoot) }; + } + + noteFile(path: string): void { + if (!this.workspaceRoot) return; + const relative = canonicalRelative(this.copy?.root ?? this.workspaceRoot, path.trim()); + const key = `file:${relative}`; + if (this.stamps.has(key)) return; + this.stamps.set(key, this.copy ? this.copy.atStart.get(relative) ?? this.absentStamp(relative) : this.stamp(key)); + } + + /** Called before the copy is deleted: any file the audit found that its own commands changed means it judged its own edits. */ + closeCopy(): void { + if (!this.copy) return; + const now = hashTree(this.copy.root); + this.editedFiles = [...this.copy.atStart].filter(([path, hash]) => now.get(path) !== hash).map(([path]) => path); + } + + editedInCopy(): string[] { + return this.editedFiles; + } + + noteDocument(id: string): void { + this.note(`doc:${id.trim()}`); + } + + /** Whether anything the verdict rests on is different now. */ + changed(): boolean { + return [...this.stamps].some(([key, stamp]) => this.stamp(key) !== stamp); + } + + private note(key: string): void { + if (!this.stamps.has(key)) this.stamps.set(key, this.stamp(key)); + } + + private stamp(key: string): string { + if (key === "goal:") return JSON.stringify(this.db.prepare("select title, description, status from goals where id = ?").get(this.goalId)); + if (key.startsWith("doc:")) { + const row = this.db.prepare("select version from documents where id = ?").get(key.slice(4)) as { version: number } | undefined; + return row ? `v${row.version}` : "missing"; + } + const relative = key.slice(5); + if (inSkippedDirectory(relative)) return "untracked"; + const absolute = resolveWorkspacePath(this.workspaceRoot!, relative).absolute; + return existsSync(absolute) ? hashFile(absolute) : "missing"; + } + + private absentStamp(relative: string): string { + return inSkippedDirectory(relative) ? "untracked" : "missing"; + } +} + +/** A path as the disk spells it, so differently cased reads of one file on a case-insensitive disk share one stamp. */ +function canonicalRelative(root: string, path: string): string { + const { absolute, relativePosix } = resolveWorkspacePath(root, path); + if (!existsSync(absolute)) return relativePosix; + const actual = relative(realpathSync.native(root), realpathSync.native(absolute)).split(sep).join("/"); + return actual.startsWith("..") ? relativePosix : actual; +} + +/** Build and dependency folders are not evidence of record; they are left out of stamps, as list_files leaves them out. */ +function inSkippedDirectory(relative: string): boolean { + return relative.split("/").slice(0, -1).some((part) => LIST_SKIPPED_DIRECTORIES.has(part)); +} + +function hashFile(absolute: string): string { + return createHash("sha256").update(readFileSync(absolute)).digest("hex"); +} + +function hashTree(root: string): Map { + const hashes = new Map(); + const walk = (directory: string, prefix: string): void => { + for (const entry of readdirSync(directory, { withFileTypes: true })) { + const relative = prefix ? `${prefix}/${entry.name}` : entry.name; + if (entry.isDirectory()) { + if (!LIST_SKIPPED_DIRECTORIES.has(entry.name)) walk(join(directory, entry.name), relative); + } else if (entry.isFile()) { + hashes.set(relative, hashFile(join(directory, entry.name))); + } + } + }; + walk(root, ""); + return hashes; +} + /** Checks the executor's pointers before any audit spend: every file in the workspace and every document must exist. */ export function resultPointers(db: DatabaseSync, ctx: AgentToolContext, args: JsonRecord): ResultPointers { const files = optionalStringArray(args.files, "files"); diff --git a/tests/unit/agent-goal-tools.test.ts b/tests/unit/agent-goal-tools.test.ts index 7e38a75b..df149254 100644 --- a/tests/unit/agent-goal-tools.test.ts +++ b/tests/unit/agent-goal-tools.test.ts @@ -61,6 +61,8 @@ function createHarness(): Harness { }); goalId = organization.createGoal({ orgId, title: "Ship it", description: "For real.", ownerAgentId: ceoId }); connection.db.prepare("update organizations set audit_provider_id = ?, audit_model_id = 'gpt-5.4-mini' where id = ?").run(providerId, orgId); + // The goal's kickoff work is finished, so only what a test adds can hold the goal open. + connection.db.prepare("update tasks set status = 'done' where goal_id = ?").run(goalId); }); return { connection, deps: { db: connection.db, organization, chat: new ChatService(connection.db) }, orgId, ceoId, staffId, goalId, providerId }; } @@ -313,6 +315,96 @@ describe("complete_goal with independent audit", () => { h.connection.close(); }); + test("a verdict after the auditor's commands changed the work voids itself; a differently cased read is the same file", async () => { + for (const scenario of ["repaired in the copy", "read in other case"] as const) { + const h = createHarness(); + writeFileSync(join(workspaceRoot, "src", "index.ts"), "export const app = () => 'running';\n"); + writeFileSync(join(workspaceRoot, "Case-Fixture.txt"), "same"); + let calls = 0; + const auditor = async (): Promise => { + calls += 1; + if (calls === 1) { + const path = scenario === "repaired in the copy" ? "src/index.ts" : "case-fixture.txt"; + return turnOf("Reading.", [{ id: "c1", name: "read_file", arguments: { path } }]); + } + // Read first, then "repair" and test in the copy without reading again. + if (calls === 2 && scenario === "repaired in the copy") { + return turnOf("Fixing.", [{ id: "c2", name: "run_command", arguments: { command: "node -e \"require('fs').writeFileSync('src/index.ts', 'fixed')\"" } }]); + } + return turnOf(""); + }; + const files = scenario === "repaired in the copy" ? ["src/index.ts"] : ["Case-Fixture.txt"]; + const output = await completeGoalTool(argsFor(h, h.ceoId, auditor, call({ files }))); + const caseInsensitive = existsSync(join(workspaceRoot, "CASE-FIXTURE.TXT")); + const expected = scenario === "repaired in the copy" + ? "ERROR: The auditor's commands changed src/index.ts in its copy of the workspace, so its verdict is not about the real work; call complete_goal again." + : expect.stringContaining(caseInsensitive ? "achieved" : "audit rejected"); + expect([scenario, output]).toEqual([scenario, expected]); + h.connection.close(); + } + }); + + test("a goal closes only when its other tasks are done", async () => { + const h = createHarness(); + const task = (title: string) => h.connection.transaction(() => h.deps.organization.createTask({ + orgId: h.orgId, title, description: "d", status: "in_progress", assigneeAgentId: h.staffId, risk: "low", + projectId: null, goalId: h.goalId, workspaceId: null, priority: "normal", requiresFilesystem: false, + })); + const calling = task("Close the goal"); + task("Write the docs"); + let calls = 0; + const auditor = async (): Promise => { + calls += 1; + return calls === 1 ? turnOf("Reading.", [{ id: "c1", name: "read_file", arguments: { path: "src/index.ts" } }]) : turnOf(""); + }; + const args = argsFor(h, h.ceoId, auditor, call({ files: ["src/index.ts"] })); + args.ctx.taskId = calling; + expect(await completeGoalTool(args)).toBe('ERROR: Goal "Ship it" still has unfinished tasks (Write the docs); finish or close them first.'); + expect(calls).toBe(0); + h.connection.db.prepare("update tasks set status = 'done' where title = 'Write the docs'").run(); + expect(await completeGoalTool(args)).toContain("achieved"); + h.connection.close(); + }); + + test("a goal or result that changes during the audit is judged again, not closed on the old verdict", async () => { + for (const change of ["file", "goal", "task", "read file", "read document", "before read"] as const) { + const h = createHarness(); + writeFileSync(join(workspaceRoot, "result.md"), "v1"); + writeFileSync(join(workspaceRoot, "src", "index.ts"), "export const app = () => 'running';\n"); + const notesId = h.connection.transaction(() => new DocumentService(h.connection.db).create({ orgId: h.orgId, title: "Notes", docType: "report", content: "n", authorAgentId: h.ceoId, reviewerAgentId: null, projectId: null, goalId: h.goalId, taskId: null })); + let calls = 0; + const auditor = async (): Promise => { + calls += 1; + if (calls === 1) { + // The audit's copy was taken before this turn; a change now is one the auditor never sees. + if (change === "before read") writeFileSync(join(workspaceRoot, "src", "index.ts"), "export const app = () => 'changed';\n"); + return turnOf("Reading.", [ + { id: "c1", name: "read_file", arguments: { path: "result.md" } }, + { id: "c2", name: "read_file", arguments: { path: "./src/index.ts" } }, + { id: "c3", name: "read_document", arguments: { doc_id: ` ${notesId} ` } }, + ]); + } + if (change === "file") writeFileSync(join(workspaceRoot, "result.md"), "v2"); + if (change === "read file") writeFileSync(join(workspaceRoot, "src", "index.ts"), "export const app = () => 'changed';\n"); + if (change === "read document") h.connection.db.prepare("update documents set version = version + 1 where id = ?").run(notesId); + if (change === "goal") h.connection.db.prepare("update goals set description = 'Bigger now.' where id = ?").run(h.goalId); + if (change === "task") { + h.connection.transaction(() => h.deps.organization.createTask({ + orgId: h.orgId, title: "Late addition", description: "d", status: "backlog", assigneeAgentId: h.staffId, risk: "low", + projectId: null, goalId: h.goalId, workspaceId: null, priority: "normal", requiresFilesystem: false, + })); + } + return turnOf(""); + }; + const output = await completeGoalTool(argsFor(h, h.ceoId, auditor, call({ files: ["result.md"] }))); + expect([change, output]).toEqual([change, change === "task" + ? 'ERROR: Goal "Ship it" still has unfinished tasks (Late addition); finish or close them first.' + : "ERROR: The goal or the result it points to changed during the audit; call complete_goal again so the audit judges what exists now."]); + expect(goalStatus(h)).toBe("active"); + h.connection.close(); + } + }); + test("no explicit approval means not achieved", async () => { const h = createHarness(); const auditor = async (): Promise => turnOf("Looks plausible I guess."); @@ -449,8 +541,9 @@ describe("company_status", () => { // createGoal materializes a kickoff task, so the goal starts with one // backlog item of real work before the two tasks this test adds. const output = dispatch(h, h.ceoId, "company_status", {}); - expect(output).toContain(`[active] "Ship it" (goal_id=${h.goalId}, owner: CEO) — 1 backlog, 1 in flight, 1 done`); - expect(output).toContain("Real work tasks: 3 total — 1 backlog, 1 in progress, 0 in review, 1 done"); + // The harness finished the goal's kickoff task, so it counts as done. + expect(output).toContain(`[active] "Ship it" (goal_id=${h.goalId}, owner: CEO) — 0 backlog, 1 in flight, 2 done`); + expect(output).toContain("Real work tasks: 3 total — 0 backlog, 1 in progress, 0 in review, 2 done"); expect(output).toContain(`"Build the ingestion pipeline" — in_progress, Dev`); expect(output).toContain(`"Write the launch post"`); expect(output).not.toContain("Reply to CEO");