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
This commit is contained in:
nearfolk
GitHub
parent
67ce516a84
commit
c2cfd55e97
|
|
@ -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({
|
||||
|
|
|
|||
|
|
@ -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<string,
|
|||
|
||||
/**
|
||||
* Build the short run summary that Paperclip may auto-post as an issue comment
|
||||
* when the agent leaves no comment of its own. Used only for agents whose
|
||||
* traits opt into the "lastOutputSegment" summary strategy.
|
||||
* when the agent leaves no comment of its own.
|
||||
*
|
||||
* Prefer the last non-empty *output* segment after a tool call. Intermediate
|
||||
* "let me check…" narration between tools must not become a 50k-char dump.
|
||||
|
|
@ -3877,12 +3867,9 @@ export function createAcpxEngineExecutor(deps: AcpxEngineExecutorOptions = {}) {
|
|||
// controller and never rejects; it returns a `TurnCompletion`. The step
|
||||
// bodies below record the external result for the coordinator to reproduce.
|
||||
const runTurn = async (_ready: StartupReady): Promise<TurnCompletion> => {
|
||||
// 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<string, string>();
|
||||
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
|
||||
|
|
|
|||
|
|
@ -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" });
|
||||
});
|
||||
|
|
|
|||
Loading…
Reference in New Issue