mirror of
https://github.com/backnotprop/plannotator.git
synced 2026-09-14 14:17:26 +08:00
fix(amp): relay structured decisions without prose classification (#1476)
Closes #1456. Adds plannotator review --json emitting one { decision, message } record from the same builder as the plaintext output, routes Amp solely on the decision field, and removes the substring classifier that could silently drop feedback mentioning phrases like 'no feedback'. Invalid or legacy CLI output produces a recoverable update notice, never a guessed decision. Claude-Session: https://claude.ai/code/session_019GV6EKtzh8Pf9GA2rrBLNf
This commit is contained in:
FND
@@ -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:
|
||||
|
||||
@@ -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<void>): Promise<void> {
|
||||
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<string, (ctx: PluginCommandContext) => Promise<void>>();
|
||||
plugin({
|
||||
logger: { log() {} },
|
||||
registerCommand(name: string, _options: unknown, handler: (ctx: PluginCommandContext) => Promise<void>) {
|
||||
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];
|
||||
|
||||
@@ -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(trimmed) as Partial<AnnotateDecision>;
|
||||
const parsed = JSON.parse(raw) as Partial<ReviewDecision> | null;
|
||||
if (
|
||||
parsed &&
|
||||
typeof parsed === "object" &&
|
||||
(parsed.decision === "approved" ||
|
||||
parsed.decision === "dismissed" ||
|
||||
parsed.decision === "annotated")
|
||||
parsed.decision === "annotated") &&
|
||||
typeof parsed.message === "string"
|
||||
) {
|
||||
return {
|
||||
decision: parsed.decision,
|
||||
feedback: typeof parsed.feedback === "string" ? parsed.feedback : undefined,
|
||||
};
|
||||
return { decision: parsed.decision, message: parsed.message };
|
||||
}
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
|
||||
return null;
|
||||
}
|
||||
|
||||
function parseAnnotateDecision(raw: string): AnnotateDecision | null {
|
||||
try {
|
||||
const parsed = JSON.parse(raw) as Partial<AnnotateDecision> | null;
|
||||
if (
|
||||
parsed &&
|
||||
typeof parsed === "object" &&
|
||||
(parsed.decision === "approved" ||
|
||||
parsed.decision === "dismissed" ||
|
||||
parsed.decision === "annotated") &&
|
||||
(parsed.feedback === undefined || typeof parsed.feedback === "string")
|
||||
) {
|
||||
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<string | nul
|
||||
async function handleReviewResult(ctx: CommandContext, result: RunResult): Promise<void> {
|
||||
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<void> {
|
||||
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,
|
||||
|
||||
@@ -175,7 +175,7 @@ export function formatTopLevelHelp(): string {
|
||||
export const SUBCOMMAND_HELP: Record<string, string> = {
|
||||
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<string, string> = {
|
||||
" --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",
|
||||
|
||||
@@ -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") {
|
||||
|
||||
@@ -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,
|
||||
),
|
||||
);
|
||||
});
|
||||
});
|
||||
@@ -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;
|
||||
|
||||
@@ -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 <command> --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.
|
||||
|
||||
Reference in New Issue
Block a user