diff --git a/AGENTS.md b/AGENTS.md index d092293a..74b2c0e0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -389,9 +389,11 @@ keyboard and header can never disagree. Transport routing is pure in `packages/editor/annotateDecision.ts`: `Done` and every note post `/api/feedback` (a note becomes a `GLOBAL_COMMENT` at submit time with a one-render deferred submit — zero server change), so `formatAnnotateOutcome` shapes and strict-gate exit codes are byte-identical to the old -keyboard-only zero submit; only gate-mode approvals reach `/api/approve`. The non-gated -"Done with a note…" is distinguished from "Request changes…" solely by the approval-framing -sentence (`buildCompleteAnnotateFeedback`'s `approvalFraming`), and the only confirm left is the +keyboard-only zero submit; only gate-mode approvals reach `/api/approve`. The non-gated empty +menu carries a single composer, "Send a note…" (maintainer ruling: the old "Done with a note…" / +"Request changes…" pair differed only by framing on the same transport and was collapsed into +one item); the approval-framing sentence (`buildCompleteAnnotateFeedback`'s `approvalFraming`) +now serves only the non-gated discard path, and the only confirm left is the explicit `Done/Approve, discard n annotations…` menu item (plus the pre-existing close-with-content warning). Compact/touch rows are generated from the same spec, so a visible positive decision exists in every state; composer rows open `DecisionNoteDialog`. The header flip diff --git a/packages/editor/App.decisionControl.test.tsx b/packages/editor/App.decisionControl.test.tsx index b5bca572..1d4b62e9 100644 --- a/packages/editor/App.decisionControl.test.tsx +++ b/packages/editor/App.decisionControl.test.tsx @@ -11,9 +11,8 @@ * - The note must reach the agent AS A GLOBAL COMMENT in both the exported * feedback string AND the annotations array, one render after the commit — * a same-tick submit sends the pre-note payload and silently drops it. - * - "Done with a note…" and "Request changes…" must NOT collapse into one - * payload: only the approval framing distinguishes them on the one - * feedback transport. + * - "Send a note…" (the collapsed non-gate composer) must post plain, + * unframed feedback: a note is a note, never a fabricated approval. * - Gate mode's empty primary must approve through /api/approve (that is * what makes a strict gate exit 0). * - The discard confirm must drop the annotations from the posted body, not @@ -280,12 +279,12 @@ describe.if(hasDom)("annotate decision control", () => { expect(submissions[0]!.feedback).toBe(ANNOTATE_NO_FEEDBACK_SENTENCE); }); - test("Request changes… posts one GLOBAL_COMMENT change request without approval framing", async () => { + test("Send a note… posts one GLOBAL_COMMENT without approval framing", async () => { setStorageBackend(memoryBackend); seedAnnouncementsSeen(); await mountAnnotate(); - await openComposer("Request changes"); + await openComposer("Send a note"); await typeNote("tighten the intro"); await pressNoteKey("Enter", { metaKey: true }); @@ -299,23 +298,20 @@ describe.if(hasDom)("annotate decision control", () => { expect(body.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(false); }); - test("Done with a note… posts the same note WITH the approval framing sentence", async () => { + // Maintainer ruling (empty-menu collapse): the non-gate empty menu is ONE + // composer. A resurrected "Done with a note…" row would silently split the + // decision back into two costumes. + test("the empty non-gate menu offers exactly one composer item", async () => { setStorageBackend(memoryBackend); seedAnnouncementsSeen(); await mountAnnotate(); - await openComposer("Done with a note"); - await typeNote("looks fine, watch the migration"); - await pressNoteKey("Enter", { ctrlKey: true }); + await act(async () => caretButton()!.click()); + await settle(); - expect(submissions).toHaveLength(1); - const body = submissions[0]!; - expect(body.endpoint).toBe("feedback"); - const notes = globalComments(body); - expect(notes).toHaveLength(1); - expect(notes[0]!.text).toBe("looks fine, watch the migration"); - expect(body.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(true); - expect(body.feedback).toContain("looks fine, watch the migration"); + const items = Array.from(document.querySelectorAll('[role="menuitem"]')); + expect(items).toHaveLength(1); + expect(menuItem("Done with a note")).toBeUndefined(); }); test("the note rides alongside annotations already in the session", async () => { @@ -405,55 +401,55 @@ describe.if(hasDom)("annotate decision control", () => { seedAnnouncementsSeen(); await mountAnnotate(); - await openComposer("Request changes"); + await openComposer("Send a note"); await typeNote("not ready to send"); await pressNoteKey("Escape"); // Back at the menu, nothing submitted. expect(noteInput()).toBeNull(); - expect(menuItem("Request changes")).toBeDefined(); + expect(menuItem("Send a note")).toBeDefined(); expect(submissions).toHaveLength(0); - const item = menuItem("Request changes")!; + const item = menuItem("Send a note")!; await act(async () => item.click()); await settle(); expect(noteInput()?.value).toBe("not ready to send"); }); // L3 pin: a failed POST must keep the captured decision armed — retrying - // through the primary re-posts the SAME route with the approval framing - // intact. Without it, the retry would re-derive from live state (the note - // raised hasFeedbackToSend) and silently reframe "Done with a note" as a - // bare change request. - test("a failed note submit keeps the decision armed; retry keeps the captured framing", async () => { + // through the primary replays THAT decision. Without it, the retry would + // either drop the armed note or commit it a second time (two + // GLOBAL_COMMENTs from one composer submit). + test("a failed note submit keeps the decision armed; retry replays it without a second note", async () => { setStorageBackend(memoryBackend); seedAnnouncementsSeen(); failFeedbackPosts = 1; await mountAnnotate(); - await openComposer("Done with a note"); + await openComposer("Send a note"); await typeNote("hold the line"); await pressNoteKey("Enter", { metaKey: true }); - // First attempt: captured framing, but the POST failed — no completion. + // First attempt: captured decision posted, but the POST failed — no + // completion overlay, the session stays reviewable. expect(submissions).toHaveLength(1); expect(submissions[0]!.endpoint).toBe("feedback"); - expect(submissions[0]!.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(true); + expect(submissions[0]!.feedback).toContain("hold the line"); const primary = primaryButton(); expect(primary).not.toBeNull(); // still reviewable, not submitted - // Retry via the primary: the armed decision replays its captured route - // and framing rather than the bare (now Send Feedback) primary. + // Retry via the primary: the armed decision replays with the SAME single + // note — never a re-commit, never a dropped note. await act(async () => primary!.click()); await settle(); expect(submissions).toHaveLength(2); const retry = submissions[1]!; expect(retry.endpoint).toBe("feedback"); - expect(retry.feedback!.startsWith(ANNOTATE_NO_FEEDBACK_SENTENCE)).toBe(true); expect(retry.feedback).toContain("hold the line"); const notes = globalComments(retry); expect(notes).toHaveLength(1); + expect(notes[0]!.text).toBe("hold the line"); }); // The HTML pinpoint Esc ladder must still receive Escape when the popover diff --git a/packages/editor/App.tsx b/packages/editor/App.tsx index 64a842b3..ce9c9571 100644 --- a/packages/editor/App.tsx +++ b/packages/editor/App.tsx @@ -2808,7 +2808,7 @@ const App: React.FC = () => { /** Discard flow: every annotation source is dropped, so the builder * emits the legacy zero payload (plus any direct-edit sections). */ discardAnnotations?: boolean; - /** Positive-finish framing for the non-gated note (spec §3.1). */ + /** Positive-finish framing for the non-gated discard (spec §3.1). */ approvalFraming?: boolean; }, ): string => { @@ -3686,7 +3686,8 @@ const App: React.FC = () => { /** Discard-and-finish (post-confirm): annotations dropped, the payload is * the legacy "reviewed, no feedback" record. */ discardAnnotations?: boolean; - /** "Done with a note…" — approval framing on the one feedback string. */ + /** Approval framing on the one feedback string — the non-gated discard + * path only, since the empty-menu collapse removed the framed note. */ approvalFraming?: boolean; }): Promise => { setIsSubmitting(true); @@ -5023,8 +5024,9 @@ const App: React.FC = () => { if (pendingDecisionSubmit) { // L3: a failed note submit stays armed with its captured route/framing; // the next primary invocation retries THAT decision, never the bare - // primary (which would silently reframe "Done with a note" as a change - // request once the note raised hasFeedbackToSend). + // primary (which would re-derive from live state — dropping a gate's + // captured approve route, or re-committing the note — once the note + // raised hasFeedbackToSend). dispatchPendingDecision(pendingDecisionSubmit); return; } diff --git a/packages/editor/annotateDecision.ts b/packages/editor/annotateDecision.ts index 2ed722dc..2b0bb07a 100644 --- a/packages/editor/annotateDecision.ts +++ b/packages/editor/annotateDecision.ts @@ -16,8 +16,10 @@ import type { CompactPlanAction } from "@plannotator/ui/components/PlanHeaderMen export type AnnotateDecisionRoute = | { kind: "primary" } /** Commit the note as a GLOBAL_COMMENT, then submit on the next render - * (#1436 mechanism). `route` picks the endpoint; `approvalFraming` marks - * the non-gated positive finish ("Done with a note…"). */ + * (#1436 mechanism). `route` picks the endpoint. `approvalFraming` is kept + * for the framing machinery (the App's discard path still frames its + * positive finish), but since the empty-menu collapse no spec-emitted note + * route sets it — every menu note posts plain, unframed feedback. */ | { kind: "note"; route: "feedback" | "approve"; approvalFraming: boolean } /** Direct approve with the live feedback riding along (gate + capability). */ | { kind: "approve-with-notes" } @@ -32,13 +34,16 @@ export function resolveAnnotateDecisionAction( case "primary": return { kind: "primary" }; case "note-with-approval": - // Gate: the note rides the approval body (/api/approve). Non-gate has - // no approve channel, so "Done with a note…" posts /api/feedback with - // the approval-framing sentence — the only place a menu choice changes - // payload text rather than endpoint (spec §3.1 "framing"). + // Gate: the note rides the approval body (/api/approve). The non-gate + // arm is UNREACHABLE from the spec since the empty-menu collapse + // (maintainer ruling: the non-gate menu's one composer is + // 'request-changes' / "Send a note…"); it stays only because the id + // union is shared with the gate. If a stray dispatch ever lands here it + // must behave like the collapsed item — plain feedback, and never + // fabricated approval framing. return ctx.gate ? { kind: "note", route: "approve", approvalFraming: false } - : { kind: "note", route: "feedback", approvalFraming: true }; + : { kind: "note", route: "feedback", approvalFraming: false }; case "request-changes": case "note-with-feedback": // The two differ only by state (empty vs feedback), never by transport. diff --git a/packages/editor/annotateSubmission.test.ts b/packages/editor/annotateSubmission.test.ts index 48265ccc..e957376c 100644 --- a/packages/editor/annotateSubmission.test.ts +++ b/packages/editor/annotateSubmission.test.ts @@ -231,10 +231,10 @@ describe("annotate approval submission", () => { expect(feedback).toContain("(line 5) "); }); - // Guards the two menu items collapsing into one payload (spec §3.1 framing): - // "Done with a note…" and "Request changes…" both post one GLOBAL_COMMENT to - // /api/feedback, so only the framing prefix distinguishes approval from a - // change request. + // Guards the framing machinery the discard path still rides: a framed + // payload prefixes the zero-state sentence, an unframed one never does, and + // at zero content the frame is idempotent (the discard body stays + // byte-identical to the legacy zero payload). test("approval framing prefixes the zero-state sentence before the note", () => { const note: Annotation = { id: "global-note-1", diff --git a/packages/editor/annotateSubmission.ts b/packages/editor/annotateSubmission.ts index 8976b723..47db27d4 100644 --- a/packages/editor/annotateSubmission.ts +++ b/packages/editor/annotateSubmission.ts @@ -77,12 +77,13 @@ export interface CompleteAnnotateFeedbackInput { savedFileChangesSection: string; messageEntries?: MessageAnnotationEntry[]; /** - * Positive-finish framing ("Done with a note…"). Non-gated annotate has no - * approve channel — every outcome is one feedback string — so without this - * the approval-note and change-request menu items would post byte-identical - * bodies. This is the ONLY place a menu choice changes payload text rather - * than endpoint: it prefixes the zero-state sentence before the note's - * global-comment section (idempotent when the text already is the sentence). + * Positive-finish framing. Non-gated annotate has no approve channel — + * every outcome is one feedback string — so this prefixes the zero-state + * sentence before the annotation sections (idempotent when the text already + * is the sentence). Since the empty-menu collapse (the maintainer merged + * "Done with a note…" into the single unframed "Send a note…"), the only + * caller is the non-gated discard path, which frames its positive finish + * over any direct edits that still ride along. */ approvalFraming?: boolean; } diff --git a/packages/editor/decisionHandlers.test.ts b/packages/editor/decisionHandlers.test.ts index 4606cb74..bf08f4b8 100644 --- a/packages/editor/decisionHandlers.test.ts +++ b/packages/editor/decisionHandlers.test.ts @@ -50,14 +50,23 @@ describe("annotate decision handler exhaustiveness", () => { // Guards the endpoint matrix (spec §3.1/§6.1): Done and every note stay on // /api/feedback so formatAnnotateOutcome shapes and strict-gate exit codes // are untouched; only gate-mode approvals reach /api/approve. - test("note and discard routes follow the gate's transport, framing only on the non-gated positive finish", () => { + test("note and discard routes follow the gate's transport; no menu note ever carries approval framing", () => { const gated = { gate: true }; const ungated = { gate: false }; expect(resolveAnnotateDecisionAction("note-with-approval", gated)) .toEqual({ kind: "note", route: "approve", approvalFraming: false }); + // The non-gate arm is dead code by construction since the empty-menu + // collapse. Assert the unreachability itself (so this pin cannot pass + // vacuously over a route the spec quietly resurrects)… + for (const input of annotateInputs()) { + if (input.gate) continue; + expect(buildDecisionSpec(input).items.map((item) => item.id)) + .not.toContain("note-with-approval"); + } + // …and pin that even a stray dispatch cannot fabricate approval framing. expect(resolveAnnotateDecisionAction("note-with-approval", ungated)) - .toEqual({ kind: "note", route: "feedback", approvalFraming: true }); + .toEqual({ kind: "note", route: "feedback", approvalFraming: false }); for (const ctx of [gated, ungated]) { // The two differ only by state, never by transport or framing. diff --git a/packages/editor/shortcuts.ts b/packages/editor/shortcuts.ts index 379cdab0..3505a13c 100644 --- a/packages/editor/shortcuts.ts +++ b/packages/editor/shortcuts.ts @@ -31,7 +31,7 @@ export const planEditorShortcuts = defineShortcutScope({ displayOrder: 10, }, submitAnnotations: { - description: 'Done / Send feedback — whichever the header shows', + description: 'Done / Send feedback, whichever the header shows', bindings: ['Mod+Enter'], section: 'Actions', hint: 'Fires the adaptive header primary: Done (or Approve in gate mode) with nothing to send, Send Feedback otherwise.', diff --git a/packages/review-editor/shortcuts.ts b/packages/review-editor/shortcuts.ts index 97c29f34..56b16cf8 100644 --- a/packages/review-editor/shortcuts.ts +++ b/packages/review-editor/shortcuts.ts @@ -20,7 +20,7 @@ export const reviewEditorShortcuts = defineShortcutScope({ title: 'Review Editor', shortcuts: { submit: { - description: 'Approve / Send feedback — whichever the header shows', + description: 'Approve / Send feedback, whichever the header shows', bindings: ['Mod+Enter'], section: 'Actions', hint: 'Fires the adaptive header primary: Approve with no annotations, Send Feedback otherwise.', diff --git a/packages/ui/shortcuts.test.ts b/packages/ui/shortcuts.test.ts index 633e9e49..41b3c438 100644 --- a/packages/ui/shortcuts.test.ts +++ b/packages/ui/shortcuts.test.ts @@ -101,7 +101,9 @@ describe('shortcuts', () => { expect(getShortcut(planReviewSettingsShortcutRegistry, 'plan-review-editor-settings', 'submitPlan')?.description).toBe('Approve / Send feedback'); expect(getShortcut(planReviewSettingsShortcutRegistry, 'plan-review-editor-settings', 'submitAnnotations')).toBeUndefined(); - expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-editor-settings', 'submitAnnotations')?.description).toBe('Done / Send feedback — whichever the header shows'); + // Fact-guard, not a prose pin: the annotate registry's submit action must + // describe the adaptive Done/Send primary (distinct from plan review's). + expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-editor-settings', 'submitAnnotations')?.description).toContain('Done / Send feedback'); expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-editor-settings', 'submitPlan')).toBeUndefined(); expect(getShortcut(annotateSettingsShortcutRegistry, 'annotate-sidebar', 'toggleContents')?.description).toBe('Toggle Contents sidebar'); diff --git a/packages/ui/utils/decisionSpec.test.ts b/packages/ui/utils/decisionSpec.test.ts index 0a88a0c2..1ed86478 100644 --- a/packages/ui/utils/decisionSpec.test.ts +++ b/packages/ui/utils/decisionSpec.test.ts @@ -24,7 +24,7 @@ function itemIds(spec: DecisionSpec): string[] { describe('buildDecisionSpec state matrix', () => { // Guards the model itself: each row of the spec's state table produces the // expected primary and the expected ordered menu. - it('annotate, no feedback, no gate → Done + note/request-changes', () => { + it('annotate, no feedback, no gate → Done + the single Send a note composer', () => { const spec = buildDecisionSpec({ app: 'annotate', gate: false, count: 0, hasFeedback: false, approvalNotesSupported: false, }); @@ -33,16 +33,13 @@ describe('buildDecisionSpec state matrix', () => { // it must never wear the success tone or check icon Approve wears. expect(spec.primary.tone).toBe('neutral'); expect(spec.primary.icon).toBeUndefined(); - expect(itemIds(spec)).toEqual(['note-with-approval', 'request-changes']); - // Frozen copy (maintainer-approved): 'Request changes…'. - expect(spec.items[1].label).toBe('Request changes…'); - // "Done with a note…" posts /api/feedback — never capability-gated. The - // two composers must stay DISTINCT actions (positive finish vs change - // request) — the labels themselves are free prose. - expect(spec.items[0].composer?.tone).toBe('neutral'); - expect(spec.items[1].dividerBefore).toBe(true); - expect(spec.items[1].composer?.tone).toBe('primary'); - expect(spec.items[0].composer?.actionLabel).not.toBe(spec.items[1].composer?.actionLabel); + // Maintainer ruling (empty-menu collapse): ONE composer item — the old + // "Done with a note…" / "Request changes…" pair differed only by framing + // on the same transport and must not come back. Label is free prose. + expect(itemIds(spec)).toEqual(['request-changes']); + expect(spec.items[0].composer).toBeDefined(); + expect(spec.items[0].composer?.tone).toBe('primary'); + expect(spec.items[0].dividerBefore).toBe(false); }); it('annotate, no feedback, gate → Approve; approve-note item only with the capability', () => { @@ -105,8 +102,6 @@ describe('buildDecisionSpec state matrix', () => { expect(delivered.primary.label).toBe('Done'); // frozen copy, maintainer-approved expect(delivered.primary.title).not.toContain('no feedback'); - const deliveredNote = delivered.items.find((item) => item.id === 'note-with-approval')!; - expect(deliveredNote.subtitle).not.toContain('no feedback'); // The two states must actually differ — a regression that ignores the // flag would silently restore the lying tooltip. expect(delivered.primary.title).not.toBe(plain.primary.title); @@ -167,13 +162,53 @@ describe('buildDecisionSpec invariants', () => { it('approvalNotesSupported: false ⇒ no approve-carrying item anywhere', () => { for (const input of allInputs()) { if (input.approvalNotesSupported) continue; - if (input.app === 'annotate' && !input.gate) continue; // no approve channel at all const ids = itemIds(buildDecisionSpec(input)); expect(ids).not.toContain('approve-with-notes'); expect(ids).not.toContain('note-with-approval'); } }); + // Maintainer ruling (empty-menu collapse): without a gate there is no + // approval channel, so no approve-carrying id may appear in any non-gate + // annotate state, capability advert or not — this is also what keeps the + // non-gate 'note-with-approval' arm in annotateDecision.ts dead code. + it('non-gate annotate never emits an approve-carrying item', () => { + for (const input of allInputs()) { + if (input.app !== 'annotate' || input.gate) continue; + const ids = itemIds(buildDecisionSpec(input)); + expect(ids).not.toContain('note-with-approval'); + expect(ids).not.toContain('approve-with-notes'); + } + }); + + // Maintainer ruling: no user-facing decision-control string carries an em + // dash. Sweeps every field the control renders, across both arms. + it('no user-facing string contains an em dash', () => { + const inputs: DecisionSpecInput[] = [ + ...allInputs(), + ...([0, 1, 3] as const).flatMap((count) => + [false, true].map((selfAuthored): DecisionSpecInput => ({ + app: 'review', gate: true, count, hasFeedback: count > 0, + approvalNotesSupported: false, + platform: { label: 'GitHub', mrLabel: 'PR', selfAuthored }, + }))), + ...allInputs().map((input) => ({ ...input, feedbackDelivered: true })), + ]; + for (const input of inputs) { + const spec = buildDecisionSpec(input); + const strings = [ + spec.primary.label, spec.primary.shortLabel, spec.primary.mobileLabel, + spec.primary.title, + ...spec.items.flatMap((item) => [ + item.label, item.subtitle, + item.composer?.title, item.composer?.actionLabel, item.composer?.placeholder, + item.confirm?.title, item.confirm?.message, item.confirm?.confirmText, + ]), + ]; + for (const value of strings) expect(value ?? '').not.toContain('—'); + } + }); + // Guards a refactor that drops the one remaining guard dialog. it('every discard item carries a confirm', () => { for (const input of allInputs()) { diff --git a/packages/ui/utils/decisionSpec.ts b/packages/ui/utils/decisionSpec.ts index e61d5b85..6bbb9521 100644 --- a/packages/ui/utils/decisionSpec.ts +++ b/packages/ui/utils/decisionSpec.ts @@ -10,14 +10,16 @@ * app-shared chrome and is deliberately absent from the README supported-import * list and the strict-consumer tsconfig. * - * Labels, subtitles and confirm strings are the approved prototype's, verbatim - * (DESIGN_final-proposal.html `spec()`), which is authoritative over any older - * branch or mock copy. + * Labels, subtitles and confirm strings are the approved prototype's + * (DESIGN_final-proposal.html `spec()`), authoritative over any older branch + * or mock copy — except where a later maintainer ruling supersedes it: the + * non-gate empty menu carries ONE composer ("Send a note…"), and no + * user-facing string uses an em dash. */ export type DecisionActionId = | 'primary' // the left segment - | 'note-with-approval' // "Done with a note…" / "Approve with a note…" + | 'note-with-approval' // "Approve with a note…" (approval flows only) | 'request-changes' // "Request changes…" | 'note-with-feedback' // "Send with a note…" | 'approve-with-notes' // review + gate-annotate; capability-gated @@ -139,74 +141,73 @@ function annotationNoun(count: number): string { * gets `Done`. */ function buildEmptySpec(input: DecisionSpecInput, approvalFlow: boolean): DecisionSpec { - // "Done with a note…" posts /api/feedback like every other non-gated annotate - // outcome, so it is never capability-gated. "Approve with a note…" carries a - // note on the approve channel, which four runtimes still discard — it renders - // only where the advert says delivery works (never an item that silently - // drops content). - const positive: DecisionMenuItem | null = approvalFlow - ? input.approvalNotesSupported - ? { - id: 'note-with-approval', - label: 'Approve with a note…', - subtitle: 'Approve and send a short note with it', + // "Approve with a note…" carries a note on the approve channel, which four + // runtimes still discard — it renders only where the advert says delivery + // works (never an item that silently drops content). Non-gate annotate has + // no approve channel and no positive-note item at all: the maintainer ruled + // the old "Done with a note…" / "Request changes…" pair collapsed into the + // single "Send a note…" below, because their only difference was framing on + // the same /api/feedback transport. + const positive: DecisionMenuItem | null = approvalFlow && input.approvalNotesSupported + ? { + id: 'note-with-approval', + label: 'Approve with a note…', + subtitle: 'Approve and send a short note with it', + tone: 'success', + icon: 'check', + composer: { + title: 'Approve with a note', + actionLabel: 'Approve and send note', tone: 'success', icon: 'check', - composer: { - title: 'Approve with a note', - actionLabel: 'Approve — send note', - tone: 'success', - icon: 'check', - placeholder: DECISION_NOTE_PLACEHOLDER, - }, - } - : null - : { - id: 'note-with-approval', - label: 'Done with a note…', - // Maintainer ruling (post-demo): without a gate there IS no approval — - // Done is a positive finish, not an approve — so this row must not - // wear the approval costume (success tone + check). Neutral tone, - // send icon, and copy that never says "approval". Free prose, NOT - // frozen. (M1: the delivered variant names the session record.) - subtitle: input.feedbackDelivered - ? 'Finish and send a short note with the session record' - : 'Finish and send a short note with it', - tone: 'neutral', + placeholder: DECISION_NOTE_PLACEHOLDER, + }, + } + : null; + + const requestChanges: DecisionMenuItem = approvalFlow + ? { + id: 'request-changes', + // Frozen copy (maintainer-approved): 'Request changes…'. + label: 'Request changes…', + subtitle: 'Write overall feedback, sent as a change request', + tone: 'primary', icon: 'send', + dividerBefore: positive !== null, composer: { - title: 'Done with a note', - actionLabel: 'Done — send note', - tone: 'neutral', + title: 'Request changes', + actionLabel: 'Send as feedback', + tone: 'primary', + icon: 'send', + placeholder: DECISION_NOTE_PLACEHOLDER, + }, + } + : { + // Maintainer ruling (empty-menu collapse): the one non-gate composer. + // Same id and route as the old change request (plain /api/feedback, + // no approval framing) — only the copy is new. Free prose, NOT frozen. + id: 'request-changes', + label: 'Send a note…', + subtitle: 'Write a note and send it as feedback', + tone: 'primary', + icon: 'send', + dividerBefore: false, + composer: { + title: 'Send a note', + actionLabel: 'Send as feedback', + tone: 'primary', icon: 'send', placeholder: DECISION_NOTE_PLACEHOLDER, }, }; - const requestChanges: DecisionMenuItem = { - id: 'request-changes', - // Frozen copy (maintainer-approved): 'Request changes…'. - label: 'Request changes…', - subtitle: 'Write overall feedback — sent as a change request', - tone: 'primary', - icon: 'send', - dividerBefore: positive !== null, - composer: { - title: 'Request changes', - actionLabel: 'Send as feedback', - tone: 'primary', - icon: 'send', - placeholder: DECISION_NOTE_PLACEHOLDER, - }, - }; - return { primary: approvalFlow ? { id: 'primary', // Frozen copy (maintainer-approved): 'Approve'. label: 'Approve', - title: 'Approve — no changes requested', + title: 'Approve: no changes requested', tone: 'success', icon: 'check', } @@ -219,8 +220,8 @@ function buildEmptySpec(input: DecisionSpecInput, approvalFlow: boolean): Decisi // on stdout may never have seen the terminal delivery), so the // tooltip must not claim "no feedback". Free prose, NOT frozen. title: input.feedbackDelivered - ? 'Finish — sends the session record (feedback already shared in the terminal)' - : 'Finish — records that you reviewed with no feedback', + ? 'Finish: sends the session record (feedback already shared in the terminal)' + : 'Finish: records that you reviewed with no feedback', // Maintainer ruling (post-demo): Done without a gate is a positive // finish, NOT an approval — no success tone, no check icon, so it // can never be mistaken for the gate/review Approve. @@ -292,7 +293,7 @@ function buildFeedbackSpec(input: DecisionSpecInput, approvalFlow: boolean): Dec label: approvalFlow ? `Approve, discard ${count} ${noun}…` : `Done, discard ${count} ${noun}…`, - subtitle: 'Asks to confirm — the annotations are not sent', + subtitle: 'Asks to confirm: the annotations are not sent', tone: 'destructive', icon: 'check', dividerBefore: dividerPending, @@ -354,7 +355,7 @@ function buildPlatformSpec(input: DecisionSpecInput, platform: DecisionPlatformI id: 'primary', // Frozen copy (maintainer-approved): 'Approve'. label: 'Approve', - title: selfAuthored ? selfReason : 'Approve - no changes needed', + title: selfAuthored ? selfReason : 'Approve: no changes needed', tone: 'success', icon: 'check', ...(selfAuthored ? { muted: true } : {}), @@ -376,7 +377,7 @@ function buildPlatformSpec(input: DecisionSpecInput, platform: DecisionPlatformI id: 'request-changes', // Frozen copy (maintainer-approved): 'Request changes…'. label: 'Request changes…', - subtitle: 'Overall feedback, zero line comments — via the dialog', + subtitle: 'Overall feedback, zero line comments, via the dialog', tone: 'primary', icon: 'send', dividerBefore: true, @@ -409,7 +410,7 @@ function buildPlatformSpec(input: DecisionSpecInput, platform: DecisionPlatformI { id: 'note-with-feedback', label: 'Post comments, then…', - subtitle: 'Request changes / stay neutral — chosen in the dialog', + subtitle: 'Request changes or stay neutral, chosen in the dialog', tone: 'primary', icon: 'send', dividerBefore: true,