From 2f48721cea4a6fd9b32574c4e41fdb06fe33230b Mon Sep 17 00:00:00 2001 From: rNoz Date: Sun, 19 Jul 2026 23:14:55 +0200 Subject: [PATCH 1/4] feat(annotate): add strict atomic result output --- AGENTS.md | 6 + apps/hook/server/annotate-command.test.ts | 178 +++++++++++++++ apps/hook/server/annotate-command.ts | 59 +++++ apps/hook/server/cli.test.ts | 130 +++++++++++ apps/hook/server/cli.ts | 67 +++++- apps/hook/server/index.ts | 56 +++-- .../server/strict-annotate-result.test.ts | 211 ++++++++++++++++++ apps/hook/server/strict-annotate-result.ts | 111 +++++++++ .../annotate-gates-and-json-responses.md | 24 +- 9 files changed, 824 insertions(+), 18 deletions(-) create mode 100644 apps/hook/server/annotate-command.test.ts create mode 100644 apps/hook/server/annotate-command.ts create mode 100644 apps/hook/server/strict-annotate-result.test.ts create mode 100644 apps/hook/server/strict-annotate-result.ts diff --git a/AGENTS.md b/AGENTS.md index edfd5c293..2ea9e69fa 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -268,6 +268,12 @@ User annotates content, provides feedback Send Annotations → feedback sent to agent session ``` +### Strict direct annotate results + +Direct `plannotator annotate` invocations may add `--require-approval` and/or `--result-file ` only with `--gate --json`; both reject `--hook` and are not shared with OpenCode/Pi slash-command parsing. Legacy plaintext, JSON, hook, and exit behavior remains unchanged when neither strict option is present. + +Strict decisions use one newline-terminated JSON record on stdout and, when requested, identical bytes in the result file. Approval exits `0`; with `--require-approval`, annotated and dismissed decisions are published before a nonzero exit. Result paths resolve from the invocation working directory, require an existing parent and absent destination, and publish via a flushed/closed `0600` same-directory temporary file plus an atomic no-clobber hard link—never copy or overwrite fallback. Keep reviewed sources at stable project paths; unique result and diagnostic log files may use a narrow temporary directory. Explicit Close emits `dismissed`; missing results or process/browser failures are recovery cases, never approval. + ## Archive Flow ``` diff --git a/apps/hook/server/annotate-command.test.ts b/apps/hook/server/annotate-command.test.ts new file mode 100644 index 000000000..2bbe81ba2 --- /dev/null +++ b/apps/hook/server/annotate-command.test.ts @@ -0,0 +1,178 @@ +import { describe, expect, test } from "bun:test"; +import { completeAnnotateCommand } from "./annotate-command"; +import type { AnnotateOutcome } from "./strict-annotate-result"; + +interface RunResult { + events: string[]; + stdout: string[]; + resultBytes: string[]; + legacy: AnnotateOutcome[]; +} + +async function runCompletion( + outcome: AnnotateOutcome, + options: { + requireApproval?: boolean; + resultFile?: string; + resultWriter?: (path: string, serialized: string) => Promise; + } = {}, +): Promise { + const events: string[] = []; + const stdout: string[] = []; + const resultBytes: string[] = []; + const legacy: AnnotateOutcome[] = []; + + await completeAnnotateCommand({ + waitForDecision: async () => { + events.push("decision"); + return outcome; + }, + settleAfterDecision: async () => { + events.push("settle"); + }, + stopServer: () => { + events.push("stop"); + }, + requireApproval: options.requireApproval ?? false, + resultFile: options.resultFile, + writeResultFile: + options.resultWriter ?? + (async (_path, serialized) => { + events.push("result"); + resultBytes.push(`${serialized}\n`); + }), + writeStdout: async (bytes) => { + events.push("stdout"); + stdout.push(bytes); + }, + emitLegacyOutcome: (result) => { + events.push("legacy"); + legacy.push(result); + }, + exit: (code) => { + events.push(`exit:${code}`); + }, + }); + + return { events, stdout, resultBytes, legacy }; +} + +describe("completeAnnotateCommand", () => { + test("publishes approved feedback to matching stdout and result bytes", async () => { + const result = await runCompletion( + { + approved: true, + feedback: "Keep the cache bounded.", + }, + { + requireApproval: true, + resultFile: "/result.json", + }, + ); + + const expected = + '{"decision":"approved","feedback":"Keep the cache bounded."}\n'; + expect(result.resultBytes).toEqual([expected]); + expect(result.stdout).toEqual([expected]); + expect(result.events).toEqual([ + "decision", + "settle", + "stop", + "result", + "stdout", + "exit:0", + ]); + }); + + test("publishes annotated and dismissed decisions before nonzero exit", async () => { + const annotated = await runCompletion( + { approved: false, exit: false, feedback: "revise" }, + { requireApproval: true, resultFile: "/annotated.json" }, + ); + const dismissed = await runCompletion( + { exit: true, feedback: "" }, + { requireApproval: true, resultFile: "/dismissed.json" }, + ); + + expect(annotated.stdout).toEqual([ + '{"decision":"annotated","feedback":"revise"}\n', + ]); + expect(annotated.resultBytes).toEqual(annotated.stdout); + expect(annotated.events.slice(-3)).toEqual([ + "result", + "stdout", + "exit:1", + ]); + expect(dismissed.stdout).toEqual([ + '{"decision":"dismissed"}\n', + ]); + expect(dismissed.resultBytes).toEqual(dismissed.stdout); + expect(dismissed.events.slice(-3)).toEqual([ + "result", + "stdout", + "exit:1", + ]); + }); + + test("delegates legacy output unchanged and exits zero", async () => { + const outcome = { exit: false, feedback: "legacy feedback" }; + const result = await runCompletion(outcome); + + expect(result.legacy).toEqual([outcome]); + expect(result.stdout).toEqual([]); + expect(result.resultBytes).toEqual([]); + expect(result.events.slice(-2)).toEqual(["legacy", "exit:0"]); + }); + + test("supports each strict option independently", async () => { + const resultFileOnly = await runCompletion( + { approved: false, feedback: "revise" }, + { resultFile: "/result.json" }, + ); + const approvalOnly = await runCompletion( + { exit: true, feedback: "" }, + { requireApproval: true }, + ); + + expect(resultFileOnly.events.slice(-3)).toEqual([ + "result", + "stdout", + "exit:0", + ]); + expect(resultFileOnly.resultBytes).toEqual(resultFileOnly.stdout); + expect(approvalOnly.events.slice(-2)).toEqual(["stdout", "exit:1"]); + expect(approvalOnly.resultBytes).toEqual([]); + }); + + test("does not emit stdout or exit when result publication fails", async () => { + const events: string[] = []; + + await expect( + completeAnnotateCommand({ + waitForDecision: async () => ({ + approved: false, + feedback: "revise", + }), + settleAfterDecision: async () => {}, + stopServer: () => {}, + requireApproval: true, + resultFile: "/raced.json", + writeResultFile: async () => { + events.push("result"); + throw new Error("destination appeared"); + }, + writeStdout: async () => { + events.push("stdout"); + }, + emitLegacyOutcome: () => { + events.push("legacy"); + }, + exit: (code) => { + events.push(`exit:${code}`); + }, + }), + ).rejects.toThrow("destination appeared"); + + expect(events).toEqual(["result"]); + }); +}); diff --git a/apps/hook/server/annotate-command.ts b/apps/hook/server/annotate-command.ts new file mode 100644 index 000000000..9946178e4 --- /dev/null +++ b/apps/hook/server/annotate-command.ts @@ -0,0 +1,59 @@ +import { + annotateOutcomeExitCode, + serializeStrictAnnotateResult, + writeAnnotateResultFile, + type AnnotateOutcome, +} from "./strict-annotate-result"; + +export interface CompleteAnnotateCommandOptions { + waitForDecision: () => Promise; + settleAfterDecision: () => Promise; + stopServer: () => void; + requireApproval: boolean; + resultFile?: string; + writeResultFile?: ( + resultFile: string, + serialized: string, + ) => Promise; + writeStdout?: (output: string) => Promise; + emitLegacyOutcome: (result: AnnotateOutcome) => void; + exit?: (code: number) => void; +} + +export function writeStdout(output: string): Promise { + return new Promise((resolve, reject) => { + process.stdout.write(output, (error) => { + if (error) reject(error); + else resolve(); + }); + }); +} + +export async function completeAnnotateCommand({ + waitForDecision, + settleAfterDecision, + stopServer, + requireApproval, + resultFile, + writeResultFile = writeAnnotateResultFile, + writeStdout: outputWriter = writeStdout, + emitLegacyOutcome, + exit = process.exit, +}: CompleteAnnotateCommandOptions): Promise { + const result = await waitForDecision(); + await settleAfterDecision(); + stopServer(); + + if (requireApproval || resultFile) { + const serialized = serializeStrictAnnotateResult(result); + if (resultFile) { + await writeResultFile(resultFile, serialized); + } + await outputWriter(`${serialized}\n`); + exit(annotateOutcomeExitCode(result, requireApproval)); + return; + } + + emitLegacyOutcome(result); + exit(0); +} diff --git a/apps/hook/server/cli.test.ts b/apps/hook/server/cli.test.ts index 48803220d..f180dc99d 100644 --- a/apps/hook/server/cli.test.ts +++ b/apps/hook/server/cli.test.ts @@ -9,6 +9,7 @@ import { isSubcommandHelpInvocation, isTopLevelHelpInvocation, isVersionInvocation, + parseStrictAnnotateOptions, } from "./cli"; describe("CLI top-level help", () => { @@ -100,12 +101,141 @@ describe("CLI subcommand help", () => { expect(formatSubcommandHelp("review")).toContain("--gitbutler"); expect(formatSubcommandHelp("review")).toContain("PR_URL"); expect(formatSubcommandHelp("annotate")).toContain("--no-jina"); + expect(formatSubcommandHelp("annotate")).toContain("--require-approval"); + expect(formatSubcommandHelp("annotate")).toContain("--result-file "); + expect(formatSubcommandHelp("annotate-last")).not.toContain( + "--require-approval", + ); expect(formatSubcommandHelp("sessions")).toContain("--open [N]"); // unknown key falls back to top-level help expect(formatSubcommandHelp("nope")).toBe(formatTopLevelHelp()); }); }); +describe("strict annotate CLI options", () => { + test("extracts strict options before or after the target path", () => { + const strictOrderings = [ + ["plan.md", "--require-approval", "--result-file", "result.json"], + ["plan.md", "--result-file", "result.json", "--require-approval"], + ["--require-approval", "plan.md", "--result-file", "result.json"], + ["--require-approval", "--result-file", "result.json", "plan.md"], + ["--result-file", "result.json", "plan.md", "--require-approval"], + ["--result-file", "result.json", "--require-approval", "plan.md"], + ]; + + for (const ordering of strictOrderings) { + expect( + parseStrictAnnotateOptions([ + "annotate", + ...ordering, + "--gate", + "--json", + ]), + ).toEqual({ + requireApproval: true, + resultFile: "result.json", + remainingArgs: ["annotate", "plan.md", "--gate", "--json"], + }); + } + }); + + test("allows either strict option independently", () => { + expect( + parseStrictAnnotateOptions([ + "annotate", + "plan.md", + "--gate", + "--json", + "--require-approval", + ]), + ).toEqual({ + requireApproval: true, + remainingArgs: ["annotate", "plan.md", "--gate", "--json"], + }); + expect( + parseStrictAnnotateOptions([ + "annotate", + "--result-file", + "result.json", + "plan.md", + "--gate", + "--json", + ]), + ).toEqual({ + requireApproval: false, + resultFile: "result.json", + remainingArgs: ["annotate", "plan.md", "--gate", "--json"], + }); + }); + + test("leaves ordinary direct arguments unchanged", () => { + const args = [ + "annotate", + "plan.md", + "--gate", + "--json", + "--markdown", + ]; + expect(parseStrictAnnotateOptions(args)).toEqual({ + requireApproval: false, + remainingArgs: args, + }); + }); + + test("requires annotate --gate --json without --hook", () => { + for (const args of [ + ["review", "--gate", "--json", "--require-approval"], + ["annotate-last", "--gate", "--json", "--require-approval"], + ["annotate", "plan.md", "--json", "--require-approval"], + ["annotate", "plan.md", "--gate", "--require-approval"], + [ + "annotate", + "plan.md", + "--gate", + "--json", + "--hook", + "--require-approval", + ], + ]) { + expect(() => parseStrictAnnotateOptions(args)).toThrow(); + } + }); + + test("rejects missing and duplicate strict option values", () => { + expect(() => + parseStrictAnnotateOptions([ + "annotate", + "plan.md", + "--gate", + "--json", + "--result-file", + ]), + ).toThrow("Missing value for --result-file"); + expect(() => + parseStrictAnnotateOptions([ + "annotate", + "plan.md", + "--gate", + "--json", + "--result-file", + "first.json", + "--result-file", + "second.json", + ]), + ).toThrow("--result-file may only be specified once"); + expect(() => + parseStrictAnnotateOptions([ + "annotate", + "plan.md", + "--gate", + "--json", + "--require-approval", + "--require-approval", + ]), + ).toThrow("--require-approval may only be specified once"); + }); +}); + describe("CLI --version", () => { test("recognizes --version and -v", () => { expect(isVersionInvocation(["--version"])).toBe(true); diff --git a/apps/hook/server/cli.ts b/apps/hook/server/cli.ts index 00d7dafa5..c38e0e97d 100644 --- a/apps/hook/server/cli.ts +++ b/apps/hook/server/cli.ts @@ -1,5 +1,64 @@ const HELP_FLAGS = new Set(["--help", "-h"]); +export interface ParsedStrictAnnotateOptions { + requireApproval: boolean; + resultFile?: string; + remainingArgs: string[]; +} + +export function parseStrictAnnotateOptions( + args: string[], +): ParsedStrictAnnotateOptions { + let requireApproval = false; + let resultFile: string | undefined; + const remainingArgs: string[] = []; + + for (let index = 0; index < args.length; index += 1) { + const arg = args[index]; + if (arg === "--require-approval") { + if (requireApproval) { + throw new Error("--require-approval may only be specified once"); + } + requireApproval = true; + continue; + } + if (arg === "--result-file") { + if (resultFile !== undefined) { + throw new Error("--result-file may only be specified once"); + } + const value = args[index + 1]; + if (!value || value.startsWith("--")) { + throw new Error("Missing value for --result-file"); + } + resultFile = value; + index += 1; + continue; + } + remainingArgs.push(arg); + } + + if (!requireApproval && resultFile === undefined) { + return { requireApproval: false, remainingArgs }; + } + if (remainingArgs[0] !== "annotate") { + throw new Error( + "--require-approval and --result-file are only valid with annotate", + ); + } + if (!remainingArgs.includes("--gate") || !remainingArgs.includes("--json")) { + throw new Error( + "--require-approval and --result-file require --gate --json", + ); + } + if (remainingArgs.includes("--hook")) { + throw new Error( + "--require-approval and --result-file cannot be used with --hook", + ); + } + + return { requireApproval, resultFile, remainingArgs }; +} + /** True when any token is a help flag (`--help` / `-h`). */ export function hasHelpFlag(args: string[]): boolean { return args.some((arg) => HELP_FLAGS.has(arg)); @@ -33,7 +92,7 @@ export function formatTopLevelHelp(): string { " plannotator --version, -v", " plannotator [--browser ]", " plannotator review [--git | --gitbutler] [PR_URL]", - " plannotator annotate [--markdown] [--no-jina] [--gate] [--json] [--hook]", + " plannotator annotate [--markdown] [--no-jina] [--gate] [--json] [--hook] [--require-approval] [--result-file ]", " plannotator annotate-last [--stdin] [--gate] [--json] [--hook]", " plannotator setup-goal [--json]", " plannotator last", @@ -76,7 +135,7 @@ const SUBCOMMAND_HELP: Record = { ].join("\n"), annotate: [ "Usage:", - " plannotator annotate [--markdown] [--no-jina] [--gate] [--json] [--hook]", + " plannotator annotate [--markdown] [--no-jina] [--gate] [--json] [--hook] [--require-approval] [--result-file ]", "", "Open a markdown/text/HTML file, a URL, or a folder of documents in the annotation UI.", "", @@ -86,6 +145,10 @@ const SUBCOMMAND_HELP: Record = { " --gate Add an Approve button (review-gate UX)", " --json Emit a structured decision JSON on stdout", " --hook Emit hook-native JSON (block/pass) for PostToolUse/Stop hooks", + " --require-approval", + " Exit nonzero unless the reviewer approves (requires --gate --json)", + " --result-file ", + " Atomically publish the stdout JSON (requires --gate --json)", ].join("\n"), "annotate-last": [ "Usage:", diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 2df24c7cc..8ac62c20e 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -142,7 +142,13 @@ import { isSubcommandHelpInvocation, isTopLevelHelpInvocation, isVersionInvocation, + parseStrictAnnotateOptions, } from "./cli"; +import { completeAnnotateCommand } from "./annotate-command"; +import { + assertResultPathAvailable, + resolveResultFilePath, +} from "./strict-annotate-result"; import path from "path"; import { tmpdir } from "os"; import { buildLocalWorkspaceReview, type WorkspaceDiffType } from "@plannotator/server/review-workspace"; @@ -157,7 +163,24 @@ import reviewHtml from "../dist/review.html" with { type: "text" }; const reviewHtmlContent = reviewHtml as unknown as string; // Check for subcommand -const args = process.argv.slice(2); +let parsedStrictAnnotateOptions; +try { + parsedStrictAnnotateOptions = parseStrictAnnotateOptions( + process.argv.slice(2), + ); +} catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); +} +const args = parsedStrictAnnotateOptions.remainingArgs; +const requireApprovalFlag = + parsedStrictAnnotateOptions.requireApproval; +const resultFile = parsedStrictAnnotateOptions.resultFile + ? resolveResultFilePath( + parsedStrictAnnotateOptions.resultFile, + process.env.PLANNOTATOR_CWD || process.cwd(), + ) + : undefined; // Global flag: --browser const browserIdx = args.indexOf("--browser"); @@ -901,7 +924,7 @@ if (args[0] === "sessions") { const rawFilePath = args[1]; if (!rawFilePath) { - console.error("Usage: plannotator annotate [--markdown] [--no-jina] [--gate] [--json] [--hook]"); + console.error("Usage: plannotator annotate [--markdown] [--no-jina] [--gate] [--json] [--hook] [--require-approval] [--result-file ]"); process.exit(1); } @@ -913,6 +936,15 @@ if (args[0] === "sessions") { // Use PLANNOTATOR_CWD if set (original working directory before script cd'd) const projectRoot = process.env.PLANNOTATOR_CWD || process.cwd(); + if (resultFile) { + try { + await assertResultPathAvailable(resultFile); + } catch (error) { + console.error(error instanceof Error ? error.message : String(error)); + process.exit(1); + } + } + if (process.env.PLANNOTATOR_DEBUG) { console.error(`[DEBUG] Project root: ${projectRoot}`); console.error(`[DEBUG] File path arg: ${filePath}`); @@ -1079,18 +1111,14 @@ if (args[0] === "sessions") { : `annotate-${isUrl ? hostnameOrFallback(absolutePath) : path.basename(absolutePath)}`, }); - // Wait for user feedback - const result = await server.waitForDecision(); - - // Give browser time to receive response and update UI - await Bun.sleep(1500); - - // Cleanup - server.stop(); - - // Output feedback (captured by slash command) - emitAnnotateOutcome(result); - process.exit(0); + await completeAnnotateCommand({ + waitForDecision: server.waitForDecision, + settleAfterDecision: () => Bun.sleep(1500), + stopServer: server.stop, + requireApproval: requireApprovalFlag, + resultFile, + emitLegacyOutcome: emitAnnotateOutcome, + }); } else if (args[0] === "annotate-last" || args[0] === "last") { // ============================================ diff --git a/apps/hook/server/strict-annotate-result.test.ts b/apps/hook/server/strict-annotate-result.test.ts new file mode 100644 index 000000000..d71b703f8 --- /dev/null +++ b/apps/hook/server/strict-annotate-result.test.ts @@ -0,0 +1,211 @@ +import { afterEach, describe, expect, test } from "bun:test"; +import { + link, + mkdtemp, + open, + readFile, + readdir, + rm, + stat, + symlink, + unlink, + writeFile, +} from "node:fs/promises"; +import { join } from "node:path"; +import { tmpdir } from "node:os"; +import { + annotateOutcomeExitCode, + assertResultPathAvailable, + resolveResultFilePath, + serializeStrictAnnotateResult, + writeAnnotateResultFile, +} from "./strict-annotate-result"; + +const temporaryDirectories: string[] = []; + +async function makeTemporaryDirectory(): Promise { + const directory = await mkdtemp(join(tmpdir(), "plannotator-result-")); + temporaryDirectories.push(directory); + return directory; +} + +afterEach(async () => { + await Promise.all( + temporaryDirectories.splice(0).map((directory) => + rm(directory, { recursive: true, force: true }), + ), + ); +}); + +describe("strict annotate result serialization", () => { + test("serializes approval without feedback", () => { + expect( + serializeStrictAnnotateResult({ approved: true, feedback: "" }), + ).toBe('{"decision":"approved"}'); + }); + + test("serializes approval with feedback", () => { + expect( + serializeStrictAnnotateResult({ + approved: true, + feedback: "Keep the cache bounded.", + }), + ).toBe( + '{"decision":"approved","feedback":"Keep the cache bounded."}', + ); + }); + + test("serializes annotated and dismissed decisions", () => { + expect( + serializeStrictAnnotateResult({ + approved: false, + exit: false, + feedback: "revise", + }), + ).toBe('{"decision":"annotated","feedback":"revise"}'); + expect( + serializeStrictAnnotateResult({ exit: true, feedback: "" }), + ).toBe('{"decision":"dismissed"}'); + }); +}); + +describe("strict annotate exit policy", () => { + test("requires approval when requested", () => { + expect( + annotateOutcomeExitCode( + { approved: false, exit: false, feedback: "revise" }, + true, + ), + ).toBe(1); + expect( + annotateOutcomeExitCode({ approved: true, feedback: "" }, true), + ).toBe(0); + }); + + test("keeps legacy outcomes successful", () => { + expect( + annotateOutcomeExitCode({ exit: true, feedback: "" }, false), + ).toBe(0); + }); +}); + +describe("atomic annotate result publication", () => { + test("resolves relative result paths from the invocation directory", () => { + expect( + resolveResultFilePath("results/review.json", "/workspace/project"), + ).toBe("/workspace/project/results/review.json"); + expect( + resolveResultFilePath("/var/tmp/review.json", "/workspace/project"), + ).toBe("/var/tmp/review.json"); + }); + + test("publishes one complete private newline-terminated record", async () => { + const directory = await makeTemporaryDirectory(); + const resultFile = join(directory, "result.json"); + + await assertResultPathAvailable(resultFile); + await writeAnnotateResultFile( + resultFile, + '{"decision":"approved"}', + ); + + expect(await readFile(resultFile, "utf8")).toBe( + '{"decision":"approved"}\n', + ); + if (process.platform !== "win32") { + expect((await stat(resultFile)).mode & 0o077).toBe(0); + } + expect(await readdir(directory)).toEqual(["result.json"]); + }); + + test("rejects a missing parent and an existing destination", async () => { + const directory = await makeTemporaryDirectory(); + const missingParentResult = join(directory, "missing", "result.json"); + const existingResult = join(directory, "existing.json"); + await writeFile(existingResult, "existing", "utf8"); + + await expect( + assertResultPathAvailable(missingParentResult), + ).rejects.toThrow( + `Result file parent does not exist: ${join(directory, "missing")}`, + ); + await expect( + assertResultPathAvailable(existingResult), + ).rejects.toThrow(`Result file already exists: ${existingResult}`); + expect(await readdir(directory)).toEqual(["existing.json"]); + }); + + test.skipIf(process.platform === "win32")( + "rejects a dangling destination symlink before startup", + async () => { + const directory = await makeTemporaryDirectory(); + const resultFile = join(directory, "result.json"); + await symlink(join(directory, "missing-target"), resultFile); + + await expect( + assertResultPathAvailable(resultFile), + ).rejects.toThrow(`Result file already exists: ${resultFile}`); + }, + ); + + test("never overwrites a destination created after validation", async () => { + const directory = await makeTemporaryDirectory(); + const resultFile = join(directory, "result.json"); + await assertResultPathAvailable(resultFile); + await writeFile(resultFile, "raced", { mode: 0o600 }); + + await expect( + writeAnnotateResultFile( + resultFile, + '{"decision":"approved"}', + ), + ).rejects.toThrow(); + + expect(await readFile(resultFile, "utf8")).toBe("raced"); + expect(await readdir(directory)).toEqual(["result.json"]); + }); + + test("removes the temporary file when writing fails", async () => { + const directory = await makeTemporaryDirectory(); + const resultFile = join(directory, "result.json"); + + await expect( + writeAnnotateResultFile( + resultFile, + '{"decision":"approved"}', + { + open, + link, + unlink, + write: async () => { + throw new Error("write failed"); + }, + }, + ), + ).rejects.toThrow("write failed"); + + expect(await readdir(directory)).toEqual([]); + }); + + test("fails closed when hard-link publication is unavailable", async () => { + const directory = await makeTemporaryDirectory(); + const resultFile = join(directory, "result.json"); + + await expect( + writeAnnotateResultFile( + resultFile, + '{"decision":"approved"}', + { + open, + link: async () => { + throw new Error("hard links unavailable"); + }, + unlink, + write: (handle, contents) => handle.writeFile(contents, "utf8"), + }, + ), + ).rejects.toThrow("hard links unavailable"); + + expect(await readdir(directory)).toEqual([]); + }); +}); diff --git a/apps/hook/server/strict-annotate-result.ts b/apps/hook/server/strict-annotate-result.ts new file mode 100644 index 000000000..28ff86376 --- /dev/null +++ b/apps/hook/server/strict-annotate-result.ts @@ -0,0 +1,111 @@ +import { existsSync, lstatSync, statSync } from "node:fs"; +import { + link, + open, + unlink, + type FileHandle, +} from "node:fs/promises"; +import { randomUUID } from "node:crypto"; +import { basename, dirname, join, resolve } from "node:path"; + +export interface AnnotateOutcome { + feedback: string; + exit?: boolean; + approved?: boolean; +} + +export function serializeStrictAnnotateResult( + result: AnnotateOutcome, +): string { + if (result.approved) { + return JSON.stringify({ + decision: "approved", + ...(result.feedback ? { feedback: result.feedback } : {}), + }); + } + if (result.exit) return JSON.stringify({ decision: "dismissed" }); + return JSON.stringify({ + decision: "annotated", + feedback: result.feedback || "", + }); +} + +export function annotateOutcomeExitCode( + result: AnnotateOutcome, + requireApproval: boolean, +): number { + return requireApproval && !result.approved ? 1 : 0; +} + +export function resolveResultFilePath( + resultFile: string, + invocationCwd: string, +): string { + return resolve(invocationCwd, resultFile); +} + +export async function assertResultPathAvailable( + resultFile: string, +): Promise { + let destinationExists = existsSync(resultFile); + if (!destinationExists) { + try { + lstatSync(resultFile); + destinationExists = true; + } catch (error) { + if ( + !(error instanceof Error) || + !("code" in error) || + error.code !== "ENOENT" + ) { + throw error; + } + } + } + if (destinationExists) { + throw new Error(`Result file already exists: ${resultFile}`); + } + const parent = dirname(resultFile); + if (!existsSync(parent) || !statSync(parent).isDirectory()) { + throw new Error(`Result file parent does not exist: ${parent}`); + } +} + +interface ResultFileOperations { + open: typeof open; + link: typeof link; + unlink: typeof unlink; + write: (handle: FileHandle, contents: string) => Promise; +} + +const defaultResultFileOperations: ResultFileOperations = { + open, + link, + unlink, + write: (handle, contents) => handle.writeFile(contents, "utf8"), +}; + +export async function writeAnnotateResultFile( + resultFile: string, + serialized: string, + operations: ResultFileOperations = defaultResultFileOperations, +): Promise { + const temporary = join( + dirname(resultFile), + `.${basename(resultFile)}.${process.pid}.${randomUUID()}.tmp`, + ); + let handle: FileHandle | null = null; + try { + handle = await operations.open(temporary, "wx", 0o600); + await operations.write(handle, `${serialized}\n`); + await handle.sync(); + await handle.close(); + handle = null; + await operations.link(temporary, resultFile); + await operations.unlink(temporary); + } catch (error) { + await handle?.close().catch(() => {}); + await operations.unlink(temporary).catch(() => {}); + throw error; + } +} diff --git a/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md b/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md index 9ece15c47..8a94cc6f8 100644 --- a/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md +++ b/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md @@ -6,7 +6,7 @@ sidebar: section: "Guides" --- -`plannotator annotate` and `plannotator annotate-last` accept three flags that turn markdown annotation into a full review gate with structured output. +`plannotator annotate` and `plannotator annotate-last` accept three flags that turn markdown annotation into a full review gate with structured output. Direct `plannotator annotate` invocations also support strict automation with `--require-approval` and `--result-file`. ## Capabilities @@ -94,6 +94,26 @@ This is the recommended approach for hook integrations. The `{"decision":"block" The flag is accepted silently on OpenCode and Pi for the same reason `--json` is: those harnesses don't use stdout as the signal channel. +## Strict direct gates + +For a fail-closed direct CLI gate, add either or both strict options: + +```sh +plannotator annotate docs/plan.md --gate --json \ + --require-approval \ + --result-file .tmp/plan-review-result.json +``` + +- **`--require-approval`** exits `0` only for `approved`. `annotated` and `dismissed` still publish their valid JSON decision, then exit nonzero. +- **`--result-file `** atomically publishes the same newline-terminated JSON bytes written to stdout. The path is resolved from the invocation working directory. +- Both options require `--gate --json`, are available only on direct `annotate` invocations, and cannot be combined with `--hook`. Hook output and exit behavior are unchanged. + +The result-file parent directory must already exist and the destination must not. Plannotator writes a private `0600` temporary file in the same directory, flushes and closes it, then publishes it with an atomic no-clobber hard link. It never overwrites an existing destination or falls back to a non-atomic copy. Use a unique result path for every invocation. + +Keep the reviewed source at a stable project path so revisions and version history continue to refer to the same artifact. Result and diagnostic log files can instead live in a narrowly scoped temporary directory. + +Clicking Close publishes `{"decision":"dismissed"}`. Closing or crashing the browser outside that explicit action is not guaranteed to produce a decision; callers should treat a missing result or failed process as a recovery case, never as approval. + ## Primary use cases ### Spec-driven development frameworks @@ -116,4 +136,4 @@ See [Hook Integration](/docs/guides/hook-integration/) for copy-paste recipes th ## Exit codes -Every decision exits `0`. Signals live on stdout. This keeps Plannotator composable with harnesses that use exit codes for their own purposes. +By default, every decision exits `0`; existing plaintext, JSON, and hook integrations are unchanged. With `--require-approval`, only `approved` exits `0`. `annotated` and `dismissed` publish their JSON result before exiting nonzero. From 074899c1976090431f82a281cac38ea486274cac Mon Sep 17 00:00:00 2001 From: Michael Ramos Date: Mon, 20 Jul 2026 13:35:54 -0700 Subject: [PATCH 2/4] feat(annotate): exit 2 for strict-gate usage and publication errors Adopt the grep convention for the strict annotate gate's exit codes: 0 = approved, 1 = negative human outcome (annotated/dismissed under --require-approval), 2 = the gate itself was misconfigured or could not start/deliver a decision. Previously all usage/startup/validation failures shared exit 1 with "reviewer did not approve", so callers could not tell a denied review from a broken gate. - parseStrictAnnotateOptions failures (bad flag combos, strict flags outside annotate --gate --json) now exit 2 - --result-file preflight failures (missing parent, pre-existing or dangling-symlink destination) now exit 2 - post-decision publication failures (destination raced into existence, hard links unavailable, stdout write failure) now exit 2: they deliver no decision record at all, so the code's own fail-closed handling presents them as environment errors, never as a reviewer outcome -- and never approval, since only 0 means approved - decision outcomes keep 0/1 exactly as before; signal deaths keep 128+n - document the contract in AGENTS.md and the annotate-gates guide Claude-Session: https://claude.ai/code/session_01YXkgsNucxDwAL4GdR4XYRk --- AGENTS.md | 2 +- apps/hook/server/annotate-command.test.ts | 91 +++++++++++++------ apps/hook/server/annotate-command.ts | 19 +++- apps/hook/server/cli.ts | 3 +- apps/hook/server/index.ts | 7 +- .../server/strict-annotate-result.test.ts | 10 ++ apps/hook/server/strict-annotate-result.ts | 9 ++ .../annotate-gates-and-json-responses.md | 2 +- 8 files changed, 109 insertions(+), 34 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 2ea9e69fa..06eba9b5b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -272,7 +272,7 @@ Send Annotations → feedback sent to agent session Direct `plannotator annotate` invocations may add `--require-approval` and/or `--result-file ` only with `--gate --json`; both reject `--hook` and are not shared with OpenCode/Pi slash-command parsing. Legacy plaintext, JSON, hook, and exit behavior remains unchanged when neither strict option is present. -Strict decisions use one newline-terminated JSON record on stdout and, when requested, identical bytes in the result file. Approval exits `0`; with `--require-approval`, annotated and dismissed decisions are published before a nonzero exit. Result paths resolve from the invocation working directory, require an existing parent and absent destination, and publish via a flushed/closed `0600` same-directory temporary file plus an atomic no-clobber hard link—never copy or overwrite fallback. Keep reviewed sources at stable project paths; unique result and diagnostic log files may use a narrow temporary directory. Explicit Close emits `dismissed`; missing results or process/browser failures are recovery cases, never approval. +Strict decisions use one newline-terminated JSON record on stdout and, when requested, identical bytes in the result file. Exit codes follow the grep convention: approval exits `0`; with `--require-approval`, annotated and dismissed decisions are published before exiting `1` (negative human outcome); usage/startup/validation failures — bad flag combinations, strict flags outside `annotate --gate --json`, a missing `--result-file` parent, a pre-existing or dangling-symlink destination — exit `2` (the gate itself was misconfigured or could not start). Post-decision publication failures (destination appears between validation and publish, hard links unavailable, stdout write failure) also exit `2`: they deliver no decision record at all, so they present as environment errors — "the gate could not deliver a decision" — never as a reviewer outcome, and never as approval (still fail-closed, since only `0` means approved). Signal deaths keep `128+n`. Result paths resolve from the invocation working directory, require an existing parent and absent destination, and publish via a flushed/closed `0600` same-directory temporary file plus an atomic no-clobber hard link—never copy or overwrite fallback. Keep reviewed sources at stable project paths; unique result and diagnostic log files may use a narrow temporary directory. Explicit Close emits `dismissed`; missing results or process/browser failures are recovery cases, never approval. ## Archive Flow diff --git a/apps/hook/server/annotate-command.test.ts b/apps/hook/server/annotate-command.test.ts index 2bbe81ba2..902efd5fa 100644 --- a/apps/hook/server/annotate-command.test.ts +++ b/apps/hook/server/annotate-command.test.ts @@ -144,35 +144,74 @@ describe("completeAnnotateCommand", () => { expect(approvalOnly.resultBytes).toEqual([]); }); - test("does not emit stdout or exit when result publication fails", async () => { + test("exits 2 without emitting stdout when result publication fails", async () => { const events: string[] = []; + const errors: string[] = []; - await expect( - completeAnnotateCommand({ - waitForDecision: async () => ({ - approved: false, - feedback: "revise", - }), - settleAfterDecision: async () => {}, - stopServer: () => {}, - requireApproval: true, - resultFile: "/raced.json", - writeResultFile: async () => { - events.push("result"); - throw new Error("destination appeared"); - }, - writeStdout: async () => { - events.push("stdout"); - }, - emitLegacyOutcome: () => { - events.push("legacy"); - }, - exit: (code) => { - events.push(`exit:${code}`); - }, + await completeAnnotateCommand({ + waitForDecision: async () => ({ + approved: false, + feedback: "revise", + }), + settleAfterDecision: async () => {}, + stopServer: () => {}, + requireApproval: true, + resultFile: "/raced.json", + writeResultFile: async () => { + events.push("result"); + throw new Error("destination appeared"); + }, + writeStdout: async () => { + events.push("stdout"); + }, + emitLegacyOutcome: () => { + events.push("legacy"); + }, + exit: (code) => { + events.push(`exit:${code}`); + }, + logError: (message) => { + errors.push(message); + }, + }); + + // No decision record was delivered, so this is an environment error + // (exit 2), never a reviewer outcome — and never approval. + expect(events).toEqual(["result", "exit:2"]); + expect(errors).toEqual(["destination appeared"]); + }); + + test("exits 2 when the stdout record cannot be written", async () => { + const events: string[] = []; + const errors: string[] = []; + + await completeAnnotateCommand({ + waitForDecision: async () => ({ + approved: true, + feedback: "", }), - ).rejects.toThrow("destination appeared"); + settleAfterDecision: async () => {}, + stopServer: () => {}, + requireApproval: true, + writeResultFile: async () => { + events.push("result"); + }, + writeStdout: async () => { + events.push("stdout"); + throw new Error("stdout closed"); + }, + emitLegacyOutcome: () => { + events.push("legacy"); + }, + exit: (code) => { + events.push(`exit:${code}`); + }, + logError: (message) => { + errors.push(message); + }, + }); - expect(events).toEqual(["result"]); + expect(events).toEqual(["stdout", "exit:2"]); + expect(errors).toEqual(["stdout closed"]); }); }); diff --git a/apps/hook/server/annotate-command.ts b/apps/hook/server/annotate-command.ts index 9946178e4..c0acdff6c 100644 --- a/apps/hook/server/annotate-command.ts +++ b/apps/hook/server/annotate-command.ts @@ -1,6 +1,7 @@ import { annotateOutcomeExitCode, serializeStrictAnnotateResult, + STRICT_GATE_ERROR_EXIT_CODE, writeAnnotateResultFile, type AnnotateOutcome, } from "./strict-annotate-result"; @@ -18,6 +19,7 @@ export interface CompleteAnnotateCommandOptions { writeStdout?: (output: string) => Promise; emitLegacyOutcome: (result: AnnotateOutcome) => void; exit?: (code: number) => void; + logError?: (message: string) => void; } export function writeStdout(output: string): Promise { @@ -39,6 +41,7 @@ export async function completeAnnotateCommand({ writeStdout: outputWriter = writeStdout, emitLegacyOutcome, exit = process.exit, + logError = (message) => console.error(message), }: CompleteAnnotateCommandOptions): Promise { const result = await waitForDecision(); await settleAfterDecision(); @@ -46,10 +49,20 @@ export async function completeAnnotateCommand({ if (requireApproval || resultFile) { const serialized = serializeStrictAnnotateResult(result); - if (resultFile) { - await writeResultFile(resultFile, serialized); + try { + if (resultFile) { + await writeResultFile(resultFile, serialized); + } + await outputWriter(`${serialized}\n`); + } catch (error) { + // Publication failed: no decision record was delivered, so this is an + // environment error ("the gate could not deliver a decision"), not a + // reviewer outcome. Exit 2 — fail-closed, but distinct from exit 1's + // "gate ran and the reviewer did not approve". + logError(error instanceof Error ? error.message : String(error)); + exit(STRICT_GATE_ERROR_EXIT_CODE); + return; } - await outputWriter(`${serialized}\n`); exit(annotateOutcomeExitCode(result, requireApproval)); return; } diff --git a/apps/hook/server/cli.ts b/apps/hook/server/cli.ts index c38e0e97d..4ade5d4ab 100644 --- a/apps/hook/server/cli.ts +++ b/apps/hook/server/cli.ts @@ -146,7 +146,8 @@ const SUBCOMMAND_HELP: Record = { " --json Emit a structured decision JSON on stdout", " --hook Emit hook-native JSON (block/pass) for PostToolUse/Stop hooks", " --require-approval", - " Exit nonzero unless the reviewer approves (requires --gate --json)", + " Exit 1 unless the reviewer approves (requires --gate --json;", + " usage/startup errors exit 2)", " --result-file ", " Atomically publish the stdout JSON (requires --gate --json)", ].join("\n"), diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 8ac62c20e..9d8cf3573 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -148,6 +148,7 @@ import { completeAnnotateCommand } from "./annotate-command"; import { assertResultPathAvailable, resolveResultFilePath, + STRICT_GATE_ERROR_EXIT_CODE, } from "./strict-annotate-result"; import path from "path"; import { tmpdir } from "os"; @@ -170,7 +171,8 @@ try { ); } catch (error) { console.error(error instanceof Error ? error.message : String(error)); - process.exit(1); + // Usage error: the gate was misconfigured, not a reviewer decision. + process.exit(STRICT_GATE_ERROR_EXIT_CODE); } const args = parsedStrictAnnotateOptions.remainingArgs; const requireApprovalFlag = @@ -941,7 +943,8 @@ if (args[0] === "sessions") { await assertResultPathAvailable(resultFile); } catch (error) { console.error(error instanceof Error ? error.message : String(error)); - process.exit(1); + // Startup validation error: the gate could not start. + process.exit(STRICT_GATE_ERROR_EXIT_CODE); } } diff --git a/apps/hook/server/strict-annotate-result.test.ts b/apps/hook/server/strict-annotate-result.test.ts index d71b703f8..f0f14cee5 100644 --- a/apps/hook/server/strict-annotate-result.test.ts +++ b/apps/hook/server/strict-annotate-result.test.ts @@ -18,6 +18,7 @@ import { assertResultPathAvailable, resolveResultFilePath, serializeStrictAnnotateResult, + STRICT_GATE_ERROR_EXIT_CODE, writeAnnotateResultFile, } from "./strict-annotate-result"; @@ -87,6 +88,15 @@ describe("strict annotate exit policy", () => { annotateOutcomeExitCode({ exit: true, feedback: "" }, false), ).toBe(0); }); + + test("reserves exit 2 for gate errors, distinct from decision outcomes", () => { + // grep convention: 0 = approved, 1 = negative human outcome, + // 2 = the gate itself was misconfigured or could not start. + expect(STRICT_GATE_ERROR_EXIT_CODE).toBe(2); + expect( + annotateOutcomeExitCode({ exit: true, feedback: "" }, true), + ).toBe(1); + }); }); describe("atomic annotate result publication", () => { diff --git a/apps/hook/server/strict-annotate-result.ts b/apps/hook/server/strict-annotate-result.ts index 28ff86376..b2ccf4bd1 100644 --- a/apps/hook/server/strict-annotate-result.ts +++ b/apps/hook/server/strict-annotate-result.ts @@ -14,6 +14,15 @@ export interface AnnotateOutcome { approved?: boolean; } +/** + * Exit code for gate errors, following the grep convention: + * `0` = approved, `1` = negative human outcome (annotated/dismissed under + * `--require-approval`), `2` = the gate itself was misconfigured or could not + * start/deliver a decision (usage, startup, validation, and publication + * failures). A gate error never publishes a decision record. + */ +export const STRICT_GATE_ERROR_EXIT_CODE = 2; + export function serializeStrictAnnotateResult( result: AnnotateOutcome, ): string { diff --git a/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md b/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md index 8a94cc6f8..6a98cf639 100644 --- a/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md +++ b/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md @@ -136,4 +136,4 @@ See [Hook Integration](/docs/guides/hook-integration/) for copy-paste recipes th ## Exit codes -By default, every decision exits `0`; existing plaintext, JSON, and hook integrations are unchanged. With `--require-approval`, only `approved` exits `0`. `annotated` and `dismissed` publish their JSON result before exiting nonzero. +By default, every decision exits `0`; existing plaintext, JSON, and hook integrations are unchanged. With `--require-approval`, only `approved` exits `0`; `annotated` and `dismissed` publish their JSON result before exiting `1`. Strict invocations that are misconfigured or cannot start or deliver a decision — bad flag combinations, an invalid `--result-file` destination, or a failed atomic publish — exit `2` without publishing a decision, following the grep convention (`0` approved, `1` not approved, `2` the gate itself errored). From c57ebea59a8b2d8a784a1bdc89f86a89f6c86252 Mon Sep 17 00:00:00 2001 From: rNoz Date: Fri, 24 Jul 2026 09:24:07 +0200 Subject: [PATCH 3/4] feat(annotate): preserve notes on structured approval --- apps/hook/server/annotate-output.test.ts | 51 +++++ apps/hook/server/annotate-output.ts | 59 ++++++ apps/hook/server/index.ts | 65 +++--- .../annotate-gates-and-json-responses.md | 18 +- .../content/docs/guides/custom-feedback.md | 24 ++- apps/opencode-plugin/cli-bridge.test.ts | 77 +++++++ apps/opencode-plugin/cli-bridge.ts | 82 ++++++-- apps/opencode-plugin/commands.test.ts | 132 ++++++++++++ apps/opencode-plugin/commands.ts | 55 +++-- apps/opencode-plugin/embedded.test.ts | 52 +++++ apps/opencode-plugin/embedded.ts | 37 +++- apps/opencode-plugin/index.ts | 25 +-- .../prompt-delivery-error.test.ts | 44 ++++ apps/opencode-plugin/prompt-delivery-error.ts | 54 +++++ apps/pi-extension/annotate-outcome.test.ts | 51 +++++ apps/pi-extension/annotate-outcome.ts | 33 +++ apps/pi-extension/index.ts | 75 ++++--- apps/pi-extension/plannotator-browser.ts | 1 + apps/pi-extension/server.test.ts | 110 ++++++++++ apps/pi-extension/server/serverAnnotate.ts | 53 ++++- packages/editor/App.tsx | 188 +++++++++++++----- packages/editor/annotateSubmission.test.ts | 149 ++++++++++++++ packages/editor/annotateSubmission.ts | 156 +++++++++++++++ packages/editor/components/AppHeader.tsx | 8 +- packages/server/annotate.test.ts | 120 +++++++++++ packages/server/annotate.ts | 37 +++- packages/shared/config.ts | 1 + packages/shared/prompts-integration.test.ts | 22 ++ packages/shared/prompts.test.ts | 89 ++++++++- packages/shared/prompts.ts | 25 +++ 30 files changed, 1714 insertions(+), 179 deletions(-) create mode 100644 apps/hook/server/annotate-output.test.ts create mode 100644 apps/hook/server/annotate-output.ts create mode 100644 apps/opencode-plugin/embedded.test.ts create mode 100644 apps/opencode-plugin/prompt-delivery-error.test.ts create mode 100644 apps/opencode-plugin/prompt-delivery-error.ts create mode 100644 apps/pi-extension/annotate-outcome.test.ts create mode 100644 apps/pi-extension/annotate-outcome.ts create mode 100644 packages/editor/annotateSubmission.test.ts create mode 100644 packages/editor/annotateSubmission.ts diff --git a/apps/hook/server/annotate-output.test.ts b/apps/hook/server/annotate-output.test.ts new file mode 100644 index 000000000..2129fdecf --- /dev/null +++ b/apps/hook/server/annotate-output.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, test } from "bun:test"; +import { + formatAnnotateOutcome, + supportsAnnotateApprovalNotes, +} from "./annotate-output"; + +describe("annotate stdout", () => { + test("preserves legacy plaintext output byte-for-byte", () => { + expect(formatAnnotateOutcome( + { feedback: "", approved: true }, + { hook: false, json: false }, + )).toBe("The user approved."); + expect(formatAnnotateOutcome( + { feedback: "", exit: true }, + { hook: false, json: false }, + )).toBeNull(); + expect(formatAnnotateOutcome( + { feedback: "Revise this.", approved: false }, + { hook: false, json: false }, + )).toBe("Revise this."); + }); + + test("preserves legacy hook output byte-for-byte", () => { + expect(formatAnnotateOutcome( + { feedback: "Keep the retry bounded.", approved: true }, + { hook: true, json: true }, + )).toBeNull(); + expect(formatAnnotateOutcome( + { feedback: "Revise this." }, + { hook: true, json: false }, + )).toBe('{"decision":"block","reason":"Revise this."}'); + }); + + test("includes nonempty feedback only on direct JSON approval", () => { + expect(formatAnnotateOutcome( + { feedback: "Keep the retry bounded.", approved: true }, + { hook: false, json: true }, + )).toBe('{"decision":"approved","feedback":"Keep the retry bounded."}'); + expect(formatAnnotateOutcome( + { feedback: "", approved: true }, + { hook: false, json: true }, + )).toBe('{"decision":"approved"}'); + }); + + test("advertises approval notes only for gated direct JSON", () => { + expect(supportsAnnotateApprovalNotes({ gate: true, json: true, hook: false })).toBe(true); + expect(supportsAnnotateApprovalNotes({ gate: false, json: true, hook: false })).toBe(false); + expect(supportsAnnotateApprovalNotes({ gate: true, json: false, hook: false })).toBe(false); + expect(supportsAnnotateApprovalNotes({ gate: true, json: true, hook: true })).toBe(false); + }); +}); diff --git a/apps/hook/server/annotate-output.ts b/apps/hook/server/annotate-output.ts new file mode 100644 index 000000000..a9c4a3fc4 --- /dev/null +++ b/apps/hook/server/annotate-output.ts @@ -0,0 +1,59 @@ +import type { AnnotateOutcome } from "./strict-annotate-result"; + +export type { AnnotateOutcome } from "./strict-annotate-result"; + +export interface AnnotateOutputOptions { + hook: boolean; + json: boolean; +} + +export interface AnnotateApprovalCapabilityOptions extends AnnotateOutputOptions { + gate: boolean; +} + +const APPROVED_PLAINTEXT_MARKER = "The user approved."; + +export function supportsAnnotateApprovalNotes( + options: AnnotateApprovalCapabilityOptions, +): boolean { + return options.gate && options.json && !options.hook; +} + +export function formatAnnotateOutcome( + result: AnnotateOutcome, + options: AnnotateOutputOptions, +): string | null { + if (options.hook) { + if (result.approved || result.exit) return null; + return result.feedback + ? JSON.stringify({ decision: "block", reason: result.feedback }) + : null; + } + + if (options.json) { + if (result.approved) { + return JSON.stringify({ + decision: "approved", + ...(result.feedback ? { feedback: result.feedback } : {}), + }); + } + if (result.exit) return JSON.stringify({ decision: "dismissed" }); + return JSON.stringify({ + decision: "annotated", + feedback: result.feedback || "", + }); + } + + if (result.exit) return null; + if (result.approved) return APPROVED_PLAINTEXT_MARKER; + return result.feedback || null; +} + +export function createAnnotateOutcomeEmitter( + options: AnnotateOutputOptions, +): (result: AnnotateOutcome) => void { + return (result) => { + const output = formatAnnotateOutcome(result, options); + if (output !== null) console.log(output); + }; +} diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index 9d8cf3573..5b7d57743 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -153,6 +153,10 @@ import { import path from "path"; import { tmpdir } from "os"; import { buildLocalWorkspaceReview, type WorkspaceDiffType } from "@plannotator/server/review-workspace"; +import { + createAnnotateOutcomeEmitter, + supportsAnnotateApprovalNotes, +} from "./annotate-output"; // Embed the built HTML at compile time // @ts-ignore - Bun import attribute for text @@ -229,42 +233,10 @@ if (renderMarkdownFlag) args.splice(renderMarkdownIdx, 1); // Plaintext (default): // Close → empty. Approve → "The user approved." Annotate → feedback. // -// TODO: The plaintext --gate approval sentinel must stay as the exact string -// "The user approved." because slash command templates (plannotator-annotate.md, -// plannotator-last.md) instruct the agent to match it literally. Making this -// configurable requires updating those templates to accept dynamic values or -// switching gate mode to structured output only. -const APPROVED_PLAINTEXT_MARKER = "The user approved."; - -function emitAnnotateOutcome(result: { - feedback: string; - exit?: boolean; - approved?: boolean; -}): void { - if (hookFlag) { - if (result.approved || result.exit) return; - if (result.feedback) { - console.log(JSON.stringify({ decision: "block", reason: result.feedback })); - } - return; - } - if (jsonFlag) { - if (result.approved) { - console.log(JSON.stringify({ decision: "approved" })); - } else if (result.exit) { - console.log(JSON.stringify({ decision: "dismissed" })); - } else { - console.log(JSON.stringify({ decision: "annotated", feedback: result.feedback || "" })); - } - return; - } - if (result.exit) return; - if (result.approved) { - console.log(APPROVED_PLAINTEXT_MARKER); - return; - } - if (result.feedback) console.log(result.feedback); -} +const emitAnnotateOutcome = createAnnotateOutcomeEmitter({ + hook: hookFlag, + json: jsonFlag, +}); async function loadGoalSetupBundle( stage: GoalSetupStage, @@ -432,7 +404,10 @@ function emitOpenCodeAnnotateOutcome(result: { feedbackScope?: "message" | "messages"; }): void { if (result.approved) { - console.log(JSON.stringify({ decision: "approved" })); + console.log(JSON.stringify({ + decision: "approved", + ...(result.feedback ? { feedback: result.feedback } : {}), + })); return; } if (result.exit) { @@ -1080,6 +1055,11 @@ if (args[0] === "sessions") { shareBaseUrl, pasteApiUrl, gate: gateFlag, + approvalNotesSupported: supportsAnnotateApprovalNotes({ + gate: gateFlag, + json: jsonFlag, + hook: hookFlag, + }), rawHtml, renderHtml: !!rawHtml, convertHtml: renderMarkdownFlag, @@ -1279,6 +1259,11 @@ if (args[0] === "sessions") { shareBaseUrl, pasteApiUrl, gate: gateFlag, + approvalNotesSupported: supportsAnnotateApprovalNotes({ + gate: gateFlag, + json: jsonFlag, + hook: hookFlag, + }), htmlContent: planHtmlContent, recentMessages: pickerMessages, onReady: async (url, isRemote, port) => { @@ -1623,6 +1608,7 @@ if (args[0] === "sessions") { shareBaseUrl: bridgeShareBaseUrl, pasteApiUrl: bridgePasteApiUrl, gate: input.gate === true, + approvalNotesSupported: input.gate === true, htmlContent: planHtmlContent, onReady: (url, isRemote, port) => { handleAnnotateServerReady(url, isRemote, port); @@ -1774,6 +1760,11 @@ if (args[0] === "sessions") { sharingEnabled, shareBaseUrl, gate: gateFlag, + approvalNotesSupported: supportsAnnotateApprovalNotes({ + gate: gateFlag, + json: jsonFlag, + hook: hookFlag, + }), htmlContent: planHtmlContent, onReady: async (url, isRemote, port) => { handleAnnotateServerReady(url, isRemote, port); diff --git a/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md b/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md index 6a98cf639..9057d1c9f 100644 --- a/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md +++ b/apps/marketing/src/content/docs/guides/annotate-gates-and-json-responses.md @@ -24,7 +24,7 @@ section: "Guides" (none) │ 2-button │ n/a │ empty │ feedback (plaintext) --gate │ 3-button │ `The user approved.` │ empty │ feedback (plaintext) --json │ 2-button │ n/a │ {"decision":"dismissed"}│ {"decision":"annotated","feedback":"..."} - --gate --json │ 3-button │ {"decision":"approved"}│ {"decision":"dismissed"}│ {"decision":"annotated","feedback":"..."} + --gate --json │ 3-button │ {"decision":"approved","feedback":"..."}│ {"decision":"dismissed"}│ {"decision":"annotated","feedback":"..."} --hook │ 3-button │ empty │ empty │ {"decision":"block","reason":"..."} ``` @@ -33,7 +33,7 @@ section: "Guides" ```json { "decision": "approved" | "annotated" | "dismissed", - "feedback": "string (present only when decision is 'annotated')" + "feedback": "string (present for annotated decisions and approvals with notes)" } ``` @@ -45,6 +45,12 @@ section: "Guides" {"decision":"approved"} ``` +If the reviewer approves while leaving notes, direct structured transport preserves both: + +```json +{"decision":"approved","feedback":"Keep the retry bounded."} +``` + **Dismissed** (reviewer clicked Close, `--json` or `--gate --json`): ```json @@ -80,6 +86,9 @@ Structured stdout. Every decision is emitted as a JSON object with a `decision` - `--json` alone keeps the two-button UI. Only `annotated` and `dismissed` decisions are emitted. - `--gate --json` unlocks all three decisions in structured form. +- Direct `--gate --json` approval can include feedback. Transports that can + deliver approval but not attached notes warn before discarding feedback and + direct the reviewer to **Send Feedback** instead. - On OpenCode and Pi, `--json` is accepted silently. Those harnesses write back to the session directly rather than via stdout, so the flag has no effect there. Recipes remain portable. ## `--hook` @@ -92,6 +101,11 @@ Emits hook-native JSON that works directly with Claude Code and Codex PostToolUs This is the recommended approach for hook integrations. The `{"decision":"block","reason":"..."}` format is the native protocol both Claude Code and Codex use for PostToolUse and Stop hooks. No wrapper script needed. +`--hook` remains intentionally unchanged: approval is represented by empty +stdout in the native hook protocol, so it has no channel for approval notes. +Use **Send Feedback** to block with notes; that action still means “revise and +reopen,” not “approve and continue.” + The flag is accepted silently on OpenCode and Pi for the same reason `--json` is: those harnesses don't use stdout as the signal channel. ## Strict direct gates diff --git a/apps/marketing/src/content/docs/guides/custom-feedback.md b/apps/marketing/src/content/docs/guides/custom-feedback.md index b99aed8f9..beac2c484 100644 --- a/apps/marketing/src/content/docs/guides/custom-feedback.md +++ b/apps/marketing/src/content/docs/guides/custom-feedback.md @@ -49,6 +49,9 @@ These are sent when you annotate a file (`/plannotator-annotate`) or the last as |-----|---------------|-------------------| | `fileFeedback` | You annotate a file or folder | `{{fileHeader}}`, `{{filePath}}`, `{{feedback}}` | | `messageFeedback` | You annotate the last assistant message | `{{feedback}}` | +| `approvedWithNotes` | You approve a capable annotation gate with notes | `{{context}}`, `{{feedback}}` | + +`approvedWithNotes` is separate from ordinary annotation feedback so approval notes remain non-blocking. For file and folder annotations, `{{context}}` contains the target label and path (for example, `File: src/app.ts`); for message annotations it is empty. ### Review feedback @@ -74,6 +77,7 @@ Templates use `{{variable}}` placeholders. Here's what each one contains: | `{{doneMsg}}` | Optional checklist instruction or save-path info, depending on the runtime. | | `{{fileHeader}}` | Either `"File"` or `"Folder"`, depending on what was annotated. | | `{{filePath}}` | Path to the annotated file or folder. | +| `{{context}}` | Optional approval-note target context (`File: …` or `Folder: …`); empty for message annotations. | If you use a `{{variable}}` that doesn't exist for that message type, it stays in the output as-is. This means you can include literal `{{text}}` in your templates without worrying about it being stripped. @@ -125,7 +129,8 @@ Here's a config that customizes several messages at once: }, "annotate": { "fileFeedback": "# Annotations for {{filePath}}\n\n{{feedback}}\n\nPlease address these.", - "messageFeedback": "{{feedback}}\n\nRevise your response based on these notes." + "messageFeedback": "{{feedback}}\n\nRevise your response based on these notes.", + "approvedWithNotes": "The artifact is approved. These notes are not a request for another revision:\n\n{{context}}\n\n{{feedback}}\n\nDo not revise or reopen solely because of them unless explicitly requested. Carry them into subsequent work where applicable." }, "review": { "approved": "Code review passed. No changes needed." @@ -173,6 +178,23 @@ write). Execute the plan in {{planFilePath}}. {{doneMsg}} Please address the annotation feedback above. ``` +**Annotate approved with notes (default):** + +``` +# Approved with Notes + +The artifact is approved. The notes below are non-blocking guidance, +not a request for another revision. + +{{context}} + +{{feedback}} + +Do not revise or reopen the artifact solely because of these notes +unless the user explicitly requests it. Carry the notes into +subsequent work where applicable. +``` + **Review feedback suffix (default):** ``` diff --git a/apps/opencode-plugin/cli-bridge.test.ts b/apps/opencode-plugin/cli-bridge.test.ts index 50188e3aa..9bd0a3a86 100644 --- a/apps/opencode-plugin/cli-bridge.test.ts +++ b/apps/opencode-plugin/cli-bridge.test.ts @@ -4,13 +4,17 @@ import { tmpdir } from "node:os"; import path from "node:path"; import { buildAnnotateCliArgs, + buildAnnotatePromptFromBridgeOutcome, buildCliBridgeEnv, buildCliSpawnConfig, buildReviewPromptFromBridgeOutcome, + canLaunchGatedAnnotate, formatUserFacingCliStderrLine, getRecentAssistantMessages, + injectSessionPrompt, } from "./cli-bridge"; import { getReviewDeniedSuffix } from "@plannotator/shared/prompts"; +import { OpenCodePromptDeliveryError } from "./prompt-delivery-error"; describe("OpenCode CLI bridge helpers", () => { test("maps OpenCode sharing context into child CLI env", () => { @@ -71,6 +75,79 @@ describe("OpenCode CLI bridge helpers", () => { ]); }); + test("requires a session before launching a gated capable annotate bridge", () => { + expect(canLaunchGatedAnnotate({ gate: true }, undefined)).toBe(false); + expect(canLaunchGatedAnnotate({ gate: true }, "session-1")).toBe(true); + expect(canLaunchGatedAnnotate({ gate: false }, undefined)).toBe(true); + }); + + test("formats approved feedback as non-blocking notes while retaining file context", () => { + const outcome = { + decision: "approved" as const, + feedback: "Keep the retry bounded.", + }; + + const filePrompt = buildAnnotatePromptFromBridgeOutcome(outcome, { + kind: "file", + fileHeader: "File", + filePath: "plan.md", + }); + expect(filePrompt).toContain("artifact is approved"); + expect(filePrompt).toContain("non-blocking guidance"); + expect(filePrompt).toContain("File: plan.md"); + expect(filePrompt).toContain("Keep the retry bounded."); + expect(filePrompt).not.toContain("Please address"); + + const messagePrompt = buildAnnotatePromptFromBridgeOutcome(outcome, { + kind: "message", + }); + expect(messagePrompt).toContain("artifact is approved"); + expect(messagePrompt).toContain("Keep the retry bounded."); + expect(messagePrompt).not.toContain("File:"); + expect(messagePrompt).not.toContain("Please address"); + + expect(buildAnnotatePromptFromBridgeOutcome({ + decision: "approved", + feedback: "", + }, { + kind: "message", + })).toBeNull(); + }); + + test("keeps annotated feedback on the ordinary revision-request prompt path", () => { + const prompt = buildAnnotatePromptFromBridgeOutcome({ + decision: "annotated", + feedback: "Tighten the timeout.", + }, { + kind: "message", + }); + + expect(prompt).toContain("Please address"); + expect(prompt).not.toContain("artifact is approved"); + }); + + test("classifies CLI bridge prompt-delivery failures for fallback prevention", async () => { + const client = { + app: { log: mock(() => {}) }, + session: { + prompt: mock(async () => { + throw new Error("session busy"); + }), + }, + }; + + try { + await injectSessionPrompt(client, "session-1", "Approved notes"); + throw new Error("Expected prompt delivery to fail"); + } catch (error) { + expect(error).toBeInstanceOf(OpenCodePromptDeliveryError); + } + expect(client.app.log).toHaveBeenCalledWith({ + level: "error", + message: expect.stringContaining("Could not deliver Plannotator feedback"), + }); + }); + test("surfaces remote share-link stderr lines and ignores noisy stderr", () => { expect(formatUserFacingCliStderrLine(" Open this link on your local machine to review the plan:")).toBe( "Open this link on your local machine to review the plan:", diff --git a/apps/opencode-plugin/cli-bridge.ts b/apps/opencode-plugin/cli-bridge.ts index 1efbdb6d3..d7063cad8 100644 --- a/apps/opencode-plugin/cli-bridge.ts +++ b/apps/opencode-plugin/cli-bridge.ts @@ -5,11 +5,16 @@ import path from "node:path"; import { randomUUID } from "node:crypto"; import { parseAnnotateArgs, type ParsedAnnotateArgs } from "@plannotator/shared/annotate-args"; import { + getAnnotateApprovedWithNotesPrompt, getAnnotateFileFeedbackPrompt, getAnnotateMessageFeedbackPrompt, getReviewApprovedPrompt, getReviewDeniedSuffix, } from "@plannotator/shared/prompts"; +import { + deliverOpenCodePrompt, + isOpenCodePromptDeliveryError, +} from "./prompt-delivery-error"; type LogLevel = "info" | "error"; @@ -67,7 +72,7 @@ interface CliSpawnConfig { shell: false; } -interface CliAnnotateOutcome { +export interface CliAnnotateOutcome { decision?: "approved" | "dismissed" | "annotated"; feedback?: string; selectedMessageId?: string; @@ -451,6 +456,13 @@ export function buildAnnotateCliArgs(parsed: ParsedAnnotateArgs): string[] { return args; } +export function canLaunchGatedAnnotate( + parsed: Pick, + sessionId: string | undefined, +): boolean { + return !parsed.gate || Boolean(sessionId); +} + export async function runCliPlanReview(input: { client: OpenCodeClient; planContent: string; @@ -481,25 +493,25 @@ export async function runCliPlanReview(input: { return parseLastJson(result.stdout); } -async function injectSessionPrompt( +export async function injectSessionPrompt( client: OpenCodeClient, sessionId: string | undefined, text: string, options?: { agent?: string; noReply?: boolean }, ): Promise { if (!sessionId || !text.trim()) return; - try { - await client.session?.prompt?.({ + await deliverOpenCodePrompt({ + client, + prompt: { path: { id: sessionId }, body: { ...(options?.agent && { agent: options.agent }), ...(options?.noReply && { noReply: true }), parts: [{ type: "text", text }], }, - }); - } catch { - // Session may be unavailable or busy. - } + }, + failureMessage: "Could not deliver Plannotator feedback to the OpenCode session.", + }); } export async function getRecentAssistantMessages( @@ -576,6 +588,35 @@ function getAnnotateFileHeader(filePath: string, cwd?: string): "File" | "Folder } } +export function buildAnnotatePromptFromBridgeOutcome( + outcome: CliAnnotateOutcome, + target: + | { kind: "file"; fileHeader: "File" | "Folder"; filePath: string } + | { kind: "message" }, +): string | null { + if (outcome.decision === "dismissed" || !outcome.feedback?.trim()) return null; + if (outcome.decision !== "annotated" && outcome.decision !== "approved") return null; + + if (outcome.decision === "approved") { + return getAnnotateApprovedWithNotesPrompt("opencode", undefined, { + context: target.kind === "file" + ? `${target.fileHeader}: ${target.filePath}` + : undefined, + feedback: outcome.feedback, + }); + } + + return target.kind === "message" + ? getAnnotateMessageFeedbackPrompt("opencode", undefined, { + feedback: outcome.feedback, + }) + : getAnnotateFileFeedbackPrompt("opencode", undefined, { + fileHeader: target.fileHeader, + filePath: target.filePath, + feedback: outcome.feedback, + }); +} + export async function handleCliCommand(input: { command: string; client: OpenCodeClient; @@ -619,6 +660,10 @@ export async function handleCliCommand(input: { log(input.client, "error", "Usage: /plannotator-annotate [--markdown] [--no-jina] [--gate] [--json]"); return; } + if (!canLaunchGatedAnnotate(parsed, input.sessionId)) { + log(input.client, "error", "No active session."); + return; + } const result = await runPlannotatorCli({ client: input.client, @@ -634,15 +679,16 @@ export async function handleCliCommand(input: { logCliWarnings(input.client, result.stderr); const outcome = parseLastJson(result.stdout); - if (outcome.decision === "annotated" && outcome.feedback) { + const prompt = buildAnnotatePromptFromBridgeOutcome(outcome, { + kind: "file", + fileHeader: getAnnotateFileHeader(parsed.filePath, input.cwd), + filePath: parsed.filePath, + }); + if (prompt) { await injectSessionPrompt( input.client, input.sessionId, - getAnnotateFileFeedbackPrompt("opencode", undefined, { - fileHeader: getAnnotateFileHeader(parsed.filePath, input.cwd), - filePath: parsed.filePath, - feedback: outcome.feedback, - }), + prompt, ); } return; @@ -680,11 +726,14 @@ export async function handleCliCommand(input: { logCliWarnings(input.client, result.stderr); const outcome = parseLastJson(result.stdout); - if (outcome.decision === "annotated" && outcome.feedback) { + const prompt = buildAnnotatePromptFromBridgeOutcome(outcome, { + kind: "message", + }); + if (prompt) { await injectSessionPrompt( input.client, input.sessionId, - getAnnotateMessageFeedbackPrompt("opencode", undefined, { feedback: outcome.feedback }), + prompt, ); } return; @@ -692,5 +741,6 @@ export async function handleCliCommand(input: { } catch (error) { log(input.client, "error", `[Plannotator] ${error instanceof Error ? error.message : String(error)}`); + if (isOpenCodePromptDeliveryError(error)) throw error; } } diff --git a/apps/opencode-plugin/commands.test.ts b/apps/opencode-plugin/commands.test.ts index 6c4c3b94d..4789cf676 100644 --- a/apps/opencode-plugin/commands.test.ts +++ b/apps/opencode-plugin/commands.test.ts @@ -3,6 +3,7 @@ import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs"; import { tmpdir } from "os"; import path from "path"; import { handleAnnotateCommand, handleAnnotateLastCommand } from "./commands"; +import { OpenCodePromptDeliveryError } from "./prompt-delivery-error"; // Inject the annotate-server stub through CommandDeps rather than // `mock.module`. Bun's module mocks are process-global and cannot be unset, @@ -54,6 +55,102 @@ afterEach(() => { }); describe("handleAnnotateCommand", () => { + test("advertises approval notes only when an OpenCode session is available", async () => { + const projectRoot = makeTempDir(); + const filePath = path.join(projectRoot, "plan.md"); + writeFileSync(filePath, "# Plan\n"); + + const withSession = makeDeps(); + withSession.directory = projectRoot; + await handleAnnotateCommand( + { properties: { arguments: "plan.md --gate", sessionID: "session-123" } }, + withSession, + ); + expect(startAnnotateServerMock.mock.calls[0]?.[0].approvalNotesSupported).toBe(true); + + startAnnotateServerMock.mockClear(); + const withoutSession = makeDeps(); + withoutSession.directory = projectRoot; + await handleAnnotateCommand( + { properties: { arguments: "plan.md --gate" } }, + withoutSession, + ); + expect(startAnnotateServerMock.mock.calls[0]?.[0].approvalNotesSupported).toBe(false); + }); + + test("injects approved feedback as non-blocking notes with file context", async () => { + const projectRoot = makeTempDir(); + const filePath = path.join(projectRoot, "plan.md"); + writeFileSync(filePath, "# Plan\n"); + const deps: any = makeDeps(); + deps.directory = projectRoot; + deps.startAnnotateServer = mock(async (options: any) => ({ + port: 0, + url: "http://localhost", + isRemote: false, + options, + waitForDecision: async () => ({ + approved: true, + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + }), + stop: () => {}, + })); + + await handleAnnotateCommand( + { properties: { arguments: "plan.md --gate", sessionID: "session-123" } }, + deps, + ); + + expect(deps.client.session.prompt).toHaveBeenCalledTimes(1); + const prompt = deps.client.session.prompt.mock.calls[0]?.[0].body.parts[0].text; + expect(prompt).toContain("artifact is approved"); + expect(prompt).toContain("non-blocking guidance"); + expect(prompt).toContain(`File: ${filePath}`); + expect(prompt).toContain("Keep the retry bounded."); + expect(prompt).not.toContain("Please address"); + }); + + test("logs and rejects when approved file notes cannot be injected", async () => { + const projectRoot = makeTempDir(); + const filePath = path.join(projectRoot, "plan.md"); + writeFileSync(filePath, "# Plan\n"); + const deps: any = makeDeps(); + deps.directory = projectRoot; + deps.client.session.prompt = mock(async () => { + throw new Error("session busy"); + }); + deps.startAnnotateServer = mock(async () => ({ + port: 0, + url: "http://localhost", + isRemote: false, + waitForDecision: async () => ({ + approved: true, + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + }), + stop: () => {}, + })); + + try { + await handleAnnotateCommand( + { properties: { arguments: "plan.md --gate", sessionID: "session-123" } }, + deps, + ); + throw new Error("Expected prompt delivery to fail"); + } catch (error) { + expect(error).toBeInstanceOf(OpenCodePromptDeliveryError); + expect(error).toHaveProperty( + "message", + "Could not deliver approved annotation notes to the OpenCode session.", + ); + } + expect(deps.client.app.log).toHaveBeenCalledWith({ + level: "error", + message: expect.stringContaining("Could not deliver approved annotation notes"), + }); + }); + test("strips wrapping quotes from HTML paths and forwards pasteApiUrl", async () => { const projectRoot = makeTempDir(); const docsDir = path.join(projectRoot, "docs"); @@ -132,6 +229,41 @@ describe("handleAnnotateCommand", () => { }); describe("handleAnnotateLastCommand", () => { + test("returns approved feedback and advertises support for an active session", async () => { + const deps: any = makeDeps(); + deps.client.session.messages = mock(async (_input: unknown) => ({ + data: [ + { + info: { role: "assistant" }, + parts: [{ type: "text", text: "Latest assistant message" }], + }, + ], + })); + deps.startAnnotateServer = mock(async (options: any) => ({ + port: 0, + url: "http://localhost", + isRemote: false, + options, + waitForDecision: async () => ({ + approved: true, + feedback: "Retain this caveat.", + annotations: [{ id: "a1" }], + }), + stop: () => {}, + })); + + const outcome = await handleAnnotateLastCommand( + { properties: { sessionID: "session-123", arguments: "--gate" } }, + deps, + ); + + expect(deps.startAnnotateServer.mock.calls[0]?.[0].approvalNotesSupported).toBe(true); + expect(outcome).toEqual({ + approved: true, + feedback: "Retain this caveat.", + }); + }); + test("forwards pasteApiUrl for annotate-last sessions", async () => { const deps = makeDeps(); deps.client.session.messages = mock(async (_input: unknown) => ({ diff --git a/apps/opencode-plugin/commands.ts b/apps/opencode-plugin/commands.ts index d40a6757d..ca2892c36 100644 --- a/apps/opencode-plugin/commands.ts +++ b/apps/opencode-plugin/commands.ts @@ -18,6 +18,7 @@ import { detectProjectName } from "@plannotator/server/project"; import { parsePRUrl, checkPRAuth, fetchPR, getCliName, getMRLabel, getMRNumberLabel, getDisplayRepo } from "@plannotator/server/pr"; import { loadConfig, resolveDefaultDiffType, resolveUseJina } from "@plannotator/shared/config"; import { + getAnnotateApprovedWithNotesPrompt, getReviewApprovedPrompt, getReviewDeniedSuffix, getAnnotateFileFeedbackPrompt, @@ -31,6 +32,7 @@ import { urlToMarkdown, isConvertedSource } from "@plannotator/shared/url-to-mar import { buildLocalWorkspaceReview, type WorkspaceDiffType } from "@plannotator/server/review-workspace"; import { statSync } from "fs"; import path from "path"; +import { deliverOpenCodePrompt } from "./prompt-delivery-error"; /** Shared dependencies injected by the plugin */ export interface CommandDeps { @@ -213,6 +215,8 @@ export async function handleAnnotateCommand( // parseAnnotateArgs strips leading @ on filePath (reference-mode convention). // `rawFilePath` preserves it for the scoped-package markdown fallback. const { filePath, rawFilePath, gate, renderMarkdown: renderMarkdownFlag, noJina } = parseAnnotateArgs(rawArgs); + // @ts-ignore - Event properties contain sessionID + const sessionId = event.properties?.sessionID; if (!filePath) { client.app.log({ level: "error", message: "Usage: /plannotator-annotate [--markdown] [--no-jina] [--gate] [--json]" }); @@ -335,6 +339,7 @@ export async function handleAnnotateCommand( shareBaseUrl: getShareBaseUrl(), pasteApiUrl: getPasteApiUrl(), gate, + approvalNotesSupported: Boolean(sessionId), agentCwd, htmlContent, onReady: (url, isRemote, port) => { @@ -347,46 +352,50 @@ export async function handleAnnotateCommand( await Bun.sleep(1500); server.stop(); - // Both exit and approve are "no-op for the agent" — skip session injection. - if (result.exit || result.approved) { + if (result.exit || (result.approved && !result.feedback)) { return; } if (result.feedback) { - // @ts-ignore - Event properties contain sessionID - const sessionId = event.properties?.sessionID; - if (sessionId) { - try { - await client.session.prompt({ + const text = result.approved + ? getAnnotateApprovedWithNotesPrompt("opencode", undefined, { + context: `${isFolder ? "Folder" : "File"}: ${absolutePath}`, + feedback: result.feedback, + }) + : getAnnotateFileFeedbackPrompt("opencode", undefined, { + fileHeader: isFolder ? "Folder" : "File", + filePath: absolutePath, + feedback: result.feedback, + }); + await deliverOpenCodePrompt({ + client, + prompt: { path: { id: sessionId }, body: { parts: [{ type: "text", - text: getAnnotateFileFeedbackPrompt("opencode", undefined, { - fileHeader: isFolder ? "Folder" : "File", - filePath: absolutePath, - feedback: result.feedback, - }), + text, }], }, - }); - } catch { - // Session may not be available - } + }, + failureMessage: result.approved + ? "Could not deliver approved annotation notes to the OpenCode session." + : "Could not deliver annotation feedback to the OpenCode session.", + }); } } } /** * Handle /plannotator-last command. - * Called from command.execute.before — returns the feedback string - * so the caller can set it as output.parts for the agent to see. + * Called from command.execute.before — returns approval-aware feedback so the + * caller can choose the correct prompt semantics before injecting it. */ export async function handleAnnotateLastCommand( event: any, deps: CommandDeps -): Promise { +): Promise<{ approved: boolean; feedback: string } | null> { const { client, htmlContent, getSharingEnabled, getShareBaseUrl, getPasteApiUrl } = deps; const startServer = deps.startAnnotateServer ?? startAnnotateServer; @@ -448,6 +457,7 @@ export async function handleAnnotateLastCommand( shareBaseUrl: getShareBaseUrl(), pasteApiUrl: getPasteApiUrl(), gate, + approvalNotesSupported: true, htmlContent, onReady: (url, isRemote, port) => { handleAnnotateServerReady(url, isRemote, port); @@ -459,10 +469,11 @@ export async function handleAnnotateLastCommand( await Bun.sleep(1500); server.stop(); - // Both exit and approve signal "don't inject feedback" — return null. - if (result.exit || result.approved) { + if (result.exit || (result.approved && !result.feedback)) { return null; } - return result.feedback || null; + return result.feedback + ? { approved: Boolean(result.approved), feedback: result.feedback } + : null; } diff --git a/apps/opencode-plugin/embedded.test.ts b/apps/opencode-plugin/embedded.test.ts new file mode 100644 index 000000000..134003eed --- /dev/null +++ b/apps/opencode-plugin/embedded.test.ts @@ -0,0 +1,52 @@ +import { describe, expect, mock, test } from "bun:test"; +import { deliverEmbeddedAnnotateMessagePrompt } from "./embedded"; +import { OpenCodePromptDeliveryError } from "./prompt-delivery-error"; + +describe("embedded annotate prompt delivery", () => { + test("delivers approved message feedback as non-blocking notes", async () => { + const client = { + app: { log: mock(() => {}) }, + session: { prompt: mock(async () => {}) }, + }; + + await deliverEmbeddedAnnotateMessagePrompt({ + client, + sessionId: "session-1", + approved: true, + feedback: "Retain this caveat.", + }); + + const prompt = client.session.prompt.mock.calls[0]?.[0].body.parts[0].text; + expect(prompt).toContain("artifact is approved"); + expect(prompt).toContain("non-blocking guidance"); + expect(prompt).toContain("Retain this caveat."); + expect(prompt).not.toContain("Please address"); + }); + + test("logs and rejects when approved message notes cannot be injected", async () => { + const client = { + app: { log: mock(() => {}) }, + session: { + prompt: mock(async () => { + throw new Error("session busy"); + }), + }, + }; + + try { + await deliverEmbeddedAnnotateMessagePrompt({ + client, + sessionId: "session-1", + approved: true, + feedback: "Retain this caveat.", + }); + throw new Error("Expected prompt delivery to fail"); + } catch (error) { + expect(error).toBeInstanceOf(OpenCodePromptDeliveryError); + } + expect(client.app.log).toHaveBeenCalledWith({ + level: "error", + message: expect.stringContaining("Could not deliver approved annotation notes"), + }); + }); +}); diff --git a/apps/opencode-plugin/embedded.ts b/apps/opencode-plugin/embedded.ts index d1d5e75a9..871ba2664 100644 --- a/apps/opencode-plugin/embedded.ts +++ b/apps/opencode-plugin/embedded.ts @@ -3,6 +3,11 @@ import { waitForPlanReviewCloseDelay, waitForPlanReviewDecision, } from "@plannotator/shared/plan-review-lifecycle"; +import { + getAnnotateApprovedWithNotesPrompt, + getAnnotateMessageFeedbackPrompt, +} from "@plannotator/shared/prompts"; +import { deliverOpenCodePrompt } from "./prompt-delivery-error"; export interface EmbeddedPlanReviewInput { client: any; @@ -33,6 +38,34 @@ async function loadCommandHandlers() { return await import("./commands"); } +export async function deliverEmbeddedAnnotateMessagePrompt(input: { + client: any; + sessionId: string; + approved: boolean; + feedback: string; +}): Promise { + const text = input.approved + ? getAnnotateApprovedWithNotesPrompt("opencode", undefined, { + feedback: input.feedback, + }) + : getAnnotateMessageFeedbackPrompt("opencode", undefined, { + feedback: input.feedback, + }); + + await deliverOpenCodePrompt({ + client: input.client, + prompt: { + path: { id: input.sessionId }, + body: { + parts: [{ type: "text", text }], + }, + }, + failureMessage: input.approved + ? "Could not deliver approved annotation notes to the OpenCode session." + : "Could not deliver annotation feedback to the OpenCode session.", + }); +} + export async function runEmbeddedPlanReview( input: EmbeddedPlanReviewInput, ): Promise { @@ -83,7 +116,7 @@ export async function handleEmbeddedCommand( getPasteApiUrl: () => string | undefined; directory?: string; }, -): Promise<{ feedback?: string | null }> { +): Promise<{ approved?: boolean; feedback?: string | null }> { const { handleReviewCommand, handleAnnotateCommand, @@ -91,7 +124,7 @@ export async function handleEmbeddedCommand( } = await loadCommandHandlers(); if (command === "plannotator-last") { - return { feedback: await handleAnnotateLastCommand(event, deps) }; + return await handleAnnotateLastCommand(event, deps) ?? {}; } if (command === "plannotator-annotate") { diff --git a/apps/opencode-plugin/index.ts b/apps/opencode-plugin/index.ts index 48f56fa41..81365e28d 100644 --- a/apps/opencode-plugin/index.ts +++ b/apps/opencode-plugin/index.ts @@ -25,7 +25,6 @@ import { getPlanApprovedPrompt, getPlanApprovedWithNotesPrompt, getPlanToolName, - getAnnotateMessageFeedbackPrompt, } from "@plannotator/shared/prompts"; import { loadConfig, resolveSharingEnabled } from "@plannotator/shared/config"; import { readImprovementHook } from "@plannotator/shared/improvement-hooks"; @@ -60,6 +59,7 @@ import { type OpenCodeBridgeContext, type OpenCodePlanReviewResult, } from "./cli-bridge"; +import { shouldFallbackAfterEmbeddedError } from "./prompt-delivery-error"; // Lazy-load HTML at first use instead of embedding in the bundle. // The two SPA files are ~20 MB combined — inlining them as string literals @@ -542,23 +542,18 @@ Do NOT proceed with implementation until your plan is approved.`); }; const result = await embedded.handleEmbeddedCommand(cmd, event, deps); if (cmd === "plannotator-last" && result.feedback) { - try { - await ctx.client.session.prompt({ - path: { id: input.sessionID }, - body: { - parts: [{ - type: "text", - text: getAnnotateMessageFeedbackPrompt("opencode", undefined, { feedback: result.feedback }), - }], - }, - }); - } catch { - // Session may not be available - } + await embedded.deliverEmbeddedAnnotateMessagePrompt({ + client: ctx.client, + sessionId: input.sessionID, + approved: Boolean(result.approved), + feedback: result.feedback, + }); } return; } catch (error) { - if (workflowOptions.runtime === "embedded") throw error; + if (!shouldFallbackAfterEmbeddedError(workflowOptions.runtime, error)) { + throw error; + } try { void ctx.client.app.log({ level: "error", diff --git a/apps/opencode-plugin/prompt-delivery-error.test.ts b/apps/opencode-plugin/prompt-delivery-error.test.ts new file mode 100644 index 000000000..d5b58d2e8 --- /dev/null +++ b/apps/opencode-plugin/prompt-delivery-error.test.ts @@ -0,0 +1,44 @@ +import { describe, expect, test } from "bun:test"; +import { + deliverOpenCodePrompt, + OpenCodePromptDeliveryError, + shouldFallbackAfterEmbeddedError, +} from "./prompt-delivery-error"; + +describe("OpenCode prompt-delivery errors", () => { + test("never falls back after prompt delivery fails", () => { + const deliveryError = new OpenCodePromptDeliveryError("delivery failed"); + + expect(shouldFallbackAfterEmbeddedError("auto", deliveryError)).toBe(false); + expect(shouldFallbackAfterEmbeddedError("embedded", deliveryError)).toBe(false); + }); + + test("recognizes delivery errors across separately bundled runtimes", () => { + const bundledError = Object.assign(new Error("delivery failed"), { + name: "OpenCodePromptDeliveryError", + code: "PLANNOTATOR_OPENCODE_PROMPT_DELIVERY", + }); + + expect(shouldFallbackAfterEmbeddedError("auto", bundledError)).toBe(false); + }); + + test("keeps existing fallback policy for runtime startup failures", () => { + const startupError = new Error("embedded bundle unavailable"); + + expect(shouldFallbackAfterEmbeddedError("auto", startupError)).toBe(true); + expect(shouldFallbackAfterEmbeddedError("embedded", startupError)).toBe(false); + }); + + test("treats a missing session prompt API as a delivery failure", async () => { + const client = { + app: { log: () => {} }, + session: {}, + }; + + await expect(deliverOpenCodePrompt({ + client, + prompt: { path: { id: "session-1" } }, + failureMessage: "Could not deliver notes.", + })).rejects.toBeInstanceOf(OpenCodePromptDeliveryError); + }); +}); diff --git a/apps/opencode-plugin/prompt-delivery-error.ts b/apps/opencode-plugin/prompt-delivery-error.ts new file mode 100644 index 000000000..58b1a015c --- /dev/null +++ b/apps/opencode-plugin/prompt-delivery-error.ts @@ -0,0 +1,54 @@ +const OPENCODE_PROMPT_DELIVERY_ERROR_CODE = + "PLANNOTATOR_OPENCODE_PROMPT_DELIVERY"; + +export class OpenCodePromptDeliveryError extends Error { + readonly code = OPENCODE_PROMPT_DELIVERY_ERROR_CODE; + + constructor(message: string, options?: ErrorOptions) { + super(message, options); + this.name = "OpenCodePromptDeliveryError"; + } +} + +export function isOpenCodePromptDeliveryError( + error: unknown, +): error is OpenCodePromptDeliveryError { + return error instanceof OpenCodePromptDeliveryError + || ( + error instanceof Error + && (error as Error & { code?: string }).code + === OPENCODE_PROMPT_DELIVERY_ERROR_CODE + ); +} + +export function shouldFallbackAfterEmbeddedError( + runtime: string, + error: unknown, +): boolean { + return runtime !== "embedded" && !isOpenCodePromptDeliveryError(error); +} + +export async function deliverOpenCodePrompt(input: { + client: any; + prompt: unknown; + failureMessage: string; +}): Promise { + try { + if (typeof input.client.session?.prompt !== "function") { + throw new Error("OpenCode session prompt API is unavailable."); + } + await input.client.session.prompt(input.prompt); + } catch (error) { + try { + void input.client.app?.log?.({ + level: "error", + message: `[Plannotator] ${input.failureMessage}`, + }); + } catch { + // Preserve the delivery failure if logging is unavailable. + } + throw new OpenCodePromptDeliveryError(input.failureMessage, { + cause: error, + }); + } +} diff --git a/apps/pi-extension/annotate-outcome.test.ts b/apps/pi-extension/annotate-outcome.test.ts new file mode 100644 index 000000000..920c95f62 --- /dev/null +++ b/apps/pi-extension/annotate-outcome.test.ts @@ -0,0 +1,51 @@ +import { describe, expect, test } from "bun:test"; +import { classifyAnnotateOutcome } from "./annotate-outcome"; + +describe("Pi annotate outcomes", () => { + test("delivers approved file feedback before the approval notification", () => { + expect(classifyAnnotateOutcome({ + feedback: "Keep the retry bounded.", + approved: true, + })).toEqual({ + feedback: "Keep the retry bounded.", + notification: "approved", + promptKind: "approved-with-notes", + }); + }); + + test("delivers approved last-message feedback before the approval notification", () => { + expect(classifyAnnotateOutcome({ + feedback: "Retain this caveat.", + approved: true, + selectedMessageId: "message-2", + })).toEqual({ + feedback: "Retain this caveat.", + notification: "approved", + promptKind: "approved-with-notes", + }); + }); + + test("keeps no-feedback approval as a notification-only outcome", () => { + expect(classifyAnnotateOutcome({ + feedback: "", + approved: true, + })).toEqual({ + feedback: null, + notification: "approved", + promptKind: null, + }); + }); + + test("keeps ordinary feedback and exits distinct", () => { + expect(classifyAnnotateOutcome({ feedback: "Revise this." })).toEqual({ + feedback: "Revise this.", + notification: null, + promptKind: "feedback", + }); + expect(classifyAnnotateOutcome({ feedback: "", exit: true })).toEqual({ + feedback: null, + notification: "closed", + promptKind: null, + }); + }); +}); diff --git a/apps/pi-extension/annotate-outcome.ts b/apps/pi-extension/annotate-outcome.ts new file mode 100644 index 000000000..75a9ac096 --- /dev/null +++ b/apps/pi-extension/annotate-outcome.ts @@ -0,0 +1,33 @@ +export interface PiAnnotateDecision { + feedback: string; + exit?: boolean; + approved?: boolean; + selectedMessageId?: string; + feedbackScope?: "message" | "messages"; +} + +export interface ClassifiedAnnotateOutcome { + feedback: string | null; + notification: "approved" | "closed" | null; + promptKind: "approved-with-notes" | "feedback" | null; +} + +export function classifyAnnotateOutcome( + result: PiAnnotateDecision, +): ClassifiedAnnotateOutcome { + if (result.exit) { + return { feedback: null, notification: "closed", promptKind: null }; + } + if (result.approved) { + return { + feedback: result.feedback || null, + notification: "approved", + promptKind: result.feedback ? "approved-with-notes" : null, + }; + } + return { + feedback: result.feedback || null, + notification: null, + promptKind: result.feedback ? "feedback" : null, + }; +} diff --git a/apps/pi-extension/index.ts b/apps/pi-extension/index.ts index a43c28dcf..a4e405a38 100644 --- a/apps/pi-extension/index.ts +++ b/apps/pi-extension/index.ts @@ -68,6 +68,7 @@ import { stripPlanningOnlyTools, } from "./tool-scope.ts"; import { isRemoteSession } from "./server/network.ts"; +import { classifyAnnotateOutcome } from "./annotate-outcome.ts"; // ── Types ────────────────────────────────────────────────────────────── @@ -650,30 +651,44 @@ export default function plannotator(pi: ExtensionAPI): void { .waitForDecision() .then(async (result) => { try { - if (result.exit) { + const outcome = classifyAnnotateOutcome(result); + if (outcome.notification === "closed") { safeNotify(ctx, "Annotation session closed.", "info", origin); return; } - if (result.approved) { - safeNotify(ctx, "Annotation approved.", "info", origin); - return; - } - if (!result.feedback) { + if (!outcome.feedback) { + if (outcome.notification === "approved") { + safeNotify(ctx, "Annotation approved.", "info", origin); + return; + } safeNotify(ctx, "Annotation closed (no feedback).", "info", origin); return; } - const { getAnnotateFileFeedbackPrompt } = await loadPlannotatorPrompts(); + const { + getAnnotateApprovedWithNotesPrompt, + getAnnotateFileFeedbackPrompt, + } = await loadPlannotatorPrompts(); + const context = `${isFolder ? "Folder" : "File"}: ${absolutePath}`; + const prompt = outcome.promptKind === "approved-with-notes" + ? getAnnotateApprovedWithNotesPrompt("pi", loadConfig(), { + context, + feedback: outcome.feedback, + }) + : getAnnotateFileFeedbackPrompt("pi", loadConfig(), { + fileHeader: isFolder ? "Folder" : "File", + filePath: absolutePath, + feedback: outcome.feedback, + }); sendUserMessageWithCurrentSessionFallback( pi, - getAnnotateFileFeedbackPrompt("pi", loadConfig(), { - fileHeader: isFolder ? "Folder" : "File", - filePath: absolutePath, - feedback: result.feedback, - }), + prompt, { deliverAs: "followUp" }, "Plannotator annotation feedback could not be sent", origin, ); + if (outcome.notification === "approved") { + safeNotify(ctx, "Annotation approved.", "info", origin); + } } catch (err) { reportBackgroundError(ctx, "Plannotator annotation feedback could not be sent", err, origin); } @@ -726,15 +741,16 @@ export default function plannotator(pi: ExtensionAPI): void { .waitForDecision() .then(async (result) => { try { - if (result.exit) { + const outcome = classifyAnnotateOutcome(result); + if (outcome.notification === "closed") { safeNotify(ctx, "Annotation session closed.", "info", origin); return; } - if (result.approved) { - safeNotify(ctx, "Message approved.", "info", origin); - return; - } - if (!result.feedback) { + if (!outcome.feedback) { + if (outcome.notification === "approved") { + safeNotify(ctx, "Message approved.", "info", origin); + return; + } safeNotify(ctx, "Annotation closed (no feedback).", "info", origin); return; } @@ -744,18 +760,29 @@ export default function plannotator(pi: ExtensionAPI): void { ? findAssistantMessageByEntryId(ctx, result.selectedMessageId) ?? snapshot : snapshot; const feedback = result.feedbackScope !== "messages" && shouldAnchorLastMessageFeedback(ctx, target.entryId, origin) - ? anchorMessageFeedback(result.feedback, target.text) - : result.feedback; - const { getAnnotateMessageFeedbackPrompt } = await loadPlannotatorPrompts(); + ? anchorMessageFeedback(outcome.feedback, target.text) + : outcome.feedback; + const { + getAnnotateApprovedWithNotesPrompt, + getAnnotateMessageFeedbackPrompt, + } = await loadPlannotatorPrompts(); + const prompt = outcome.promptKind === "approved-with-notes" + ? getAnnotateApprovedWithNotesPrompt("pi", loadConfig(), { + feedback, + }) + : getAnnotateMessageFeedbackPrompt("pi", loadConfig(), { + feedback, + }); sendUserMessageWithCurrentSessionFallback( pi, - getAnnotateMessageFeedbackPrompt("pi", loadConfig(), { - feedback, - }), + prompt, { deliverAs: "followUp" }, "Plannotator message annotation feedback could not be sent", origin, ); + if (outcome.notification === "approved") { + safeNotify(ctx, "Message approved.", "info", origin); + } } catch (err) { reportBackgroundError(ctx, "Plannotator message annotation feedback could not be sent", err, origin); } diff --git a/apps/pi-extension/plannotator-browser.ts b/apps/pi-extension/plannotator-browser.ts index e3155e1ad..a58b73d0f 100644 --- a/apps/pi-extension/plannotator-browser.ts +++ b/apps/pi-extension/plannotator-browser.ts @@ -589,6 +589,7 @@ export async function startMarkdownAnnotationSession( sourceInfo, sourceConverted, gate, + approvalNotesSupported: true, rawHtml, renderHtml, convertHtml, diff --git a/apps/pi-extension/server.test.ts b/apps/pi-extension/server.test.ts index c10dbb7cc..d28e412bf 100644 --- a/apps/pi-extension/server.test.ts +++ b/apps/pi-extension/server.test.ts @@ -325,6 +325,116 @@ describe("pi startup file-cache warm", () => { }); }); +describe("pi annotate approval notes", () => { + test("returns the explicit approval-notes capability", async () => { + for (const approvalNotesSupported of [true, false]) { + delete process.env.PLANNOTATOR_PORT; + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(makeTempDir("plannotator-pi-approval-capability-"), "test.md"), + htmlContent: "annotate", + origin: "pi", + approvalNotesSupported, + }); + + try { + const response = await fetch(`${server.url}/api/plan`); + const plan = await response.json() as { approvalNotesSupported?: boolean }; + expect(plan.approvalNotesSupported).toBe(approvalNotesSupported); + } finally { + server.stop(); + } + } + }); + + test("preserves feedback and annotations on approval", async () => { + delete process.env.PLANNOTATOR_PORT; + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(makeTempDir("plannotator-pi-approval-notes-"), "test.md"), + htmlContent: "annotate", + origin: "pi", + approvalNotesSupported: true, + }); + + try { + const response = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + draftGeneration: 3, + }), + }); + expect(response.status).toBe(200); + expect(await server.waitForDecision()).toEqual({ + approved: true, + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + }); + } finally { + server.stop(); + } + }); + + test("keeps bodyless approval compatible", async () => { + delete process.env.PLANNOTATOR_PORT; + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(makeTempDir("plannotator-pi-approval-bodyless-"), "test.md"), + htmlContent: "annotate", + origin: "pi", + }); + + try { + const response = await fetch(`${server.url}/api/approve`, { method: "POST" }); + expect(response.status).toBe(200); + expect(await server.waitForDecision()).toEqual({ + approved: true, + feedback: "", + annotations: [], + }); + } finally { + server.stop(); + } + }); + + test("rejects malformed or wrong-type approval bodies without resolving", async () => { + delete process.env.PLANNOTATOR_PORT; + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(makeTempDir("plannotator-pi-approval-invalid-"), "test.md"), + htmlContent: "annotate", + origin: "pi", + }); + + try { + const decision = server.waitForDecision(); + const malformed = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{", + }); + expect(malformed.status).toBe(400); + expect(await Promise.race([decision.then(() => "resolved"), Bun.sleep(25).then(() => "pending")])).toBe("pending"); + + const wrongType = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: 42, annotations: [] }), + }); + expect(wrongType.status).toBe(400); + expect(await Promise.race([decision.then(() => "resolved"), Bun.sleep(25).then(() => "pending")])).toBe("pending"); + + await fetch(`${server.url}/api/approve`, { method: "POST" }); + expect(await decision).toEqual({ approved: true, feedback: "", annotations: [] }); + } finally { + server.stop(); + } + }); +}); + describe("pi review server", () => { const testIfJj = hasJj() ? test : test.skip; const testIfUnix = process.platform === "win32" ? test.skip : test; diff --git a/apps/pi-extension/server/serverAnnotate.ts b/apps/pi-extension/server/serverAnnotate.ts index 768f6540a..c9aa5e2d3 100644 --- a/apps/pi-extension/server/serverAnnotate.ts +++ b/apps/pi-extension/server/serverAnnotate.ts @@ -1,4 +1,5 @@ import { createServer } from "node:http"; +import type { IncomingMessage } from "node:http"; import { dirname, resolve as resolvePath } from "node:path"; import { existsSync, readFileSync, statSync } from "node:fs"; import { randomUUID } from "node:crypto"; @@ -68,6 +69,31 @@ export interface AnnotateServerResult { stop: () => void; } +function parseOptionalApprovalBody(req: IncomingMessage): Promise> { + return new Promise((resolve, reject) => { + let data = ""; + req.on("data", (chunk) => { + data += chunk; + }); + req.on("error", reject); + req.on("end", () => { + if (!data.trim()) { + resolve({}); + return; + } + try { + const parsed = JSON.parse(data); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error("Expected a JSON object."); + } + resolve(parsed as Record); + } catch (err) { + reject(err); + } + }); + }); +} + function createHtmlAssetRegistry() { const rootsByToken = new Map(); const tokensByRoot = new Map(); @@ -174,6 +200,7 @@ export async function startAnnotateServer(options: { sourceInfo?: string; sourceConverted?: boolean; gate?: boolean; + approvalNotesSupported?: boolean; rawHtml?: string; renderHtml?: boolean; convertHtml?: boolean; @@ -417,6 +444,7 @@ export async function startAnnotateServer(options: { sourceConverted: options.sourceConverted ?? false, sourceSave: primarySource.sourceSave, gate: options.gate ?? false, + approvalNotesSupported: options.approvalNotesSupported ?? false, renderAs: displayRawHtml ? 'html' : 'markdown', ...(displayRawHtml ? { rawHtml: displayRawHtml } : {}), ...(diffHtml ? { diffHtml } : {}), @@ -630,9 +658,28 @@ export async function startAnnotateServer(options: { resolveDecision({ feedback: "", annotations: [], exit: true }); json(res, { ok: true }); } else if (url.pathname === "/api/approve" && req.method === "POST") { - deleteDraft(draftKey, readDraftGenerationFromUrl(req)); - resolveDecision({ feedback: "", annotations: [], approved: true }); - json(res, { ok: true }); + try { + const body = await parseOptionalApprovalBody(req); + if ( + (body.feedback !== undefined && typeof body.feedback !== "string") || + (body.annotations !== undefined && !Array.isArray(body.annotations)) || + (body.codeAnnotations !== undefined && !Array.isArray(body.codeAnnotations)) || + (body.draftGeneration !== undefined && typeof body.draftGeneration !== "number") + ) { + json(res, { error: "Invalid approval body." }, 400); + return; + } + + deleteDraft(draftKey, readDraftGenerationFromBody(body)); + resolveDecision({ + feedback: (body.feedback as string | undefined) || "", + annotations: (body.annotations as unknown[] | undefined) || [], + approved: true, + }); + json(res, { ok: true }); + } catch (err) { + json(res, { error: err instanceof Error ? err.message : "Invalid JSON body." }, 400); + } } else if (url.pathname === "/api/feedback" && req.method === "POST") { try { const body = await parseBody(req); diff --git a/packages/editor/App.tsx b/packages/editor/App.tsx index 3bebfc258..54ceeafb4 100644 --- a/packages/editor/App.tsx +++ b/packages/editor/App.tsx @@ -134,10 +134,14 @@ import { buildDirectEditsSection, buildSavedFileChangePanelItems, buildSavedFileChangesSection, - composeFeedbackWithEditSections, computeEditStats, normalizeEditedMarkdown, } from './directEdits'; +import { + buildAnnotateApprovalBody, + buildCompleteAnnotateFeedback, + getAnnotateApprovalPolicy, +} from './annotateSubmission'; import { editableDocumentKey, useEditableDocuments, @@ -295,6 +299,7 @@ const App: React.FC = () => { const [showFeedbackPrompt, setShowFeedbackPrompt] = useState(false); const [showClaudeCodeWarning, setShowClaudeCodeWarning] = useState(false); const [showExitWarning, setShowExitWarning] = useState(false); + const [showApproveWithNotesConfirmation, setShowApproveWithNotesConfirmation] = useState(false); const [showSourceFileEditWarning, setShowSourceFileEditWarning] = useState(false); const [sourceFileEditWarningAction, setSourceFileEditWarningAction] = useState('send-feedback'); const sourceFileEditWarningContinuationRef = useRef<(() => void | Promise) | null>(null); @@ -377,6 +382,7 @@ const App: React.FC = () => { const [globalAttachments, setGlobalAttachments] = useState([]); const [annotateMode, setAnnotateMode] = useState(false); const [gate, setGate] = useState(false); + const [approvalNotesSupported, setApprovalNotesSupported] = useState(false); const [annotateSource, setAnnotateSource] = useState<'file' | 'message' | 'folder' | null>(null); const [recentMessages, setRecentMessages] = useState([]); const [selectedMessageId, setSelectedMessageId] = useState(null); @@ -875,7 +881,7 @@ const App: React.FC = () => { if (document.querySelector('[data-plannotator-confirm-dialog="true"]')) return false; if (showExport || showImport || showFeedbackPrompt || showClaudeCodeWarning || showSourceFileEditWarning || - showExitWarning || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return false; + showExitWarning || showApproveWithNotesConfirmation || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return false; if (submitted || isSubmitting || isExiting || isEditingMarkdown) return false; const target = event.target as HTMLElement | null; @@ -891,6 +897,7 @@ const App: React.FC = () => { showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, + showApproveWithNotesConfirmation, showAgentWarning, showPermissionModeSetup, pendingPasteImage, @@ -1000,8 +1007,8 @@ const App: React.FC = () => { const buildMessageAnnotationEntries = React.useCallback((): MessageAnnotationEntry[] => { if (annotateSource !== 'message' || recentMessages.length === 0) return []; // Must be a PURE read: this runs on the render path via - // currentFeedbackPayload (useMemo) -> getCurrentFeedbackPayload -> - // buildFullAnnotationsOutput. saveCurrentMessageState() writes React state + // currentFeedbackPayload (useMemo) -> getCurrentFeedbackPayload. + // saveCurrentMessageState() writes React state // (setCachedMessageAnnotationCounts), which during render is an infinite // re-render loop in multi-message mode (#949). getMessageStatesWithCurrent // returns the same merged data without the setState side effect; the cache @@ -1310,17 +1317,6 @@ const App: React.FC = () => { linkedDocHook.docAnnotationCount + globalAttachments.length; - const buildFullAnnotationsOutput = React.useCallback((): string => { - if (messageMultiSelectMode) { - let output = exportMessageAnnotations(buildMessageAnnotationEntries()); - if (editorAnnotations.length > 0) { - output += `\n\n${exportEditorAnnotations(editorAnnotations)}`; - } - return output; - } - return ''; - }, [messageMultiSelectMode, buildMessageAnnotationEntries, editorAnnotations]); - const annotationsOutput = useMemo(() => { const docAnnotations = linkedDocHook.getDocAnnotations(); const hasDocAnnotations = Array.from(docAnnotations.values()).some( @@ -2062,20 +2058,50 @@ const App: React.FC = () => { ); }, [savedFileChanges]); - // Prepends the Direct Edits section to annotation feedback. When edits exist - // but there are no annotations, the "no feedback" sentinel is replaced rather - // than appended to. - const composeFeedback = useCallback((annotationsText: string, checkedSavedFileChanges = savedFileChanges): string => { - return composeFeedbackWithEditSections( - annotationsText, - buildEditsSection(), - buildSavedChangesSection(checkedSavedFileChanges), - ); - }, [buildEditsSection, buildSavedChangesSection]); - const getCurrentFeedbackPayload = useCallback((checkedSavedFileChanges = savedFileChanges): string => { - return composeFeedback(messageMultiSelectMode ? buildFullAnnotationsOutput() : annotationsOutput, checkedSavedFileChanges); - }, [annotationsOutput, buildFullAnnotationsOutput, composeFeedback, messageMultiSelectMode, savedFileChanges]); + const linkedDocuments = linkedDocHook.getDocAnnotations(); + const activeConverted = linkedDocHook.isActive + ? (linkedDocuments.get(linkedDocHook.filepath ?? '')?.isConverted ?? false) + : sourceConverted; + return buildCompleteAnnotateFeedback({ + blocks, + annotations: allAnnotations, + globalAttachments, + linkedDocuments, + editorAnnotations, + codeAnnotations, + title: annotateSource === 'message' + ? 'Message Feedback' + : annotateSource === 'folder' + ? 'Folder Feedback' + : annotateSource === 'file' + ? 'File Feedback' + : 'Plan Feedback', + subject: annotateSource ?? 'plan', + sourceConverted: activeConverted, + directEditsSection: buildEditsSection(), + savedFileChangesSection: buildSavedChangesSection(checkedSavedFileChanges), + ...(messageMultiSelectMode + ? { messageEntries: buildMessageAnnotationEntries() } + : {}), + }); + }, [ + allAnnotations, + annotateSource, + blocks, + buildEditsSection, + buildMessageAnnotationEntries, + buildSavedChangesSection, + codeAnnotations, + editorAnnotations, + globalAttachments, + linkedDocHook.filepath, + linkedDocHook.getDocAnnotations, + linkedDocHook.isActive, + messageMultiSelectMode, + savedFileChanges, + sourceConverted, + ]); const withDraftGeneration = useCallback((path: string): string => { const separator = path.includes('?') ? '&' : '?'; @@ -2254,7 +2280,7 @@ const App: React.FC = () => { if (!res.ok) throw new Error('Not in API mode'); return res.json(); }) - .then((data: { plan: string; origin?: Origin; mode?: 'annotate' | 'annotate-last' | 'annotate-folder' | 'archive' | 'goal-setup'; goalSetup?: GoalSetupBundle; filePath?: string; sourceInfo?: string; sourceConverted?: boolean; sourceSave?: SourceSaveCapability; gate?: boolean; renderAs?: 'html' | 'markdown'; rawHtml?: string; shareHtml?: string; diffHtml?: string; convertHtml?: boolean; sharingEnabled?: boolean; shareBaseUrl?: string; pasteApiUrl?: string; repoInfo?: { display: string; branch?: string; host?: string }; previousPlan?: string | null; versionInfo?: { version: number; totalVersions: number; project: string }; archivePlans?: ArchivedPlan[]; projectRoot?: string; isWSL?: boolean; serverConfig?: { displayName?: string; gitUser?: string }; recentMessages?: PickerMessage[]; agentTerminal?: AgentTerminalCapability; feedbackTemplates?: AnnotateFeedbackTemplates }) => { + .then((data: { plan: string; origin?: Origin; mode?: 'annotate' | 'annotate-last' | 'annotate-folder' | 'archive' | 'goal-setup'; goalSetup?: GoalSetupBundle; filePath?: string; sourceInfo?: string; sourceConverted?: boolean; sourceSave?: SourceSaveCapability; gate?: boolean; approvalNotesSupported?: boolean; renderAs?: 'html' | 'markdown'; rawHtml?: string; shareHtml?: string; diffHtml?: string; convertHtml?: boolean; sharingEnabled?: boolean; shareBaseUrl?: string; pasteApiUrl?: string; repoInfo?: { display: string; branch?: string; host?: string }; previousPlan?: string | null; versionInfo?: { version: number; totalVersions: number; project: string }; archivePlans?: ArchivedPlan[]; projectRoot?: string; isWSL?: boolean; serverConfig?: { displayName?: string; gitUser?: string }; recentMessages?: PickerMessage[]; agentTerminal?: AgentTerminalCapability; feedbackTemplates?: AnnotateFeedbackTemplates }) => { // Initialize config store with server-provided values (config file > cookie > default) configStore.init(data.serverConfig); // Session-level force-markdown preference (--markdown); threaded into folder/linked @@ -2298,6 +2324,7 @@ const App: React.FC = () => { if (data.mode === 'annotate' || data.mode === 'annotate-last' || data.mode === 'annotate-folder') { setAnnotateMode(true); setGate(data.gate ?? false); + setApprovalNotesSupported(data.approvalNotesSupported ?? false); } if (data.mode === 'annotate-folder') { sidebar.open('files'); @@ -2651,6 +2678,11 @@ const App: React.FC = () => { const hasFeedbackToSend = hasFeedbackContent && !isCurrentFeedbackDeliveredToAgent; + const annotateApprovalPolicy = getAnnotateApprovalPolicy({ + gate, + approvalNotesSupported, + hasFeedback: hasFeedbackToSend, + }); // API mode handlers const handleApprove = async () => { @@ -2834,15 +2866,52 @@ const App: React.FC = () => { } }; - // Annotate gate-mode handler — approves the artifact without feedback + // Annotate gate-mode handler — capable transports preserve complete feedback. const handleAnnotateApprove = async () => { setIsSubmitting(true); try { - await fetch(withDraftGeneration('/api/approve'), { method: 'POST' }); + snapshotActiveEditableDocument(); + const checkedSavedFileChanges = await validateSavedFileChangesBeforeSubmit(); + if (checkedSavedFileChanges === null) { + setIsSubmitting(false); + return; + } + // hasFeedbackToSend (not hasFeedbackContent) so notes already delivered + // via the agent terminal are not re-sent on approve. + const feedback = hasFeedbackToSend + ? getCurrentFeedbackPayload(checkedSavedFileChanges) + : ''; + const res = await fetch('/api/approve', { + method: 'POST', + headers: { 'Content-Type': 'application/json' }, + body: JSON.stringify(buildAnnotateApprovalBody({ + supported: approvalNotesSupported, + draftGeneration: getDraftGeneration(), + feedback, + annotations: allAnnotations, + codeAnnotations, + })), + }); + if (!res.ok) throw new Error('Failed to approve'); + dismissDraft(); setSubmitted('approved'); } catch { setIsSubmitting(false); + scheduleDraftSaveAfterSubmitFailure(); + } + }; + + const requestAnnotateApprove = () => { + if (hasFeedbackToSend && !approvalNotesSupported) { + setExitWarningAction('approve'); + setShowExitWarning(true); + return; + } + if (annotateApprovalPolicy.confirmation) { + setShowApproveWithNotesConfirmation(true); + return; } + handleAnnotateApprove(); }; // Exit annotation session without sending feedback @@ -2924,7 +2993,7 @@ const App: React.FC = () => { // Don't intercept if any modal is open if (showExport || showImport || showFeedbackPrompt || showClaudeCodeWarning || showSourceFileEditWarning || - showExitWarning || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return; + showExitWarning || showApproveWithNotesConfirmation || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return; // Don't intercept if already submitted, submitting, or exiting if (submitted || isSubmitting || isExiting || goalSetupAction.isSubmitting) return; @@ -2953,12 +3022,13 @@ const App: React.FC = () => { e.preventDefault(); - // Annotate mode: gate-enabled + no annotations → approve (empty stdout). - // Otherwise: send feedback. + // Annotate mode: gate-enabled + no annotations → approve. With feedback + // present, Mod+Enter always means Send Feedback — Approve-with-Notes is + // reachable only via the header button and its confirmation dialog. if (annotateMode) { if (gate && !hasFeedbackToSend) { - if (maybeConfirmUnsavedSourceFileEdits('approve', () => handleAnnotateApprove())) return; - handleAnnotateApprove(); + if (maybeConfirmUnsavedSourceFileEdits('approve', requestAnnotateApprove)) return; + requestAnnotateApprove(); return; } if (maybeConfirmUnsavedSourceFileEdits('send-feedback', () => handleAnnotateFeedback())) return; @@ -2993,10 +3063,10 @@ const App: React.FC = () => { window.addEventListener('keydown', handleKeyDown); return () => window.removeEventListener('keydown', handleKeyDown); }, [ - showExport, showImport, showFeedbackPrompt, showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, showAgentWarning, + showExport, showImport, showFeedbackPrompt, showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, showApproveWithNotesConfirmation, showAgentWarning, showPermissionModeSetup, pendingPasteImage, submitted, isSubmitting, isExiting, goalSetupAction.isSubmitting, isApiMode, isEditingMarkdown, linkedDocHook.isActive, annotations.length, codeAnnotations.length, externalAnnotations.length, annotateMode, - gate, hasFeedbackToSend, goalSetupMode, goalSetupAction.canSubmit, isAgentTerminalReady, + gate, approvalNotesSupported, hasFeedbackToSend, goalSetupMode, goalSetupAction.canSubmit, isAgentTerminalReady, annotateSource, origin, getAgentWarning, maybeConfirmUnsavedSourceFileEdits, ]); @@ -3657,7 +3727,7 @@ const App: React.FC = () => { if (showExport || showFeedbackPrompt || showClaudeCodeWarning || showSourceFileEditWarning || - showExitWarning || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return; + showExitWarning || showApproveWithNotesConfirmation || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return; if (submitted || !isApiMode) return; @@ -3691,7 +3761,7 @@ const App: React.FC = () => { window.addEventListener('keydown', handleSaveShortcut); return () => window.removeEventListener('keydown', handleSaveShortcut); }, [ - showExport, showFeedbackPrompt, showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, showAgentWarning, + showExport, showFeedbackPrompt, showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, showApproveWithNotesConfirmation, showAgentWarning, showPermissionModeSetup, pendingPasteImage, submitted, isApiMode, isEditingMarkdown, handleSaveEditedSourceFile, displayedMarkdown, annotationsOutput, ]); @@ -3706,7 +3776,7 @@ const App: React.FC = () => { if (showExport || showFeedbackPrompt || showClaudeCodeWarning || showSourceFileEditWarning || - showExitWarning || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return; + showExitWarning || showApproveWithNotesConfirmation || showAgentWarning || showPermissionModeSetup || pendingPasteImage) return; if (submitted) return; @@ -3717,7 +3787,7 @@ const App: React.FC = () => { window.addEventListener('keydown', handlePrintShortcut); return () => window.removeEventListener('keydown', handlePrintShortcut); }, [ - showExport, showFeedbackPrompt, showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, showAgentWarning, + showExport, showFeedbackPrompt, showClaudeCodeWarning, showSourceFileEditWarning, showExitWarning, showApproveWithNotesConfirmation, showAgentWarning, showPermissionModeSetup, pendingPasteImage, submitted, ]); @@ -3785,12 +3855,7 @@ const App: React.FC = () => { const approve = () => { const h = headerHandlersRef.current; if (annotateMode) { - if (hasFeedbackToSend) { - setExitWarningAction('approve'); - setShowExitWarning(true); - return; - } - h.handleAnnotateApprove(); + requestAnnotateApprove(); return; } if (origin === 'claude-code' && hasFeedbackToSend) { @@ -3809,7 +3874,7 @@ const App: React.FC = () => { }; if (maybeConfirmUnsavedSourceFileEdits('approve', approve)) return; approve(); - }, [annotateMode, hasFeedbackToSend, maybeConfirmUnsavedSourceFileEdits, origin]); + }, [annotateMode, maybeConfirmUnsavedSourceFileEdits, origin, requestAnnotateApprove]); const handleHeaderAnnotateFeedback = useCallback(() => { const sendFeedback = () => headerHandlersRef.current.handleAnnotateFeedback(); @@ -3818,10 +3883,9 @@ const App: React.FC = () => { }, [maybeConfirmUnsavedSourceFileEdits]); const handleHeaderAnnotateApprove = useCallback(() => { - const approve = () => headerHandlersRef.current.handleAnnotateApprove(); - if (maybeConfirmUnsavedSourceFileEdits('approve', approve)) return; - approve(); - }, [maybeConfirmUnsavedSourceFileEdits]); + if (maybeConfirmUnsavedSourceFileEdits('approve', requestAnnotateApprove)) return; + requestAnnotateApprove(); + }, [maybeConfirmUnsavedSourceFileEdits, requestAnnotateApprove]); const handleHeaderDownloadAnnotations = useCallback(() => headerHandlersRef.current.handleDownloadAnnotations(), []); const handleHeaderCopyAgentInstructions = useCallback(() => headerHandlersRef.current.handleCopyAgentInstructions(), []); const handleHeaderCopyShareLink = useCallback(() => headerHandlersRef.current.handleCopyShareLink(), []); @@ -3911,6 +3975,8 @@ const App: React.FC = () => { agentName={agentName} availableAgents={availableAgents} showAnnotationsWarning={hasFeedbackToSend} + annotateApproveLabel={annotateApprovalPolicy.label} + annotateApproveTitle={annotateApprovalPolicy.title} callbackConfig={callbackConfig} taterMode={taterMode} mobileSettingsOpen={mobileSettingsOpen} @@ -4680,6 +4746,22 @@ const App: React.FC = () => { showCancel /> + {/* Capable annotate gates distinguish approval notes from change requests. */} + setShowApproveWithNotesConfirmation(false)} + onConfirm={() => { + setShowApproveWithNotesConfirmation(false); + handleAnnotateApprove(); + }} + title={annotateApprovalPolicy.confirmation?.title ?? 'Approve with Notes?'} + message={annotateApprovalPolicy.confirmation?.message ?? ''} + confirmText={annotateApprovalPolicy.confirmation?.confirmText ?? 'Approve with Notes'} + cancelText="Cancel" + variant="warning" + showCancel + /> + {/* Unsent feedback warning dialog — reused by Close and (in gate mode) Approve */} { + test("includes notes only when the transport supports approval notes", () => { + const input = { + draftGeneration: 4, + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + codeAnnotations: [{ id: "c1" }], + }; + + expect(buildAnnotateApprovalBody({ supported: true, ...input })).toEqual(input); + expect(buildAnnotateApprovalBody({ supported: false, ...input })).toEqual({ + draftGeneration: 4, + }); + }); + + test("labels capable feedback approvals and requires a non-blocking confirmation", () => { + expect(getAnnotateApprovalPolicy({ + gate: true, + approvalNotesSupported: true, + hasFeedback: true, + })).toEqual({ + label: "Approve with Notes", + title: "Approve with Notes — send notes as non-blocking guidance", + confirmation: { + title: "Approve with Notes?", + message: "This approves the artifact, sends your notes as non-blocking guidance, and closes the gate. Unlike Send Feedback, it does not request changes.", + confirmText: "Approve with Notes", + }, + }); + }); + + test("keeps ordinary approval presentation when notes are absent or unsupported", () => { + expect(getAnnotateApprovalPolicy({ + gate: true, + approvalNotesSupported: true, + hasFeedback: false, + })).toEqual({ + label: "Approve", + title: "Approve — no changes requested", + confirmation: null, + }); + expect(getAnnotateApprovalPolicy({ + gate: true, + approvalNotesSupported: false, + hasFeedback: true, + })).toEqual({ + label: "Approve", + title: "Approve — no changes requested", + confirmation: null, + }); + }); + + test("composes every annotate feedback source into approval notes", () => { + const markdown = "# Retry\n\nRetry forever."; + const blocks = parseMarkdownToBlocks(markdown); + const paragraph = blocks.find((block) => block.type === "paragraph"); + if (!paragraph) throw new Error("expected paragraph block"); + + const annotation: Annotation = { + id: "a1", + blockId: paragraph.id, + startOffset: 0, + endOffset: 5, + type: AnnotationType.COMMENT, + text: "Keep the retry bounded.", + originalText: "Retry", + createdA: 1, + images: [{ path: "/tmp/retry.png", name: "retry-diagram" }], + }; + const linkedAnnotation: Annotation = { + ...annotation, + id: "linked-1", + text: "Update the linked runbook.", + originalText: "Runbook", + images: undefined, + }; + const linkedDocuments = new Map([ + ["/docs/runbook.md", { + annotations: [linkedAnnotation], + globalAttachments: [], + markdown: "# Runbook\n\nRunbook", + }], + ]); + const codeAnnotation: CodeAnnotation = { + id: "c1", + type: "comment", + filePath: "src/retry.ts", + lineStart: 8, + lineEnd: 8, + side: "new", + text: "Cap this loop.", + originalCode: "while (true)", + createdAt: 1, + }; + const editorAnnotation: EditorAnnotation = { + id: "e1", + filePath: "src/config.ts", + selectedText: "MAX_RETRIES", + lineStart: 3, + lineEnd: 3, + comment: "Make the limit configurable.", + createdAt: 1, + }; + + const feedback = buildCompleteAnnotateFeedback({ + blocks, + annotations: [annotation], + globalAttachments: [{ path: "/tmp/global.png", name: "global-reference" }], + linkedDocuments, + editorAnnotations: [editorAnnotation], + codeAnnotations: [codeAnnotation], + title: "File Feedback", + subject: "file", + sourceConverted: false, + directEditsSection: "# Direct Edits\n\nBound the retry loop.", + savedFileChangesSection: "# Saved File Changes\n\n## /docs/retry.md", + }); + + expect(feedback).toContain("retry-diagram"); + expect(feedback).toContain("global-reference"); + expect(feedback).toContain("# Code File Feedback"); + expect(feedback).toContain("# Direct Edits"); + expect(feedback).toContain("# Linked Document Feedback"); + expect(feedback).toContain("# Editor File Annotations"); + expect(feedback).toContain("# Saved File Changes"); + + expect(buildAnnotateApprovalBody({ + supported: true, + draftGeneration: 4, + feedback, + annotations: [annotation], + codeAnnotations: [codeAnnotation], + })).toMatchObject({ + draftGeneration: 4, + feedback, + annotations: [annotation], + codeAnnotations: [codeAnnotation], + }); + }); +}); diff --git a/packages/editor/annotateSubmission.ts b/packages/editor/annotateSubmission.ts new file mode 100644 index 000000000..42622381d --- /dev/null +++ b/packages/editor/annotateSubmission.ts @@ -0,0 +1,156 @@ +import type { + Annotation, + Block, + CodeAnnotation, + EditorAnnotation, + ImageAttachment, +} from "@plannotator/ui/types"; +import { + exportAnnotations, + exportCodeFileAnnotations, + exportEditorAnnotations, + exportLinkedDocAnnotations, + exportMessageAnnotations, + parseMarkdownToBlocks, + type LinkedDocAnnotationEntry, + type MessageAnnotationEntry, +} from "@plannotator/ui/utils/parser"; +import { composeFeedbackWithEditSections } from "./directEdits"; + +export interface AnnotateApprovalBodyInput { + supported: boolean; + draftGeneration: number; + feedback: string; + annotations: unknown[]; + codeAnnotations: unknown[]; +} + +export interface AnnotateApprovalPolicy { + label: string; + title: string; + confirmation: { + title: string; + message: string; + confirmText: string; + } | null; +} + +export function getAnnotateApprovalPolicy(input: { + gate: boolean; + approvalNotesSupported: boolean; + hasFeedback: boolean; +}): AnnotateApprovalPolicy { + if (input.gate && input.approvalNotesSupported && input.hasFeedback) { + return { + label: "Approve with Notes", + title: "Approve with Notes — send notes as non-blocking guidance", + confirmation: { + title: "Approve with Notes?", + message: "This approves the artifact, sends your notes as non-blocking guidance, and closes the gate. Unlike Send Feedback, it does not request changes.", + confirmText: "Approve with Notes", + }, + }; + } + return { + label: "Approve", + title: "Approve — no changes requested", + confirmation: null, + }; +} + +export function buildAnnotateApprovalBody( + input: AnnotateApprovalBodyInput, +): { + draftGeneration: number; + feedback?: string; + annotations?: unknown[]; + codeAnnotations?: unknown[]; +} { + if (!input.supported) { + return { draftGeneration: input.draftGeneration }; + } + return { + draftGeneration: input.draftGeneration, + feedback: input.feedback, + annotations: input.annotations, + codeAnnotations: input.codeAnnotations, + }; +} + +export interface CompleteAnnotateFeedbackInput { + blocks: Block[]; + annotations: Annotation[]; + globalAttachments: ImageAttachment[]; + linkedDocuments: Map; + editorAnnotations: EditorAnnotation[]; + codeAnnotations: CodeAnnotation[]; + title: string; + subject: string; + sourceConverted: boolean; + directEditsSection: string; + savedFileChangesSection: string; + messageEntries?: MessageAnnotationEntry[]; +} + +export function buildCompleteAnnotateFeedback( + input: CompleteAnnotateFeedbackInput, +): string { + let annotationsText: string; + + if (input.messageEntries) { + annotationsText = exportMessageAnnotations(input.messageEntries); + if (input.editorAnnotations.length > 0) { + annotationsText += `\n\n${exportEditorAnnotations(input.editorAnnotations)}`; + } + } else { + const hasLinkedAnnotations = Array.from(input.linkedDocuments.values()).some( + (entry) => entry.annotations.length > 0 || entry.globalAttachments.length > 0, + ); + const hasDocumentAnnotations = + input.annotations.length > 0 || input.globalAttachments.length > 0; + const hasEditorAnnotations = input.editorAnnotations.length > 0; + const hasCodeAnnotations = input.codeAnnotations.length > 0; + + if ( + !hasDocumentAnnotations && + !hasLinkedAnnotations && + !hasEditorAnnotations && + !hasCodeAnnotations + ) { + annotationsText = "User reviewed the document and has no feedback."; + } else { + annotationsText = hasDocumentAnnotations + ? exportAnnotations( + input.blocks, + input.annotations, + input.globalAttachments, + input.title, + input.subject, + { sourceConverted: input.sourceConverted }, + ) + : ""; + + if (hasLinkedAnnotations) { + const enriched = new Map(); + for (const [filepath, entry] of input.linkedDocuments) { + enriched.set(filepath, entry.markdown + ? { ...entry, blocks: parseMarkdownToBlocks(entry.markdown) } + : entry); + } + annotationsText += exportLinkedDocAnnotations(enriched); + } + if (hasEditorAnnotations) { + annotationsText += exportEditorAnnotations(input.editorAnnotations); + } + if (hasCodeAnnotations) { + annotationsText += exportCodeFileAnnotations(input.codeAnnotations); + } + } + } + + return composeFeedbackWithEditSections( + annotationsText, + input.directEditsSection, + input.savedFileChangesSection, + ); +} diff --git a/packages/editor/components/AppHeader.tsx b/packages/editor/components/AppHeader.tsx index 24a1b8639..a03b58e27 100644 --- a/packages/editor/components/AppHeader.tsx +++ b/packages/editor/components/AppHeader.tsx @@ -43,6 +43,8 @@ interface AppHeaderProps { agentName: string; availableAgents: Agent[]; showAnnotationsWarning: boolean; + annotateApproveLabel: string; + annotateApproveTitle: string; // Callback config (null when no bot callback) callbackConfig: CallbackConfig | null; @@ -119,6 +121,8 @@ export const AppHeader = React.memo(({ agentName, availableAgents, showAnnotationsWarning, + annotateApproveLabel, + annotateApproveTitle, callbackConfig, taterMode, mobileSettingsOpen, @@ -281,7 +285,9 @@ export const AppHeader = React.memo(({ disabled={isSubmitting || (annotateMode && isExiting)} isLoading={isSubmitting} dimmed={!annotateMode && (origin === 'claude-code' || origin === 'gemini-cli') && showAnnotationsWarning} - title={annotateMode ? 'Approve — no changes requested' : undefined} + label={annotateMode ? annotateApproveLabel : undefined} + mobileLabel={annotateMode ? annotateApproveLabel : undefined} + title={annotateMode ? annotateApproveTitle : undefined} /> {!annotateMode && (origin === 'claude-code' || origin === 'gemini-cli') && showAnnotationsWarning && (
diff --git a/packages/server/annotate.test.ts b/packages/server/annotate.test.ts index 86b3034c4..459912d8b 100644 --- a/packages/server/annotate.test.ts +++ b/packages/server/annotate.test.ts @@ -503,3 +503,123 @@ describe("annotate server: source save", () => { } }); }); + +describe("annotate server: approval notes", () => { + let savedPort: string | undefined; + let savedRemote: string | undefined; + + beforeEach(() => { + savedPort = process.env.PLANNOTATOR_PORT; + savedRemote = process.env.PLANNOTATOR_REMOTE; + delete process.env.PLANNOTATOR_PORT; + process.env.PLANNOTATOR_REMOTE = "0"; + }); + + afterEach(() => { + if (savedPort === undefined) delete process.env.PLANNOTATOR_PORT; + else process.env.PLANNOTATOR_PORT = savedPort; + if (savedRemote === undefined) delete process.env.PLANNOTATOR_REMOTE; + else process.env.PLANNOTATOR_REMOTE = savedRemote; + }); + + test("returns the explicit approval-notes capability", async () => { + for (const approvalNotesSupported of [true, false]) { + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(tmpdir(), "approval-capability.md"), + htmlContent: MINIMAL_HTML, + approvalNotesSupported, + }); + + try { + const response = await fetch(`${server.url}/api/plan`); + const plan = await response.json() as { approvalNotesSupported?: boolean }; + expect(plan.approvalNotesSupported).toBe(approvalNotesSupported); + } finally { + server.stop(); + } + } + }); + + test("preserves feedback and annotations on approval", async () => { + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(tmpdir(), "approval-notes.md"), + htmlContent: MINIMAL_HTML, + approvalNotesSupported: true, + }); + + try { + const response = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + draftGeneration: 3, + }), + }); + + expect(response.status).toBe(200); + expect(await server.waitForDecision()).toEqual({ + approved: true, + feedback: "Keep the retry bounded.", + annotations: [{ id: "a1" }], + }); + } finally { + server.stop(); + } + }); + + test("keeps bodyless approval compatible", async () => { + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(tmpdir(), "approval-bodyless.md"), + htmlContent: MINIMAL_HTML, + }); + + try { + const response = await fetch(`${server.url}/api/approve`, { method: "POST" }); + expect(response.status).toBe(200); + expect(await server.waitForDecision()).toEqual({ + approved: true, + feedback: "", + annotations: [], + }); + } finally { + server.stop(); + } + }); + + test("rejects malformed or wrong-type approval bodies without resolving", async () => { + const server = await startAnnotateServer({ + markdown: "# Test", + filePath: join(tmpdir(), "approval-invalid.md"), + htmlContent: MINIMAL_HTML, + }); + + try { + const decision = server.waitForDecision(); + const malformed = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{", + }); + expect(malformed.status).toBe(400); + expect(await Promise.race([decision.then(() => "resolved"), Bun.sleep(25).then(() => "pending")])).toBe("pending"); + + const wrongType = await fetch(`${server.url}/api/approve`, { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify({ feedback: 42, annotations: [] }), + }); + expect(wrongType.status).toBe(400); + expect(await Promise.race([decision.then(() => "resolved"), Bun.sleep(25).then(() => "pending")])).toBe("pending"); + + await fetch(`${server.url}/api/approve`, { method: "POST" }); + expect(await decision).toEqual({ approved: true, feedback: "", annotations: [] }); + } finally { + server.stop(); + } + }); +}); diff --git a/packages/server/annotate.ts b/packages/server/annotate.ts index 31ec9a652..637f7dd0d 100644 --- a/packages/server/annotate.ts +++ b/packages/server/annotate.ts @@ -85,6 +85,8 @@ export interface AnnotateServerOptions { sourceConverted?: boolean; /** Enable review-gate UX: adds an Approve button alongside Close/Send Annotations */ gate?: boolean; + /** Whether this transport can deliver feedback attached to an approval. */ + approvalNotesSupported?: boolean; /** Raw HTML content for direct iframe rendering. */ rawHtml?: string; /** Render HTML as-is in an iframe. */ @@ -147,6 +149,7 @@ export async function startAnnotateServer( shareBaseUrl, pasteApiUrl, gate = false, + approvalNotesSupported = false, rawHtml, renderHtml = false, convertHtml = false, @@ -388,6 +391,7 @@ export async function startAnnotateServer( sourceConverted: sourceConverted ?? false, sourceSave: primarySource.sourceSave, gate, + approvalNotesSupported, renderAs: displayRawHtml ? 'html' as const : 'markdown' as const, ...(displayRawHtml ? { rawHtml: displayRawHtml } : {}), ...(diffHtml ? { diffHtml } : {}), @@ -658,8 +662,37 @@ export async function startAnnotateServer( // API: Approve the annotation session (review-gate UX) if (url.pathname === "/api/approve" && req.method === "POST") { - deleteDraft(draftKey, readDraftGenerationFromUrl(req)); - resolveDecision({ feedback: "", annotations: [], approved: true }); + const rawBody = await req.text(); + let body: Record = {}; + if (rawBody.trim()) { + try { + const parsed = JSON.parse(rawBody); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error("Expected a JSON object."); + } + body = parsed as Record; + } catch (err) { + return Response.json( + { error: err instanceof Error ? err.message : "Invalid JSON body." }, + { status: 400 }, + ); + } + } + if ( + (body.feedback !== undefined && typeof body.feedback !== "string") || + (body.annotations !== undefined && !Array.isArray(body.annotations)) || + (body.codeAnnotations !== undefined && !Array.isArray(body.codeAnnotations)) || + (body.draftGeneration !== undefined && typeof body.draftGeneration !== "number") + ) { + return Response.json({ error: "Invalid approval body." }, { status: 400 }); + } + + deleteDraft(draftKey, readDraftGenerationFromBody(body)); + resolveDecision({ + feedback: (body.feedback as string | undefined) || "", + annotations: (body.annotations as unknown[] | undefined) || [], + approved: true, + }); return Response.json({ ok: true }); } diff --git a/packages/shared/config.ts b/packages/shared/config.ts index 689462897..73a901cf5 100644 --- a/packages/shared/config.ts +++ b/packages/shared/config.ts @@ -53,6 +53,7 @@ export interface PromptConfig { fileFeedback?: string; messageFeedback?: string; approved?: string; + approvedWithNotes?: string; }; } diff --git a/packages/shared/prompts-integration.test.ts b/packages/shared/prompts-integration.test.ts index 3a5229639..1b4bbfc33 100644 --- a/packages/shared/prompts-integration.test.ts +++ b/packages/shared/prompts-integration.test.ts @@ -332,6 +332,28 @@ describe("prompts integration (config from disk)", () => { expect(result).toBe("LGTM"); }); + test("annotate approved-with-notes reads override from config.json", async () => { + writeConfig({ + prompts: { + annotate: { + approvedWithNotes: "APPROVED {{context}}\n\nNotes: {{feedback}}", + }, + }, + }); + + const result = await runScript(` + import { getAnnotateApprovedWithNotesPrompt } from "./packages/shared/prompts"; + console.log(getAnnotateApprovedWithNotesPrompt("opencode", undefined, { + context: "File: src/app.ts", + feedback: "Keep the retry bounded.", + })); + `); + + expect(result).toBe( + "APPROVED File: src/app.ts\n\nNotes: Keep the retry bounded.", + ); + }); + // ── Review denied suffix ───────────────────────────────────────────── test("review denied suffix reads override from config.json", async () => { diff --git a/packages/shared/prompts.test.ts b/packages/shared/prompts.test.ts index d33c02c63..67c87ed46 100644 --- a/packages/shared/prompts.test.ts +++ b/packages/shared/prompts.test.ts @@ -9,6 +9,7 @@ import { DEFAULT_ANNOTATE_FILE_FEEDBACK_PROMPT, DEFAULT_ANNOTATE_MESSAGE_FEEDBACK_PROMPT, DEFAULT_ANNOTATE_APPROVED_PROMPT, + DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT, DEFAULT_REVIEW_DENIED_SUFFIX, getConfiguredPrompt, getReviewApprovedPrompt, @@ -21,6 +22,7 @@ import { getAnnotateMessageFeedbackPrompt, getAnnotateMessageFeedbackTemplate, getAnnotateApprovedPrompt, + getAnnotateApprovedWithNotesPrompt, getReviewDeniedSuffix, resolveTemplate, getPlanToolName, @@ -350,6 +352,90 @@ describe("getAnnotateApprovedPrompt", () => { }); }); +describe("getAnnotateApprovedWithNotesPrompt", () => { + test("frames approved file notes as non-blocking guidance with target context", () => { + const result = getAnnotateApprovedWithNotesPrompt("opencode", {}, { + context: "File: /src/app.ts", + feedback: "Keep the retry bounded.", + }); + + expect(result).toContain("artifact is approved"); + expect(result).toContain("non-blocking guidance"); + expect(result).toContain("not a request for another revision"); + expect(result).toContain("File: /src/app.ts"); + expect(result).toContain("Keep the retry bounded."); + expect(result).toContain( + "Do not revise or reopen the artifact solely because of these notes unless the user explicitly requests it", + ); + expect(result).toContain("Carry the notes into subsequent work where applicable"); + expect(result).not.toMatch(/\baddress\b/i); + expect(result).toBe( + resolveTemplate(DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT, { + contextBlock: "File: /src/app.ts\n\n", + feedback: "Keep the retry bounded.", + }), + ); + }); + + test("omits target context for approved message notes", () => { + const result = getAnnotateApprovedWithNotesPrompt("pi", {}, { + feedback: "Retain this caveat.", + }); + + expect(result).toContain("Retain this caveat."); + expect(result).not.toContain("{{context}}"); + expect(result).not.toContain("File:"); + }); + + test("resolves {{context}} to empty in custom templates for message annotations", () => { + // The OpenCode CLI-bridge message path passes `context: undefined` + // (there is no target file); the key being present must not leave a + // literal `{{context}}` in a custom template. + const result = getAnnotateApprovedWithNotesPrompt("opencode", { + prompts: { + annotate: { + approvedWithNotes: "APPROVED {{context}}\n\nGuidance: {{feedback}}", + }, + }, + }, { + context: undefined, + feedback: "Retain this caveat.", + }); + + expect(result).toBe("APPROVED \n\nGuidance: Retain this caveat."); + expect(result).not.toContain("{{context}}"); + }); + + test("uses the single configurable approvedWithNotes override", () => { + const result = getAnnotateApprovedWithNotesPrompt("pi", { + prompts: { + annotate: { + approvedWithNotes: "APPROVED {{context}}\n\nGuidance: {{feedback}}", + }, + }, + }, { + context: "Folder: /src", + feedback: "Keep names stable.", + }); + + expect(result).toBe("APPROVED Folder: /src\n\nGuidance: Keep names stable."); + }); + + test("preserves configured template whitespace", () => { + const result = getAnnotateApprovedWithNotesPrompt("pi", { + prompts: { + annotate: { + approvedWithNotes: "Approved.\n\n\n{{feedback}}", + }, + }, + }, { + feedback: "Keep names stable.", + }); + + expect(result).toBe("Approved.\n\n\nKeep names stable."); + }); +}); + // ─── A4b. Review denied suffix ─────────────────────────────────────────────── describe("getReviewDeniedSuffix", () => { @@ -450,10 +536,11 @@ describe("mergePromptConfig (expanded)", () => { test("merges annotate section", () => { const merged = mergePromptConfig( { annotate: { approved: "A" } }, - { annotate: { fileFeedback: "F" } }, + { annotate: { fileFeedback: "F", approvedWithNotes: "N" } }, ); expect(merged?.annotate?.approved).toBe("A"); expect(merged?.annotate?.fileFeedback).toBe("F"); + expect(merged?.annotate?.approvedWithNotes).toBe("N"); }); test("deep merges runtimes within plan section", () => { diff --git a/packages/shared/prompts.ts b/packages/shared/prompts.ts index 255f5ac2e..12dc02970 100644 --- a/packages/shared/prompts.ts +++ b/packages/shared/prompts.ts @@ -61,6 +61,9 @@ export const DEFAULT_ANNOTATE_MESSAGE_FEEDBACK_PROMPT = export const DEFAULT_ANNOTATE_APPROVED_PROMPT = "The user approved."; +export const DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT = + "# Approved with Notes\n\nThe artifact is approved. The notes below are non-blocking guidance, not a request for another revision.\n\n{{contextBlock}}{{feedback}}\n\nDo not revise or reopen the artifact solely because of these notes unless the user explicitly requests it. Carry the notes into subsequent work where applicable."; + // ─── Core resolver ─────────────────────────────────────────────────────────── type PromptSection = "review" | "plan" | "annotate"; @@ -262,3 +265,25 @@ export function getAnnotateApprovedPrompt( fallback: DEFAULT_ANNOTATE_APPROVED_PROMPT, }); } + +export function getAnnotateApprovedWithNotesPrompt( + runtime?: PromptRuntime | null, + config?: PlannotatorConfig, + vars?: FeedbackVars, +): string { + const template = getConfiguredPrompt({ + section: "annotate", + key: "approvedWithNotes", + runtime, + config, + fallback: DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT, + }); + // Spread vars first so an undefined `context` (e.g. the message-annotation + // path, which has no target file) cannot clobber the defaults and leave a + // literal `{{context}}` in custom templates. + return resolveTemplate(template, { + ...vars, + context: vars?.context ?? "", + contextBlock: vars?.contextBlock ?? (vars?.context ? `${vars.context}\n\n` : ""), + }); +} From e6e6e21281a5aee0fd7de043f0766ddc0b823efc Mon Sep 17 00:00:00 2001 From: rNoz Date: Fri, 24 Jul 2026 10:28:00 +0200 Subject: [PATCH 4/4] test(pi): use exact annotate outcome import --- apps/pi-extension/annotate-outcome.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/apps/pi-extension/annotate-outcome.test.ts b/apps/pi-extension/annotate-outcome.test.ts index 920c95f62..3e9ec6a2f 100644 --- a/apps/pi-extension/annotate-outcome.test.ts +++ b/apps/pi-extension/annotate-outcome.test.ts @@ -1,5 +1,5 @@ import { describe, expect, test } from "bun:test"; -import { classifyAnnotateOutcome } from "./annotate-outcome"; +import { classifyAnnotateOutcome } from "./annotate-outcome.ts"; describe("Pi annotate outcomes", () => { test("delivers approved file feedback before the approval notification", () => {