feat(review): approve-with-notes — advert read, delivery routes, LGTM placeholder removed (PR5)

Client half of spec §6.4, extending PR3's tripwire exactly as its comment
instructed:

- REVIEW_APPROVAL_NOTES_SUPPORTED is deleted. The spec input is now the
  server advert, read off /api/diff (and re-read from any diff payload that
  carries it) through readApprovalNotesAdvert — absent reads false, so an old
  server renders no approve-carrying items and a new server against an old
  client changes nothing.
- The approve-with-notes route stops being a marked refusal: it forks on
  withAnnotations ('Approve with notes' ships the live annotations plus their
  export; 'Approve with a note…' ships the composer note alone) and lands on
  handleApprove via the new pure buildReviewApprovalBody.
- handleApprove drops the 'LGTM - no changes requested.' placeholder: a bare
  approval sends feedback '' — consumers now print approve-time feedback, and
  the empty body is what makes the archive's lgtm decision reachable and
  stops bare approvals writing sidecars (spec §6.2 fact 1).
- reviewDecision.test.ts: the PR3 tripwire becomes the delivery assertion
  (under a true advert every approve-carrying item's payload carries the
  content, never the placeholder) plus the absent-advert-is-false pin.
- App.decisionControl.test.tsx: bare-approval assertions move to the empty
  body, the advert-off test pins the old-server payload shape, and a new
  advert-on case proves 'Approve with notes' posts the live annotations.

Claude-Session: https://claude.ai/code/session_01Drrzd1x4EfnH9N3z7nNwo9

packages/review-editor/App.decisionControl.test.tsx

@@ -3,10 +3,11 @@
  * payloads + E16-review, through the real posted /api/feedback body.
  *
  * Regressions each test guards:
- *  - Empty-state `Approve` must post the byte-identical legacy LGTM body
- *    (`approved: true`, the placeholder feedback, `annotations: []`): every
- *    consumer branches on the `approved` flag and PR5's placeholder removal
- *    is deliberately NOT in this PR (spec §6.4).
+ *  - Empty-state `Approve` must post the bare approval body (`approved: true`,
+ *    `feedback: ''`, `annotations: []`): the LGTM placeholder was removed with
+ *    PR5's delivery (spec §6.4) — consumers now print approve-time feedback,
+ *    so any filler here would be appended to every approval, and the empty
+ *    body is what makes the archive's `lgtm` decision reachable.
  *  - `Send Feedback` must post the live annotations with `approved: false` —
  *    the state where the old header offered a data-destroying Approve.
  *  - `Request changes…` must deliver the note as a `scope:'general'`
