Keep malformed file edits from aborting agent turns

packages/workshop-backend/__tests__/agent-step-validation.test.ts

@@ -0,0 +1,106 @@
+import {env} from "cloudflare:workers";
+import {runInDurableObject} from "cloudflare:test";
+import {afterEach, describe, expect, it, vi} from "vitest";
+import {createFauxCore, fauxAssistantMessage, fauxText, fauxToolCall} from "@earendil-works/pi-ai";
+import type {OverseerDurableObject} from "../src/overseer";
+import {runAgent} from "../src/agent";
+
+declare module "cloudflare:workers" {
+  interface ProvidedEnv { TEST_OVERSEER: DurableObjectNamespace<OverseerDurableObject>; }
+}
+const USER = {type: "user" as const, id: "test-user", name: "Test"};
+const AUTHOR = {type: "agent" as const, id: "faux-model", name: "Faux"};
+const SECRET = "sentinel-file-content-must-not-be-logged";
+type Impl = OverseerDurableObject["impl"];
+async function inKernel(test: (impl: Impl) => Promise<void>) {
+  await runInDurableObject(env.TEST_OVERSEER.getByName(crypto.randomUUID()), async instance => {
+    const impl = instance.impl, timestamp = impl.getChatTimestamp();
+    impl.storage.chatMeta.put({id: 1, title: "Validation", started: timestamp, lastActive: timestamp});
+    impl.addChatMessages(1, USER, [{type: "message", message: "Create an app."}]);
+    impl.getInstanceInstructions = async () => "";
+    await test(impl);
+  });
+}
+function diagnostics(spy: ReturnType<typeof vi.spyOn>) {
+  return spy.mock.calls.map(([entry]) => entry as Record<string, unknown>)
+    .filter(entry => entry.event === "agent.step.change.invalid");
+}
+
+describe("agent transform validation boundaries", () => {
+  afterEach(() => vi.restoreAllMocks());
+
+  it("records an empty-filename tool error after creation and continues with a corrected write", () => inKernel(async impl => {
+    const warnings = vi.spyOn(console, "warn").mockImplementation(() => {});
+    const faux = createFauxCore({models: [{id: "faux-model"}]});
+    const tool = (name: string, args: object) => fauxAssistantMessage(fauxToolCall(name, args), {stopReason: "toolUse"});
+    faux.setResponses([
+      tool("createGadget", {title: "Board", bindingName: "BOARD"}),
+      tool("writeFile", {workpiece: "BOARD", filename: "", content: SECRET}),
+      tool("writeFile", {workpiece: "BOARD", filename: "server.js", content: "export default {};"}),
+      fauxAssistantMessage(fauxText("Done.")),
+    ]);
+    await runAgent(impl, {model: faux.getModel(), stream: faux.stream}, 1, AUTHOR,
+      new AbortController().signal, USER, {provider: "cloudflare", model: "faux-model", apiToken: ""});
+    const calls = [...impl.storage.chats.list()].flatMap(message => message.type === "message" ? message.toolCalls ?? [] : []);
+    const writes = calls.filter(call => call.toolName === "writeFile");
+    expect(writes).toHaveLength(2);
+    expect(writes[0].error).toMatch(/file path is empty/);
+    expect(writes[1].error).toBeUndefined();
+    const gadget = [...impl.storage.gadgets.list()].find(piece => piece.type === "gadget" && piece.bindingName === "BOARD")!;
+    const files = (await impl.getCurrentChatContent(1, impl.storage.chatMeta.get(1)!)).get(gadget.id)!;
+    expect(files.get("server.js")).toBe("export default {};");
+    expect(files.has("")).toBe(false);
+    const entries = diagnostics(warnings);
+    expect(entries).toHaveLength(1);
+    expect(entries[0]).toMatchObject({changeStage: "buffer", changeWorkpieceCount: 1, changeFileCount: 1, changeEmptyPathCount: 1});
+    expect(JSON.stringify(entries)).not.toContain(SECRET);
+  }));
+
+  it.each([
+    {name: "a rejected write", toolName: "writeFile", args: {filename: "", content: SECRET}, error: /file path is empty/},
+    {name: "an unmatched edit", toolName: "editFile", args: {filename: "server.js", textToReplace: "missing", replacement: "wrong"}, error: /No matching text/},
+    {name: "a no-op edit", toolName: "editFile", args: {filename: "server.js", textToReplace: "original", replacement: "original"}, error: undefined},
+  ])("preserves committed read knowledge after $name", ({toolName, args, error}) => inKernel(async impl => {
+    const commitId = await impl.gitStore.writeFilesAsCommit(new Map([
+      ["server.js", "original\n"], ["other.js", "untouched\n"],
+    ]), {parents: [], author: {name: "Test", email: "test@example.com"}, message: "Initial", timestamp: new Date(0)});
+    const gadget = impl.createGadget("Board", "BOARD", undefined, undefined, commitId);
+    const faux = createFauxCore({models: [{id: "faux-model"}]});
+    faux.setResponses([
+      fauxAssistantMessage([
+        fauxToolCall("readFile", {workpiece: "BOARD", filename: "server.js"}),
+        fauxToolCall("readFile", {workpiece: "BOARD", filename: "other.js"}),
+        fauxToolCall(toolName, {workpiece: "BOARD", ...args}),
+        fauxToolCall("editFile", {workpiece: "BOARD", filename: "server.js", textToReplace: "original", replacement: "updated"}),
+        fauxToolCall("editFile", {workpiece: "BOARD", filename: "other.js", textToReplace: "untouched", replacement: "also updated"}),
+      ], {stopReason: "toolUse"}),
+      fauxAssistantMessage(fauxText("Done.")),
+    ]);
+    await runAgent(impl, {model: faux.getModel(), stream: faux.stream}, 1, AUTHOR,
+      new AbortController().signal, USER, {provider: "cloudflare", model: "faux-model", apiToken: ""});
+    const calls = [...impl.storage.chats.list()].flatMap(message => message.type === "message" ? message.toolCalls ?? [] : []);
+    expect(calls).toHaveLength(5);
+    if (error) expect(calls[2].error).toMatch(error);
+    else expect(calls[2].error).toBeUndefined();
+    expect(calls[3].error).toBeUndefined();
+    expect(calls[4].error).toBeUndefined();
+    const files = (await impl.getCurrentChatContent(1, impl.storage.chatMeta.get(1)!)).get(gadget.id)!;
+    expect(files.get("server.js")).toBe("updated\n");
+    expect(files.get("other.js")).toBe("also updated\n");
+    expect(files.has("")).toBe(false);
+    expect(impl.getGadgetHead(gadget.id)).toBe(commitId);
+  }));
+
+  it("fails closed at persistence with bounded shape diagnostics but no file contents", () => inKernel(async impl => {
+    const errors = vi.spyOn(console, "error").mockImplementation(() => {});
+    await expect(impl.commitAgentStep(1, AUTHOR, [], {
+      changes: [{change: {0: [["", {set: SECRET}]]}}], createdGadgets: [], createdWorktrees: [],
+      addedBindings: [], worktreeCommits: [], blueprintMerges: [],
+    })).rejects.toThrow(/file path is empty/);
+    const entries = diagnostics(errors);
+    expect(entries).toHaveLength(1);
+    expect(entries[0]).toMatchObject({changeStage: "persist", changeIndex: 0, changeWorkpieceCount: 1, changeFileCount: 1, changeEmptyPathCount: 1});
+    expect(JSON.stringify(entries)).not.toContain(SECRET);
+    expect([...impl.storage.chatChanges.list()]).toEqual([]);
+  }));
+});

