diff --git a/apps/amp-plugin/README.md b/apps/amp-plugin/README.md index 3e3f977a3..b0d74f9c3 100644 --- a/apps/amp-plugin/README.md +++ b/apps/amp-plugin/README.md @@ -34,6 +34,26 @@ For project-local installation, copy the plugin to: .amp/plugins/plannotator.ts ``` +## CLI compatibility and feedback recovery + +The review commands require a CLI that supports `plannotator review --json` and +returns a structured `{ decision, message }` result. The plugin uses the decision +to distinguish a dismissal from feedback or approval, never words in the +reviewer's text. Review feedback and approval instructions are appended to the +Amp thread; a dismissed review only shows a notification. Running +`plannotator review` directly still produces plaintext by default. + +Annotation commands use the CLI's `{ decision, feedback? }` JSON result. Feedback +and approval notes are appended using your configured annotation prompts; an +approval without notes only shows an approval notification. + +If an older CLI returns plaintext, or a command succeeds with malformed or +missing structured output, the plugin shows an **invalid structured output** +notification instead of guessing a decision. The notice includes the captured +stdout and stderr so you can recover any feedback manually. Update the CLI with +the install command above, then reload the Amp plugin. If you use +`PLANNOTATOR_BIN` or a source-entry override, update that selected CLI as well. + ## Local Development From a Plannotator checkout: diff --git a/apps/amp-plugin/plannotator.test.ts b/apps/amp-plugin/plannotator.test.ts index 14d082381..dff94ac6f 100644 --- a/apps/amp-plugin/plannotator.test.ts +++ b/apps/amp-plugin/plannotator.test.ts @@ -1,18 +1,16 @@ import { describe, expect, test } from "bun:test"; -import { mkdtempSync, rmSync, writeFileSync } from "node:fs"; +import { copyFileSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; import { pathToFileURL } from "node:url"; +import type { PluginAPI, PluginCommandContext } from "@ampcode/plugin"; import { buildEnv, buildPlannotatorEnv, extractTextFromThreadMessage, findFirstPositionalArg, - formatAnnotationFeedback, getPlannotatorDataDir, getPlannotatorCommandCandidates, - isNoActionFeedback, - parseAnnotateDecision, parseReviewTargetInput, resolveAmpWorkspaceRoot, resolveCwd, @@ -35,81 +33,6 @@ describe("Amp Plannotator plugin helpers", () => { expect(text).toBe("First paragraph.\n\nSecond paragraph."); }); - test("parses structured annotate decisions", () => { - expect(parseAnnotateDecision('{"decision":"approved"}')).toEqual({ decision: "approved" }); - expect(parseAnnotateDecision("")).toEqual({ decision: "dismissed" }); - expect(parseAnnotateDecision("plain feedback")).toBeNull(); - }); - - test("wraps actionable annotation feedback for Amp thread append", () => { - expect( - formatAnnotationFeedback( - { decision: "annotated", feedback: "Comment: tighten this section." }, - { kind: "message" }, - ), - ).toBe( - "# Message Annotations\n\nComment: tighten this section.\n\nPlease address the annotation feedback above.", - ); - }); - - test("wraps file annotation feedback with target path", () => { - expect( - formatAnnotationFeedback( - { decision: "annotated", feedback: "Comment: tighten this section." }, - { kind: "file", filePath: "docs/plan.md" }, - ), - ).toBe( - "# Markdown Annotations\n\nFile: docs/plan.md\n\nComment: tighten this section.\n\nPlease address the annotation feedback above.", - ); - }); - - // #1137: approved decisions carrying Approve-with-Notes feedback (#1092) - // were silently dropped — formatAnnotationFeedback returned null for - // anything that was not "annotated". - test("surfaces approved-with-notes feedback for message annotations", () => { - const result = formatAnnotationFeedback( - { decision: "approved", feedback: "Ship it, but rename the flag before GA." }, - { kind: "message" }, - ); - - expect(result).toBe( - "# Approved with Notes\n\nThe artifact is approved. The notes below are non-blocking guidance, not a request for another revision.\n\nShip it, but rename the flag before GA.\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.", - ); - }); - - test("surfaces approved-with-notes feedback with the file context", () => { - const result = formatAnnotationFeedback( - { decision: "approved", feedback: "Fine as-is; consider splitting later." }, - { kind: "file", filePath: "docs/plan.md" }, - ); - - expect(result).toContain("# Approved with Notes"); - expect(result).toContain("File: docs/plan.md\n\nFine as-is; consider splitting later."); - }); - - test("keeps note-less and dismissed decisions silent", () => { - expect( - formatAnnotationFeedback({ decision: "approved" }, { kind: "message" }), - ).toBeNull(); - expect( - formatAnnotationFeedback( - { decision: "approved", feedback: " " }, - { kind: "message" }, - ), - ).toBeNull(); - expect( - formatAnnotationFeedback( - { decision: "dismissed", feedback: "should never surface" }, - { kind: "message" }, - ), - ).toBeNull(); - }); - - test("detects non-action outputs", () => { - expect(isNoActionFeedback("Review session closed without feedback.")).toBe(true); - expect(isNoActionFeedback("Code review completed — no changes requested.")).toBe(false); - expect(isNoActionFeedback("Please fix this bug.")).toBe(false); - }); test("splits review target arguments without invoking a shell", () => { expect(splitCommandArgs("--git https://github.com/org/repo/pull/1")).toEqual([ @@ -326,6 +249,242 @@ describe("Amp Plannotator plugin helpers", () => { }); }); +describe("Amp Plannotator registered commands", () => { + test.each(["plannotator-review", "plannotator-review-target"])( + "%s delivers rendered review messages and classifies only the decision", + async (command) => { + await withCommandHarness(async ({ run }) => { + // #1456: this reviewer sentence used to be mistaken for a closed session. + const feedback = "This path has no feedback loop, add one."; + for (const result of [ + { decision: "annotated", message: ` Review guidance:\n\n${feedback}\n ` }, + { decision: "approved", message: `Approved with non-blocking notes:\n\n${feedback}` }, + { decision: "approved", message: "Code review completed — no changes requested." }, + ]) { + const delivered = await run(command, JSON.stringify(result), { input: "--git" }); + expect(delivered.appended).toEqual([{ type: "user-message", content: result.message }]); + expect(delivered.notifications).toEqual([]); + } + + // Deliberately not a legacy close phrase: the decision must control delivery. + const message = "The reviewer closed this session."; + const dismissed = await run(command, JSON.stringify({ decision: "dismissed", message })); + expect(dismissed.appended).toEqual([]); + expect(dismissed.notifications).toEqual([message]); + }); + }, + ); + + test.each(["plannotator-annotate", "plannotator-last"])( + "%s delivers annotations and approval notes without interpreting reviewer prose", + async (command) => { + await withCommandHarness(async ({ run }) => { + const feedback = "This path has no feedback loop, add one."; + for (const decision of ["annotated", "approved"]) { + const delivered = await run(command, JSON.stringify({ decision, feedback })); + expect(delivered.appended).toEqual([ + { type: "user-message", content: expect.stringContaining(feedback) }, + ]); + expect(delivered.notifications).toEqual([]); + const content = delivered.appended[0].content; + if (command === "plannotator-annotate") { + expect(content).toContain("docs/plan.md"); + } else { + expect(content).not.toContain("docs/plan.md"); + } + if (decision === "approved") { + // These semantics distinguish approval notes from a revision request. + expect(content).toContain("non-blocking"); + expect(content).toContain("Do not revise or reopen"); + } + } + + for (const feedback of [undefined, " "]) { + const approved = await run(command, JSON.stringify({ decision: "approved", feedback })); + expect(approved.appended).toEqual([]); + expect(approved.notifications).toEqual([expect.stringMatching(/approved/i)]); + } + + const dismissed = await run(command, JSON.stringify({ decision: "dismissed", feedback })); + expect(dismissed.appended).toEqual([]); + expect(dismissed.notifications).toEqual([expect.stringMatching(/closed/i)]); + + const empty = await run(command, JSON.stringify({ decision: "annotated", feedback: "" })); + expect(empty.appended).toEqual([]); + expect(empty.notifications).toEqual([expect.stringMatching(/closed/i)]); + }); + }, + ); + + test("retains configured annotation prompts and Amp approval-note precedence", async () => { + await withCommandHarness(async ({ run, configPath }) => { + writeFileSync(configPath, JSON.stringify({ + prompts: { + annotate: { + fileFeedback: "File guidance for {{filePath}}:\n{{feedback}}", + messageFeedback: "Generic message guidance:\n{{feedback}}", + approvedWithNotes: "Generic approval:\n{{feedback}}", + runtimes: { + amp: { + messageFeedback: "Amp message guidance:\n{{feedback}}", + approvedWithNotes: "Amp approval:\n{{contextBlock}}{{feedback}}", + }, + }, + }, + }, + })); + const feedback = "This path has no feedback loop, add one."; + const file = await run("plannotator-annotate", JSON.stringify({ decision: "annotated", feedback })); + expect(file.appended).toEqual([ + { type: "user-message", content: `File guidance for docs/plan.md:\n${feedback}` }, + ]); + const message = await run("plannotator-last", JSON.stringify({ decision: "annotated", feedback })); + expect(message.appended).toEqual([ + { type: "user-message", content: `Amp message guidance:\n${feedback}` }, + ]); + const approved = await run("plannotator-annotate", JSON.stringify({ decision: "approved", feedback })); + expect(approved.appended).toEqual([ + { type: "user-message", content: `Amp approval:\nFile: docs/plan.md\n\n${feedback}` }, + ]); + }); + }); + + test.each([ + ["plannotator-review", "legacy plaintext", "This path has no feedback loop, add one."], + ["plannotator-review", "empty stdout", ""], + ["plannotator-review", "malformed JSON", '{"decision":"annotated","message":'], + ["plannotator-review", "non-object JSON", "null"], + ["plannotator-review", "missing rendered message", '{"decision":"approved","feedback":"Keep these notes."}'], + ["plannotator-review", "invalid decision", '{"decision":"rejected","message":"Keep these notes."}'], + ["plannotator-review", "invalid message type", '{"decision":"approved","message":{"text":"Keep these notes."}}'], + ["plannotator-review", "multiple records", '{"decision":"approved","message":"First"}\n{"decision":"annotated","message":"Second"}'], + ["plannotator-annotate", "legacy plaintext", "This path has no feedback loop, add one."], + ["plannotator-annotate", "empty stdout", ""], + ["plannotator-annotate", "invalid feedback type", '{"decision":"approved","feedback":{"text":"Keep these notes."}}'], + ])("%s rejects %s and preserves captured output for recovery", async (command, _label, stdout) => { + await withCommandHarness(async ({ run }) => { + const stderr = "Diagnostic from the CLI"; + const delivered = await run(command, stdout, { stderr }); + expect(delivered.appended).toEqual([]); + expect(delivered.notifications).toEqual([ + expect.stringMatching(/invalid structured output/i), + ]); + const notice = delivered.notifications[0]; + expect(notice).toMatch(/update.*CLI/i); + expect(notice).toContain("https://plannotator.ai/docs/getting-started/installation/"); + expect(notice).toContain(stderr); + if (stdout) { + expect(notice).toContain(stdout); + } else { + expect(notice).toMatch(/empty stdout/i); + } + }); + }); + + test("keeps process failures distinct from invalid successful output", async () => { + await withCommandHarness(async ({ run }) => { + const stdout = "Partial reviewer output"; + const stderr = "Unable to finish the review"; + const delivered = await run("plannotator-review", stdout, { stderr, status: 7 }); + expect(delivered.appended).toEqual([]); + expect(delivered.notifications).toEqual([expect.stringMatching(/review failed/i)]); + expect(delivered.notifications[0]).toContain(stdout); + expect(delivered.notifications[0]).toContain(stderr); + }); + }); +}); + +interface CommandHarness { + configPath: string; + run: ( + command: string, + stdout: string, + options?: { input?: string; stderr?: string; status?: number }, + ) => Promise<{ + appended: Array<{ type: string; content: string }>; + notifications: string[]; + }>; +} + +async function withCommandHarness(run: (harness: CommandHarness) => Promise): Promise { + const root = mkdtempSync(join(tmpdir(), "plannotator-amp-commands-")); + const originalEnv = { ...process.env }; + try { + const home = join(root, "home"); + const dataDir = join(root, "data"); + mkdirSync(home); + mkdirSync(dataDir); + const cliPath = join(root, "fake-cli.ts"); + const resultPath = join(root, "result.json"); + const pluginPath = join(root, "plannotator.ts"); + // A separate module instance keeps the cached CLI runtime local to this test. + copyFileSync(join(import.meta.dir, "plannotator.ts"), pluginPath); + writeFileSync(cliPath, ` +const result = await Bun.file(${JSON.stringify(resultPath)}).json(); +if (process.argv.includes("--stdin")) await Bun.stdin.text(); +process.stdout.write(process.argv.includes("--json") ? result.stdout : "CLI plaintext without --json"); +process.stderr.write(result.stderr ?? ""); +process.exit(result.status ?? 0); +`); + Object.assign(process.env, { + HOME: home, + USERPROFILE: home, + XDG_CONFIG_HOME: join(home, ".config"), + XDG_DATA_HOME: join(home, ".local", "share"), + XDG_CACHE_HOME: join(home, ".cache"), + PLANNOTATOR_DATA_DIR: dataDir, + PLANNOTATOR_CWD: root, + PLANNOTATOR_AMP_SOURCE_ENTRY: cliPath, + AMP_LOG_FILE: join(root, "missing-amp.log"), + PWD: root, + }); + delete process.env.PLANNOTATOR_AMP_USE_SOURCE; + delete process.env.PLANNOTATOR_BIN; + + const { default: plugin } = await import(pathToFileURL(pluginPath).href); + const commands = new Map Promise>(); + plugin({ + logger: { log() {} }, + registerCommand(name: string, _options: unknown, handler: (ctx: PluginCommandContext) => Promise) { + commands.set(name, handler); + }, + } as unknown as PluginAPI); + + await run({ + configPath: join(dataDir, "config.json"), + async run(command, stdout, options = {}) { + writeFileSync(resultPath, JSON.stringify({ stdout, stderr: options.stderr, status: options.status })); + const appended: Array<{ type: string; content: string }> = []; + const notifications: string[] = []; + const ctx = { + ui: { + input: async () => options.input ?? "docs/plan.md", + notify: async (message: string) => { notifications.push(message); }, + }, + thread: { + append: async (messages: typeof appended) => { appended.push(...messages); }, + messages: async () => [{ + role: "assistant", + id: "message-1", + content: [{ type: "text", text: "The answer to annotate." }], + }], + }, + } as unknown as PluginCommandContext; + const handler = commands.get(command); + if (!handler) throw new Error(`Command was not registered: ${command}`); + await handler(ctx); + return { appended, notifications }; + }, + }); + } finally { + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) delete process.env[key]; + } + for (const [key, value] of Object.entries(originalEnv)) restoreEnv(key, value); + rmSync(root, { recursive: true, force: true }); + } +} + function restoreEnv(key: string, value: string | undefined): void { if (value === undefined) { delete process.env[key]; diff --git a/apps/amp-plugin/plannotator.ts b/apps/amp-plugin/plannotator.ts index f8e092a01..b260eb076 100644 --- a/apps/amp-plugin/plannotator.ts +++ b/apps/amp-plugin/plannotator.ts @@ -31,6 +31,11 @@ interface RunResult { error?: string; } +interface ReviewDecision { + decision: "approved" | "dismissed" | "annotated"; + message: string; +} + interface AnnotateDecision { decision: "approved" | "dismissed" | "annotated"; feedback?: string; @@ -63,7 +68,7 @@ export default function plannotatorAmpPlugin(amp: PluginAPI) { description: "Open Plannotator code review for the current workspace changes.", }, async (ctx) => { - const result = await runPlannotator(amp, ctx, ["review"]); + const result = await runPlannotator(amp, ctx, ["review", "--json"]); await handleReviewResult(ctx, result); }, ); @@ -85,7 +90,7 @@ export default function plannotatorAmpPlugin(amp: PluginAPI) { const reviewArgs = parseReviewTargetInput(target); if (!reviewArgs) return; - const result = await runPlannotator(amp, ctx, ["review", ...reviewArgs]); + const result = await runPlannotator(amp, ctx, ["review", ...reviewArgs, "--json"]); await handleReviewResult(ctx, result); }, ); @@ -174,22 +179,38 @@ export function extractTextFromThreadMessage(message: ThreadMessage): string { .trim(); } -export function parseAnnotateDecision(raw: string): AnnotateDecision | null { - const trimmed = raw.trim(); - if (!trimmed) return { decision: "dismissed" }; +function parseReviewDecision(raw: string): ReviewDecision | null { + try { + const parsed = JSON.parse(raw) as Partial | null; + if ( + parsed && + typeof parsed === "object" && + (parsed.decision === "approved" || + parsed.decision === "dismissed" || + parsed.decision === "annotated") && + typeof parsed.message === "string" + ) { + return { decision: parsed.decision, message: parsed.message }; + } + } catch { + return null; + } + + return null; +} +function parseAnnotateDecision(raw: string): AnnotateDecision | null { try { - const parsed = JSON.parse(trimmed) as Partial; + const parsed = JSON.parse(raw) as Partial | null; if ( parsed && + typeof parsed === "object" && (parsed.decision === "approved" || parsed.decision === "dismissed" || - parsed.decision === "annotated") + parsed.decision === "annotated") && + (parsed.feedback === undefined || typeof parsed.feedback === "string") ) { - return { - decision: parsed.decision, - feedback: typeof parsed.feedback === "string" ? parsed.feedback : undefined, - }; + return { decision: parsed.decision, feedback: parsed.feedback }; } } catch { return null; @@ -207,7 +228,7 @@ export function formatAnnotationFeedback( if (decision.decision !== "annotated" && decision.decision !== "approved") return null; const feedback = decision.feedback?.trim(); - if (!feedback || isNoActionFeedback(feedback)) return null; + if (!feedback) return null; const config = loadPlannotatorConfig(); @@ -238,17 +259,6 @@ export function formatAnnotationFeedback( return resolveTemplate(template, { feedback }); } -export function isNoActionFeedback(output: string): boolean { - const normalized = output.trim().toLowerCase(); - return ( - normalized === "" || - normalized === "review session closed without feedback." || - normalized === "annotation session closed." || - normalized === "approved." || - normalized === "the user approved." || - normalized.includes("has no feedback") - ); -} export function splitCommandArgs(input: string): string[] { const args: string[] = []; @@ -344,13 +354,17 @@ async function getLatestAssistantText(ctx: CommandContext): Promise { if (await notifyFailure(ctx, result, "review")) return; - const output = result.stdout.trim(); - if (isNoActionFeedback(output)) { - await ctx.ui.notify(output || "Review session closed without feedback."); + const decision = parseReviewDecision(result.stdout); + if (!decision) { + await notifyInvalidStructuredOutput(ctx, result, "review"); + return; + } + if (decision.decision === "dismissed") { + await ctx.ui.notify(decision.message); return; } - await appendFeedback(ctx, output); + await appendFeedback(ctx, decision.message); } async function handleAnnotateResult( @@ -361,7 +375,11 @@ async function handleAnnotateResult( if (await notifyFailure(ctx, result, "annotate")) return; const decision = parseAnnotateDecision(result.stdout); - if (decision?.decision === "approved") { + if (!decision) { + await notifyInvalidStructuredOutput(ctx, result, "annotate"); + return; + } + if (decision.decision === "approved") { // Approve-with-Notes (#1092): surface the reviewer's notes instead of // silently dropping them. A note-less approval keeps the old behavior. const feedback = formatAnnotationFeedback(decision, options); @@ -372,16 +390,14 @@ async function handleAnnotateResult( } return; } - if (decision?.decision === "dismissed") { + if (decision.decision === "dismissed") { await ctx.ui.notify("Annotation session closed."); return; } - const feedback = decision - ? formatAnnotationFeedback(decision, options) - : result.stdout.trim(); + const feedback = formatAnnotationFeedback(decision, options); - if (!feedback || isNoActionFeedback(feedback)) { + if (!feedback) { await ctx.ui.notify("Annotation session closed without feedback."); return; } @@ -420,6 +436,17 @@ async function notifyFailure( return true; } +async function notifyInvalidStructuredOutput( + ctx: CommandContext, + result: RunResult, + mode: "review" | "annotate", +): Promise { + const stderr = result.stderr ? `\n\nCLI stderr:\n${result.stderr}` : ""; + await ctx.ui.notify( + `Plannotator ${mode} returned invalid structured output. Update the Plannotator CLI and reload the Amp plugin: ${INSTALL_URL}\n\nNo decision was delivered to the thread. Captured output is included below so you can recover any feedback manually.\n\nCLI stdout:\n${result.stdout || "(empty stdout)"}${stderr}`, + ); +} + async function runPlannotator( amp: PluginAPI, ctx: CommandContext, diff --git a/apps/hook/server/cli.ts b/apps/hook/server/cli.ts index 7bbb728b4..cd0e017ac 100644 --- a/apps/hook/server/cli.ts +++ b/apps/hook/server/cli.ts @@ -175,7 +175,7 @@ export function formatTopLevelHelp(): string { export const SUBCOMMAND_HELP: Record = { review: [ "Usage:", - " plannotator review [--git | --gitbutler] [--local | --no-local] [--tailscale] [PR_URL]", + " plannotator review [--git | --gitbutler] [--local | --no-local] [--tailscale] [--json] [PR_URL]", "", "Review local VCS changes or a GitHub/GitLab pull request in the browser.", "", @@ -185,8 +185,16 @@ export const SUBCOMMAND_HELP: Record = { " --local For PR review, prepare a local checkout for full file access (default)", " --no-local For PR review, skip the local checkout (diff only)", " --tailscale Publish the loopback session over your tailnet via tailscale serve (HTTPS)", + " --json Emit one decision/message JSON record instead of plaintext", " PR_URL GitHub PR or GitLab MR URL to review", "", + "JSON output:", + ' { "decision": "approved" | "annotated" | "dismissed", "message": string }', + " message is the rendered plaintext output without its final console newline:", + " configured prompts, approval-with-notes framing, and annotation-dependent instructions included.", + " This differs from the raw feedback in annotate/opencode-review JSON.", + " Identify the outcome by decision, not message text.", + "", "Examples:", " plannotator review", " plannotator review --git", diff --git a/apps/hook/server/index.ts b/apps/hook/server/index.ts index aa3bfa297..010a35459 100644 --- a/apps/hook/server/index.ts +++ b/apps/hook/server/index.ts @@ -125,13 +125,11 @@ import { import { rmSync, realpathSync, existsSync } from "fs"; import { parseRemoteUrl } from "@plannotator/shared/repo"; import { - composeReviewApprovedMessage, - getReviewDeniedSuffix, getPlanDeniedPrompt, getPlanToolName, buildPlanFileRule, } from "@plannotator/shared/prompts"; -import { supportsReviewApprovalNotes } from "./review-output"; +import { buildReviewOutput, supportsReviewApprovalNotes } from "./review-output"; import { registerSession, unregisterSession, listSessions } from "@plannotator/server/sessions"; import { openBrowser } from "@plannotator/server/browser"; import { inlineHtmlLocalAssets } from "@plannotator/server/html-assets"; @@ -1089,23 +1087,8 @@ if (args[0] === "sessions") { server.stop(); // Output feedback (captured by slash command) - if (result.exit) { - console.log("Review session closed without feedback."); - } else if (result.approved) { - // PR5 delivery (spec §6.4): a bare approval prints the approved prompt, - // byte-identical to before; an approval carrying reviewer notes prints - // the approved-with-notes framing (non-blocking guidance) instead. - console.log(composeReviewApprovedMessage(detectedOrigin, result.feedback)); - } else { - console.log(result.feedback); - // Append the verification-only suffix whenever the reviewer sent annotations to - // act on — in PR mode too. Platform PR actions (approve/comment posted to - // the host) come back with an empty annotation set and a status message; - // those must NOT get the "verify findings and don't change code" instruction. - if (result.annotations.length > 0) { - console.log(getReviewDeniedSuffix(detectedOrigin)); - } - } + const output = buildReviewOutput(result, detectedOrigin); + console.log(jsonFlag ? JSON.stringify(output) : output.message); process.exit(0); } else if (args[0] === "annotate") { diff --git a/apps/hook/server/review-output.test.ts b/apps/hook/server/review-output.test.ts new file mode 100644 index 000000000..b9c57cd9f --- /dev/null +++ b/apps/hook/server/review-output.test.ts @@ -0,0 +1,121 @@ +import { describe, expect, test } from "bun:test"; +import { buildReviewOutput } from "./review-output"; + +describe("direct review output", () => { + test("approval notes remain approved and use the configured guidance framing", () => { + const feedback = "Keep the diagnostic: Review session closed without feedback."; + + expect( + buildReviewOutput( + { approved: true, feedback, annotations: [{ text: feedback }] }, + "amp", + { + prompts: { + review: { + approved: "Bare approval.", + approvedWithNotes: "Approved with non-blocking guidance:\n{{feedback}}\nContinue without reopening the review.", + denied: "Request changes.", + }, + }, + }, + ), + ).toEqual({ + decision: "approved", + message: `Approved with non-blocking guidance:\n${feedback}\nContinue without reopening the review.`, + }); + }); + + test("bare approval resolves the origin-specific prompt before the global prompt", () => { + const result = { approved: true, feedback: "", annotations: [] }; + const config = { + prompts: { + review: { + approved: "Global approval.", + runtimes: { amp: { approved: "Amp approval.\n" } }, + }, + }, + }; + + expect(buildReviewOutput(result, "amp", config)).toEqual({ + decision: "approved", + message: "Amp approval.\n", + }); + expect(buildReviewOutput(result, undefined, config)).toEqual({ + decision: "approved", + message: "Global approval.", + }); + }); + + test("PR annotations get the configured suffix with the existing plaintext newline boundary", () => { + const feedback = "# PR Review\n\nCheck the null boundary.\n"; + const suffix = "\n\nVerify the finding before changing code."; + + expect( + buildReviewOutput( + { + approved: false, + feedback, + annotations: [{ filePath: "src/cache.ts", text: "Check the null boundary." }], + }, + "amp", + { + prompts: { + review: { + denied: "Global suffix.", + runtimes: { amp: { denied: suffix } }, + }, + }, + }, + ), + ).toEqual({ + decision: "annotated", + message: `${feedback}\n${suffix}`, + }); + }); + + test("platform status without annotations does not acquire the denial suffix", () => { + expect( + buildReviewOutput( + { approved: false, feedback: "Review posted to GitHub.", annotations: [] }, + "amp", + { prompts: { review: { denied: "Verify the findings." } } }, + ), + ).toEqual({ + decision: "annotated", + message: "Review posted to GitHub.", + }); + }); + + test("submitted text identical to the close message is still feedback", () => { + const feedback = "Review session closed without feedback."; + + expect( + buildReviewOutput({ approved: false, feedback, annotations: [] }, "amp", {}), + ).toEqual({ decision: "annotated", message: feedback }); + }); + + test("dismissal takes precedence over approval and unsent annotations", () => { + const config = { + prompts: { review: { approved: "Approved.", denied: "Verify the findings." } }, + }; + const dismissed = buildReviewOutput( + { + exit: true, + approved: true, + feedback: "Unsent reviewer note.", + annotations: [{ text: "Unsent reviewer note." }], + }, + "amp", + config, + ); + + expect(dismissed.decision).toBe("dismissed"); + expect(dismissed).toEqual( + buildReviewOutput( + { exit: true, approved: false, feedback: "", annotations: [] }, + "amp", + config, + ), + ); + }); +}); diff --git a/apps/hook/server/review-output.ts b/apps/hook/server/review-output.ts index 9eb30d9d7..4c6df7b19 100644 --- a/apps/hook/server/review-output.ts +++ b/apps/hook/server/review-output.ts @@ -1,19 +1,54 @@ import type { Origin } from "@plannotator/shared/agents"; +import type { PlannotatorConfig } from "@plannotator/shared/config"; +import { + composeReviewApprovedMessage, + getReviewDeniedSuffix, +} from "@plannotator/shared/prompts"; + +interface ReviewOutcome { + approved: boolean; + feedback: string; + annotations: readonly unknown[]; + exit?: boolean; +} + +export interface ReviewOutput { + decision: "approved" | "annotated" | "dismissed"; + /** The plaintext CLI output, excluding its final console newline. */ + message: string; +} + +export function buildReviewOutput( + result: ReviewOutcome, + origin: Origin | undefined, + config?: PlannotatorConfig, +): ReviewOutput { + if (result.exit) { + return { + decision: "dismissed", + message: "Review session closed without feedback.", + }; + } + if (result.approved) { + return { + decision: "approved", + message: composeReviewApprovedMessage(origin, result.feedback, config), + }; + } + return { + decision: "annotated", + // Preserve the newline between the original feedback and suffix console.log + // calls. PR feedback gets the suffix too; zero-annotation platform status does not. + message: result.annotations.length > 0 + ? `${result.feedback}\n${getReviewDeniedSuffix(origin, config)}` + : result.feedback, + }; +} /** - * Whether the `plannotator review` CLI's decision consumer delivers - * approve-time feedback for this origin (decision-control spec §6.4). - * - * Review has no `--gate/--json/--hook` triad, so unlike - * `supportsAnnotateApprovalNotes` this is keyed on the origin's CONSUMER, not - * on flags. Every origin routed through this CLI shares the one stdout relay - * (`composeReviewApprovedMessage` at the approved branch): Claude Code reads - * the output directly, and the amp/droid plugins shell out to - * `plannotator review` and relay stdout verbatim, so they inherit the same - * delivery. That is why this currently returns true uniformly — the function - * exists as the seam where an origin whose relay drops approve-time output - * would be keyed off, so the advert can never outrun delivery for it - * (`reviewDecision.test.ts` pins the client half of that contract). + * Whether this CLI's review consumer delivers approval notes for the origin. + * Direct review renders notes in both plaintext and JSON messages. The OpenCode + * bridge also checks the plugin's declared support before advertising this. */ export function supportsReviewApprovalNotes(_origin: Origin | undefined): boolean { return true; diff --git a/apps/skills/core/plannotator/SKILL.md b/apps/skills/core/plannotator/SKILL.md index e2f4e2fd9..2d37b436a 100644 --- a/apps/skills/core/plannotator/SKILL.md +++ b/apps/skills/core/plannotator/SKILL.md @@ -30,10 +30,10 @@ This skill is the knowledge layer. The `plannotator-review`, `plannotator-annota Every review or annotate command starts a local web server, opens the browser, and blocks until the human decides. That can take minutes. Launch it with a long (or no) command timeout, or in the background, then read stdout when the process exits. Do not kill the process to "finish" a review; a session that ends without a decision reads as no feedback. -The stdout contract is the whole interface: +Stdout is the interface, but its contract is command-specific. For `annotate` and its last-message variants: - Plaintext (default): empty output on close, `The user approved.` on approve, otherwise the feedback text. Address returned feedback in the same conversation. -- `--json`: one JSON record, `{"decision":"approved"|"dismissed"|"annotated","feedback":"..."}`. An approval may still carry notes in `feedback`; treat those as guidance, not a change request. +- `--json`: one JSON record with `decision` (`approved`, `dismissed`, or `annotated`) and optional raw `feedback`. An approval may still carry notes in `feedback`; treat those as guidance, not a change request. - `--hook`: hook-native output for real PostToolUse/Stop hook contexts only. Approve/close emits nothing (hook passes); annotations emit `{"decision":"block","reason":"..."}`. `--hook` implies the gate UI. Never use it for a normal interactive invocation. `plannotator --help` prints usage without launching anything. Bare `plannotator` is the hook entry point and expects hook JSON on stdin. @@ -41,10 +41,14 @@ The stdout contract is the whole interface: ## plannotator review ```bash -plannotator review [--git | --gitbutler] [--local | --no-local] [--tailscale] [PR_URL] +plannotator review [--git | --gitbutler] [--local | --no-local] [--tailscale] [--json] [PR_URL] ``` -Reviews local VCS changes, or a pull request when a URL is given. Feedback and annotations come back on stdout when the reviewer submits; an approval comes back as an LGTM-style message. +Reviews local VCS changes, or a pull request when a URL is given. Default stdout stays plaintext: the existing close message, approval prompt, or feedback. + +With `--json`, direct review emits one record: `{ decision: 'approved' | 'annotated' | 'dismissed', message: string }`. `message` is the CLI-rendered text exactly as default plaintext would print it, without the final console newline. It includes customized prompts and non-blocking approval-with-notes framing; a denial suffix is included only when `annotations.length > 0`, including in PR mode, not for zero-annotation platform status. + +Classify the outcome only by `decision`, never by `message` text. Notes on an `approved` review are guidance, not a blocking change request. This rendered `message` contract is separate from the raw feedback JSON used by `annotate` and the unchanged `opencode-review` integration. `--hook` is annotate-only. - VCS is auto-detected (JJ, GitButler, Git, and P4 where supported). `--git` forces plain Git; `--gitbutler` forces GitButler (requires the `but` CLI 0.21.0+). Running from a non-VCS parent folder that contains nested repos produces a combined workspace diff. - The default diff is "everything a PR would show now": merge-base of the trunk vs the working tree plus untracked files. The reviewer can switch diff types in the UI; you do not control that from the CLI. diff --git a/packages/server/call-flow-install-endpoint.test.ts b/packages/server/call-flow-install-endpoint.test.ts index dec2f3399..5e331e218 100644 --- a/packages/server/call-flow-install-endpoint.test.ts +++ b/packages/server/call-flow-install-endpoint.test.ts @@ -6,15 +6,9 @@ import { join } from 'node:path'; import type { CallFlowInstallStage, CallFlowNodePreflight, CallFlowRuntimeInstallResult } from '@plannotator/shared/call-flow'; // PLANNOTATOR_DATA_DIR is only ever changed INSIDE tests (boot() below) and -// restored to its original value after each one. It must never be overridden -// at module-eval time: bun evaluates every test file's module before running -// tests in one shared process, and Pi's generated/storage.ts caches its data -// dir at import time. A module-eval override here makes storage's cached dir -// and later files' live getPlannotatorDataDir() calls disagree, which is -// exactly the Pi annotate-history / durable-submit CI failure this comment -// guards against. Config writes made by these tests target whatever dir the -// process's config module froze at first import; the snapshot/restore in -// afterAll below keeps those writes from leaking into a real config.json. +// restored after each one. Module-eval overrides would leak into other test +// files because Bun runs the suite in one shared process. The config +// snapshot/restore in afterAll also protects against shared config state. const originalDataDir = process.env.PLANNOTATOR_DATA_DIR; const originalPort = process.env.PLANNOTATOR_PORT; const originalPath = process.env.PATH; diff --git a/packages/server/storage.test.ts b/packages/server/storage.test.ts index be005caa7..8bdee91f2 100644 --- a/packages/server/storage.test.ts +++ b/packages/server/storage.test.ts @@ -174,3 +174,39 @@ describe("listVersions", () => { expect(versions[0].timestamp).toBeTruthy(); }); }); + +describe("PLANNOTATOR_DATA_DIR", () => { + test("isolates plan and history data when the data directory changes after import", () => { + const savedDataDir = process.env.PLANNOTATOR_DATA_DIR; + const firstDir = makeTempDir(); + const secondDir = makeTempDir(); + const project = "data-dir-project"; + const slug = "data-dir-plan"; + + try { + process.env.PLANNOTATOR_DATA_DIR = firstDir; + savePlan(slug, "# First plan"); + saveToHistory(project, slug, "# First version"); + expect(readFileSync(join(firstDir, "plans", `${slug}.md`), "utf-8")).toBe("# First plan"); + expect(getPlanVersion(project, slug, 1)).toBe("# First version"); + expect(getVersionCount(project, slug)).toBe(1); + + process.env.PLANNOTATOR_DATA_DIR = secondDir; + expect(getPlanVersion(project, slug, 1)).toBeNull(); + expect(getVersionCount(project, slug)).toBe(0); + savePlan(slug, "# Second plan"); + saveToHistory(project, slug, "# Second version"); + expect(readFileSync(join(secondDir, "plans", `${slug}.md`), "utf-8")).toBe("# Second plan"); + expect(getPlanVersion(project, slug, 1)).toBe("# Second version"); + expect(getVersionCount(project, slug)).toBe(1); + + process.env.PLANNOTATOR_DATA_DIR = firstDir; + expect(readFileSync(join(firstDir, "plans", `${slug}.md`), "utf-8")).toBe("# First plan"); + expect(getPlanVersion(project, slug, 1)).toBe("# First version"); + expect(getVersionCount(project, slug)).toBe(1); + } finally { + if (savedDataDir === undefined) delete process.env.PLANNOTATOR_DATA_DIR; + else process.env.PLANNOTATOR_DATA_DIR = savedDataDir; + } + }); +}); diff --git a/packages/shared/storage.ts b/packages/shared/storage.ts index cc4e12f66..5c3662043 100644 --- a/packages/shared/storage.ts +++ b/packages/shared/storage.ts @@ -13,8 +13,6 @@ import { sanitizeTag } from "./project"; import { resolveUserPath } from "./resolve-file"; import { getPlannotatorDataDir } from "./data-dir"; -const DATA_DIR = getPlannotatorDataDir(); - /** * Get the plan storage directory, creating it if needed. * Cross-platform: uses os.homedir() for Windows/macOS/Linux compatibility. @@ -26,7 +24,7 @@ export function getPlanDir(customPath?: string | null): string { if (customPath?.trim()) { planDir = resolveUserPath(customPath); } else { - planDir = join(DATA_DIR, "plans"); + planDir = join(getPlannotatorDataDir(), "plans"); } mkdirSync(planDir, { recursive: true }); @@ -195,7 +193,7 @@ export function readArchivedPlan(filename: string, customPath?: string | null): * Not affected by the customPath setting (that only affects decision saves). */ export function getHistoryDir(project: string, slug: string): string { - const historyDir = join(DATA_DIR, "history", project, slug); + const historyDir = join(getPlannotatorDataDir(), "history", project, slug); mkdirSync(historyDir, { recursive: true }); return historyDir; } @@ -294,7 +292,7 @@ export function getPlanVersion( slug: string, version: number ): string | null { - const historyDir = join(DATA_DIR, "history", project, slug); + const historyDir = join(getPlannotatorDataDir(), "history", project, slug); const fileName = `${String(version).padStart(3, "0")}.md`; const filePath = join(historyDir, fileName); @@ -314,7 +312,7 @@ export function getPlanVersionPath( slug: string, version: number ): string | null { - const historyDir = join(DATA_DIR, "history", project, slug); + const historyDir = join(getPlannotatorDataDir(), "history", project, slug); const fileName = `${String(version).padStart(3, "0")}.md`; const filePath = join(historyDir, fileName); return existsSync(filePath) ? filePath : null; @@ -325,7 +323,7 @@ export function getPlanVersionPath( * Returns 0 if the directory doesn't exist. */ export function getVersionCount(project: string, slug: string): number { - const historyDir = join(DATA_DIR, "history", project, slug); + const historyDir = join(getPlannotatorDataDir(), "history", project, slug); try { const entries = readdirSync(historyDir); return entries.filter((e) => /^\d+\.md$/.test(e)).length; @@ -342,7 +340,7 @@ export function listVersions( project: string, slug: string ): Array<{ version: number; timestamp: string }> { - const historyDir = join(DATA_DIR, "history", project, slug); + const historyDir = join(getPlannotatorDataDir(), "history", project, slug); try { const entries = readdirSync(historyDir); const versions: Array<{ version: number; timestamp: string }> = []; @@ -372,7 +370,7 @@ export function listVersions( export function listProjectPlans( project: string ): Array<{ slug: string; versions: number; lastModified: string }> { - const projectDir = join(DATA_DIR, "history", project); + const projectDir = join(getPlannotatorDataDir(), "history", project); try { const entries = readdirSync(projectDir, { withFileTypes: true }); const plans: Array<{ slug: string; versions: number; lastModified: string }> = [];