scriptoriumadmin/Roo-Code
1
0
Fork 0
mirror of https://github.com/RooVetGit/Roo-Code.git synced 2026-09-06 08:18:39 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions

fix: address review feedback for lossless terminal output

Browse source 
- 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
This commit is contained in:
Hannes Rudolph 2026-01-27 12:51:40 -07:00
parent f5faf0ff93
commit 6bc2a14d61
4 changed files with 49 additions and 36 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

16
src/core/prompts/tools/native-tools/read_command_output.ts
View file

@ -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,
},
},

4
src/core/tools/ExecuteCommandTool.ts
View file

@ -263,10 +263,10 @@ export async function executeCommandInTerminal(
if (interceptor) {
persistedResult = await interceptor.finalize()
}
// Continue using compressed output for UI display
result = Terminal.compressTerminalOutput(output ?? "")
task.say("command_output", result)
completed = true
} finally {

17
src/integrations/terminal/OutputInterceptor.ts
View file

@ -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,7 +301,7 @@ 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) {
@ -306,10 +309,14 @@ export class OutputInterceptor {
* }
* ```
*/
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

48
src/integrations/terminal/__tests__/OutputInterceptor.test.ts
View file

@ -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,7 +284,7 @@ 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"))
@ -288,7 +292,7 @@ describe("OutputInterceptor", () => {
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)