packages/workshop-backend/__tests__/blueprints.test.ts

@@ -1822,7 +1822,8 @@ describe("applying a blueprint to a gadget", () => {
       let [call] = [...impl.storage.chats.list({ prefix: `${keyString(1)}.` })]
           .flatMap(message => message.type === "message" ? message.toolCalls ?? [] : []);
       expect(call).toMatchObject({ toolName: "createGadget" });
-      expect(call.error).toMatch(/Too many code changes in one step/);
+      // Schema validation now enforces the same raised cap before the step-buffer check.
+      expect(call.error).toMatch(/code change is too large/);
 
       // The call still leaves the gadget it created, with no files. Nothing says it was made
       // from the release, so accepting it does not put the release in its history...

packages/workshop-backend/src/agent.ts

@@ -1,5 +1,5 @@
 import { AiChatMessage, AiChatAuthorInfo, AiToolCall, AiChatMessageBody, AiChatStreamEvent, AlwaysAvailableResource, BlueprintBinding, BlueprintMerge, BlueprintOutput, ChatGadgetPin, ChatGadgetPinRecord, MainlineMergeGadget, WorkpieceId, type AiModelConfig, isTextLikeAttachmentMimeType, validateBindingName } from '@gadgets/workshop-shared/api';
-import { applyCodeChange, codeChangeSerializedSize, MAX_CODE_CHANGE_SIZE, replaceSpanChange,
+import { applyCodeChange, codeChangeSerializedSize, MAX_CODE_CHANGE_SIZE, replaceSpanChange, validateCodeChangeSchema,
   type CodeContent, type CodeChange, type FileChange } from '@gadgets/workshop-shared/code-change';
 import { PDF_MIME_TYPE, modelApiSupportsPdfAttachments } from './chat-attachment-pdf';
 import { AgentCatalog, ObservationDescription } from '@gadgets/workshop-shared/gatekeeper';
@@ -42,6 +42,18 @@ import { formatGrep, type GrepScan } from "./grep";
 
 const logger = createWorkshopLogger("workshop.agent");
 
+/** Content-free counters for rejected transforms; diagnostics must contain no paths or text. */
+export function agentCodeChangeShape(change: CodeChange) {
+  const shape = {changeWorkpieceCount: Object.keys(change).length, changeFileCount: 0, changeEmptyPathCount: 0};
+  for (const entries of Object.values(change)) if (Array.isArray(entries)) {
+    for (const entry of entries) if (Array.isArray(entry)) {
+      ++shape.changeFileCount;
+      if (entry[0] === "") ++shape.changeEmptyPathCount;
+    }
+  }
+  return shape;
+}
+
 /**
  * How much text one tool result may put in front of the model. About 8k tokens: a handful of
  * results fit inside the compaction headroom of the smallest supported window. Each tool that can
@@ -1772,7 +1784,7 @@ async function runAgentPass(
   // changed (see applyReplayedPin). The one way knowledge goes stale is another chat's accept
   // moving an unpinned gadget's head, and a stamp is checked against the head at
   // the two points where it would otherwise be trusted: editFile's gate on an unpinned gadget,
-  // and the establishment of a pin (anchorKnowledgeToPin), after which reads are session-served
+  // and the establishment of a pin (knowledgeAtPin), after which reads are session-served
   // and the gate is skipped. At an epoch boundary session-tracking entries take an oid stamp of
   // their own (resetSessionEpoch). Deliberately not carried across a compaction boundary: a
   // read the summary swallowed no longer tells the agent what the text is, so re-reading is
@@ -1794,10 +1806,12 @@ async function runAgentPass(
   // pin at the same commit); idempotent, since a second call finds nothing stamped. Worktrees
   // need none of this: only this chat's accept moves the commit an unpinned worktree reads at,
   // and stamps carried across that accept are of content it committed, so a worktree's stamps
-  // always match its pin base.
-  let anchorKnowledgeToPin = async (gadgetId: WorkpieceId, baseCommit: string) => {
+  // always match its pin base. Prepare a copy: live tools install it only after the edit
+  // buffers successfully, so a rejected or no-op edit cannot settle reads without a pin.
+  let knowledgeAtPin = async (gadgetId: WorkpieceId, baseCommit: string) => {
     let files = filesRead.get(gadgetId);
     if (files === undefined) return;
+    files = new Map(files);
     for (let [filename, stamp] of files) {
       if (stamp === undefined) continue;
       if (await hooks.fileOidAtCommit(baseCommit, filename) === stamp) {
@@ -1806,6 +1820,7 @@ async function runAgentPass(
         files.delete(filename);
       }
     }
+    return files;
   };
 
   // Resolve a file tool's optional `workpiece` parameter -- the chat binding name of the target
@@ -1875,7 +1890,7 @@ async function runAgentPass(
   // editFile forces a re-read.
   //
   // A stamped entry normally carries over untouched: its workpiece was unpinned (a pinned
-  // gadget's stamps were settled by anchorKnowledgeToPin), so the session holds nothing of the
+  // gadget's stamps were settled by knowledgeAtPin), so the session holds nothing of the
   // file and the stamp is as true after the accept as before. The exception is a worktree,
   // whose stamps are never settled at its pin: a *user* edit of a file the model read unpinned
   // lands in the session content while the entry keeps the pre-edit stamp (the model's own
@@ -1940,7 +1955,8 @@ async function runAgentPass(
     sessionContent = new Map(sessionContent);
     sessionContent.set(pin.gadgetId, files);
     pinnedGadgets.add(pin.gadgetId);
-    await anchorKnowledgeToPin(pin.gadgetId, pin.baseCommit);
+    let knowledge = await knowledgeAtPin(pin.gadgetId, pin.baseCommit);
+    if (knowledge !== undefined) filesRead.set(pin.gadgetId, knowledge);
     return forgotten.toSorted();
   };
 
@@ -2812,6 +2828,13 @@ async function runAgentPass(
   let appendAgentEdit = (
       workpieceId: WorkpieceId, change: CodeChange,
       pin?: {baseCommit: string, baseFiles: Map<string, string>}) => {
+    try { validateCodeChangeSchema(change); }
+    catch (error) {
+      logger.warn("invalid agent transform rejected before buffering", {
+        event: "agent.step.change.invalid", chatId, changeStage: "buffer", ...agentCodeChangeShape(change),
+      });
+      throw error;
+    }
     // Bound the step's total: the barrier writes the buffer as exactly one "changes" message,
     // which must fit in one storage record, or the step's edits would be lost. Composition never
     // exceeds the sum of its inputs' sizes, so the summed size bounds the message's change.
@@ -3269,22 +3292,23 @@ async function runAgentPass(
           // The first write to an unpinned gadget with committed code pins it at the current
           // head (a whole-file overwrite is coherent against any base, so no read gate here --
           // but the gadget's *other* stamped reads are settled against that head now, see
-          // anchorKnowledgeToPin). Gadgets with no committed code stay unpinned; their content
+          // knowledgeAtPin). Gadgets with no committed code stay unpinned; their content
           // builds up from changes. (A worktree's first-write pin needs no declaration;
           // appendAgentEdit handles it.)
           let pin: {baseCommit: string, baseFiles: Map<string, string>} | undefined;
           if (!pinnedGadgets.has(resolved.workpieceId)) {
             let head = hooks.getGadgetHead(resolved.workpieceId);
             if (head !== undefined) {
               pin = {baseCommit: head, baseFiles: await hooks.readCommitFiles(head)};
-              await anchorKnowledgeToPin(resolved.workpieceId, head);
             }
           }
 
+          let knowledge = pin && await knowledgeAtPin(resolved.workpieceId, pin.baseCommit);
           // A whole-file write is a `set`: valid against any state, so replay and concurrent
           // transforms can never mis-anchor it.
           appendAgentEdit(resolved.workpieceId,
               {[resolved.workpieceId]: [[filename, {set: newContent}]]}, pin);
+          if (knowledge !== undefined) filesRead.set(resolved.workpieceId, knowledge);
 
           // The agent knows exactly what's in the file, so add it to the `filesRead` set so
           // that it can make further edits without rewriting.
@@ -3353,8 +3377,6 @@ async function runAgentPass(
               }
               if (!hooks.isWorktree(resolved.workpieceId)) {
                 pin = {baseCommit: head, baseFiles: await hooks.readCommitFiles(head)};
-                // This file passed the gate; the gadget's other stamped reads are settled here.
-                await anchorKnowledgeToPin(resolved.workpieceId, head);
               }
             }
           }
@@ -3375,8 +3397,10 @@ async function runAgentPass(
           let pos = findEditPos(before, textToReplace);
           if (replacement !== textToReplace) {
             let edit = replaceSpanChange(before.length, pos, textToReplace, replacement);
+            let knowledge = pin && await knowledgeAtPin(resolved.workpieceId, pin.baseCommit);
             appendAgentEdit(
                 resolved.workpieceId, {[resolved.workpieceId]: [[filename, {edit}]]}, pin);
+            if (knowledge !== undefined) filesRead.set(resolved.workpieceId, knowledge);
             // Like writeFile: the agent knows the file's exact resulting content, so the entry
             // becomes session knowledge (the gadget is pinned now, so a commit stamp -- which
             // predates this edit -- would be the wrong thing to carry forward).

packages/workshop-backend/src/observability.ts

@@ -9,6 +9,11 @@ export type WorkshopObservabilityFields = {
   blueprintId: string;
   callbackInitiated: boolean;
   chatId: number;
+  changeStage: "buffer" | "persist";
+  changeIndex: number;
+  changeWorkpieceCount: number;
+  changeFileCount: number;
+  changeEmptyPathCount: number;
   claimedType: string;
   commitCount: number;
   credentialKind: "workshop" | "cloudflare-access";

packages/workshop-backend/src/overseer.ts

@@ -44,7 +44,7 @@ import {
   getAiGatewayLogCost,
   type AiGatewayLogRoute,
 } from "./ai-gateway";
-import { AgentGadgetInfo, AgentHooks, SeedBindingInfo, alwaysAvailableResources, formatMissingBlueprintBindings, runAgent, summarizeArgs, type AgentStepChange, type AiChatMessageBodyWithModelData, type ChatHistory, type WorktreeTurnAccess, GIT_BINDING_NAME } from "./agent";
+import { AgentGadgetInfo, AgentHooks, SeedBindingInfo, agentCodeChangeShape, alwaysAvailableResources, formatMissingBlueprintBindings, runAgent, summarizeArgs, type AgentStepChange, type AiChatMessageBodyWithModelData, type ChatHistory, type WorktreeTurnAccess, GIT_BINDING_NAME } from "./agent";
 import { WorktreeSessionImpl } from "./worktree-session";
 import { GitImpl } from "./git-binding";
 import { scanWorkpieceForGrep, type GrepScan } from "./grep";
@@ -2665,8 +2665,14 @@ class OverseerImpl implements AgentHooks {
     let meta = this.storage.chatMeta.get(chatId);
     if (!meta) return false;  // chat deleted mid-turn
 
-    for (let {change} of step.changes) {
-      validateCodeChangeSchema(change);
+    for (const [changeIndex, {change}] of step.changes.entries()) {
+      try { validateCodeChangeSchema(change); }
+      catch (error) {
+        this.logger.error("invalid agent transform rejected at persistence", {
+          event: "agent.step.change.invalid", chatId, changeStage: "persist", changeIndex, ...agentCodeChangeShape(change),
+        });
+        throw error;
+      }
     }
 
     // The worktrees this step pins: every worktree of this chat its rows or commits touch that