@@ -14,13 +15,15 @@
  *    AND inside the exported `## General` section, one render after the
  *    commit — a same-tick submit posts the pre-note payload and silently
  *    drops it (#1449 transport).
- *  - The discard confirm must post the plain LGTM (empty annotations), and
+ *  - The discard confirm must post the bare approval (empty annotations), and
  *    nothing before the confirm.
  *  - Mod+Enter always equals the visible primary.
  *  - Approve-carrying menu items must be absent while the server does not
- *    advertise approval-note delivery (four runtimes still discard feedback
- *    on approve — spec §2.2's "never render an item that silently drops
- *    content").
+ *    advertise approval-note delivery — an OLD server's payload has no
+ *    `approvalNotesSupported`, which must read as false (spec §2.2's "never
+ *    render an item that silently drops content") — and under a capable
+ *    server's advert `Approve with notes` must deliver the live annotations
+ *    on the approval body.
  *  - Compact/touch must offer a visible positive decision row at zero and it
  *    must post (E16-review: touch has no Mod+Enter).
  */
@@ -129,6 +132,10 @@ let submissions: SubmittedBody[] = [];
 /** How many upcoming /api/feedback POSTs answer 500. Each failed attempt is
  *  still recorded in `submissions` so its captured body can be asserted. */
 let failFeedbackPosts = 0;
+/** When true, /api/diff carries `approvalNotesSupported: true` — the capable
+ *  server. Default false mimics an OLD server whose payload has no such
+ *  field at all, pinning that absent reads as not-capable. */
+let advertiseApprovalNotes = false;
 
 const PATCH = [
   "diff --git a/src/parse.ts b/src/parse.ts",
@@ -170,6 +177,9 @@ function makeFetch(): typeof fetch {
         diffType: "uncommitted",
         base: null,
         hideWhitespace: false,
+        // Absent (not false) in the old-server shape: the pre-advert payload
+        // simply had no such field.
+        ...(advertiseApprovalNotes ? { approvalNotesSupported: true } : {}),
       });
     }
     if (url.pathname === "/api/diff/fresh") return Response.json({ fresh: true });
@@ -273,11 +283,6 @@ async function pressNoteKey(key: string, init: KeyboardEventInit = {}): Promise<
   await settle();
 }
 
-// Frozen copy (maintainer-approved, spec §6.4): today's approve placeholder.
-// PR5 removes it together with the consumer changes; until then this exact
-// string is what every runtime receives on approve.
-const LGTM_PLACEHOLDER = "LGTM - no changes requested.";
-
 afterEach(async () => {
   if (root) await act(async () => root?.unmount());
   root = null;
@@ -288,6 +293,7 @@ afterEach(async () => {
   if (hasDom && originalMatchMedia) window.matchMedia = originalMatchMedia;
   submissions = [];
   failFeedbackPosts = 0;
+  advertiseApprovalNotes = false;
   seededExternalAnnotations = [];
   memory.clear();
   resetStorageBackend();
@@ -299,7 +305,7 @@ afterAll(() => {
 });
 
 describe.if(hasDom)("review decision control (agent mode)", () => {
-  test("empty-state Approve posts the legacy LGTM body", async () => {
+  test("empty-state Approve posts the bare approval body (no LGTM placeholder)", async () => {
     await mountReview();
 
     expect(primaryButton()!.title).toContain("Approve");
@@ -310,7 +316,10 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
     const body = submissions[0]!;
     expect(body.endpoint).toBe("feedback");
     expect(body.approved).toBe(true);
-    expect(body.feedback).toBe(LGTM_PLACEHOLDER);
+    // Empty since PR5: consumers print approve-time feedback, so the old
+    // placeholder would be appended to every approval; '' is also what lets
+    // the archive record a bare approval as `lgtm` with no sidecar.
+    expect(body.feedback).toBe("");
     expect(body.annotations).toEqual([]);
   });
 
@@ -355,7 +364,7 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
     expect(body.feedback).toContain("rebase on main before merging");
   });
 
-  test("discard confirm posts the plain LGTM, and nothing before the confirm", async () => {
+  test("discard confirm posts the bare approval, and nothing before the confirm", async () => {
     seededExternalAnnotations = [EXTERNAL_FINDING];
     await mountReview();
     await settle();
@@ -378,7 +387,8 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
     expect(submissions).toHaveLength(1);
     const body = submissions[0]!;
     expect(body.approved).toBe(true);
-    expect(body.feedback).toBe(LGTM_PLACEHOLDER);
+    // Discard means discard: bare approval, no placeholder, no annotations.
+    expect(body.feedback).toBe("");
     expect(body.annotations).toEqual([]);
   });
 
@@ -411,7 +421,7 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
     expect(submissions).toHaveLength(1);
     const body = submissions[0]!;
     expect(body.approved).toBe(true);
-    expect(body.feedback).toBe(LGTM_PLACEHOLDER);
+    expect(body.feedback).toBe("");
     expect(body.annotations).toEqual([]);
   });
 
@@ -468,10 +478,11 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
     expect(submissions[0]!.annotations).toEqual([]);
   });
 
-  // Spec §2.2's single mechanism: until PR5 lands the server advert AND the
-  // consumer delivery, an approve-carrying item would silently discard its
-  // note on four runtimes — so it must not render at all.
-  test("approve-with-notes items are absent while the advert is off", async () => {
+  // Spec §2.2's single mechanism + the compatibility matrix (spec §6.4):
+  // the mock /api/diff here carries NO approvalNotesSupported field — the
+  // old-server shape — which must read as not-capable, so an approve-carrying
+  // item can never render where its note would be silently discarded.
+  test("approve-with-notes items are absent when the server sends no advert (old server)", async () => {
     seededExternalAnnotations = [EXTERNAL_FINDING];
     await mountReview();
     await settle();
@@ -486,6 +497,32 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
     await settle();
   });
 
+  // PR5 delivery, App-level (spec §6.4): under a capable server's advert the
+  // menu offers `Approve with notes`, and choosing it posts `approved: true`
+  // with the live annotations riding AND their export as the feedback — the
+  // string consumers print after the approved prompt. A regression to the old
+  // handleApprove (empty annotations, placeholder feedback) approves while
+  // silently discarding the reviewer's findings.
+  test("under the advert, Approve with notes delivers the live annotations on the approval", async () => {
+    advertiseApprovalNotes = true;
+    seededExternalAnnotations = [EXTERNAL_FINDING];
+    await mountReview();
+    await settle();
+    await settle();
+
+    await openMenu();
+    const item = menuItem("Approve with notes");
+    if (!item) throw new Error("Approve with notes did not render under the advert");
+    await act(async () => item.click());
+    await settle();
+
+    expect(submissions).toHaveLength(1);
+    const body = submissions[0]!;
+    expect(body.approved).toBe(true);
+    expect((body.annotations ?? []).some((a) => a.id === "ext-1")).toBe(true);
+    expect(body.feedback).toContain("still drops null");
+  });
+
   test("empty-state menu carries only Request changes… (no Approve with a note…)", async () => {
     await mountReview();
 
@@ -549,7 +586,7 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
 
   // Guards the exact regression this project exists to fix on the surface
   // that has no Mod+Enter (E16-review): compact at zero must offer a visible
-  // positive decision row, and it must post the legacy approve body.
+  // positive decision row, and it must post the bare approval body.
   test("compact touch offers a positive decision row at zero and it posts", async () => {
     // SAFETY: implements the MediaQueryList surface the shell hooks consume;
     // coarse-pointer matches put the app in its compact touch layout.
@@ -580,7 +617,7 @@ describe.if(hasDom)("review decision control (agent mode)", () => {
 
     expect(submissions).toHaveLength(1);
     expect(submissions[0]!.approved).toBe(true);
-    expect(submissions[0]!.feedback).toBe(LGTM_PLACEHOLDER);
+    expect(submissions[0]!.feedback).toBe("");
     expect(submissions[0]!.annotations).toEqual([]);
   });
 });

packages/review-editor/App.tsx

@@ -16,11 +16,12 @@ import { FeedbackButton, ApproveButton, ExitButton } from '@plannotator/ui/compo
 import { buildDecisionSpec, type DecisionActionId, type DecisionMenuItem } from '@plannotator/ui/utils/decisionSpec';
 import { DecisionControl, DecisionNoteDialog, type DecisionHandler } from '@plannotator/ui/components/DecisionControl';
 import {
+  buildReviewApprovalBody,
   compactPrimaryIdForReviewDecision,
   compactRowIdForReviewDecisionItem,
   createGeneralReviewComment,
+  readApprovalNotesAdvert,
   resolveReviewDecisionAction,
-  REVIEW_APPROVAL_NOTES_SUPPORTED,
 } from './reviewDecision';
 import { useUpdateCheck } from '@plannotator/ui/hooks/useUpdateCheck';
 import { storage } from '@plannotator/ui/utils/storage';
@@ -620,6 +621,11 @@ const ReviewApp: React.FC = () => {
   const [compactDecisionComposer, setCompactDecisionComposer] = useState<DecisionMenuItem['id'] | null>(null);
   const [compactDecisionConfirm, setCompactDecisionConfirm] = useState<DecisionMenuItem['id'] | null>(null);
   const [sharingEnabled, setSharingEnabled] = useState(true);
+  // Server capability advert (spec §6.4): does this session's decision
+  // consumer deliver approve-time feedback? Defaults false so an old server
+  // that never sends the field renders no approve-carrying items (PR3
+  // behavior); read off every diff payload that carries it.
+  const [approvalNotesSupported, setApprovalNotesSupported] = useState(false);
   const [repoInfo, setRepoInfo] = useState<{ display: string; branch?: string } | null>(null);
 
   useEffect(() => {
@@ -1825,6 +1831,7 @@ const ReviewApp: React.FC = () => {
         diffOptions?: DiffOption[];
         agentCwd?: string | null;
         sharingEnabled?: boolean;
+        approvalNotesSupported?: boolean;
         repoInfo?: { display: string; branch?: string };
         prMetadata?: PRMetadata;
         prStackInfo?: PRStackInfo | null;
@@ -1883,6 +1890,7 @@ const ReviewApp: React.FC = () => {
         }
         if (data.agentCwd !== undefined) setAgentCwd(data.agentCwd);
         if (data.sharingEnabled !== undefined) setSharingEnabled(data.sharingEnabled);
+        setApprovalNotesSupported(readApprovalNotesAdvert(data.approvalNotesSupported));
         if (data.repoInfo) setRepoInfo(data.repoInfo);
         updatePRSession({
           ...(data.prMetadata && { prMetadata: data.prMetadata }),
@@ -2588,6 +2596,7 @@ const ReviewApp: React.FC = () => {
         commitInfo?: CommitDiffInfo;
         generatedFiles?: string[];
         baseBehindRemote?: boolean;
+        approvalNotesSupported?: boolean;
         superseded?: boolean;
       };
 
@@ -2603,6 +2612,12 @@ const ReviewApp: React.FC = () => {
       // switches never get here either, keeping the memo for a later retry.
       if (!isCommitDiffType(data.diffType)) preCommitDiffRef.current = null;
       setSnapshotId(data.snapshotId);
+      // Session-constant in practice, but re-read from any payload that
+      // carries it so the client stays in lockstep with whatever it last
+      // applied (the server echoes the advert on the whole diff family).
+      if (data.approvalNotesSupported !== undefined) {
+        setApprovalNotesSupported(readApprovalNotesAdvert(data.approvalNotesSupported));
+      }
 
       const nextFiles = orderFilesBySections(parseDiffToFiles(data.rawPatch), data.sections);
       // Rule 5 of auto-mark-viewed: a checkmark on content that has since
@@ -3561,19 +3576,26 @@ const ReviewApp: React.FC = () => {
     }
   }, [getDraftGeneration]);
 
-  // Approve without feedback (LGTM)
-  const handleApprove = useCallback(async () => {
+  // Approve — bare (LGTM), with a composer note, or with the live annotations
+  // riding along (PR5 delivery, spec §6.4). The old LGTM placeholder is gone:
+  // consumers now print approve-time feedback, so a bare approval must send
+  // `feedback: ''` (which also makes the archive's `lgtm` decision reachable
+  // and stops bare approvals writing a sidecar). Payload shape is the pure
+  // buildReviewApprovalBody, so the delivery contract is testable without
+  // mounting the App.
+  const handleApprove = useCallback(async (options?: { note?: string; withAnnotations?: boolean }) => {
     setIsApproving(true);
     try {
       const res = await fetch('/api/feedback', {
         method: 'POST',
         headers: { 'Content-Type': 'application/json' },
-        body: JSON.stringify({
+        body: JSON.stringify(buildReviewApprovalBody({
           draftGeneration: getDraftGeneration(),
-          approved: true,
-          feedback: 'LGTM - no changes requested.', // unused — integrations branch on `approved` flag
-          annotations: [],
-        }),
+          note: options?.note,
+          withAnnotations: options?.withAnnotations === true,
+          feedbackMarkdown,
+          annotations: allAnnotations,
+        })),
       });
       if (res.ok) {
         setSubmitted('approved');
@@ -3586,7 +3608,7 @@ const ReviewApp: React.FC = () => {
       setTimeout(() => setCopyFeedback(null), 2000);
       setIsApproving(false);
     }
-  }, [getDraftGeneration]);
+  }, [getDraftGeneration, feedbackMarkdown, allAnnotations]);
 
   // --- The unified review decision control, agent mode (spec §3.2/§4) ------
   // One primary, one callback: the header's left segment, the global
@@ -3694,18 +3716,17 @@ const ReviewApp: React.FC = () => {
       }
       case 'discard':
         // The DecisionControl / compact ConfirmDialog has already confirmed;
-        // handleApprove posts the plain LGTM with `annotations: []`.
+        // the bare approve posts `feedback: '', annotations: []`.
         void handleApprove();
         return;
       case 'approve-with-notes':
-        // Unreachable until PR5 (spec §6.4): buildDecisionSpec emits these
-        // ids only when approvalNotesSupported, and routing them onto today's
-        // handleApprove would silently discard the reviewer's notes. Refuse
-        // loudly rather than approve; reviewDecision.test.ts pins that the
-        // advert never emits an id whose route is still unimplemented.
-        console.error(
-          '[plannotator] approve-with-notes route reached while the approval-notes advert is off — PR5 must implement delivery before flipping the advert',
-        );
+        // PR5 delivery (spec §6.4): reachable only when the server advertised
+        // approvalNotesSupported — the session's consumer prints/sends the
+        // approve-time feedback these carry. "Approve with notes" ships the
+        // live annotations + their export; "Approve with a note…" ships the
+        // composer note alone.
+        if (submitted || busyWithDecision) return;
+        void handleApprove({ note, withAnnotations: action.withAnnotations });
         return;
     }
   }, [busyWithDecision, commitReviewNote, handleApprove, submitPrimaryDecision, submitted]);
@@ -3715,9 +3736,10 @@ const ReviewApp: React.FC = () => {
     gate: true, // review's primary positive decision IS approval
     count: totalAnnotationCount,
     hasFeedback: totalAnnotationCount > 0,
-    // Hardcoded false until PR5 (see the constant's doc in reviewDecision.ts).
-    approvalNotesSupported: REVIEW_APPROVAL_NOTES_SUPPORTED,
-  }), [totalAnnotationCount]);
+    // The server advert (spec §6.4) — false until a capable server says so,
+    // so approve-carrying items never render where notes would be discarded.
+    approvalNotesSupported,
+  }), [totalAnnotationCount, approvalNotesSupported]);
 
   const reviewDecisionHandlers = useMemo<Record<DecisionActionId, DecisionHandler>>(() => ({
     'primary': () => runReviewDecisionAction('primary'),

packages/review-editor/reviewDecision.test.ts

@@ -13,17 +13,18 @@ import {
   type DecisionSpecInput,
 } from "@plannotator/ui/utils/decisionSpec";
 import {
+  buildReviewApprovalBody,
   compactPrimaryIdForReviewDecision,
   compactRowIdForReviewDecisionItem,
   createGeneralReviewComment,
+  readApprovalNotesAdvert,
   resolveReviewDecisionAction,
-  REVIEW_APPROVAL_NOTES_SUPPORTED,
 } from "./reviewDecision";
 import { annotationMatchesPrScope } from "./utils/annotationScope";
 
-/** Every input combination the review app can hand the spec builder. The
- *  advert is swept both ways even though PR3 hardcodes it false, so the PR5
- *  flip cannot surface an unrouted id. */
+/** Every input combination the review app can hand the spec builder — the
+ *  advert swept both ways, so neither advert state can surface an unrouted
+ *  id. */
 function reviewInputs(): DecisionSpecInput[] {
   const inputs: DecisionSpecInput[] = [];
   for (const approvalNotesSupported of [false, true])
@@ -61,44 +62,102 @@ describe("review decision handler exhaustiveness", () => {
 
   // Guards the single-transport matrix (spec §3.2/§6.1): request-changes and
   // note-with-feedback differ only by state, never by route; the confirm item
-  // is the discard flow; the approve-carrying ids stay on the
-  // capability-gated PR5 path — routing one to the plain-note flow would
-  // misdeliver an approval as a change request.
+  // is the discard flow; both approve-carrying ids land on the PR5 delivery
+  // path — routing one to the plain-note flow would misdeliver an approval as
+  // a change request — and fork only on WHAT rides the approval.
   test("the routes fork only on approved, never on which menu state emitted them", () => {
     expect(resolveReviewDecisionAction("note-with-feedback"))
       .toEqual(resolveReviewDecisionAction("request-changes"));
     expect(resolveReviewDecisionAction("request-changes").kind).toBe("note");
     expect(resolveReviewDecisionAction("discard-and-finish").kind).toBe("discard");
     expect(resolveReviewDecisionAction("note-with-approval"))
-      .toEqual(resolveReviewDecisionAction("approve-with-notes"));
+      .toEqual({ kind: "approve-with-notes", withAnnotations: false });
+    expect(resolveReviewDecisionAction("approve-with-notes"))
+      .toEqual({ kind: "approve-with-notes", withAnnotations: true });
   });
 
-  // The PR5 contract (spec §6.4): the advert must never outrun delivery. The
-  // App's advert input is REVIEW_APPROVAL_NOTES_SUPPORTED; every id a spec
-  // built with it can emit must map to an IMPLEMENTED route. Today that holds
-  // because the advert is false. PR5 flips the advert (constant → server
-  // advert read) and this test then fails until the `approve-with-notes`
-  // route stops being a marked refusal (`implemented: false`) — i.e. until
-  // delivery is actually wired. Extend it to a delivery assertion in PR5;
-  // deleting the constant without updating this test breaks it at import,
-  // which is the point.
-  test("the advert never emits an id whose route is an unimplemented refusal", () => {
+  // The PR5 contract (spec §6.4), extended from PR3's tripwire exactly as its
+  // comment instructed: the advert may only emit approve-carrying ids whose
+  // route DELIVERS the content. The refusal marker is gone, so the assertion
+  // is now about the wire body: under a true advert every approve-carrying
+  // item builds an approval payload that carries the reviewer's content —
+  // the composer note as the feedback, or the live annotations + their
+  // export — never an empty body and never the removed LGTM placeholder.
+  test("under a true advert, every approve-carrying item's payload delivers the content", () => {
+    const EXPORT = "# Code Review Feedback\n\n## General\n\n- overall note\n";
+    const NOTE_ANNOTATION = createGeneralReviewComment("overall note")!;
     for (const count of [0, 2]) {
       const spec = buildDecisionSpec({
         app: "review",
         gate: true,
         count,
         hasFeedback: count > 0,
-        approvalNotesSupported: REVIEW_APPROVAL_NOTES_SUPPORTED,
+        approvalNotesSupported: true,
       });
-      for (const item of spec.items) {
+      const approveCarrying = spec.items.filter(
+        (item) => resolveReviewDecisionAction(item.id).kind === "approve-with-notes",
+      );
+      // A true advert must actually light an approve-carrying item in both
+      // states — otherwise delivery shipped but the menu never offers it.
+      expect(approveCarrying.length).toBeGreaterThan(0);
+      for (const item of approveCarrying) {
         const route = resolveReviewDecisionAction(item.id);
-        const implemented = "implemented" in route ? route.implemented : true;
-        expect(implemented).toBe(true);
+        if (route.kind !== "approve-with-notes") throw new Error("unreachable");
+        const body = buildReviewApprovalBody({
+          draftGeneration: 1,
+          note: item.composer ? "ship it, but rename the flag" : undefined,
+          withAnnotations: route.withAnnotations,
+          feedbackMarkdown: EXPORT,
+          annotations: [NOTE_ANNOTATION],
+        });
+        expect(body.approved).toBe(true);
+        if (route.withAnnotations) {
+          // "Approve with notes": the annotations ride for archive
+          // provenance, and their export is the feedback the consumer prints
+          // after the approved prompt.
+          expect(body.feedback).toBe(EXPORT);
+          expect(body.annotations).toEqual([NOTE_ANNOTATION]);
+        } else {
+          // "Approve with a note…": the note IS the feedback.
+          expect(body.feedback).toBe("ship it, but rename the flag");
+          expect(body.annotations).toEqual([]);
+        }
       }
     }
   });
 
+  // Compatibility matrix (spec §6.4): old server / new client — a payload
+  // without the field reads false, so no approve-carrying item renders (the
+  // PR3 behavior); and the new bare approval sends `feedback: ''` instead of
+  // the removed LGTM placeholder, which is what makes the archive's `lgtm`
+  // decision reachable and stops bare approvals writing sidecars.
+  test("absent advert reads false, and a bare approval carries no placeholder", () => {
+    expect(readApprovalNotesAdvert(undefined)).toBe(false);
+    // Only a literal true is capable — a truthy string or number is not.
+    expect(readApprovalNotesAdvert("true")).toBe(false);
+    expect(readApprovalNotesAdvert(1)).toBe(false);
+    expect(readApprovalNotesAdvert(true)).toBe(true);
+
+    const spec = buildDecisionSpec({
+      app: "review",
+      gate: true,
+      count: 2,
+      hasFeedback: true,
+      approvalNotesSupported: readApprovalNotesAdvert(undefined),
+    });
+    for (const item of spec.items) {
+      expect(resolveReviewDecisionAction(item.id).kind).not.toBe("approve-with-notes");
+    }
+
+    const bare = buildReviewApprovalBody({
+      draftGeneration: 3,
+      withAnnotations: false,
+      feedbackMarkdown: "# Code Review Feedback\n",
+      annotations: [createGeneralReviewComment("x")!],
+    });
+    expect(bare).toEqual({ draftGeneration: 3, approved: true, feedback: "", annotations: [] });
+  });
+
   // Guards the compact surface: row ids double as React keys, so a collision
   // hides a decision row on touch — the silent-data-loss class the #1436
   // review flagged (E16-review).

packages/review-editor/reviewDecision.ts

@@ -7,24 +7,27 @@ import type { CompactReviewAction } from './components/ReviewHeaderMenu';
  *
  * `buildDecisionSpec` decides WHAT the header offers; this module decides
  * WHERE each choice goes. Review is single-transport (spec §3.2/§6.1): every
- * decision POSTs `/api/feedback`, with `approved` as the only fork — notes
- * commit a `scope:'general'` CodeAnnotation and ride the change-request send,
- * the post-confirm discard is the same plain LGTM the Approve primary posts
- * (`handleApprove` already sends `annotations: []`). Kept pure (no React, no
+ * decision POSTs `/api/feedback`, with `approved` as the only fork —
+ * change-request notes commit a `scope:'general'` CodeAnnotation and ride the
+ * send, approvals post `buildReviewApprovalBody` (bare, with a note, or with
+ * the live annotations — PR5 delivery), and the post-confirm discard is the
+ * same bare approve the Approve primary posts. Kept pure (no React, no
  * App import) so the §8C handler-exhaustiveness test runs in the plain
  * `bun test` lane: every id the spec can emit must resolve here, and an id
  * added to `decisionSpec.ts` without a route fails the exhaustive switch.
  */
 /**
- * Whether the runtime delivers approve-carrying notes. Hardcoded false until
- * PR5 ships the two-runtime delivery + the `/api/diff`-family advert
- * (spec §6.4); flipping it without that server work would render
- * approve-carrying items whose notes four of the runtimes still discard.
- * PR5 replaces this constant with the server advert read AND must mark the
- * `approve-with-notes` route implemented in the same change —
- * `reviewDecision.test.ts` fails on an advert that outruns delivery.
+ * Reads the `approvalNotesSupported` capability advert off a diff payload
+ * (`/api/diff` and the switch/PR family — the server echoes it on all four).
+ * Anything but a literal `true` reads as false: an OLD server that never
+ * sends the field advertises "not capable" and the client renders no
+ * approve-carrying items — exactly the PR3 behavior. A NEW server against an
+ * old client changes nothing either (the field is simply ignored). Pinned by
+ * `reviewDecision.test.ts`.
  */
-export const REVIEW_APPROVAL_NOTES_SUPPORTED = false;
+export function readApprovalNotesAdvert(value: unknown): boolean {
+  return value === true;
+}
 
 export type ReviewDecisionRoute =
   /** The adaptive primary: Approve at zero, Send Feedback otherwise. */
@@ -32,14 +35,15 @@ export type ReviewDecisionRoute =
   /** Commit the note as a scope:'general' CodeAnnotation, then submit on the
    *  next render (the payload builders close over `allAnnotations`). */
   | { kind: 'note' }
-  /** Post-confirm discard: the plain LGTM approve (annotations dropped). */
+  /** Post-confirm discard: the bare approve (annotations dropped). */
   | { kind: 'discard' }
-  /** Approve with the live feedback riding along. Capability-gated: the spec
-   *  emits its ids only when `approvalNotesSupported`, which no review server
-   *  advertises until PR5 (spec §6.4). `implemented: false` marks the App
-   *  wiring as a refusal — the contract test pins that the advert never
-   *  emits an id whose route is unimplemented. */
-  | { kind: 'approve-with-notes'; implemented: false };
+  /** Approve with content riding along (PR5 delivery, spec §6.4). The spec
+   *  emits these ids only when the server advertises `approvalNotesSupported`
+   *  — i.e. when this session's decision consumer prints/sends approve-time
+   *  feedback instead of discarding it. `withAnnotations` distinguishes
+   *  "Approve with notes" (the live annotations + their export ride the
+   *  approval) from "Approve with a note…" (the composer note alone). */
+  | { kind: 'approve-with-notes'; withAnnotations: boolean };
 
 export function resolveReviewDecisionAction(id: DecisionActionId): ReviewDecisionRoute {
   switch (id) {
@@ -50,15 +54,60 @@ export function resolveReviewDecisionAction(id: DecisionActionId): ReviewDecisio
       // The two differ only by state (empty vs feedback), never by transport.
       return { kind: 'note' };
     case 'note-with-approval':
+      return { kind: 'approve-with-notes', withAnnotations: false };
     case 'approve-with-notes':
-      // Both approve-carrying items land on the same PR5 delivery path; until
-      // the advert flips, neither id is ever emitted.
-      return { kind: 'approve-with-notes', implemented: false };
+      return { kind: 'approve-with-notes', withAnnotations: true };
     case 'discard-and-finish':
       return { kind: 'discard' };
   }
 }
 
+export interface ReviewApprovalBodyInput {
+  draftGeneration: number;
+  /** Composer note ("Approve with a note…"); whitespace-only means none. */
+  note?: string;
+  /** "Approve with notes": the live annotations ride the approval. */
+  withAnnotations: boolean;
+  /** The same export Send Feedback posts — what the agent reads as guidance. */
+  feedbackMarkdown: string;
+  annotations: CodeAnnotation[];
+}
+
+/**
+ * The `/api/feedback` body for every approval (PR5 delivery, spec §6.4).
+ *
+ * The pre-PR5 client sent the placeholder `'LGTM - no changes requested.'` on
+ * every approval; with consumers now printing approve-time feedback, that
+ * placeholder would be appended to every bare approval, so it is gone:
+ * a bare approval sends `feedback: ''`, which is also what finally makes the
+ * archive's `lgtm` decision reachable and stops bare approvals writing a
+ * sidecar (spec §6.2 fact 1). "Approve with a note…" sends the note as the
+ * feedback; "Approve with notes" sends the live annotation export as the
+ * feedback with the annotations riding for archive provenance.
+ */
+export function buildReviewApprovalBody(input: ReviewApprovalBodyInput): {
+  draftGeneration: number;
+  approved: true;
+  feedback: string;
+  annotations: unknown[];
+} {
+  const note = input.note?.trim() ?? '';
+  if (input.withAnnotations) {
+    return {
+      draftGeneration: input.draftGeneration,
+      approved: true,
+      feedback: input.feedbackMarkdown,
+      annotations: input.annotations,
+    };
+  }
+  return {
+    draftGeneration: input.draftGeneration,
+    approved: true,
+    feedback: note,
+    annotations: [],
+  };
+}
+
 /**
  * Compact/touch row ids for the spec-driven decision rows. Ids double as
  * React keys, so they must be unique within any one spec: the composers are