mirror of
https://github.com/logancyang/obsidian-copilot.git
synced 2026-07-22 07:50:24 +00:00
* docs(plan): fix copying all text parts of an agent response (#166) Otacon-approved plan for issue logancyang/obsidian-copilot-preview#166: agentResponseText collects every text part, not just the trailing run. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(agent-mode): copy/insert the full agent response, not just trailing prose (#166) Copy and Insert/Replace acted on finalAnswerText, which reused splitTrailingText and kept only the last contiguous run of text parts — so a 'text -> thought -> text' turn dropped the earlier prose. Rename the helper to agentResponseText and have it collect every non-empty text part in stream order (joined with blank lines, then cleanMessageForCopy). splitTrailingText is untouched, so the 'Worked for X' folding is unchanged. Fixes logancyang/obsidian-copilot-preview#166 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(plan): archive completed plan for #166 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * update docs * test(agent-mode): clarify interleaved-prose fixture wording (#166) The flipped test reused fixture strings that said the early prose 'should NOT be copied', which now contradicts the assertion (it is copied). Reword the strings to describe the new behavior. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
60dc554c1f
commit
bd60f10f56
6 changed files with 189 additions and 26 deletions
1
.gitignore
vendored
1
.gitignore
vendored
|
|
@ -39,3 +39,4 @@ copilot/acp-frames.old.ndjson
|
|||
|
||||
# Models.dev catalog cache (disposable runtime cache under .copilot/)
|
||||
.copilot/model-catalog-cache.json
|
||||
.otacon/
|
||||
|
|
|
|||
|
|
@ -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<br/>(trailing run only)"]
|
||||
ST -->|"final = [B]"| OLD["Copy / Insert = 'B' ❌"]
|
||||
P --> FA["agentResponseText<br/>(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)
|
||||
|
|
@ -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(),
|
||||
|
|
|
|||
|
|
@ -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<AgentTrailProps> = ({
|
|||
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 ? (
|
||||
<AgentMessageActions text={answer} app={app} />
|
||||
|
|
|
|||
|
|
@ -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("<think>internal</think>The 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("");
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
Loading…
Reference in a new issue