fix(coding-agent): prevented duplicate artifact saves and mismatched ids in bash output

2026-07-28 02:05:43 +02:00
parent cfd335d2b1
commit bbe0236a0f
5 changed files with 88 additions and 17 deletions
@@ -2,11 +2,12 @@

 ## [Unreleased]

+## [17.1.7] - 2026-07-27
+
 ### Fixed

 - Restoring a prompt with image attachments via esc-esc branch or `/tree` now re-attaches the images to the composer draft: previously only the text (with its `[Image #N]` markers) was restored, so resubmitting sent the literal marker with no image.
-
-## [17.1.7] - 2026-07-27
+- Fixed large bash/eval/ssh output citing two different artifact ids in one result — the truncation notice said `Read artifact://N for full output` while the footer said `Artifact: N+1`. The streaming sink's head and tail windows each had a full budget, so a middle-elided inline body could reach `headBytes + spillThreshold` and always re-tripped the final-defense inline byte cap, which truncated a second time (two elision markers), saved a duplicate already-truncated artifact, and left the notice's line ranges stale. The head and tail windows now share the spill-threshold budget (head clamped to half), the cap budget derives from the configured threshold plus notice slack, and when the cap does fire on a sink-spilled result it references the existing raw artifact instead of saving a copy.

 ### Added

