From 525d363ed0486c6ddb1e65c73e8dc16ed1e3eb3c Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 20 Apr 2026 14:59:57 -0400 Subject: [PATCH 1/3] fix(addie): guard against raw JSON envelopes leaking into Slack responses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addie occasionally posted bare JSON (e.g. a JSON.stringify'd tool result) to Slack channels when Claude echoed a tool response verbatim instead of interpreting it. Root cause: a broad "CRITICAL - TOOL OUTPUT VISIBILITY" rule in behaviors.md — intended for the markdown-formatted draft_github_issue output — was being over-applied to tools that return JSON.stringify payloads. Two-layer fix: 1. Rendering guard: new guardBareJsonEnvelope() in security.ts detects outbound responses that are a bare JSON object/array and wraps them in a ```json fenced code block, logging a warning. Wired into all five Slack send paths in bolt-app.ts (streaming fallback, non-streaming DM, app_mention, active-thread reply, proposed channel reply, reaction response). 2. Prompt rule tightening: the "copy tool output verbatim" instruction is now scoped strictly to draft_github_issue. A new explicit rule forbids echoing raw JSON from other tools. Includes 10 unit tests covering: bare object/array wrapping, whitespace handling, normal prose passthrough, embedded JSON passthrough, already-fenced JSON passthrough, malformed JSON passthrough, and nested tool-result envelopes. Closes #2566. Co-Authored-By: Claude Opus 4.7 (1M context) --- .changeset/addie-slack-json-envelope-guard.md | 4 + server/src/addie/bolt-app.ts | 22 +++-- server/src/addie/rules/behaviors.md | 6 +- server/src/addie/security.ts | 41 +++++++++ tests/addie/slack-json-envelope-guard.test.ts | 86 +++++++++++++++++++ 5 files changed, 151 insertions(+), 8 deletions(-) create mode 100644 .changeset/addie-slack-json-envelope-guard.md create mode 100644 tests/addie/slack-json-envelope-guard.test.ts diff --git a/.changeset/addie-slack-json-envelope-guard.md b/.changeset/addie-slack-json-envelope-guard.md new file mode 100644 index 0000000000..7e376e79c8 --- /dev/null +++ b/.changeset/addie-slack-json-envelope-guard.md @@ -0,0 +1,4 @@ +--- +--- + +Guard against raw JSON envelopes reaching Slack message bodies. A new `guardBareJsonEnvelope` helper wraps any outbound response that is a bare JSON object or array (from a tool result Claude echoed verbatim) in a ```json fenced code block, and logs a warning so we can observe occurrences. Applied across all five Slack send paths in `bolt-app.ts`. Prompt rules tightened: the "copy tool output" instruction is now scoped strictly to `draft_github_issue`, and a new explicit rule forbids echoing raw JSON from other tools. diff --git a/server/src/addie/bolt-app.ts b/server/src/addie/bolt-app.ts index 3bdf44edfe..63717a6797 100644 --- a/server/src/addie/bolt-app.ts +++ b/server/src/addie/bolt-app.ts @@ -84,6 +84,7 @@ import { validateOutput, wrapUrlsForSlack, extractMarkdownImages, + guardBareJsonEnvelope, logInteraction, } from './security.js'; import type { RequestTools } from './claude-client.js'; @@ -1624,7 +1625,8 @@ async function handleUserMessage({ // Fallback: send via say() so the user isn't left without a response try { const fallbackValidation = validateOutput(fullText); - const { text: fallbackText, images: fallbackImages } = extractMarkdownImages(fallbackValidation.sanitized); + const guarded = guardBareJsonEnvelope(fallbackValidation.sanitized); + const { text: fallbackText, images: fallbackImages } = extractMarkdownImages(guarded.text); const slackText = wrapUrlsForSlack(fallbackText); await say({ text: slackText, @@ -1650,7 +1652,8 @@ async function handleUserMessage({ // Send response via say() with feedback buttons and inline images const outputValidation = validateOutput(response.text); - const { text: textWithoutImages, images } = extractMarkdownImages(outputValidation.sanitized); + const guarded = guardBareJsonEnvelope(outputValidation.sanitized); + const { text: textWithoutImages, images } = extractMarkdownImages(guarded.text); const slackText = wrapUrlsForSlack(textWithoutImages); try { await say({ @@ -2108,11 +2111,12 @@ async function handleAppMention({ // Validate output const outputValidation = validateOutput(response.text); + const mentionGuarded = guardBareJsonEnvelope(outputValidation.sanitized); // Send response in thread (must explicitly pass thread_ts for app_mention events) try { await say({ - text: wrapUrlsForSlack(outputValidation.sanitized), + text: wrapUrlsForSlack(mentionGuarded.text), thread_ts: threadTs, }); } catch (error) { @@ -3065,6 +3069,7 @@ async function handleDirectMessage( // Validate output const outputValidation = validateOutput(response.text); + const dmGuarded = guardBareJsonEnvelope(outputValidation.sanitized); // Always thread the response to the user's message. This ensures: // 1. Slack Assistant "Chat" tab: response appears inline in the conversation @@ -3076,7 +3081,7 @@ async function handleDirectMessage( try { const postResult = await boltApp.client.chat.postMessage({ channel: channelId, - text: wrapUrlsForSlack(outputValidation.sanitized), + text: wrapUrlsForSlack(dmGuarded.text), thread_ts: replyThreadTs, }); responseTs = postResult.ts; @@ -3442,12 +3447,13 @@ async function handleActiveThreadReply({ // Validate output const outputValidation = validateOutput(response.text); + const activeThreadGuarded = guardBareJsonEnvelope(outputValidation.sanitized); // Send response in the thread try { await boltApp.client.chat.postMessage({ channel: channelId, - text: wrapUrlsForSlack(outputValidation.sanitized), + text: wrapUrlsForSlack(activeThreadGuarded.text), thread_ts: threadTs, // Reply in the thread }); } catch (error) { @@ -4048,10 +4054,11 @@ async function handleChannelMessage({ }); // Post the response directly to the channel thread + const proposedGuarded = guardBareJsonEnvelope(outputValidation.sanitized); try { await boltApp?.client.chat.postMessage({ channel: channelId, - text: wrapUrlsForSlack(outputValidation.sanitized), + text: wrapUrlsForSlack(proposedGuarded.text), thread_ts: threadTs, }); logger.info({ channelId, userId }, 'Addie Bolt: Posted response to channel'); @@ -4803,10 +4810,11 @@ async function handleReactionAdded({ } // Send response in thread + const reactionGuarded = guardBareJsonEnvelope(response.text); try { await client.chat.postMessage({ channel: itemChannel, - text: wrapUrlsForSlack(response.text), + text: wrapUrlsForSlack(reactionGuarded.text), thread_ts: threadTs, }); } catch (error) { diff --git a/server/src/addie/rules/behaviors.md b/server/src/addie/rules/behaviors.md index 312c1b5b19..8095650b80 100644 --- a/server/src/addie/rules/behaviors.md +++ b/server/src/addie/rules/behaviors.md @@ -261,7 +261,11 @@ Use draft_github_issue to generate a pre-filled GitHub URL. - The tool name and parameters that caused the error (sanitized of PII) - What the expected behavior was vs what actually happened -**CRITICAL - TOOL OUTPUT VISIBILITY**: Users CANNOT see tool outputs directly. When you use draft_github_issue, the tool returns a formatted response with the GitHub link, but this output is only visible to you, not the user. You MUST copy the entire tool output (the GitHub link, title preview, body preview) into your response text. +**CRITICAL - DRAFT GITHUB ISSUE OUTPUT**: The `draft_github_issue` tool returns a formatted markdown block (a GitHub link, title, and body preview) that is meant to be shown to the user verbatim. Users CANNOT see tool outputs directly, so for this tool specifically you MUST copy the entire markdown response — link, title preview, body preview — into your reply text. + +This rule applies ONLY to `draft_github_issue`. For every other tool, treat the tool output as context for you — summarize it in natural language, do not paste the raw response into the user's message. + +**NEVER echo raw JSON tool output**: many tools return JSON.stringify'd objects. That is structured data for you to interpret. Never paste a bare `{...}` or `[...]` payload into a user response. If a user explicitly asks to see raw data, wrap it in a ```json fenced code block. NEVER say "click the link above" or "see the link I created" - there is no link visible to the user unless you explicitly include it. Always format your response like: diff --git a/server/src/addie/security.ts b/server/src/addie/security.ts index dfb2996b41..af53994eef 100644 --- a/server/src/addie/security.ts +++ b/server/src/addie/security.ts @@ -238,6 +238,47 @@ export function wrapUrlsForSlack(text: string): string { ); } +export interface BareJsonGuardResult { + text: string; + wasWrapped: boolean; +} + +/** + * Wrap a response in a ```json fence if it's a bare JSON envelope (starts with + * `{` or `[` and parses cleanly). Tool results are meant to be interpreted by + * Claude, not echoed verbatim; if one ends up in the Slack message body, this + * keeps it readable and flags it for investigation. + * + * Leaves everything else — normal prose, already-fenced JSON, mixed content — + * untouched. + */ +export function guardBareJsonEnvelope(text: string): BareJsonGuardResult { + const trimmed = text.trim(); + if (trimmed.length < 2) return { text, wasWrapped: false }; + + const first = trimmed[0]; + if (first !== '{' && first !== '[') return { text, wasWrapped: false }; + + // Skip if the response already starts with a code fence that wraps the JSON. + if (/^```/.test(text.trimStart())) return { text, wasWrapped: false }; + + try { + JSON.parse(trimmed); + } catch { + return { text, wasWrapped: false }; + } + + logger.warn( + { length: text.length, preview: trimmed.slice(0, 200) }, + 'Addie: Raw JSON envelope detected in outbound response — wrapping in code fence', + ); + + return { + text: '```json\n' + trimmed + '\n```', + wasWrapped: true, + }; +} + /** * Extract markdown images from text and return them separately. * Used to convert markdown image syntax into Slack Block Kit image blocks, diff --git a/tests/addie/slack-json-envelope-guard.test.ts b/tests/addie/slack-json-envelope-guard.test.ts new file mode 100644 index 0000000000..f52a378bb8 --- /dev/null +++ b/tests/addie/slack-json-envelope-guard.test.ts @@ -0,0 +1,86 @@ +import { describe, test, expect, vi } from 'vitest'; + +vi.mock('../../server/src/logger.js', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + createLogger: vi.fn().mockReturnValue({ + info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), + }), +})); + +import { guardBareJsonEnvelope } from '../../server/src/addie/security.js'; + +describe('guardBareJsonEnvelope', () => { + test('wraps a bare JSON object in a json code fence', () => { + const input = '{"has_portrait": false, "reason": "not generated"}'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(true); + expect(text.startsWith('```json\n')).toBe(true); + expect(text.endsWith('\n```')).toBe(true); + expect(text).toContain('"has_portrait"'); + }); + + test('wraps a bare JSON array in a json code fence', () => { + const input = '[{"id": 1}, {"id": 2}]'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(true); + expect(text.startsWith('```json\n')).toBe(true); + }); + + test('wraps JSON with surrounding whitespace', () => { + const input = '\n\n {"ok": true} \n'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(true); + expect(text).toContain('"ok"'); + }); + + test('leaves normal prose untouched', () => { + const input = 'Here is your answer. The portrait has not been generated yet.'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(false); + expect(text).toBe(input); + }); + + test('leaves markdown with embedded JSON untouched', () => { + const input = 'Your portrait status:\n\n```json\n{"has_portrait": false}\n```'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(false); + expect(text).toBe(input); + }); + + test('leaves responses that start with a code fence untouched', () => { + const input = '```json\n{"foo": "bar"}\n```'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(false); + expect(text).toBe(input); + }); + + test('leaves malformed JSON-looking text untouched', () => { + const input = '{this is not valid json at all'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(false); + expect(text).toBe(input); + }); + + test('leaves empty or single-char input untouched', () => { + expect(guardBareJsonEnvelope('').wasWrapped).toBe(false); + expect(guardBareJsonEnvelope('{').wasWrapped).toBe(false); + }); + + test('leaves prose that starts with a brace but is not JSON untouched', () => { + const input = '{Not JSON} just using a brace stylistically.'; + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(false); + expect(text).toBe(input); + }); + + test('wraps a nested JSON envelope (simulates a stringified tool result)', () => { + const input = JSON.stringify({ + type: 'tool_result', + content: { members: [{ id: 'u1', name: 'Alice' }] }, + }); + const { text, wasWrapped } = guardBareJsonEnvelope(input); + expect(wasWrapped).toBe(true); + expect(text).toContain('"tool_result"'); + expect(text.startsWith('```json\n')).toBe(true); + }); +}); From 53added232f08b4f82460fd920d7a4efdf76311a Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 20 Apr 2026 15:09:28 -0400 Subject: [PATCH 2/3] refactor(addie): address reviewer feedback on JSON envelope guard MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security: - Drop response-content preview from the warning log. The guard fires precisely when the payload is most likely to contain PII or secrets, so a 200-char preview would leak exactly what we're trying to contain. Log shape metadata only (length, firstChar, looksLikeArray). - Remove the "if user asks, wrap in a fenced block" escape hatch in behaviors.md. Tool outputs aren't user-addressable content regardless of the framing of the ask. Rule now reads: never paste raw JSON, never quote it to satisfy a "see the data" request; summarize what the data shows or surface the specific values asked about. Correctness: - Run guard BEFORE validateOutput. validateOutput truncates at 10k chars; an oversized JSON envelope would arrive at the guard as a truncated string that no longer parses — the exact leaked-raw-JSON scenario this guard exists to prevent. Swapping order preserves the guard's parseability check on the original LLM output. Observability: - Add a required pathTag context arg to the guard so the warning log identifies which of the five send paths is leaking. Tests: - Update 10 existing tests to pass the new context arg. - Add a case for "JSON + trailing prose" (the naturally-shaped Claude regression) that documents the guard's known limitation and locks in the expected passthrough behavior. Refs #2566. Co-Authored-By: Claude Opus 4.7 (1M context) --- server/src/addie/bolt-app.ts | 38 +++++++++---------- server/src/addie/rules/behaviors.md | 2 +- server/src/addie/security.ts | 23 +++++++++-- tests/addie/slack-json-envelope-guard.test.ts | 34 +++++++++++------ 4 files changed, 62 insertions(+), 35 deletions(-) diff --git a/server/src/addie/bolt-app.ts b/server/src/addie/bolt-app.ts index 63717a6797..f76a24bbe2 100644 --- a/server/src/addie/bolt-app.ts +++ b/server/src/addie/bolt-app.ts @@ -1624,9 +1624,9 @@ async function handleUserMessage({ logger.warn({ stopError }, 'Addie Bolt: Stream stop failed, falling back to say()'); // Fallback: send via say() so the user isn't left without a response try { - const fallbackValidation = validateOutput(fullText); - const guarded = guardBareJsonEnvelope(fallbackValidation.sanitized); - const { text: fallbackText, images: fallbackImages } = extractMarkdownImages(guarded.text); + const guarded = guardBareJsonEnvelope(fullText, { pathTag: 'dm-streaming-fallback' }); + const fallbackValidation = validateOutput(guarded.text); + const { text: fallbackText, images: fallbackImages } = extractMarkdownImages(fallbackValidation.sanitized); const slackText = wrapUrlsForSlack(fallbackText); await say({ text: slackText, @@ -1651,9 +1651,9 @@ async function handleUserMessage({ fullText = response.text; // Send response via say() with feedback buttons and inline images - const outputValidation = validateOutput(response.text); - const guarded = guardBareJsonEnvelope(outputValidation.sanitized); - const { text: textWithoutImages, images } = extractMarkdownImages(guarded.text); + const guarded = guardBareJsonEnvelope(response.text, { pathTag: 'dm-non-streaming' }); + const outputValidation = validateOutput(guarded.text); + const { text: textWithoutImages, images } = extractMarkdownImages(outputValidation.sanitized); const slackText = wrapUrlsForSlack(textWithoutImages); try { await say({ @@ -2110,13 +2110,13 @@ async function handleAppMention({ } // Validate output - const outputValidation = validateOutput(response.text); - const mentionGuarded = guardBareJsonEnvelope(outputValidation.sanitized); + const mentionGuarded = guardBareJsonEnvelope(response.text, { pathTag: 'app-mention' }); + const outputValidation = validateOutput(mentionGuarded.text); // Send response in thread (must explicitly pass thread_ts for app_mention events) try { await say({ - text: wrapUrlsForSlack(mentionGuarded.text), + text: wrapUrlsForSlack(outputValidation.sanitized), thread_ts: threadTs, }); } catch (error) { @@ -3068,8 +3068,8 @@ async function handleDirectMessage( } // Validate output - const outputValidation = validateOutput(response.text); - const dmGuarded = guardBareJsonEnvelope(outputValidation.sanitized); + const dmGuarded = guardBareJsonEnvelope(response.text, { pathTag: 'dm-assistant' }); + const outputValidation = validateOutput(dmGuarded.text); // Always thread the response to the user's message. This ensures: // 1. Slack Assistant "Chat" tab: response appears inline in the conversation @@ -3081,7 +3081,7 @@ async function handleDirectMessage( try { const postResult = await boltApp.client.chat.postMessage({ channel: channelId, - text: wrapUrlsForSlack(dmGuarded.text), + text: wrapUrlsForSlack(outputValidation.sanitized), thread_ts: replyThreadTs, }); responseTs = postResult.ts; @@ -3446,14 +3446,14 @@ async function handleActiveThreadReply({ } // Validate output - const outputValidation = validateOutput(response.text); - const activeThreadGuarded = guardBareJsonEnvelope(outputValidation.sanitized); + const activeThreadGuarded = guardBareJsonEnvelope(response.text, { pathTag: 'active-thread-reply' }); + const outputValidation = validateOutput(activeThreadGuarded.text); // Send response in the thread try { await boltApp.client.chat.postMessage({ channel: channelId, - text: wrapUrlsForSlack(activeThreadGuarded.text), + text: wrapUrlsForSlack(outputValidation.sanitized), thread_ts: threadTs, // Reply in the thread }); } catch (error) { @@ -4017,7 +4017,8 @@ async function handleChannelMessage({ } // Validate the output - const outputValidation = validateOutput(response.text); + const proposedGuarded = guardBareJsonEnvelope(response.text, { pathTag: 'proposed-channel-response' }); + const outputValidation = validateOutput(proposedGuarded.text); if (outputValidation.flagged) { logger.warn({ channelId, reason: outputValidation.reason }, 'Addie Bolt: Proposed response flagged'); return; @@ -4054,11 +4055,10 @@ async function handleChannelMessage({ }); // Post the response directly to the channel thread - const proposedGuarded = guardBareJsonEnvelope(outputValidation.sanitized); try { await boltApp?.client.chat.postMessage({ channel: channelId, - text: wrapUrlsForSlack(proposedGuarded.text), + text: wrapUrlsForSlack(outputValidation.sanitized), thread_ts: threadTs, }); logger.info({ channelId, userId }, 'Addie Bolt: Posted response to channel'); @@ -4810,7 +4810,7 @@ async function handleReactionAdded({ } // Send response in thread - const reactionGuarded = guardBareJsonEnvelope(response.text); + const reactionGuarded = guardBareJsonEnvelope(response.text, { pathTag: 'reaction-response' }); try { await client.chat.postMessage({ channel: itemChannel, diff --git a/server/src/addie/rules/behaviors.md b/server/src/addie/rules/behaviors.md index 8095650b80..f8d5a320a6 100644 --- a/server/src/addie/rules/behaviors.md +++ b/server/src/addie/rules/behaviors.md @@ -265,7 +265,7 @@ Use draft_github_issue to generate a pre-filled GitHub URL. This rule applies ONLY to `draft_github_issue`. For every other tool, treat the tool output as context for you — summarize it in natural language, do not paste the raw response into the user's message. -**NEVER echo raw JSON tool output**: many tools return JSON.stringify'd objects. That is structured data for you to interpret. Never paste a bare `{...}` or `[...]` payload into a user response. If a user explicitly asks to see raw data, wrap it in a ```json fenced code block. +**NEVER echo raw JSON tool output**: many tools return JSON.stringify'd objects. That is structured data for you to interpret, not content for the user. Never paste a bare `{...}` or `[...]` payload into a user response, and never quote tool output in a code block or transcript to satisfy a request to "see the raw data" — tool results are not user-addressable content. Summarize what the data shows in natural language; if the user needs something the summary can't capture, surface the specific values they asked about instead of dumping the envelope. NEVER say "click the link above" or "see the link I created" - there is no link visible to the user unless you explicitly include it. Always format your response like: diff --git a/server/src/addie/security.ts b/server/src/addie/security.ts index af53994eef..d9b8d74978 100644 --- a/server/src/addie/security.ts +++ b/server/src/addie/security.ts @@ -249,10 +249,20 @@ export interface BareJsonGuardResult { * Claude, not echoed verbatim; if one ends up in the Slack message body, this * keeps it readable and flags it for investigation. * - * Leaves everything else — normal prose, already-fenced JSON, mixed content — - * untouched. + * Known limitation: this catches the common "bare JSON at the top of the + * message" case. A response that starts with a short prose prefix before the + * JSON (e.g. "Here is the result: {...}") will not be wrapped. The prompt + * rule in behaviors.md is the primary control; this guard is a safety net. + * + * The log line intentionally does NOT include the response content, because + * the cases that trigger this wrap are exactly the cases where the payload + * is most likely to contain PII, Stripe data, or other secrets pulled from a + * tool result. */ -export function guardBareJsonEnvelope(text: string): BareJsonGuardResult { +export function guardBareJsonEnvelope( + text: string, + context: { pathTag: string }, +): BareJsonGuardResult { const trimmed = text.trim(); if (trimmed.length < 2) return { text, wasWrapped: false }; @@ -269,7 +279,12 @@ export function guardBareJsonEnvelope(text: string): BareJsonGuardResult { } logger.warn( - { length: text.length, preview: trimmed.slice(0, 200) }, + { + pathTag: context.pathTag, + length: text.length, + firstChar: first, + looksLikeArray: first === '[', + }, 'Addie: Raw JSON envelope detected in outbound response — wrapping in code fence', ); diff --git a/tests/addie/slack-json-envelope-guard.test.ts b/tests/addie/slack-json-envelope-guard.test.ts index f52a378bb8..37a8c0fc5e 100644 --- a/tests/addie/slack-json-envelope-guard.test.ts +++ b/tests/addie/slack-json-envelope-guard.test.ts @@ -9,10 +9,12 @@ vi.mock('../../server/src/logger.js', () => ({ import { guardBareJsonEnvelope } from '../../server/src/addie/security.js'; +const ctx = { pathTag: 'test' }; + describe('guardBareJsonEnvelope', () => { test('wraps a bare JSON object in a json code fence', () => { const input = '{"has_portrait": false, "reason": "not generated"}'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(true); expect(text.startsWith('```json\n')).toBe(true); expect(text.endsWith('\n```')).toBe(true); @@ -21,54 +23,54 @@ describe('guardBareJsonEnvelope', () => { test('wraps a bare JSON array in a json code fence', () => { const input = '[{"id": 1}, {"id": 2}]'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(true); expect(text.startsWith('```json\n')).toBe(true); }); test('wraps JSON with surrounding whitespace', () => { const input = '\n\n {"ok": true} \n'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(true); expect(text).toContain('"ok"'); }); test('leaves normal prose untouched', () => { const input = 'Here is your answer. The portrait has not been generated yet.'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(false); expect(text).toBe(input); }); test('leaves markdown with embedded JSON untouched', () => { const input = 'Your portrait status:\n\n```json\n{"has_portrait": false}\n```'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(false); expect(text).toBe(input); }); test('leaves responses that start with a code fence untouched', () => { const input = '```json\n{"foo": "bar"}\n```'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(false); expect(text).toBe(input); }); test('leaves malformed JSON-looking text untouched', () => { const input = '{this is not valid json at all'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(false); expect(text).toBe(input); }); test('leaves empty or single-char input untouched', () => { - expect(guardBareJsonEnvelope('').wasWrapped).toBe(false); - expect(guardBareJsonEnvelope('{').wasWrapped).toBe(false); + expect(guardBareJsonEnvelope('', ctx).wasWrapped).toBe(false); + expect(guardBareJsonEnvelope('{', ctx).wasWrapped).toBe(false); }); test('leaves prose that starts with a brace but is not JSON untouched', () => { const input = '{Not JSON} just using a brace stylistically.'; - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(false); expect(text).toBe(input); }); @@ -78,9 +80,19 @@ describe('guardBareJsonEnvelope', () => { type: 'tool_result', content: { members: [{ id: 'u1', name: 'Alice' }] }, }); - const { text, wasWrapped } = guardBareJsonEnvelope(input); + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); expect(wasWrapped).toBe(true); expect(text).toContain('"tool_result"'); expect(text.startsWith('```json\n')).toBe(true); }); + + test('leaves JSON followed by trailing prose untouched (trailing content breaks parse)', () => { + // The natural "Claude pasted tool output then added commentary" shape. + // JSON.parse rejects trailing chars, so this falls through to the prompt rule. + // Documented limitation — the guard is a safety net, not a complete fix. + const input = '{"ok": true}\n\nAnd here is a quick summary of what it shows...'; + const { text, wasWrapped } = guardBareJsonEnvelope(input, ctx); + expect(wasWrapped).toBe(false); + expect(text).toBe(input); + }); }); From e261c9115bbad2e28cd559c272c4d825f2e94bfb Mon Sep 17 00:00:00 2001 From: Brian O'Kelley Date: Mon, 20 Apr 2026 15:18:32 -0400 Subject: [PATCH 3/3] fix(addie): tolerate multi-envelope + reasoning output in composeMessage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ACTUAL root cause of issue #2566, traced via prod admin API. Sample from production thread a88ea720 (Apr 18): {"text": "Hey Luk — you joined a working group..."} Wait, let me reconsider — that violates rule #3 (open-ended question) and rule on unreplied (2 unreplied = lighter touch...) {"text": "The agentic ads protocol docs just got a solid update..."} Claude Sonnet was returning a JSON envelope, then a reflection paragraph, then a revised envelope — and JSON.parse rejects trailing content after the first `}`. The fallback path (extractUserFacingMessage) didn't strip bare-JSON paragraphs either, so the entire raw model output — envelope + reasoning + revised envelope — landed in the user's Slack DM verbatim. Two fixes: - parseEnvelope() scans for all top-level {...} objects using a string/escape-aware bracket counter, parses each, and returns the LAST valid one. Claude's final decision after any reconsideration wins. This replaces the strict JSON.parse in composeMessage. - extractUserFacingMessage() now drops paragraphs that parse as JSON objects on their own, as a safety net for the fallback path. 13 regression tests, including the exact production repro shape. The Slack-path guard landed earlier in this PR is still useful defense-in-depth against any Claude-echoing-JSON case in reply paths, but the primary bug was here in the outreach composer. Refs #2566. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/addie/services/engagement-planner.ts | 85 +++++++++++++-- .../addie/engagement-planner-envelope.test.ts | 100 ++++++++++++++++++ 2 files changed, 178 insertions(+), 7 deletions(-) create mode 100644 tests/addie/engagement-planner-envelope.test.ts diff --git a/server/src/addie/services/engagement-planner.ts b/server/src/addie/services/engagement-planner.ts index c6cfd5f1cf..dc08ac7410 100644 --- a/server/src/addie/services/engagement-planner.ts +++ b/server/src/addie/services/engagement-planner.ts @@ -413,10 +413,13 @@ export async function composeMessage( text = stripCodeFences(text); - // Try to parse as JSON first for both Slack and email. - try { - const parsed = JSON.parse(text); + // Claude sometimes returns a JSON envelope followed by reflection text + // ("Wait, let me reconsider…") and then another envelope. JSON.parse rejects + // trailing content, so prefer the last well-formed top-level object — + // Claude's final decision after any reconsideration wins. + const parsed = parseEnvelope(text); + if (parsed) { if (parsed.skip) { logger.info( { person_id: ctx.relationship.id, reason: parsed.reason }, @@ -437,7 +440,7 @@ export async function composeMessage( }; } - if (parsed.subject && parsed.body) { + if (parsed.subject && parsed.body && typeof parsed.subject === 'string' && typeof parsed.body === 'string') { const cleanedBody = extractUserFacingMessage(parsed.body, 'email'); if (!cleanedBody) { logger.warn({ person_id: ctx.relationship.id }, 'Model returned non-user-facing email body'); @@ -450,8 +453,6 @@ export async function composeMessage( goalHint: inferGoalHint(cleanedBody, ctx.engagementOpportunities), }; } - } catch { - // Not JSON — salvage the user-facing message text if possible. } const cleanedText = extractUserFacingMessage(text, channel); @@ -481,6 +482,73 @@ function stripCodeFences(text: string): string { return codeFenceMatch ? codeFenceMatch[1].trim() : text; } +/** + * Find all top-level `{...}` objects in the text and return the LAST one that + * parses as JSON. Covers the case where Claude emits an envelope, then a + * reflection paragraph, then a revised envelope — the revision is the answer. + * + * Uses a string/escape-aware bracket counter so quoted braces don't throw off + * the depth count. + */ +export function parseEnvelope(text: string): Record | null { + const envelopes: Record[] = []; + let cursor = 0; + + while (cursor < text.length) { + const openIdx = text.indexOf('{', cursor); + if (openIdx === -1) break; + + let depth = 0; + let inString = false; + let escape = false; + let closeIdx = -1; + + for (let i = openIdx; i < text.length; i++) { + const c = text[i]; + if (escape) { escape = false; continue; } + if (c === '\\') { escape = true; continue; } + if (c === '"') { inString = !inString; continue; } + if (inString) continue; + if (c === '{') depth++; + else if (c === '}') { + depth--; + if (depth === 0) { closeIdx = i; break; } + } + } + + if (closeIdx === -1) break; + + try { + const candidate = JSON.parse(text.slice(openIdx, closeIdx + 1)); + if (candidate && typeof candidate === 'object' && !Array.isArray(candidate)) { + envelopes.push(candidate as Record); + } + } catch { + // Not valid JSON at this boundary; keep scanning. + } + + cursor = closeIdx + 1; + } + + return envelopes.length > 0 ? envelopes[envelopes.length - 1] : null; +} + +/** + * A paragraph that parses as a JSON object on its own — these are envelope + * fragments that leaked past `parseEnvelope` (e.g. missing/malformed closing + * brace would leave them intact). Defense in depth for the fallback path. + */ +function looksLikeJsonEnvelopeParagraph(paragraph: string): boolean { + const t = paragraph.trim(); + if (!t.startsWith('{') || !t.endsWith('}')) return false; + try { + const p = JSON.parse(t); + return p && typeof p === 'object' && !Array.isArray(p); + } catch { + return false; + } +} + const REASONING_PREFIX_PATTERNS = [ /^thinking about this one/i, /^thinking through this one/i, @@ -570,7 +638,10 @@ export function extractUserFacingMessage(rawText: string, channel: 'slack' | 'em const paragraphs = text .split(/\n\s*\n/) .map(paragraph => paragraph.trim()) - .filter(Boolean); + .filter(Boolean) + // Drop bare JSON envelope paragraphs — those are model scratch output that + // should never appear in a user-facing Slack or email message. + .filter(paragraph => !looksLikeJsonEnvelopeParagraph(paragraph)); let firstUserFacingIndex = 0; while (firstUserFacingIndex < paragraphs.length && looksLikeReasoningParagraph(paragraphs[firstUserFacingIndex])) { diff --git a/tests/addie/engagement-planner-envelope.test.ts b/tests/addie/engagement-planner-envelope.test.ts new file mode 100644 index 0000000000..a5c9186b0c --- /dev/null +++ b/tests/addie/engagement-planner-envelope.test.ts @@ -0,0 +1,100 @@ +import { describe, test, expect, vi } from 'vitest'; + +vi.mock('../../server/src/logger.js', () => ({ + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + createLogger: vi.fn().mockReturnValue({ + info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn(), + }), +})); + +import { + parseEnvelope, + extractUserFacingMessage, +} from '../../server/src/addie/services/engagement-planner.js'; + +describe('parseEnvelope', () => { + test('parses a clean JSON envelope', () => { + const out = parseEnvelope('{"text": "Hello"}'); + expect(out).toEqual({ text: 'Hello' }); + }); + + test('parses a JSON envelope with surrounding whitespace', () => { + const out = parseEnvelope('\n\n {"text": "Hello"} \n'); + expect(out).toEqual({ text: 'Hello' }); + }); + + test('picks the LAST valid envelope when model reconsiders', () => { + // Exact shape from production thread a88ea720 — Claude returned a draft, + // then a reflection paragraph, then a revised draft. The revised one + // (final decision after reconsideration) should win. + const raw = + '{"text": "Hey Luk — you joined a working group but I don\'t see any activity yet. Which one did you land in?"}\n\n' + + 'Wait, let me reconsider — that violates rule #3 (open-ended question) and rule on unreplied (2 unreplied = lighter touch, pure value, no asks).\n\n' + + '{"text": "The agentic ads protocol docs just got a solid update — good reference if you\'re building in this space. Want me to drop the link?"}'; + + const out = parseEnvelope(raw); + expect(out).toBeDefined(); + expect(out?.text).toContain('agentic ads protocol docs'); + expect(out?.text).not.toContain('working group'); + }); + + test('extracts JSON preceded by short reasoning prefix', () => { + const raw = 'Thinking about this one — lighter touch seems right.\n\n{"text": "Hope all is well!"}'; + const out = parseEnvelope(raw); + expect(out).toEqual({ text: 'Hope all is well!' }); + }); + + test('parses email envelopes with nested quoted braces', () => { + const out = parseEnvelope('{"subject": "Update", "body": "Your code: `if (x) {return}` — done."}'); + expect(out?.subject).toBe('Update'); + expect(out?.body).toContain('if (x)'); + }); + + test('returns null on plain prose with no JSON object', () => { + expect(parseEnvelope('Just a regular message with no JSON.')).toBeNull(); + }); + + test('returns null on malformed JSON with no valid top-level object', () => { + expect(parseEnvelope('{text: broken')).toBeNull(); + }); + + test('handles escaped quotes inside strings without breaking bracket count', () => { + const out = parseEnvelope('{"text": "She said \\"hi\\" — so I replied."}'); + expect(out?.text).toBe('She said "hi" — so I replied.'); + }); + + test('handles braces inside strings without breaking bracket count', () => { + const out = parseEnvelope('{"text": "Use {placeholder} syntax for templates."}'); + expect(out?.text).toBe('Use {placeholder} syntax for templates.'); + }); + + test('passes over a {"skip": ...} control payload verbatim', () => { + const out = parseEnvelope('{"skip": true, "reason": "nothing meaningful to say"}'); + expect(out).toEqual({ skip: true, reason: 'nothing meaningful to say' }); + }); +}); + +describe('extractUserFacingMessage — JSON envelope stripping', () => { + test('strips a bare JSON envelope paragraph that slipped through', () => { + // If a JSON envelope fragment reaches the fallback path, it should be + // dropped rather than sent to the user. + const raw = '{"text": "Hey there!"}\n\nAnd some follow-up prose that actually belongs in the message.'; + const out = extractUserFacingMessage(raw, 'slack'); + expect(out).toBe('And some follow-up prose that actually belongs in the message.'); + }); + + test('leaves prose that merely starts with a brace untouched', () => { + const raw = '{Brace used stylistically} — this is a real user-facing message.'; + const out = extractUserFacingMessage(raw, 'slack'); + expect(out).toBe(raw); + }); + + test('returns null when only JSON envelopes and reasoning remain', () => { + const raw = + '{"text": "Draft one"}\n\n' + + 'Thinking about this one — lighter touch seems right.\n\n' + + '{"text": "Draft two"}'; + const out = extractUserFacingMessage(raw, 'slack'); + expect(out).toBeNull(); + }); +});