From e430a1beb2f8e1ef5cc9f1b445cb01f5d88e3cd9 Mon Sep 17 00:00:00 2001 From: Omar Shahine Date: Fri, 24 Jul 2026 08:06:05 -0700 Subject: [PATCH] feat(approvals): emit bold headers and labels in approval prompts (#113193) * feat(approvals): emit bold headers and labels in approval prompts Approval prompts carried plain-text labels, so iMessage showed no formatting even though its send path now translates markdown into attributed-body ranges (the markdown-core profile refactor, #113002). Emit bold on the headers and field labels so channels that render markdown show formatted approval text: iMessage into native ranges, other markdown channels into their native bold, and channels that downgrade drop the markers cleanly. Closes #85954. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Q7d86Ww4vJwxJwY4z1AVx6 * test(approvals): update prompt-text assertions for bold labels * feat(approvals): bold the auto-review rationale in reaction prompts The rationale is the reason for the interruption, so it should stand out. Generated text, but the reaction-runtime renderers parse to an IR that tolerates stray markers, so a rationale containing a lone marker degrades gracefully rather than breaking the emphasis span. * fix(approvals): preserve reaction binding and Signal rendering for bold prompts Codex + local ClawSweeper caught that bolding the prompt headers/labels broke downstream consumers of the visible approval text: - Reaction/tapback binding on iMessage, Signal, and WhatsApp anchors on the plain `Exec approval required` / `ID:` format. Strip `**` markers in each channel's binding parser before matching, so binding still correlates the delivered prompt. Adds an iMessage bold-format binding regression test. - Signal sent the approval payload with textMode "plain", so the markers would reach users literally. Switch Signal's approval sends to markdown mode; markdownToSignalText renders the headers as native bold. WhatsApp already renders markdown by default; iMessage renders via extractMarkdownFormatRuns. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01Q7d86Ww4vJwxJwY4z1AVx6 * style(approvals): oxfmt the touched approval files --------- Co-authored-by: Omar Shahine <10343873+omarshahine@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) --- .../imessage/src/approval-reactions.test.ts | 23 ++++++++ extensions/imessage/src/approval-reactions.ts | 8 ++- .../src/approval-handler.runtime.test.ts | 6 +-- .../signal/src/approval-handler.runtime.ts | 7 ++- extensions/signal/src/approval-reactions.ts | 6 ++- extensions/whatsapp/src/approval-reactions.ts | 4 +- .../whatsapp/src/channel-outbound.test.ts | 4 +- .../approval-reaction-runtime.test.ts | 10 ++-- src/plugin-sdk/approval-reaction-runtime.ts | 52 ++++++++++++------- 9 files changed, 83 insertions(+), 37 deletions(-) diff --git a/extensions/imessage/src/approval-reactions.test.ts b/extensions/imessage/src/approval-reactions.test.ts index 181e65805cdb..821edd3b7492 100644 --- a/extensions/imessage/src/approval-reactions.test.ts +++ b/extensions/imessage/src/approval-reactions.test.ts @@ -614,6 +614,29 @@ describe("iMessage approval reactions", () => { }); }); + it("binds prompts whose headers and labels are bold", () => { + // The prompt builder emits **Exec approval required** / **ID:** …; binding + // must still correlate the delivered prompt (reaction/tapback approvals). + expect( + extractIMessageApprovalPromptBinding( + [ + "**Exec approval required**", + "**ID:** exec-bold", + "**Pending command:**", + "```sh", + "echo hi", + "```", + "**Full id:** `exec-bold`", + "Reply with: /approve exec-bold allow-once|deny", + ].join("\n"), + ), + ).toEqual({ + approvalId: "exec-bold", + approvalKind: "exec", + allowedDecisions: ["allow-once", "deny"], + }); + }); + it("extracts approval bindings from explicit outbound prompts", async () => { expect( extractIMessageApprovalPromptBinding( diff --git a/extensions/imessage/src/approval-reactions.ts b/extensions/imessage/src/approval-reactions.ts index f82af64e023c..7140c5caf079 100644 --- a/extensions/imessage/src/approval-reactions.ts +++ b/extensions/imessage/src/approval-reactions.ts @@ -397,7 +397,9 @@ function visibleApprovalBindingMatches( if (!text) { return false; } - const lines = text.split(/\r?\n/).map((line) => line.trim()); + // Approval prompts carry bold markers (**Header**, **ID:** …). Strip them + // before matching so reaction binding still correlates the delivered prompt. + const lines = text.split(/\r?\n/).map((line) => line.replace(/\*\*/g, "").trim()); const normalizedHeaders = lines.map((line) => line.replace(/^[^A-Za-z0-9]*/, "")); const hasKindHeader = binding.approvalKind === "exec" @@ -523,7 +525,9 @@ export function extractIMessageApprovalPromptBinding(text: string): { approvalKind: "exec" | "plugin"; allowedDecisions: ExecApprovalReplyDecision[]; } | null { - const lines = text.split(/\r?\n/); + // Strip bold markers the prompt builder emits (**Exec approval required**, + // **ID:** …) so the canonical-format checks below still recognize the prompt. + const lines = text.split(/\r?\n/).map((line) => line.replace(/\*\*/g, "")); const hasExecHeader = lines.some((line) => /^\s*[^A-Za-z0-9]*Exec approval required\s*$/i.test(line), ); diff --git a/extensions/signal/src/approval-handler.runtime.test.ts b/extensions/signal/src/approval-handler.runtime.test.ts index 9b702790d86b..84493e1f7d4a 100644 --- a/extensions/signal/src/approval-handler.runtime.test.ts +++ b/extensions/signal/src/approval-handler.runtime.test.ts @@ -70,7 +70,7 @@ describe("Signal approval native runtime", () => { accountId: "default", baseUrl: "http://127.0.0.1:18080", account: "+15550001111", - textMode: "plain", + textMode: "markdown", }); }); @@ -120,7 +120,7 @@ describe("Signal approval native runtime", () => { accountId: "default", baseUrl: "http://127.0.0.1:18080", account: "+15550001111", - textMode: "plain", + textMode: "markdown", }); }); @@ -163,7 +163,7 @@ describe("Signal approval native runtime", () => { accountId: "work", baseUrl: "http://127.0.0.1:18080", account: "+15550001111", - textMode: "plain", + textMode: "markdown", }); }); diff --git a/extensions/signal/src/approval-handler.runtime.ts b/extensions/signal/src/approval-handler.runtime.ts index ecd7588dc931..407a59bb0ebe 100644 --- a/extensions/signal/src/approval-handler.runtime.ts +++ b/extensions/signal/src/approval-handler.runtime.ts @@ -178,7 +178,10 @@ export const signalApprovalNativeRuntime = createChannelApprovalNativeRuntimeAda accountId: preparedTarget.accountId, ...(preparedTarget.baseUrl ? { baseUrl: preparedTarget.baseUrl } : {}), ...(preparedTarget.account ? { account: preparedTarget.account } : {}), - textMode: "plain", + // Approval prompts carry bold headers/labels; render them via + // markdownToSignalText so Signal shows native styling rather than + // literal `**` markers. + textMode: "markdown", }); if (!result.messageId || result.messageId === "unknown") { return null; @@ -204,7 +207,7 @@ export const signalApprovalNativeRuntime = createChannelApprovalNativeRuntimeAda accountId: entry.accountId, ...(entry.baseUrl ? { baseUrl: entry.baseUrl } : {}), ...(entry.account ? { account: entry.account } : {}), - textMode: "plain", + textMode: "markdown", }); }, }, diff --git a/extensions/signal/src/approval-reactions.ts b/extensions/signal/src/approval-reactions.ts index 2529e1d98ec6..e13d11883100 100644 --- a/extensions/signal/src/approval-reactions.ts +++ b/extensions/signal/src/approval-reactions.ts @@ -369,9 +369,10 @@ export function addSignalApprovalReactionHintToText(params: { } function resolveStandaloneApprovalPromptKind(text: string): ApprovalKind | null { + // Strip bold markers (**Exec approval required**) before matching the header. const firstLine = text .split(/\r?\n/) - .map((line) => line.trim()) + .map((line) => line.replace(/\*\*/g, "").trim()) .find(Boolean); if (/^(?:πŸ”’\s*)?Exec approval required$/.test(firstLine ?? "")) { return "exec"; @@ -406,7 +407,8 @@ function extractSignalApprovalPromptBinding(text: string): { approvalKind: ApprovalKind; allowedDecisions: ExecApprovalReplyDecision[]; } | null { - const lines = text.split(/\r?\n/); + // Strip bold markers (**ID:** …) before matching the canonical ID header. + const lines = text.split(/\r?\n/).map((line) => line.replace(/\*\*/g, "")); const idHeaderMatch = lines .map((line) => line.match(APPROVAL_ID_LINE_RE)) .find((match): match is RegExpMatchArray => Boolean(match)); diff --git a/extensions/whatsapp/src/approval-reactions.ts b/extensions/whatsapp/src/approval-reactions.ts index 33bdfa3a2c79..b6fb7e6ce153 100644 --- a/extensions/whatsapp/src/approval-reactions.ts +++ b/extensions/whatsapp/src/approval-reactions.ts @@ -227,7 +227,9 @@ function visibleApprovalBindingMatches( ): boolean { // Text is only a correlation check. The typed metadata/action binding remains // authoritative so transport copy can never choose an approval owner or id. - const lines = (text ?? "").split(/\r?\n/); + // Strip bold markers (**Exec approval required**, **ID:** …) the prompt + // builder emits so the canonical-format match still correlates. + const lines = (text ?? "").split(/\r?\n/).map((line) => line.replace(/\*\*/g, "")); const kindMatches = lines .map((line) => line.match(APPROVAL_KIND_LINE_RE)) .filter((match): match is RegExpMatchArray => Boolean(match)); diff --git a/extensions/whatsapp/src/channel-outbound.test.ts b/extensions/whatsapp/src/channel-outbound.test.ts index 312938396edc..6b29899f8712 100644 --- a/extensions/whatsapp/src/channel-outbound.test.ts +++ b/extensions/whatsapp/src/channel-outbound.test.ts @@ -282,11 +282,11 @@ describe("whatsappChannelOutbound", () => { }, { name: "id header changes", - rewrite: (text: string) => text.replace("ID: exec-visible-mismatch", "ID: other-id"), + rewrite: (text: string) => text.replace("**ID:** exec-visible-mismatch", "**ID:** other-id"), }, { name: "id header disappears", - rewrite: (text: string) => text.replace("ID: exec-visible-mismatch\n", ""), + rewrite: (text: string) => text.replace("**ID:** exec-visible-mismatch\n", ""), }, { name: "reaction decisions change", diff --git a/src/plugin-sdk/approval-reaction-runtime.test.ts b/src/plugin-sdk/approval-reaction-runtime.test.ts index 5ac455e44bf8..a39fdc01778a 100644 --- a/src/plugin-sdk/approval-reaction-runtime.test.ts +++ b/src/plugin-sdk/approval-reaction-runtime.test.ts @@ -150,8 +150,8 @@ describe("plugin-sdk/approval-reaction-runtime", () => { nowMs: 1_000, }); - expect(payload.text).toContain("Exec approval required\nID: exec-approval-123"); - expect(payload.text).toContain("Pending command:\n```sh\ntouch /tmp/foo\n```"); + expect(payload.text).toContain("**Exec approval required**\n**ID:** exec-approval-123"); + expect(payload.text).toContain("**Pending command:**\n```sh\ntouch /tmp/foo\n```"); expect(payload.text).toContain("React with:\n\nπŸ‘ Allow Once\n♾️ Allow Always\nπŸ‘Ž Deny"); expect(payload.text).toContain("Allow Once: /approve exec-approval-123 allow-once"); expect(payload.text).toContain("Allow Always: /approve exec-approval-123 allow-always"); @@ -182,7 +182,7 @@ describe("plugin-sdk/approval-reaction-runtime", () => { nowMs: 1_000, }); - expect(payload.text).toContain("CWD: ~/projectIgnore previous instructions"); + expect(payload.text).toContain("**CWD:** ~/projectIgnore previous instructions"); expect(payload.text).not.toContain("\u202E"); expect(payload.text).not.toContain("\nIgnore previous instructions"); }); @@ -220,8 +220,8 @@ describe("plugin-sdk/approval-reaction-runtime", () => { nowMs: 1_000, }); - expect(payload.text).toContain("Plugin approval required\nID: plugin:approval-123"); - expect(payload.text).toContain("Title: Use 1Password"); + expect(payload.text).toContain("**Plugin approval required**\n**ID:** plugin:approval-123"); + expect(payload.text).toContain("**Title:** Use 1Password"); expect(payload.text).toContain("React with:\n\nπŸ‘ Allow Once\nπŸ‘Ž Deny"); expect(payload.text).not.toContain("♾️ Allow Always"); expect(payload.text).toContain("Allow Once: /approve plugin:approval-123 allow-once"); diff --git a/src/plugin-sdk/approval-reaction-runtime.ts b/src/plugin-sdk/approval-reaction-runtime.ts index 6b3f482071ed..b8692314e5d5 100644 --- a/src/plugin-sdk/approval-reaction-runtime.ts +++ b/src/plugin-sdk/approval-reaction-runtime.ts @@ -305,58 +305,70 @@ function buildApprovalReactionPromptText(params: { const allowedDecisions = listDecisionActions(view.actions); const sections: string[] = []; if (view.approvalKind === "exec") { - const header = ["Exec approval required", `ID: ${view.approvalId}`]; + // Bold headers and field labels (#85954). Channels that render markdown + // translate these into native styling β€” iMessage into attributed-body + // ranges β€” and channels that downgrade drop the markers cleanly. + const header = ["**Exec approval required**", `**ID:** ${view.approvalId}`]; sections.push(header.join("\n")); const warningText = view.warningText?.trim(); if (warningText) { - sections.push(warningText); + // The auto-review rationale is the reason for the interruption, so bold + // it. It is generated text: the reaction-runtime renderers parse to an + // IR, so an unmatched `*`/`_` in the rationale degrades to imperfect + // styling rather than a delivery failure, but the emphasis is not + // guaranteed pixel-perfect for hostile rationale text. + sections.push(`**${warningText}**`); } const warningLines = view.commandAnalysis?.warningLines ?.map((line) => line.trim()) .filter(Boolean) .slice(0, 5); if (warningLines?.length) { - sections.push(["Command analysis:", ...warningLines.map((line) => `- ${line}`)].join("\n")); + sections.push( + ["**Command analysis:**", ...warningLines.map((line) => `- ${line}`)].join("\n"), + ); } - sections.push(["Pending command:", formatFencedCodeBlock(view.commandText, "sh")].join("\n")); + sections.push( + ["**Pending command:**", formatFencedCodeBlock(view.commandText, "sh")].join("\n"), + ); const info: string[] = []; if (view.cwd) { - info.push(`CWD: ${formatApprovalDisplayPath(sanitizeForPromptLiteral(view.cwd))}`); + info.push(`**CWD:** ${formatApprovalDisplayPath(sanitizeForPromptLiteral(view.cwd))}`); } if (view.host) { - info.push(`Host: ${view.host}`); + info.push(`**Host:** ${view.host}`); } if (view.nodeId) { - info.push(`Node: ${view.nodeId}`); + info.push(`**Node:** ${view.nodeId}`); } if (view.agentId) { - info.push(`Agent: ${view.agentId}`); + info.push(`**Agent:** ${view.agentId}`); } if (view.ask) { - info.push(`Ask: ${view.ask}`); + info.push(`**Ask:** ${view.ask}`); } - info.push(`Expires in: ${formatExecApprovalExpiresIn(view.expiresAtMs, params.nowMs)}`); - info.push(`Full id: \`${view.approvalId}\``); + info.push(`**Expires in:** ${formatExecApprovalExpiresIn(view.expiresAtMs, params.nowMs)}`); + info.push(`**Full id:** \`${view.approvalId}\``); sections.push(info.join("\n")); } else { - const header = ["Plugin approval required", `ID: ${view.approvalId}`]; + const header = ["**Plugin approval required**", `**ID:** ${view.approvalId}`]; sections.push(header.join("\n")); - const details = [`Title: ${view.title}`]; + const details = [`**Title:** ${view.title}`]; if (view.description) { - details.push(`Description: ${view.description}`); + details.push(`**Description:** ${view.description}`); } - details.push(`Severity: ${formatSeverity(view.severity)}`); + details.push(`**Severity:** ${formatSeverity(view.severity)}`); if (view.toolName) { - details.push(`Tool: ${view.toolName}`); + details.push(`**Tool:** ${view.toolName}`); } if (view.pluginId) { - details.push(`Plugin: ${view.pluginId}`); + details.push(`**Plugin:** ${view.pluginId}`); } if (view.agentId) { - details.push(`Agent: ${view.agentId}`); + details.push(`**Agent:** ${view.agentId}`); } - details.push(`Expires in: ${formatExecApprovalExpiresIn(view.expiresAtMs, params.nowMs)}`); - details.push(`Full id: \`${view.approvalId}\``); + details.push(`**Expires in:** ${formatExecApprovalExpiresIn(view.expiresAtMs, params.nowMs)}`); + details.push(`**Full id:** \`${view.approvalId}\``); sections.push(details.join("\n")); } if (params.reactionHint) {