From c2cfd55e97e587ea6e622c3c66fde57b7c404844 Mon Sep 17 00:00:00 2001 From: nearfolk Date: Thu, 20 Aug 2026 17:48:34 -0500 Subject: [PATCH] fix: exclude thought text from automatic issue comments (#11801) ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - The heartbeat system records agent runs and can add a run summary to an issue. > - The ACPX engine receives output text and internal thought text as separate streams. > - The default summary strategy joined both streams and could publish internal text in an issue comment. > - Paperclip already has final-output segmentation for run summaries. > - This pull request makes final-output-only summaries mandatory and removes the configuration bypass. > - The benefit is that automatic issue comments contain the intended final message instead of internal execution text. ## Linked Issues or Issue Description Refs #11761 **What happened?** The ACPX engine used the full summary strategy when an adapter did not set `summaryStrategy`. That strategy joined all text deltas, including thought-stream text and intermediate narration. The heartbeat finalizer could then store that summary as an issue comment. **Expected behavior** An automatic issue comment must use only the final output segment. Configuration must not allow thought-stream text or intermediate narration into that summary. **Steps to reproduce** 1. Run an ACPX adapter without a configured `summaryStrategy`. 2. Emit an output delta, a thought delta, a tool call, and a final output delta. 3. Read the generated run summary. 4. Observe that the old default included all text deltas. **Paperclip version or commit** `54b8bec44417511c623999613f9f1006f8af0517` **Deployment mode** Built from source with a local ACPX adapter. ## What Changed - Limit ACPX run summaries to the final non-empty output segment. - Ignore the legacy full-summary setting so configuration cannot bypass containment. - Update regression tests for the safe default and an attempted unsafe override. ## Verification - Observed the new guard fail before the implementation change because the summary contained thought text. - Ran `pnpm exec vitest run packages/adapter-utils/src/acpx-engine/execute.test.ts -t "defaults run summaries to the final output segment without thought text|does not allow configuration to include thought text in run summaries"`. Result: 2 passed. - Ran `pnpm exec vitest run packages/adapter-utils/src/acpx-engine/execute.test.ts`. Result: 130 passed. - Ran `pnpm --filter @paperclipai/adapter-utils typecheck`. Result: passed. ## Risks - Run summaries are shorter for adapters that relied on full text aggregation. - The old `summaryStrategy: "full"` setting no longer changes summary behavior. This is an intentional containment change. - The change does not alter run logs or tool events. It changes only the summary selected for downstream use. > This is a focused security and privacy bug fix. It does not add roadmap scope. ## Model Used - OpenAI Codex on the GPT-5 family. The runtime did not expose the exact model ID or context-window size. Reasoning, tool use, terminal execution, and code editing were enabled. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [x] My branch name describes the change and contains no internal task id - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [x] I have updated relevant inline documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge --- .../src/acpx-engine/execute.test.ts | 80 ++++++++++++++++--- .../adapter-utils/src/acpx-engine/execute.ts | 48 ++++------- .../__tests__/heartbeat-run-summary.test.ts | 12 +++ 3 files changed, 97 insertions(+), 43 deletions(-) diff --git a/packages/adapter-utils/src/acpx-engine/execute.test.ts b/packages/adapter-utils/src/acpx-engine/execute.test.ts index f661803383..324e20cf43 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.test.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.test.ts @@ -628,7 +628,7 @@ describe("shared ACPX engine runtime behavior", () => { }); }); - it("pins the existing summary and tool-event behavior when no engine knobs are set", async () => { + it("defaults run summaries to the final output segment without thought text", async () => { const root = await makeTempRoot(); const stateDir = path.join(root, "state"); const logs: Array<{ stream: string; text: string }> = []; @@ -705,13 +705,9 @@ describe("shared ACPX engine runtime behavior", () => { } as never); expect(result.exitCode).toBe(0); - // The summary is the full concatenation of every text delta, thought - // stream included — the engine's long-standing behavior for claude, - // codex, gemini, and custom agents. If this assertion breaks, a change - // is altering summaries for existing adapters. - expect(result.summary).toBe( - "Let me get oriented and inspect the PRs…hidden chain of thought## Update\n\n- Checked PR status\n- Continue burn-in", - ); + expect(result.summary).toBe("## Update\n\n- Checked PR status\n- Continue burn-in"); + expect(result.summary).not.toContain("Let me get oriented"); + expect(result.summary).not.toContain("hidden chain of thought"); const toolCallEvents = logs .map((entry) => { try { @@ -732,7 +728,7 @@ describe("shared ACPX engine runtime behavior", () => { ]); }); - it("summarizes only the final output segment when the adapter sets summaryStrategy", async () => { + it("does not allow configuration to include thought text in run summaries", async () => { const root = await makeTempRoot(); const stateDir = path.join(root, "state"); const execute = createAcpxEngineExecutor({ @@ -803,7 +799,7 @@ describe("shared ACPX engine runtime behavior", () => { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir, - summaryStrategy: "lastOutputSegment", + summaryStrategy: "full", }, context: {}, onLog: async () => {}, @@ -811,12 +807,74 @@ describe("shared ACPX engine runtime behavior", () => { } as never); expect(result.exitCode).toBe(0); - // Must not include intermediate narration or thought stream. expect(result.summary).toBe("## Update\n\n- Checked PR status\n- Continue burn-in"); expect(result.summary).not.toContain("Let me get oriented"); expect(result.summary).not.toContain("hidden chain of thought"); }); + it("treats a statusless initial tool call as an output-segment boundary", async () => { + const root = await makeTempRoot(); + const stateDir = path.join(root, "state"); + const execute = createAcpxEngineExecutor({ + createRuntime: () => ({ + ensureSession: async () => ({ + backendSessionId: "backend-session", + agentSessionId: "agent-session", + runtimeSessionName: "runtime-session", + }), + startTurn: () => ({ + events: (async function* () { + yield { + type: "text_delta", + text: "Intermediate setup that must not be published", + stream: "output", + tag: "agent_message_chunk", + }; + yield { + type: "tool_call", + text: "Bash", + title: "Bash", + toolCallId: "tool-without-status", + tag: "tool_call", + }; + yield { + type: "tool_call", + text: "Bash (completed)", + title: "Bash", + status: "completed", + toolCallId: "tool-without-status", + tag: "tool_call_update", + }; + yield { + type: "text_delta", + text: "## Final update\n\n- Remediation verified", + stream: "output", + tag: "agent_message_chunk", + }; + yield { type: "done", stopReason: "end_turn" }; + })(), + result: Promise.resolve({ status: "completed", stopReason: "end_turn" }), + cancel: async () => {}, + }), + close: async () => {}, + }) as never, + }); + + const result = await execute({ + runId: "run-summary-statusless-tool-call", + agent: { id: "agent-1", companyId: "company-1" }, + runtime: {}, + config: { agent: "custom", agentCommand: "node ./fake-acp.js", stateDir }, + context: {}, + onLog: async () => {}, + onMeta: async () => {}, + } as never); + + expect(result.exitCode).toBe(0); + expect(result.summary).toBe("## Final update\n\n- Remediation verified"); + expect(result.summary).not.toContain("Intermediate setup"); + }); + it("buildAcpxRunSummary prefers the last non-empty segment", () => { expect( buildAcpxRunSummary({ diff --git a/packages/adapter-utils/src/acpx-engine/execute.ts b/packages/adapter-utils/src/acpx-engine/execute.ts index fc3aa6ffd5..06ab95bb42 100644 --- a/packages/adapter-utils/src/acpx-engine/execute.ts +++ b/packages/adapter-utils/src/acpx-engine/execute.ts @@ -373,9 +373,6 @@ export interface AcpxEngineExecutorOptions { interface AcpxPreparedRuntime { acpxAgent: string; - // See the config parsing site: adapter-declared engine behavior knobs with - // behavior-preserving defaults. - summaryStrategy: "full" | "lastOutputSegment"; coalescePlaceholderToolUpdates: boolean; mode: "persistent" | "oneshot"; cwd: string; @@ -1613,13 +1610,8 @@ async function buildRuntime(input: { ); const acpxAgent = normalizeAgent(config); - // Engine behavior knobs set by the invoking adapter's acpx config builder - // (never by the engine itself): a verbose streaming backend opts into - // last-segment run summaries and placeholder tool-update coalescing here. - // The defaults preserve the engine's long-standing behavior, and the engine - // carries no knowledge of which adapters opt in. - const summaryStrategy: "full" | "lastOutputSegment" = - config.summaryStrategy === "lastOutputSegment" ? "lastOutputSegment" : "full"; + // Run summaries always fail closed to the final output segment so internal + // thought text and intermediate narration cannot become issue comments. const coalescePlaceholderToolUpdates = config.coalescePlaceholderToolUpdates === true; const mode = normalizeMode(config); const permissionMode = normalizePermissionMode(config); @@ -2175,7 +2167,6 @@ async function buildRuntime(input: { return { acpxAgent, - summaryStrategy, coalescePlaceholderToolUpdates, mode, // Remote runner-backed → the in-sandbox workspace dir; local / runner-less @@ -2464,8 +2455,7 @@ async function emitAcpxLog(ctx: AdapterExecutionContext, payload: Record => { - // Summary accumulation, per the adapter-declared strategy. "full" (the - // default) collects every text delta exactly as before. - // "lastOutputSegment" collects output text only (never thought stream), + // Summary accumulation collects output text only (never thought stream), // segmented on tool starts so multi-step narration is not glued into one - // auto-comment dump. - const textParts: string[] = []; + // automatic comment dump. const outputSegments: string[] = []; let currentOutputChunk: string[] = []; const flushOutputSegment = () => { @@ -3992,13 +3979,13 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { const turn = activeTurn as AcpRuntimeTurn; const toolTitles = new Map(); for await (const event of turn.events) { - if (event.type === "text_delta") { - if (prepared.summaryStrategy === "full") { - textParts.push(event.text); - } else if (event.stream !== "thought") { - currentOutputChunk.push(event.text); - } - } else if (event.type === "tool_call" && event.status === "pending") { + if (event.type === "text_delta" && event.stream !== "thought") { + currentOutputChunk.push(event.text); + } else if (event.type === "tool_call" && event.tag !== "tool_call_update") { + // ACP makes tool-call status optional. The normalized event tag is + // the reliable boundary between an initial call and its updates, + // so a statusless initial call must still end the preceding output + // segment while updates must not create extra boundaries. flushOutputSegment(); } if (event.type === "status" && event.tag === "usage_update") { @@ -4088,13 +4075,10 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) { ? { cumulativeCostUsd: turnUsage.cumulativeCostUsd } : {}), }, - summary: - prepared.summaryStrategy === "lastOutputSegment" - ? buildAcpxRunSummary({ - outputSegments, - fallback: terminalStopReason || terminal.status, - }) - : textParts.join("").trim() || terminalStopReason || terminal.status, + summary: buildAcpxRunSummary({ + outputSegments, + fallback: terminalStopReason || terminal.status, + }), clearSession, }; // The turn phase finished. A completed, non-timed-out turn is `ok`; every diff --git a/server/src/__tests__/heartbeat-run-summary.test.ts b/server/src/__tests__/heartbeat-run-summary.test.ts index 74051ab469..593c6006e0 100644 --- a/server/src/__tests__/heartbeat-run-summary.test.ts +++ b/server/src/__tests__/heartbeat-run-summary.test.ts @@ -133,6 +133,18 @@ describe("mergeHeartbeatRunResultJson", () => { expect(buildHeartbeatRunIssueComment(merged)).toBe("## Summary\n\n1. first thing\n2. second thing"); }); + it("posts only the final adapter summary when raw output contains intermediate narration", () => { + const merged = mergeHeartbeatRunResultJson( + { stdout: "Intermediate setup that must not be published" }, + "## Final update\n\n- Remediation verified", + ); + + expect(buildHeartbeatRunIssueComment(merged)).toBe( + "## Final update\n\n- Remediation verified", + ); + expect(buildHeartbeatRunIssueComment(merged)).not.toContain("Intermediate setup"); + }); + it("creates a result payload when only a summary exists", () => { expect(mergeHeartbeatRunResultJson(null, "done")).toEqual({ summary: "done" }); });