feat: review agent turn edits before saving project

electron/ai-edition/chat-service.toolloop.test.ts

@@ -10,12 +10,19 @@ import {
 	createEmptyDocument,
 	documentSchema,
 } from "../../src/lib/ai-edition/schema";
+import type { AiEditionEditStatus } from "../../src/native/contracts";
 
 vi.mock("./deep-agent/service", () => ({
 	invokeOpenScreenAgent: vi.fn(),
 }));
 
-import { createSession, rewindToMessage, runChat } from "./chat-service";
+import {
+	createSession,
+	rewindToMessage,
+	runChat,
+	selectSession,
+	setEditStatus,
+} from "./chat-service";
 import { invokeOpenScreenAgent } from "./deep-agent/service";
 import type { LlmConfigStore } from "./llm-config-store";
 
@@ -146,6 +153,8 @@ describe("runChat tool loop", () => {
 		invokeMock.mockImplementationOnce(async (args) => {
 			args.sink.toolStart("addTrim", { startSec: 5, endSec: 8, reason: "silence" });
 			args.sink.toolEnd("addTrim", true, "added trim 0:05.0 – 0:08.0");
+			args.sink.toolStart("addZoom", { startSec: 10, endSec: 12 });
+			args.sink.toolEnd("addZoom", true, "added zoom 0:10.0 – 0:12.0");
 			return { text: "Done.", document: args.document, mutated: true };
 		});
 
@@ -160,8 +169,44 @@ describe("runChat tool loop", () => {
 
 		expect(result.success).toBe(true);
 		expect(result.assistantMessage?.content).toBe("Done.");
-		expect(result.toolCalls).toHaveLength(1);
+		expect(result.toolCalls).toHaveLength(2);
 		expect(result.toolCalls?.[0].summary).toMatch(/added trim/);
+		expect(result.assistantMessage?.editStatus).toBe("proposed");
+		expect(selectSession("proj_loop", session.id)?.messages[1].editStatus).toBe("proposed");
+		expect(
+			setEditStatus(
+				"proj_loop",
+				session.id,
+				result.assistantMessage!.id,
+				"invalid" as AiEditionEditStatus,
+			),
+		).toBe(false);
+		expect(setEditStatus("proj_loop", session.id, result.assistantMessage!.id, "applied")).toBe(
+			true,
+		);
+		expect(selectSession("proj_loop", session.id)?.messages[1].editStatus).toBe("applied");
+		expect(setEditStatus("proj_loop", session.id, result.assistantMessage!.id, "discarded")).toBe(
+			false,
+		);
+	});
+
+	it("does not ask for approval or report application after a read-only tool", async () => {
+		invokeMock.mockImplementationOnce(async (args) => {
+			args.sink.toolStart("getCurrentDocument", {});
+			args.sink.toolEnd("getCurrentDocument", true, "read project");
+			return { text: "The project is ready.", document: args.document, mutated: false };
+		});
+		const session = createSession("proj_read_only");
+		const result = await runChat(
+			"proj_read_only",
+			session.id,
+			"what is here?",
+			stubConfig(),
+			fixtureDocument(),
+		);
+		expect(result.document).toBeUndefined();
+		expect(result.assistantMessage?.editStatus).toBeUndefined();
+		expect(result.toolCalls?.[0]).toMatchObject({ mutating: false, summary: "read project" });
 	});
 
 	it("rewinds to before the user message, restoring the document", async () => {
@@ -240,6 +285,23 @@ describe("runChat tool loop", () => {
 		expect(result.document).toBeUndefined();
 	});
 
+	it("refuses a write and withholds a draft when no live document was supplied", async () => {
+		let editsAllowed: boolean | undefined;
+		invokeMock.mockImplementationOnce(async (args) => {
+			editsAllowed = args.editsAllowed;
+			return {
+				text: "Drafted a title.",
+				document: { ...args.document, project: { ...args.document.project, title: "Draft" } },
+				mutated: true,
+			};
+		});
+		const session = createSession("proj_no_document");
+		const result = await runChat("proj_no_document", session.id, "rename", stubConfig());
+		expect(editsAllowed).toBe(false);
+		expect(result.document).toBeUndefined();
+		expect(result.assistantMessage?.editStatus).toBeUndefined();
+	});
+
 	it("forwards text deltas, tool lifecycle, and errors through the ChatEventSink", async () => {
 		const events: { kind: string; payload: unknown }[] = [];
 		invokeMock.mockImplementationOnce(async (args) => {

electron/ai-edition/chat-service.ts

@@ -19,8 +19,10 @@ import {
 import type {
 	AiEditionChatMessage,
 	AiEditionChatResult,
+	AiEditionEditStatus,
 	AiEditionToolCallSummary,
 } from "../../src/native/contracts";
+import { isMutatingTool } from "./agent-tools";
 import {
 	applyCompaction,
 	budgetSnapshot,
@@ -230,6 +232,24 @@ export function selectSession(projectId: string, sessionId: string): ChatSession
 	};
 }
 
+/** Tool execution is provisional; only the renderer knows whether project.save succeeded. */
+export function setEditStatus(
+	projectId: string,
+	sessionId: string,
+	messageId: string,
+	status: AiEditionEditStatus,
+): boolean {
+	// IPC payloads are untrusted at runtime despite the TypeScript contract.
+	if (!["applied", "discarded", "conflict", "failed"].includes(status)) return false;
+	const message = sessionsByProject
+		.get(projectId)
+		?.get(sessionId)
+		?.messages.find((m) => m.id === messageId);
+	if (!message || message.editStatus !== "proposed") return false;
+	message.editStatus = status;
+	return true;
+}
+
 export function renameSession(
 	projectId: string,
 	sessionId: string,
@@ -362,7 +382,7 @@ export async function runChat(
 
 	session.messages.push(userMessage);
 
-	const editsAllowed = config.allowAgentEdits !== false;
+	const editsAllowed = config.allowAgentEdits !== false && workingDocument !== null;
 
 	// ponytail: NO automatic compaction here. A turn used to first check the
 	// history against a guessed 80k-token budget and, past 70% of it, block on
@@ -375,10 +395,12 @@ export async function runChat(
 
 	const history = modelHistory(session).map((m) => ({
 		role: m.role as "user" | "assistant" | "system",
-		content: m.content,
+		content: m.editStatus
+			? `[Project edits in this turn: ${m.editStatus}. Tool success only describes the draft document; only "applied" means saved.]\n${m.content}`
+			: m.content,
 	}));
 
-	const appliedToolCalls: AiEditionToolCallSummary[] = [];
+	const executedToolCalls: AiEditionToolCallSummary[] = [];
 
 	const agentSink = {
 		text: (delta: string) => emit.text(delta),
@@ -389,7 +411,7 @@ export async function runChat(
 		toolEnd: (name: string, ok: boolean, summary?: string) => {
 			emit.toolEnd(name, ok, summary);
 			if (ok && summary) {
-				appliedToolCalls.push({ name, summary });
+				executedToolCalls.push({ name, summary, mutating: isMutatingTool(name) });
 			}
 		},
 		error: (message: string) => emit.error(message),
@@ -410,6 +432,7 @@ export async function runChat(
 		userMessage: message,
 		sink: agentSink,
 		editsAllowed,
+		documentAvailable: workingDocument !== null,
 		cursor: env.cursor,
 	});
 
@@ -430,7 +453,8 @@ export async function runChat(
 		role: "assistant",
 		content: result.text,
 		createdAt: new Date().toISOString(),
-		toolCalls: appliedToolCalls.length ? appliedToolCalls : undefined,
+		toolCalls: executedToolCalls.length ? executedToolCalls : undefined,
+		editStatus: result.mutated && editsAllowed && workingDocument ? "proposed" : undefined,
 	};
 	session.messages.push(assistantMessage);
 
@@ -443,7 +467,7 @@ export async function runChat(
 		// still land. Cheap, and it makes the setting's guarantee structural
 		// rather than dependent on one predicate holding everywhere.
 		document: result.mutated && editsAllowed ? result.document : undefined,
-		toolCalls: appliedToolCalls.length ? appliedToolCalls : undefined,
+		toolCalls: executedToolCalls.length ? executedToolCalls : undefined,
 		userMessageCheckpointId: userMessage.checkpointId ?? undefined,
 	};
 }

electron/ai-edition/deep-agent/service.test.ts

@@ -418,14 +418,23 @@ describe("the prompt when the user has turned project edits off", () => {
 	});
 
 	it("says nothing of the sort on a normal turn", () => {
-		expect(buildSystemPrompt({ editsAllowed: true })).toBe(SYSTEM_PROMPT);
+		expect(buildSystemPrompt({ editsAllowed: true, reviewEdits: true })).toBe(SYSTEM_PROMPT);
+		expect(SYSTEM_PROMPT).toMatch(/draft until the user reviews and applies/);
 		expect(SYSTEM_PROMPT).not.toMatch(/PROJECT EDITS ARE CURRENTLY DISABLED/);
 	});
 
+	it("explains the missing document without claiming the global setting is off", () => {
+		const prompt = buildSystemPrompt({ editsAllowed: false, documentAvailable: false });
+		expect(prompt).toMatch(/NO PROJECT DOCUMENT IS OPEN/);
+		expect(prompt).not.toMatch(/PROJECT EDITS ARE CURRENTLY DISABLED/);
+	});
+
 	it("keeps the normal prompt as a prefix, so the editing rules still apply", () => {
 		// The consent block CONSTRAINS the turn; it does not replace the tool
 		// selection rules the model needs to describe the edit it is proposing.
-		expect(buildSystemPrompt({ editsAllowed: false }).startsWith(SYSTEM_PROMPT)).toBe(true);
+		expect(
+			buildSystemPrompt({ editsAllowed: false, reviewEdits: true }).startsWith(SYSTEM_PROMPT),
+		).toBe(true);
 	});
 });
 

electron/ai-edition/deep-agent/service.ts

@@ -100,15 +100,27 @@ const CONSENT_PROMPT_BLOCK = [
 	"- Never state or imply that an edit was applied. If the user confirms and you are still refused, tell them the 'Project edits' setting in Settings → AI has to be re-enabled first.",
 ].join("\n");
 
+const NO_DOCUMENT_PROMPT_BLOCK = [
+	"",
+	"NO PROJECT DOCUMENT IS OPEN. The document supplied for this conversation is an empty stand-in, not a project to edit.",
+	"- Do not call write tools; they will be refused. Explain that the user must open a project before edits can be proposed.",
+	"- Never claim that a project edit was applied or saved.",
+].join("\n");
+
+const EDIT_REVIEW_PROMPT_BLOCK = [
+	"",
+	"In-app chat tool edits stay in a detached draft until the user reviews and applies the whole turn; only then does OpenScreen save them. Describe edits as proposed, never as already applied or saved. The user may discard the turn, the project may change, or saving may fail.",
+].join("\n");
+
 // ponytail: exported so a test can assert that what the model receives is this
 // string and NOTHING else — the deepagents regression was invisible precisely
 // because the middlewares appended their prompts downstream of this constant.
 const BASE_SYSTEM_PROMPT = [
 	"You are an AI video editor working inside OpenScreen. The user is editing a recording.",
 	"Help them cut silences, tighten pacing, add captions, and rewrite titles.",
 	"Be concise, action-oriented, and reference the timeline or transcript by time when relevant.",
-	"You can call the tools below against the live document snapshot; the runtime executes each edit and feeds the result back into the loop.",
-	"The AxcutDocument is the single source of truth. The timeline, the transcript editor, and the chat panel are all direct editors of the same document — when the user places a clip on the timeline, the document updates immediately, and when the timeline is empty, the document has no clips. Your edits operate on the live document, so preserve the user's placed clips.",
+	"You can call the tools below against the current document snapshot; the runtime executes each edit and feeds the result back into the loop.",
+	"The AxcutDocument is the single source of truth shared by the timeline, transcript editor, and chat panel. When the user places a clip on the timeline, the document updates immediately, and when the timeline is empty, the document has no clips. Your snapshot reflects that state, so preserve the user's placed clips.",
 	"",
 	"Time-bases (do not mix them up): clips and trims are in SOURCE-time seconds of an asset; zooms, speed regions, annotations and camera-fullscreen regions are in VIRTUAL (edited-timeline) seconds — the position on the ruler after clips + trims are applied. getCurrentDocument returns all of them, clearly labelled.",
 	"",
@@ -133,16 +145,21 @@ const BASE_SYSTEM_PROMPT = [
 	"Honesty rules: if a request has NO matching tool (e.g. deleting an asset/recording, exporting), say so plainly — do not substitute a different edit and report it as the requested one. After your edits, if you are at all unsure the document ended up as intended, call getCurrentDocument and reconcile what you claim with the real state; each tool result already tells you exactly what it did, so never report a change the results don't support.",
 ].join("\n");
 
-/** The system prompt for a turn, which depends on ONE thing: whether the user
- *  currently allows the agent to write. Exported so the consent wording can be
- *  tested without a provider. */
-export function buildSystemPrompt(options: { editsAllowed: boolean }): string {
-	return options.editsAllowed ? BASE_SYSTEM_PROMPT : BASE_SYSTEM_PROMPT + CONSENT_PROMPT_BLOCK;
+/** Shared tool guidance. Only in-app chat turns opt into the draft review rules;
+ * MCP tools save each call immediately. */
+export function buildSystemPrompt(options: {
+	editsAllowed: boolean;
+	documentAvailable?: boolean;
+	reviewEdits?: boolean;
+}): string {
+	const prompt = BASE_SYSTEM_PROMPT + (options.reviewEdits ? EDIT_REVIEW_PROMPT_BLOCK : "");
+	if (options.documentAvailable === false) return prompt + NO_DOCUMENT_PROMPT_BLOCK;
+	return options.editsAllowed ? prompt : prompt + CONSENT_PROMPT_BLOCK;
 }
 
 /** The prompt of a normal (edits-allowed) turn — the overwhelmingly common
  *  case, and the string the surface tests measure the wire against. */
-export const SYSTEM_PROMPT = buildSystemPrompt({ editsAllowed: true });
+export const SYSTEM_PROMPT = buildSystemPrompt({ editsAllowed: true, reviewEdits: true });
 
 // #1028: a zoom's camera moves sit OUTSIDE its span, where nothing on the ruler shows them.
 const ZOOM_TRANSITIONS_NOTE = `A zoom HOLDS over its span: it animates in during the window just before startSec and out during the window just after endSec. Each move lasts 0.6 s + 0.55 s × ln(renderedScale) of SCREEN time (${ZOOM_TRANSITION_LEGEND}), so a speed region scales the timeline it covers by its speed: longer above 1×, shorter below. getCurrentDocument gives each zoom's zoomInFromSec and zoomOutUntilSec, in virtual seconds like the zoom. A trim inside either window cuts that move and the export jumps at the cut: leave the windows untrimmed.`;
@@ -417,10 +434,12 @@ export interface InvokeArgs {
 	history: Array<{ role: "user" | "assistant" | "system"; content: string }>;
 	userMessage: string;
 	sink: OpenScreenAgentSink;
-	/** `config.allowAgentEdits !== false`, resolved by the chat-service. When
-	 *  false the write tools refuse and the prompt tells the model to ask.
+	/** Whether edits are allowed for this turn. False if the global setting is off
+	 *  or no live document exists. The write tools refuse in either case.
 	 *  Defaults to allowed so a caller that predates the flag behaves as before. */
 	editsAllowed?: boolean;
+	/** False when chat-service supplied an empty stand-in for text-only chat. */
+	documentAvailable?: boolean;
 	/** Injected by `chat-service` from the Electron layer. Absent in tests and in
 	 *  the workbench unless one is supplied on purpose. */
 	cursor?: CursorTelemetryReader;
@@ -502,7 +521,11 @@ export async function invokeOpenScreenAgent(args: InvokeArgs): Promise<InvokeRes
 	const agent = createAgent({
 		model: chatModel,
 		tools,
-		systemPrompt: buildSystemPrompt({ editsAllowed }),
+		systemPrompt: buildSystemPrompt({
+			editsAllowed,
+			documentAvailable: args.documentAvailable,
+			reviewEdits: true,
+		}),
 		middleware: anthropicCachingMiddleware(chatModel),
 	}).withConfig({
 		// ponytail: NOT optional. LangGraph's default is 25 steps, and an

electron/ai-edition/document-service.test.ts

@@ -679,6 +679,45 @@ describe("DocumentService", () => {
 	// tail surviving past its end. The file parsed as JSON up to the splice and
 	// then died, taking the user's edits with it.
 	describe("concurrent saves", () => {
+		it.each([
+			"before-delete",
+			"after-delete",
+		])("does not recreate a project with a corrective save queued %s", async (order) => {
+			const doc = await service.createProject("Delete race");
+			const open = fs.open;
+			let release: (() => void) | undefined;
+			const held = vi.spyOn(fs, "open").mockImplementationOnce(async (file, flags, mode) => {
+				await new Promise<void>((resolve) => {
+					release = resolve;
+				});
+				return open(file, flags, mode);
+			});
+			const writing = service.saveProject(doc, order === "before-delete");
+			await vi.waitFor(() => expect(release).toBeTypeOf("function"));
+			const deleting = service.deleteProject(doc.project.id);
+			const correcting = order === "after-delete" ? service.saveProject(doc, true) : writing;
+			const settled = Promise.allSettled([writing, deleting, correcting]);
+			try {
+				release?.();
+				const results = await settled;
+				expect(results[0].status).toBe("fulfilled");
+				expect(results[1].status).toBe("fulfilled");
+				if (order === "after-delete") {
+					expect(results[2]).toMatchObject({
+						status: "rejected",
+						reason: expect.any(DocumentNotFoundError),
+					});
+				}
+				await expect(service.getProject(doc.project.id)).rejects.toBeInstanceOf(
+					DocumentNotFoundError,
+				);
+			} finally {
+				release?.();
+				await settled;
+				held.mockRestore();
+			}
+		});
+
 		/** A document whose serialised length is driven by `annotations`. */
 		const withBulk = (doc: AxcutDocument, count: number): AxcutDocument => ({
 			...doc,