fix: address review feedback for lossless terminal output

- OutputInterceptor.finalize() now awaits stream flush before returning
  This ensures artifact files are fully written before the artifact_id
  is advertised to the LLM, preventing partial reads.

- Remove strict mode from read_command_output native tool schema
  With strict: true, OpenAI requires all params in 'required', forcing
  the LLM to provide explicit null values for optional params. This
  created verbose tool calls. Now optional params can be omitted entirely.

- Update tests to handle async finalize() method

Author: hannesrudolph

src/core/prompts/tools/native-tools/read_command_output.ts

@@ -49,7 +49,11 @@ export default {
 	function: {
 		name: "read_command_output",
 		description: READ_COMMAND_OUTPUT_DESCRIPTION,
-		strict: true,
+		// Note: strict mode is intentionally disabled for this tool.
+		// With strict: true, OpenAI requires ALL properties to be in the 'required' array,
+		// which forces the LLM to always provide explicit values (even null) for optional params.
+		// This creates verbose tool calls and poor UX. By disabling strict mode, the LLM can
+		// omit optional parameters entirely, making the tool easier to use.
 		parameters: {
 			type: "object",
 			properties: {
@@ -58,21 +62,19 @@ export default {
 					description: ARTIFACT_ID_DESCRIPTION,
 				},
 				search: {
-					type: ["string", "null"],
+					type: "string",
 					description: SEARCH_DESCRIPTION,
 				},
 				offset: {
-					type: ["number", "null"],
+					type: "number",
 					description: OFFSET_DESCRIPTION,
 				},
 				limit: {
-					type: ["number", "null"],
+					type: "number",
 					description: LIMIT_DESCRIPTION,
 				},
 			},
-			// With strict: true, ALL properties must be listed in required.
-			// Optional params use union type with null (e.g., ["string", "null"]).
-			required: ["artifact_id", "search", "offset", "limit"],
+			required: ["artifact_id"],
 			additionalProperties: false,
 		},
 	},

src/core/tools/ExecuteCommandTool.ts

@@ -246,10 +246,12 @@ export async function executeCommandInTerminal(
 				// Silently handle ask errors (e.g., "Current ask promise was ignored")
 			}
 		},
