From 88be72e4d4f418c01d4372b5b6cb42489dad220e Mon Sep 17 00:00:00 2001 From: roboomp Date: Tue, 30 Jun 2026 08:04:49 +0000 Subject: [PATCH] fix(coding-agent): seed tool-args reveal with the partial JSON already in hand MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ToolArgsRevealController.setTarget initialized new entries with revealed=0, so the first message_update returned { __partialJson: "" } even when the provider had already parsed a complete chunk. For renderers without exposeRawPartialJson (e.g. write), the throttled re-parse + cached displayArgs short-circuited every subsequent setTarget, leaving the preview body blank until tool_execution_end. Seed revealed with the full incoming partialJson length on entry creation (clamped to a surrogate-safe boundary). The first frame now carries the parsed path/content immediately; subsequent message_updates extend target and the reveal ticks pace only the newly arrived bytes — no field is ever truncated because the seeded prefix is the longest the entry has seen so far. Fixes #3881 --- packages/coding-agent/CHANGELOG.md | 4 + .../src/modes/controllers/tool-args-reveal.ts | 2 +- .../event-controller-args-reveal.test.ts | 40 +++++-- .../test/tool-args-reveal.test.ts | 101 ++++++++++++------ 4 files changed, 106 insertions(+), 41 deletions(-) diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index c44e8b2da..5f5ba3f12 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed streaming tool-call previews (notably `write`) showing an empty body for the entire streaming phase by surfacing the partial JSON already in hand on the first reveal, then pacing only subsequent growth ([#3881](https://github.com/can1357/oh-my-pi/issues/3881)). + ## [16.2.7] - 2026-06-30 ### Breaking Changes diff --git a/packages/coding-agent/src/modes/controllers/tool-args-reveal.ts b/packages/coding-agent/src/modes/controllers/tool-args-reveal.ts index d19b5c5ac..d5e1d3516 100644 --- a/packages/coding-agent/src/modes/controllers/tool-args-reveal.ts +++ b/packages/coding-agent/src/modes/controllers/tool-args-reveal.ts @@ -144,7 +144,7 @@ export class ToolArgsRevealController { entry = { component: undefined, target: partialJson, - revealed: 0, + revealed: clampSliceEnd(partialJson, partialJson.length), rawInput, exposeRawPartialJson, parsedArgs: {}, diff --git a/packages/coding-agent/test/modes/controllers/event-controller-args-reveal.test.ts b/packages/coding-agent/test/modes/controllers/event-controller-args-reveal.test.ts index c87a9f862..62ca6a6af 100644 --- a/packages/coding-agent/test/modes/controllers/event-controller-args-reveal.test.ts +++ b/packages/coding-agent/test/modes/controllers/event-controller-args-reveal.test.ts @@ -91,26 +91,45 @@ describe("EventController paces streamed tool args", () => { vi.restoreAllMocks(); }); - it("reveals partialJson prefixes per frame, then snaps to final args when the JSON closes", async () => { + it("reveals the initial slice immediately, then paces growth across message_updates", async () => { await Settings.init({ inMemory: true, cwd: process.cwd() }); vi.useFakeTimers(); const updateArgsSpy = vi.spyOn(ToolExecutionComponent.prototype, "updateArgs"); const content = "x".repeat(400); const target = `{"path":"/tmp/a.ts","content":"${content}"}`; - const streaming = makeStreamingMessage([ + // Seed includes the complete `path` field (closing quote at byte 20) plus + // the opening of `content`, so the rendered preview must show the real + // path on the very first dispatch. + const seed = target.slice(0, 35); + + // First message_update: only a small slice has arrived. The reveal + // MUST surface it as-is (no empty initial frame). + const seedStreaming = makeStreamingMessage([ + { type: "toolCall", id: "tc-1", name: "write", arguments: {}, [kStreamingPartialJson]: seed }, + ]); + const { controller, pendingTools } = createFixture(seedStreaming); + await dispatch(controller, seedStreaming); + expect(pendingTools.size).toBe(1); + + // Component constructor consumes the initial render args directly; no + // updateArgs has been invoked yet, but the seeded prefix is already on + // the pending preview. + const componentRender = pendingTools.get("tc-1")!.render(80).join("\n"); + expect(Bun.stripANSI(componentRender)).toContain("/tmp/a.ts"); + + // Second message_update: the rest of the payload arrives. The controller + // paces the new backlog through reveal ticks. + const fullStreaming = makeStreamingMessage([ { type: "toolCall", id: "tc-1", name: "write", arguments: {}, [kStreamingPartialJson]: target }, ]); - const { controller, pendingTools } = createFixture(streaming); - - await dispatch(controller, streaming); - expect(pendingTools.size).toBe(1); + await dispatch(controller, fullStreaming); for (let i = 0; i < 3; i++) { vi.advanceTimersByTime(STREAMING_REVEAL_FRAME_MS); } const pacedFrames = updateArgsSpy.mock.calls.map(call => call[0] as Record); expect(pacedFrames.length).toBeGreaterThan(0); - let previousLength = 0; + let previousLength = seed.length; for (const frame of pacedFrames) { const prefix = frame.__partialJson; if (typeof prefix !== "string") throw new Error("Expected __partialJson string on paced frame"); @@ -171,12 +190,13 @@ describe("EventController paces streamed tool args", () => { ]); const { controller, pendingTools } = createFixture(streaming); - // Args still streaming: the reveal seeds the preview at an empty prefix, so - // the write head shows its `…` path placeholder rather than the real path. + // Args still streaming, but the reveal now seeds the preview with the + // full available partialJson on the very first message_update — so the + // path is already visible before the tool starts executing. await dispatch(controller, streaming); const component = pendingTools.get("tc-1"); if (!component) throw new Error("expected a pending write component"); - expect(Bun.stripANSI(component.render(80).join("\n"))).not.toContain("/tmp/exec.ts"); + expect(Bun.stripANSI(component.render(80).join("\n"))).toContain("/tmp/exec.ts"); // The closing full-args message_update never arrives (throttled `arguments` // with smoothing off, an owned-dialect projector, or a superseded turn that diff --git a/packages/coding-agent/test/tool-args-reveal.test.ts b/packages/coding-agent/test/tool-args-reveal.test.ts index a8ce21eb7..edeb740cf 100644 --- a/packages/coding-agent/test/tool-args-reveal.test.ts +++ b/packages/coding-agent/test/tool-args-reveal.test.ts @@ -51,26 +51,56 @@ describe("tool args reveal", () => { vi.useRealTimers(); }); - it("reveals raw partial JSON monotonically for renderers that consume it", () => { - vi.useFakeTimers(); - const { component, controller } = makeController(); - const content = "line one\\nline two\\nline three of a streamed write payload"; - const target = `{"path":"a.ts","content":"${content}"}`; + it("reveals what already arrived on the first setTarget call", () => { + const { controller } = makeController(); + const target = `{"path":"a.ts","content":"abc"}`; const initial = controller.setTarget( "call-1", target, jsonTarget({ fullArgs: { path: "a.ts" }, exposeRawPartialJson: true }), ); - expect(partialOf(initial)).toBe(""); + + // The provider already delivered a complete partialJson chunk; the + // controller MUST surface its parsed fields and raw prefix immediately — + // pacing applies only to subsequent growth, never to bytes already in hand. + expect(initial.path).toBe("a.ts"); + expect(initial.content).toBe("abc"); + expect(partialOf(initial)).toBe(target); + }); + + it("paces growth across successive setTarget calls for raw-prefix renderers", () => { + vi.useFakeTimers(); + const { component, controller } = makeController(); + const content = "line one\\nline two\\nline three of a streamed write payload"; + const target = `{"path":"a.ts","content":"${content}"}`; + const seed = target.slice(0, 12); + + const initial = controller.setTarget( + "call-1", + seed, + jsonTarget({ fullArgs: { path: "a.ts" }, exposeRawPartialJson: true }), + ); + // What arrived is exposed immediately, no empty initial frame. + expect(partialOf(initial)).toBe(seed); controller.bind("call-1", component); + + // More bytes arrive later — the controller now paces the new backlog. + controller.setTarget( + "call-1", + target, + jsonTarget({ fullArgs: { path: "a.ts" }, exposeRawPartialJson: true }), + ); drain(100); const partials = component.frames.map(partialOf); + expect(partials.length).toBeGreaterThan(0); expect(partials.at(-1)).toBe(target); - for (let i = 1; i < partials.length; i++) { - expect(partials[i].length).toBeGreaterThanOrEqual(partials[i - 1].length); - expect(target.startsWith(partials[i])).toBe(true); + let previous = seed.length; + for (const partial of partials) { + expect(partial.length).toBeGreaterThanOrEqual(previous); + expect(target.startsWith(partial)).toBe(true); + previous = partial.length; } }); @@ -79,15 +109,27 @@ describe("tool args reveal", () => { const requestRender = vi.fn(); const { component, controller } = makeController({ requestRender }); const target = `{"path":"a.ts","content":"${"x".repeat(1200)}"}`; + const seed = target.slice(0, 10); - const initial = controller.setTarget("call-1", target, jsonTarget()); - expect(partialOf(initial)).toBe(""); + // Seed: small initial slice, revealed immediately, sets parsedLen=seed.length. + const initial = controller.setTarget("call-1", seed, jsonTarget()); + expect(partialOf(initial)).toBe(seed); controller.bind("call-1", component); + + // Full payload arrives; the controller paces the new growth. + controller.setTarget("call-1", target, jsonTarget()); + expect(component.frames).toHaveLength(0); + + // First paced tick lands inside the small-prefix window + // (< STREAMING_JSON_PARSE_MIN_GROWTH), so a re-parse is forced and a + // frame fires. drain(1); expect(component.frames).toHaveLength(1); expect(requestRender).toHaveBeenCalledTimes(1); const firstPartial = partialOf(component.frames[0]); + // The next tick crosses into the throttled window: growth from the last + // parse hasn't yet hit STREAMING_JSON_PARSE_MIN_GROWTH, so no frame fires. drain(1); expect(component.frames).toHaveLength(1); expect(requestRender).toHaveBeenCalledTimes(1); @@ -98,21 +140,6 @@ describe("tool args reveal", () => { expect(secondPartial.length - firstPartial.length).toBeGreaterThanOrEqual(STREAMING_JSON_PARSE_MIN_GROWTH); }); - it("keeps small JSON args visible before completion", () => { - vi.useFakeTimers(); - const { component, controller } = makeController(); - const target = `{"path":"a.ts","content":"abc"}`; - - controller.setTarget("call-1", target, jsonTarget()); - controller.bind("call-1", component); - drain(20); - - const latest = component.frames.at(-1)!; - expect(latest.path).toBe("a.ts"); - expect(latest.content).toBe("abc"); - expect(partialOf(latest)).toBe(target); - }); - it("passes the full target through untouched when smoothing is disabled", () => { vi.useFakeTimers(); const requestRender = vi.fn(); @@ -132,11 +159,16 @@ describe("tool args reveal", () => { it("finish drops the reveal so no further frames are pushed", () => { vi.useFakeTimers(); const { component, controller } = makeController(); + const target = `{"path":"a.ts","content":"${"x".repeat(400)}"}`; + const seed = target.slice(0, 5); - controller.setTarget("call-1", `{"path":"a.ts","content":"abcdefghijklmnop"}`, jsonTarget()); + controller.setTarget("call-1", seed, jsonTarget()); controller.bind("call-1", component); + // Backlog of new bytes for the reveal loop to advance through. + controller.setTarget("call-1", target, jsonTarget()); drain(1); const frames = component.frames.length; + expect(frames).toBeGreaterThan(0); controller.finish("call-1"); drain(10); @@ -147,9 +179,11 @@ describe("tool args reveal", () => { vi.useFakeTimers(); const { component, controller } = makeController(); const target = `{"path":"a.ts","content":"${"x".repeat(500)}"}`; + const seed = target.slice(0, 5); - controller.setTarget("call-1", target, jsonTarget()); + controller.setTarget("call-1", seed, jsonTarget()); controller.bind("call-1", component); + controller.setTarget("call-1", target, jsonTarget()); drain(1); expect(partialOf(component.frames.at(-1)!).length).toBeLessThan(target.length); controller.flushAll(); @@ -164,9 +198,11 @@ describe("tool args reveal", () => { vi.useFakeTimers(); const { component, controller } = makeController(); const target = `{"content":"${"😀🎉".repeat(40)}"}`; + const seed = target.slice(0, 12); // before any surrogate - controller.setTarget("call-1", target, jsonTarget({ exposeRawPartialJson: true })); + controller.setTarget("call-1", seed, jsonTarget({ exposeRawPartialJson: true })); controller.bind("call-1", component); + controller.setTarget("call-1", target, jsonTarget({ exposeRawPartialJson: true })); drain(100); expect(partialOf(component.frames.at(-1)!)).toBe(target); @@ -179,11 +215,16 @@ describe("tool args reveal", () => { vi.useFakeTimers(); const { component, controller } = makeController(); const target = "*** Begin Patch\n*** Update File: a.ts\n-old\n+new\n*** End Patch"; + const seed = target.slice(0, 5); - controller.setTarget("call-1", target, rawTarget({ input: target })); + const initial = controller.setTarget("call-1", seed, rawTarget({ input: seed })); + expect(initial.input).toBe(seed); + expect(partialOf(initial)).toBe(seed); controller.bind("call-1", component); + controller.setTarget("call-1", target, rawTarget({ input: target })); drain(100); + expect(component.frames.length).toBeGreaterThan(0); for (const frame of component.frames) { expect(frame.input).toBe(partialOf(frame)); }