diff --git a/.gitignore b/.gitignore index f8536eb0..ae8ffa04 100644 --- a/.gitignore +++ b/.gitignore @@ -39,3 +39,4 @@ copilot/acp-frames.old.ndjson # Models.dev catalog cache (disposable runtime cache under .copilot/) .copilot/model-catalog-cache.json +.otacon/ diff --git a/docs/plans/archive/2026-06-16-fix-copy-all-text-parts-agent-response.md b/docs/plans/archive/2026-06-16-fix-copy-all-text-parts-agent-response.md new file mode 100644 index 00000000..95997bcb --- /dev/null +++ b/docs/plans/archive/2026-06-16-fix-copy-all-text-parts-agent-response.md @@ -0,0 +1,136 @@ +--- +title: fix-copy-all-text-parts-agent-response +session: otc_rxkcfc +revision: 2 +status: approved +created: 2026-06-16 +--- + +## Summary + +```mermaid +flowchart LR + P["parts: text A · thought · text B"] --> ST["splitTrailingText
(trailing run only)"] + ST -->|"final = [B]"| OLD["Copy / Insert = 'B' ❌"] + P --> FA["agentResponseText
(all text parts)"] + FA -->|"[A, B]"| NEW["Copy / Insert = 'A\n\nB' ✓"] +``` + +Copy / Insert truncate agent responses because the helper reuses +`splitTrailingText`, which keeps only the **last contiguous run** of `text` +parts. Fix: collect **every** non-empty `text` part in stream order, and rename +`finalAnswerText` → `agentResponseText` (it no longer returns only the polished +final answer). `splitTrailingText` (and the "Worked for X" folding) is untouched. + +## Contract + +```ts +agentResponseText(parts: AgentMessagePart[]): string +``` + +- Collects **all** `kind: "text"` parts in stream order (was: trailing run only). +- Drops empty / whitespace-only text parts, then joins with `"\n\n"`. +- Result passed through `cleanMessageForCopy` (think-tags, tool markers, 3+ + newlines → 2, trim) — unchanged sanitization. +- `[text "A", thought, text "B"]` → `"A\n\nB"`; tool-only / `[]` → `""`. + +## Decisions + +- D1: `agentResponseText` gathers all `text` parts, not the trailing run [assumed] (issue spec) +- D2: Filter empty / whitespace-only text parts before joining ← q1 +- D3: Leave `splitTrailingText` and "Worked for X" folding unchanged ← q2 / issue scope +- D4: Accept inline-vs-copy mismatch on collapsed turns (copy may include folded narration) ← q2 +- D5: Keep the `"\n\n"` separator; `cleanMessageForCopy` collapses any 3+ newline pileup ← issue open-q [assumed] +- D6: Flip the existing "trailing run only" test to assert all-text behavior [assumed] (success criteria) +- D7: Rename `finalAnswerText` → `agentResponseText`; the name "final answer" no longer fits once all text is collected [assumed] (review t1) + +| Pick | Approach | Tradeoff | +| ---- | ------------------------------------------------ | ------------------------------------------------------------- | +| ✓ | Collect all `text` parts in `agentResponseText` | completeness; copy may include mid-research narration | +| | Widen `splitTrailingText` to span dropped blocks | also changes "Worked for X" folding — explicitly out of scope | + +## Impact + +```mermaid +flowchart TD + ST["splitTrailingText (UNCHANGED)"] --> WB["WorkedForBlock folding"] + ST --> IL["inline final render"] + FA["agentResponseText (CHANGED)"] --> ACT["AgentMessageActions — Copy / Insert"] +``` + +`agentResponseText` has a single consumer: `AgentTrailView.tsx:64` → +`AgentMessageActions` (Copy / Insert). `splitTrailingText` stays as-is, so both +its consumers — the "Worked for X" fold and the inline final render — are +unaffected. Blast radius is the clipboard / editor-insert text only. + +## Phases + +### Phase 1 — Collect all text parts in `agentResponseText` + +Goal: `agentResponseText` returns every non-empty `text` part joined in stream +order, sanitized as today; `splitTrailingText` is not modified. + +Files: + +- `src/agentMode/ui/agentTrail.ts` — rename `finalAnswerText` → + `agentResponseText` and rewrite it to filter `parts` to non-empty `text` + parts, map to `.text`, `join("\n\n")`, then `cleanMessageForCopy`. Update the + doc comment to drop the "trailing run" wording. `splitTrailingText` untouched. +- `src/agentMode/ui/AgentTrailView.tsx` — update the import and the single call + site (`const answer = agentResponseText(parts)`, line 64). +- `src/agentMode/ui/agentTrail.test.ts` — update the import + `finalAnswerText` + references to `agentResponseText`; flip the existing trailing-run test; add the + interleaved-thought, interleaved-tool, whitespace, and tool-only cases. + +Verification: `npm run test -- agentTrail` green; `npm run format && npm run lint` clean. + +```gwt +Given parts [text "A", thought "Thought for < 1s", text "B"] +When agentResponseText runs +Then it returns "A\n\nB" + +Given parts [text "A", tool_call, text "B"] +When agentResponseText runs +Then it returns "A\n\nB" + +Given parts [text "A", text " ", text "B"] +When agentResponseText runs +Then it returns "A\n\nB" with no stray blank line + +Given a tool-only turn [thought, tool_call] (no prose) +When agentResponseText runs +Then it returns "" so Copy / Insert stay gated off + +Given any parts +When splitTrailingText runs +Then its research/final split is byte-for-byte unchanged (existing tests pass) +``` + +## Risks + +> [!risk] +> Copied / inserted output can now include mid-research narration the agent +> emitted between tool calls (e.g. "Let me check…"). Accepted per the issue — +> completeness over silent truncation. + +> [!risk] +> `splitTrailingText` is shared with the "Worked for X" fold. Touching it would +> regress folding. Mitigation: change only `agentResponseText`; keep all existing +> `splitTrailingText` tests green. + +## Open Questions + +None — q1 (filter empties) and q2 (scope to `agentResponseText`, accept mismatch) +are resolved. + +## Interview + +### q1 — How should finalAnswerText handle empty / whitespace-only text parts when collecting all text parts and joining with "\n\n"? + +- Options: Filter them out before joining (recommended) | Keep them; rely on cleanMessageForCopy to collapse | Keep them as-is +- Answer: Filter them out before joining + +### q2 — In a collapsed 'Worked for X' turn, the inline view shows only the trailing prose, but after this fix Copy/Insert will grab ALL prose (including mid-research narration folded into 'Worked for X'). How do we handle that see-vs-copy mismatch? + +- Options: Accept it — scope change to finalAnswerText only (per the issue) (recommended) | Also render all text parts inline in the collapse view +- Answer: Accept it — scope change to finalAnswerText only (per the issue) diff --git a/src/agentMode/ui/AgentTrailView.test.tsx b/src/agentMode/ui/AgentTrailView.test.tsx index 933b50bf..18ab74d2 100644 --- a/src/agentMode/ui/AgentTrailView.test.tsx +++ b/src/agentMode/ui/AgentTrailView.test.tsx @@ -13,7 +13,7 @@ jest.mock("@/agentMode/ui/AgentMarkdownText", () => ({ // `insertAtCursor` is a spy (its selection→replace logic is covered by the // `insertAtCursor` unit test in utils.test.ts); `cleanMessageForCopy` is a thin -// stand-in (real sanitization is covered by the `finalAnswerText` unit test) so +// stand-in (real sanitization is covered by the `agentResponseText` unit test) so // the cleaned text the buttons act on is deterministic here. jest.mock("@/utils", () => ({ cleanMessageForCopy: (s: string) => s.trim(), diff --git a/src/agentMode/ui/AgentTrailView.tsx b/src/agentMode/ui/AgentTrailView.tsx index 3ff983d0..04c46de7 100644 --- a/src/agentMode/ui/AgentTrailView.tsx +++ b/src/agentMode/ui/AgentTrailView.tsx @@ -1,7 +1,7 @@ import React, { useState } from "react"; import { + agentResponseText, buildAgentTrail, - finalAnswerText, splitTrailingText, type RenderNode, } from "@/agentMode/ui/agentTrail"; @@ -58,10 +58,10 @@ export const AgentTrail: React.FC = ({ typeof turnDurationMs === "number" && parts.length > 0; - // Copy / Insert act on the turn's final answer only. Gate them off while the - // message is still streaming and on cancelled turns (treated as having no - // user-visible answer), plus whenever there is no trailing prose to act on. - const answer = finalAnswerText(parts); + // Copy / Insert act on the agent's full textual response. Gate them off while + // the message is still streaming and on cancelled turns (treated as having no + // user-visible answer), plus whenever there is no prose to act on. + const answer = agentResponseText(parts); const actions = !isStreaming && turnStopReason !== "cancelled" && answer.length > 0 ? ( diff --git a/src/agentMode/ui/agentTrail.test.ts b/src/agentMode/ui/agentTrail.test.ts index d2054c1a..abdd410d 100644 --- a/src/agentMode/ui/agentTrail.test.ts +++ b/src/agentMode/ui/agentTrail.test.ts @@ -1,6 +1,6 @@ import { + agentResponseText, buildAgentTrail, - finalAnswerText, splitTrailingText, type RenderNode, } from "@/agentMode/ui/agentTrail"; @@ -346,30 +346,49 @@ describe("splitTrailingText", () => { }); }); -describe("finalAnswerText", () => { - it("returns only the trailing run of text parts, ignoring the research half", () => { +describe("agentResponseText", () => { + it("collects all text parts in stream order, even across interleaved research", () => { const parts: AgentMessagePart[] = [ thought("let me search the vault"), tool("a", { vendorToolName: "Grep" }), - text("Here is an early note that should NOT be copied."), + text("Early prose emitted before the research finished."), tool("b", { vendorToolName: "Read" }), - text("This is the final answer."), + text("The wrap-up after the research."), ]; - expect(finalAnswerText(parts)).toBe("This is the final answer."); + // Both prose segments are captured — the earlier one is no longer dropped + // just because a tool_call follows it. + expect(agentResponseText(parts)).toBe( + "Early prose emitted before the research finished.\n\nThe wrap-up after the research." + ); }); - it("sanitizes the trailing text the same way legacy chat copy does", () => { + it("joins text parts split by a thought", () => { + const parts: AgentMessagePart[] = [text("A"), thought("Thought for < 1s"), text("B")]; + expect(agentResponseText(parts)).toBe("A\n\nB"); + }); + + it("joins text parts split by a tool call", () => { + const parts: AgentMessagePart[] = [text("A"), tool("x"), text("B")]; + expect(agentResponseText(parts)).toBe("A\n\nB"); + }); + + it("drops a whitespace-only text part without leaving a stray blank line", () => { + const parts: AgentMessagePart[] = [text("A"), text(" "), text("B")]; + expect(agentResponseText(parts)).toBe("A\n\nB"); + }); + + it("sanitizes the text the same way legacy chat copy does", () => { const parts: AgentMessagePart[] = [ tool("a"), text("internalThe answer.\n\n\n\nMore. "), ]; // removeThinkTags strips the think block, 3+ newlines collapse to 2, and // trailing whitespace is trimmed — matching `cleanMessageForCopy`. - expect(finalAnswerText(parts)).toBe("The answer.\n\nMore."); + expect(agentResponseText(parts)).toBe("The answer.\n\nMore."); }); - it("returns an empty string when the turn produced no trailing prose", () => { - expect(finalAnswerText([thought("..."), tool("a")])).toBe(""); - expect(finalAnswerText([])).toBe(""); + it("returns an empty string when the turn produced no prose", () => { + expect(agentResponseText([thought("..."), tool("a")])).toBe(""); + expect(agentResponseText([])).toBe(""); }); }); diff --git a/src/agentMode/ui/agentTrail.ts b/src/agentMode/ui/agentTrail.ts index 29b3be1c..8f7cbe06 100644 --- a/src/agentMode/ui/agentTrail.ts +++ b/src/agentMode/ui/agentTrail.ts @@ -55,17 +55,24 @@ export function splitTrailingText(parts: AgentMessagePart[]): { } /** - * The turn's user-visible final answer, ready for the clipboard or the editor: - * the trailing run of `text` parts (per `splitTrailingText`) joined and run - * through the same sanitization legacy chat applies (`cleanMessageForCopy`), - * so tool-call cards, reasoning, plans, and chat-only artifacts never leak in. - * Returns `""` when the turn produced no trailing prose (a tool-only turn, or - * one cancelled mid-tool) — the trail UI uses that to gate the Copy / Insert + * The agent's full textual response across the turn, ready for the clipboard + * or the editor: every `text` part in stream order (not just the trailing run), + * joined and run through the same sanitization legacy chat applies + * (`cleanMessageForCopy`), so tool-call cards, reasoning, plans, and chat-only + * artifacts never leak in. Interleaving research (a `thought` or `tool_call` + * between two prose chunks) must not drop the earlier prose, so we collect all + * text parts rather than only the trailing run. Empty/whitespace-only parts are + * skipped so they don't leave stray blank lines. + * Returns `""` when the turn produced no prose (a tool-only turn, or one + * cancelled mid-tool) — the trail UI uses that to gate the Copy / Insert * affordances off so they never sit under an empty bubble. */ -export function finalAnswerText(parts: AgentMessagePart[]): string { - const { final } = splitTrailingText(parts); - return cleanMessageForCopy(final.map((p) => p.text).join("\n\n")); +export function agentResponseText(parts: AgentMessagePart[]): string { + const text = parts + .filter((p): p is TextPart => p.kind === "text" && p.text.trim().length > 0) + .map((p) => p.text) + .join("\n\n"); + return cleanMessageForCopy(text); } /**