fix(amp,droid): surface Approve-with-Notes feedback instead of dropping it

apps/amp-plugin/plannotator.test.ts

@@ -63,6 +63,48 @@ describe("Amp Plannotator plugin helpers", () => {
     );
   });
 
+  // #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);

apps/amp-plugin/plannotator.ts

@@ -16,6 +16,10 @@ const DEFAULT_ANNOTATE_FILE_FEEDBACK_PROMPT =
   "# Markdown Annotations\n\n{{fileHeader}}: {{filePath}}\n\n{{feedback}}\n\nPlease address the annotation feedback above.";
 const DEFAULT_ANNOTATE_MESSAGE_FEEDBACK_PROMPT =
   "# Message Annotations\n\n{{feedback}}\n\nPlease address the annotation feedback above.";
+// Mirrors DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT in packages/shared/prompts.ts,
+// which this self-contained plugin cannot import. Keep the two in sync.
+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.";
 
 type CommandContext = PluginCommandContext;
 type ReadyResult = "ready" | "exited" | "timeout";
@@ -198,12 +202,29 @@ export function formatAnnotationFeedback(
   decision: AnnotateDecision,
   options: { kind: "file"; filePath: string } | { kind: "message" },
 ): string | null {
-  if (decision.decision !== "annotated") return null;
+  // Approved-with-notes carries reviewer guidance in `feedback` (#1092); only
+  // dismissed decisions and note-less approvals have nothing to surface.
+  if (decision.decision !== "annotated" && decision.decision !== "approved") return null;
 
   const feedback = decision.feedback?.trim();
   if (!feedback || isNoActionFeedback(feedback)) return null;
 
   const config = loadPlannotatorConfig();
+
+  if (decision.decision === "approved") {
+    const template = getConfiguredPrompt(
+      config,
+      "approvedWithNotes",
+      DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT,
+    );
+    const context = options.kind === "file" ? `File: ${options.filePath}` : "";
+    return resolveTemplate(template, {
+      context,
+      contextBlock: context ? `${context}\n\n` : "",
+      feedback,
+    });
+  }
+
   if (options.kind === "file") {
     const template = getConfiguredPrompt(config, "fileFeedback", DEFAULT_ANNOTATE_FILE_FEEDBACK_PROMPT);
     return resolveTemplate(template, {
@@ -341,7 +362,14 @@ async function handleAnnotateResult(
 
   const decision = parseAnnotateDecision(result.stdout);
   if (decision?.decision === "approved") {
-    await ctx.ui.notify("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);
+    if (feedback) {
+      await appendFeedback(ctx, feedback);
+    } else {
+      await ctx.ui.notify("Approved.");
+    }
     return;
   }
   if (decision?.decision === "dismissed") {
@@ -890,9 +918,11 @@ type PromptConfig = {
     annotate?: {
       fileFeedback?: unknown;
       messageFeedback?: unknown;
+      approvedWithNotes?: unknown;
       runtimes?: Partial<Record<typeof RUNTIME, {
         fileFeedback?: unknown;
         messageFeedback?: unknown;
+        approvedWithNotes?: unknown;
       }>>;
     };
   };
@@ -938,7 +968,7 @@ export function getPlannotatorDataDir(): string {
 
 function getConfiguredPrompt(
   config: PromptConfig,
-  key: "fileFeedback" | "messageFeedback",
+  key: "fileFeedback" | "messageFeedback" | "approvedWithNotes",
   fallback: string,
 ): string {
   const annotate = config.prompts?.annotate;

apps/droid-plugin/lib/run-plannotator.js

@@ -90,7 +90,18 @@ function emitAnnotateDecision(rawOutput, heading) {
     const parsed = JSON.parse(output);
     if (parsed && typeof parsed === "object") {
       if (parsed.decision === "approved") {
-        process.stdout.write("Approved.\n");
+        // Approve-with-Notes (#1092): `approved` may carry reviewer notes in
+        // `feedback`. Only a note-less approval is a plain "Approved."
+        const feedback = typeof parsed.feedback === "string" ? parsed.feedback.trim() : "";
+        if (!feedback) {
+          process.stdout.write("Approved.\n");
+          return;
+        }
+        // Mirrors DEFAULT_ANNOTATE_APPROVED_WITH_NOTES_PROMPT in
+        // packages/shared/prompts.ts. Keep the two in sync.
+        process.stdout.write(
+          `# Approved with Notes\n\nThe artifact is approved. The notes below are non-blocking guidance, not a request for another revision.\n\n${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.\n`,
+        );
         return;
       }
 

apps/droid-plugin/lib/run-plannotator.test.js

@@ -0,0 +1,74 @@
+const { afterEach, beforeEach, describe, expect, test } = require("bun:test");
+const { emitAnnotateDecision } = require("./run-plannotator");
+
+// #1137: approved decisions carrying Approve-with-Notes feedback (#1092) were
+// collapsed to a bare "Approved." and the reviewer's notes were silently
+// dropped. These tests pin the full decision -> stdout contract.
+describe("emitAnnotateDecision", () => {
+  let written;
+  let originalWrite;
+
+  beforeEach(() => {
+    written = [];
+    originalWrite = process.stdout.write;
+    process.stdout.write = (chunk) => {
+      written.push(String(chunk));
+      return true;
+    };
+  });
+
+  afterEach(() => {
+    process.stdout.write = originalWrite;
+  });
+
+  const output = () => written.join("");
+
+  test("plain approval still prints Approved.", () => {
+    emitAnnotateDecision('{"decision":"approved"}', "Markdown Annotations");
+    expect(output()).toBe("Approved.\n");
+  });
+
+  test("approved-with-notes surfaces the feedback instead of dropping it", () => {
+    emitAnnotateDecision(
+      JSON.stringify({
+        decision: "approved",
+        feedback: "Ship it, but rename the flag before GA.",
+      }),
+      "Markdown Annotations",
+    );
+
+    expect(output()).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.\n",
+    );
+  });
+
+  test("approved with blank feedback prints Approved.", () => {
+    emitAnnotateDecision('{"decision":"approved","feedback":"   "}', "Markdown Annotations");
+    expect(output()).toBe("Approved.\n");
+  });
+
+  test("dismissed decision closes the session", () => {
+    emitAnnotateDecision('{"decision":"dismissed"}', "Markdown Annotations");
+    expect(output()).toBe("Annotation session closed.\n");
+  });
+
+  test("annotated decision wraps feedback under the heading", () => {
+    emitAnnotateDecision(
+      '{"decision":"annotated","feedback":"Comment: tighten this section."}',
+      "Markdown Annotations",
+    );
+    expect(output()).toBe(
+      "# Markdown Annotations\n\nComment: tighten this section.\n\nPlease address the annotation feedback above.\n",
+    );
+  });
+
+  test("empty output closes the session", () => {
+    emitAnnotateDecision("", "Markdown Annotations");
+    expect(output()).toBe("Annotation session closed.\n");
+  });
+
+  test("non-JSON output falls back to raw passthrough", () => {
+    emitAnnotateDecision("plain text feedback", "Markdown Annotations");
+    expect(output()).toBe("plain text feedback\n");
+  });
+});