-		onCompleted: (output: string | undefined) => {
-			// Finalize interceptor and get persisted result
+		onCompleted: async (output: string | undefined) => {
+			// Finalize interceptor and get persisted result.
+			// We await finalize() to ensure the artifact file is fully flushed
+			// before we advertise the artifact_id to the LLM.
 			if (interceptor) {
-				persistedResult = interceptor.finalize()
+				persistedResult = await interceptor.finalize()
 			}
 
 			// Continue using compressed output for UI display

src/integrations/terminal/OutputInterceptor.ts

@@ -288,7 +288,10 @@ export class OutputInterceptor {
 	/**
 	 * Finalize the interceptor and return the persisted output result.
 	 *
-	 * Closes any open file streams and returns a summary object containing:
+	 * Closes any open file streams and waits for them to fully flush before returning.
+	 * This ensures the artifact file is completely written and ready for reading.
+	 *
+	 * Returns a summary object containing:
 	 * - A preview of the output (head + [omitted indicator] + tail)
 	 * - The total byte count of all output
 	 * - The path to the full output file (if truncated)
@@ -298,18 +301,22 @@ export class OutputInterceptor {
 	 *
 	 * @example
 	 * ```typescript
-	 * const result = interceptor.finalize();
+	 * const result = await interceptor.finalize();
 	 * console.log(`Preview: ${result.preview}`);
 	 * console.log(`Total bytes: ${result.totalBytes}`);
 	 * if (result.truncated) {
 	 *   console.log(`Full output at: ${result.artifactPath}`);
 	 * }
 	 * ```
 	 */
-	finalize(): PersistedCommandOutput {
-		// Close write stream if open
+	async finalize(): Promise<PersistedCommandOutput> {
+		// Close write stream if open and wait for it to fully flush.
+		// This ensures the artifact is completely written before we advertise the artifact_id.
 		if (this.writeStream) {
-			this.writeStream.end()
+			await new Promise<void>((resolve, reject) => {
+				this.writeStream!.end(() => resolve())
+				this.writeStream!.on("error", reject)
+			})
 		}
 
 		// Prepare preview: head + [omission indicator] + tail

src/integrations/terminal/__tests__/OutputInterceptor.test.ts

@@ -34,10 +34,14 @@ describe("OutputInterceptor", () => {
 
 		storageDir = path.normalize("/tmp/test-storage")
 
-		// Setup mock write stream
+		// Setup mock write stream with callback support for end()
 		mockWriteStream = {
 			write: vi.fn(),
-			end: vi.fn(),
+			end: vi.fn((callback?: () => void) => {
+				// Immediately call the callback to simulate stream flush completing
+				if (callback) callback()
+			}),
+			on: vi.fn(),
 		}
 
 		vi.mocked(fs.existsSync).mockReturnValue(true)
@@ -49,7 +53,7 @@ describe("OutputInterceptor", () => {
 	})
 
 	describe("Buffering behavior", () => {
-		it("should keep small output in memory without spilling to disk", () => {
+		it("should keep small output in memory without spilling to disk", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -64,7 +68,7 @@ describe("OutputInterceptor", () => {
 			expect(interceptor.hasSpilledToDisk()).toBe(false)
 			expect(fs.createWriteStream).not.toHaveBeenCalled()
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 			expect(result.preview).toBe(smallOutput)
 			expect(result.truncated).toBe(false)
 			expect(result.artifactPath).toBe(null)
@@ -94,7 +98,7 @@ describe("OutputInterceptor", () => {
 			expect(mockWriteStream.write).toHaveBeenCalled()
 		})
 
-		it("should truncate preview after spilling to disk using head/tail split", () => {
+		it("should truncate preview after spilling to disk using head/tail split", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -109,7 +113,7 @@ describe("OutputInterceptor", () => {
 
 			expect(interceptor.hasSpilledToDisk()).toBe(true)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 			expect(result.truncated).toBe(true)
 			expect(result.artifactPath).toBe(path.join(storageDir, "cmd-12345.txt"))
 			// Preview is head (1024) + omission indicator + tail (1024)
@@ -268,7 +272,7 @@ describe("OutputInterceptor", () => {
 	})
 
 	describe("finalize() method", () => {
-		it("should return preview output for small commands", () => {
+		it("should return preview output for small commands", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -280,15 +284,15 @@ describe("OutputInterceptor", () => {
 			const output = "Hello World\n"
 			interceptor.write(output)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			expect(result.preview).toBe(output)
 			expect(result.totalBytes).toBe(Buffer.byteLength(output, "utf8"))
 			expect(result.artifactPath).toBe(null)
 			expect(result.truncated).toBe(false)
 		})
 
-		it("should return PersistedCommandOutput for large commands with head/tail preview", () => {
+		it("should return PersistedCommandOutput for large commands with head/tail preview", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -300,7 +304,7 @@ describe("OutputInterceptor", () => {
 			const largeOutput = "x".repeat(5000)
 			interceptor.write(largeOutput)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			expect(result.truncated).toBe(true)
 			expect(result.artifactPath).toBe(path.join(storageDir, "cmd-12345.txt"))
@@ -310,7 +314,7 @@ describe("OutputInterceptor", () => {
 			expect(result.preview).toContain("bytes omitted...]")
 		})
 
-		it("should close write stream when finalizing", () => {
+		it("should close write stream when finalizing", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -321,12 +325,12 @@ describe("OutputInterceptor", () => {
 
 			// Trigger spill
 			interceptor.write("x".repeat(3000))
-			interceptor.finalize()
+			await interceptor.finalize()
 
 			expect(mockWriteStream.end).toHaveBeenCalled()
 		})
 
-		it("should include correct metadata (artifactId, size, truncated flag)", () => {
+		it("should include correct metadata (artifactId, size, truncated flag)", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -338,7 +342,7 @@ describe("OutputInterceptor", () => {
 			const output = "x".repeat(5000)
 			interceptor.write(output)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			expect(result).toHaveProperty("preview")
 			expect(result).toHaveProperty("totalBytes", 5000)
@@ -432,7 +436,7 @@ describe("OutputInterceptor", () => {
 	})
 
 	describe("Head/Tail split behavior", () => {
-		it("should preserve first 50% and last 50% of output", () => {
+		it("should preserve first 50% and last 50% of output", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -450,7 +454,7 @@ describe("OutputInterceptor", () => {
 			interceptor.write(middleContent)
 			interceptor.write(tailContent)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			// Should start with HEAD content (first 1024 bytes of head budget)
 			expect(result.preview.startsWith("HEAD")).toBe(true)
@@ -461,7 +465,7 @@ describe("OutputInterceptor", () => {
 			expect(result.preview).toContain("bytes omitted...]")
 		})
 
-		it("should not add omission indicator when output fits in budget", () => {
+		it("should not add omission indicator when output fits in budget", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -473,14 +477,14 @@ describe("OutputInterceptor", () => {
 			const smallOutput = "Hello World\n"
 			interceptor.write(smallOutput)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			// No omission indicator for small output
 			expect(result.preview).toBe(smallOutput)
 			expect(result.preview).not.toContain("[...")
 		})
 
-		it("should handle output that exactly fills head budget", () => {
+		it("should handle output that exactly fills head budget", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -493,14 +497,14 @@ describe("OutputInterceptor", () => {
 			const exactHeadContent = "x".repeat(1024)
 			interceptor.write(exactHeadContent)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			// Should fit entirely in head, no truncation
 			expect(result.preview).toBe(exactHeadContent)
 			expect(result.truncated).toBe(false)
 		})
 
-		it("should split single large chunk across head and tail", () => {
+		it("should split single large chunk across head and tail", async () => {
 			const interceptor = new OutputInterceptor({
 				executionId: "12345",
 				taskId: "task-1",
@@ -514,7 +518,7 @@ describe("OutputInterceptor", () => {
 			const content = "A".repeat(1024) + "B".repeat(2000) + "C".repeat(1024)
 			interceptor.write(content)
 
-			const result = interceptor.finalize()
+			const result = await interceptor.finalize()
 
 			// Head should have A's
 			expect(result.preview.startsWith("A")).toBe(true)