@@ -53,12 +53,17 @@ export interface OutputSummary {
 export interface OutputSinkOptions {
 	artifactPath?: string;
 	artifactId?: string;
-	/** Tail buffer budget (bytes). Default DEFAULT_MAX_BYTES. */
+	/**
+	 * Total inline body budget (bytes). Default DEFAULT_MAX_BYTES. The head
+	 * window and rolling tail window share this budget, so a composed
+	 * `dump()` body never exceeds it (plus the elision marker).
+	 */
 	spillThreshold?: number;
 	/**
 	 * When > 0, the sink keeps the first `headBytes` of output in addition to
 	 * the rolling tail window. Output between the two windows is elided
-	 * (middle elision). Default 0 = tail-only behavior.
+	 * (middle elision). Clamped to `spillThreshold / 2` so the tail keeps at
+	 * least half the inline budget. Default 0 = tail-only behavior.
 	 */
 	headBytes?: number;
 	/**
@@ -799,7 +804,7 @@ export class OutputSink {
 		this.#artifactPath = artifactPath;
 		this.#artifactId = artifactId;
 		this.#spillThreshold = spillThreshold;
-		this.#headLimit = Math.max(0, headBytes);
+		this.#headLimit = Math.max(0, Math.min(headBytes, Math.floor(spillThreshold / 2)));
 		this.#maxColumns = Math.max(0, maxColumns);
 		this.#onChunk = onChunk;
 		this.#chunkThrottleMs = chunkThrottleMs;
@@ -969,16 +974,23 @@ export class OutputSink {
 		return parts.join("");
 	}

+	// The rolling tail budget is whatever the head window has not consumed of
+	// the inline budget: `spillThreshold - #headBytes`. While the head window
+	// fills it shrinks toward `spillThreshold - headLimit`; after `replace()`
+	// (head cleared) it grows back to the full threshold. This keeps
+	// `head + tail <= spillThreshold`, so the composed dump body fits the
+	// inline byte cap by construction.
+
 	#willOverflow(dataBytes: number): boolean {
 		// Triggers file mirroring as soon as the next chunk would push us over
-		// the tail budget (head retention does not change spill-to-artifact).
-		return this.#bufferBytes + dataBytes > this.#spillThreshold;
+		// the tail budget — i.e. the first byte that could be lost from memory.
+		return this.#bufferBytes + dataBytes > this.#spillThreshold - this.#headBytes;
 	}

 	#pushTail(chunk: string, dataBytes: number): void {
 		if (dataBytes === 0) return;

-		const threshold = this.#spillThreshold;
+		const threshold = Math.max(0, this.#spillThreshold - this.#headBytes);
 		const willOverflow = this.#bufferBytes + dataBytes > threshold;

 		if (!willOverflow) {
@@ -37,6 +37,7 @@ import { invalidateGithubCacheForBashCommand } from "./gh-cache-invalidation";
 import {
 	formatStyledTruncationWarning,
 	type OutputMeta,
+	resolveInlineByteCapBudget,
 	stripOutputNotice,
 	stripRawOutputArtifactNotice,
 } from "./output-meta";
@@ -655,6 +656,18 @@ export class BashTool implements AgentTool<typeof bashSchemaBase | typeof bashSc
 			details.exitCode = exitCode;
 		}

+		// Final-defense inline cap config, shared by the timeout and normal
+		// completion paths. The sink already bounds inline bodies to the spill
+		// threshold, so with the notice slack this only fires on paths that
+		// bypass the sink (client-bridge terminals, minimizer misses). When the
+		// sink spilled, its artifact already holds the full raw stream — reuse
+		// that id instead of saving a second (already-truncated) copy, so the
+		// `[raw output: artifact://N]` footer and the truncation notice agree.
+		const inlineCap = {
+			maxBytes: resolveInlineByteCapBudget(this.session.settings),
+			saveArtifact: (full: string) => result.artifactId ?? saveBashOriginalArtifact(this.session, full),
+		};
+
 		if (isTimeout) {
 			details.timedOut = true;
 			const message =
@@ -664,9 +677,7 @@ export class BashTool implements AgentTool<typeof bashSchemaBase | typeof bashSc
 			if (!normalizeResultOutput(result).startsWith(`[${message}]\n`)) {
 				outputLines.push("", `[${message}]`);
 			}
-			const timeoutOutputText = await enforceInlineByteCap(outputLines.join("\n"), {
-				saveArtifact: full => saveBashOriginalArtifact(this.session, full),
-			});
+			const timeoutOutputText = await enforceInlineByteCap(outputLines.join("\n"), inlineCap);
 			return toolResult(details)
 				.text(timeoutOutputText)
 				.truncationFromSummary(result, { direction: "tail" })
@@ -677,12 +688,8 @@ export class BashTool implements AgentTool<typeof bashSchemaBase | typeof bashSc
 		// Non-timeout cancellations and missing exit status still propagate as thrown errors.
 		this.#throwIfUnfinished(result, timeoutSec, outputText);

-		// Final defense at the tool-result boundary: no bash path (client bridge,
-		// head-retention spill, minimizer miss) may emit more than
-		// ~DEFAULT_MAX_BYTES inline. No-op for already-bounded output.
-		const cappedOutputText = await enforceInlineByteCap(outputText, {
-			saveArtifact: full => saveBashOriginalArtifact(this.session, full),
-		});
+		// No-op for already-bounded output; see `inlineCap` above.
+		const cappedOutputText = await enforceInlineByteCap(outputText, inlineCap);

 		const resultBuilder = toolResult(details)
 			.text(cappedOutputText)
@@ -631,6 +631,26 @@ export function resolveOutputSinkHeadBytes(s: Settings | undefined): number {
 	return getSpillConfig(s).headBytes;
 }

+/**
+ * Slack on top of the configured spill threshold before the final-defense
+ * inline byte cap fires. The OutputSink already bounds inline bodies to the
+ * threshold; only notice slop (wall time, exit code, elision marker,
+ * `[raw output: artifact://N]` footer) rides above it. The slack keeps the
+ * cap a genuine last resort for paths that bypass the sink (e.g. ACP
+ * client-bridge terminals) instead of re-truncating — and re-saving — every
+ * sink-elided result (the double-artifact `Artifact: N+1` vs `artifact://N`
+ * mismatch).
+ */
+const INLINE_CAP_SLACK_BYTES = 2 * 1024;
+
+/**
+ * Resolve the `enforceInlineByteCap` budget for streaming tools (bash/ssh)
+ * from session settings: the user's spill threshold plus notice slack.
+ */
+export function resolveInlineByteCapBudget(s: Settings | undefined): number {
+	return getSpillConfig(s).threshold + INLINE_CAP_SLACK_BYTES;
+}
+
 /**
 * Resolve the per-line column cap from session settings. Shared by streaming
 * executors (bash/python/ssh/eval via OutputSink) and the `read` tool's
@@ -3,6 +3,7 @@ import * as fs from "node:fs/promises";
 import * as os from "node:os";
 import * as path from "node:path";
 import {
+	enforceInlineByteCap,
 	formatHeadTruncationNotice,
 	formatMiddleElisionMarker,
 	formatTailTruncationNotice,
@@ -686,6 +687,36 @@ describe("OutputSink head-retain mode", () => {
 		// Counters realign to the authoritative buffer + the subsequent push.
 		expect(dumped.totalBytes).toBe(byteLength("OK\n[raw output: artifact://8]\n"));
 	});
+
+	test("middle-elided dump body fits the inline budget (no double truncation)", async () => {
+		// Regression: the head and tail windows each had their own full budget,
+		// so an elided dump body could reach headBytes + spillThreshold and
+		// re-trip enforceInlineByteCap at the tool-result boundary — truncating
+		// a second time and saving a duplicate artifact whose id disagreed with
+		// the truncation notice's `Read artifact://N for full output`.
+		const spillThreshold = 1000;
+		const sink = new OutputSink({ spillThreshold, headBytes: 400 });
+		const lines = Array.from({ length: 400 }, (_, i) => `line ${i}`).join("\n");
+		sink.push(lines);
+
+		const dumped = await sink.dump();
+		expect(dumped.truncated).toBe(true);
+		expect(dumped.elidedLines ?? 0).toBeGreaterThan(0);
+		// Head window + elision marker + tail window share the one budget
+		// (small slack for the marker and separators).
+		expect(byteLength(dumped.output)).toBeLessThanOrEqual(spillThreshold + 64);
+
+		let saved: string | undefined;
+		const capped = await enforceInlineByteCap(dumped.output, {
+			maxBytes: spillThreshold + 2048,
+			saveArtifact: full => {
+				saved = full;
+				return "duplicate";
+			},
+		});
+		expect(capped).toBe(dumped.output);
+		expect(saved).toBeUndefined();
+	});
 });

 describe("OutputSink maxColumns (per-line cap)", () => {