Fix guidance (#1997)

* refactor: Simplify local search guidance handling in AutonomousAgentChainRunner and CopilotPlusChainRunner

- Removed the `appendInlineCitationReminder` function and adjusted the logic in `AutonomousAgentChainRunner` to directly manage local search guidance.
- Updated `CopilotPlusChainRunner` to retain and utilize the most recent local search guidance, ensuring it is positioned correctly adjacent to user queries.
- Modified `buildLocalSearchInnerContent` to accept introductory text instead of guidance, enhancing clarity in the payload structure.
- Removed related tests for the deprecated `appendInlineCitationReminder` function, streamlining the codebase.

* refactor: Enhance localSearch payload structure and guidance handling

- Made localSearch payload self-contained by embedding RAG instructions, documents, and citation guidance directly within the `<localSearch>` block.
- Updated AutonomousAgentChainRunner and CopilotPlusChainRunner to ensure each localSearch call carries its own guidance block, improving citation accuracy in multi-search scenarios.
- Introduced a new utility function to insert guidance before the user query label, ensuring clarity in the payload structure.
- Adjusted related tests to validate the new guidance handling logic.

* docs: Add deprecation guide for IntentAnalyzer and Broca

- Created a comprehensive document outlining the responsibilities of the `IntentAnalyzer` and Broca API within the Copilot Plus intent analysis flow.
- Detailed the current functionalities, known consumers, migration constraints, and a phased migration plan to ensure feature parity and smooth transition.
- Highlighted risks and mitigation strategies associated with the deprecation process, along with open questions for further consideration.

* fix: Prevent infinite rolling animation in ToolCallBanner when result is present

Add defensive check that stops animation when tool result exists, even if isExecuting flag wasn't updated. This fixes race conditions where marker updates fail or are delayed during streaming.

🤖 Generated with [Claude Code](https://claude.ai/code)
via [Happy](https://happy.engineering)

Co-Authored-By: Claude <noreply@anthropic.com>
Co-Authored-By: Happy <yesreply@happy.engineering>

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Happy <yesreply@happy.engineering>

docs/CONTEXT_ENGINEERING.md

@@ -165,7 +165,8 @@ All tools (`localSearch`, `webSearch`, `getFileTree`, etc.) are treated uniforml
 
 - Intent analysis still Broca-based (ToolCallPlanner pending), but prompt assembly is envelope-first.
 - All tool outputs (localSearch, webSearch, file tree, etc.) formatted uniformly and prepended to user message.
-- LocalSearch payload now embeds “answer from context” guidance and citation catalog inside the tool block.
+- LocalSearch payload is self-contained: includes RAG instruction, documents, and citation guidance all within the `<localSearch>` block.
+- Each localSearch call carries its own guidance block (source catalog + citation rules), ensuring correct citations in multi-search turns.
 - Composer instructions appended once to avoid duplication; payload recorder captures tool XML alongside L3/L5.
 
 ### AutonomousAgentChainRunner
@@ -339,11 +340,11 @@ const finalUserContent = renderCiCMessage(
 <localSearch timeRange="last week">
 Answer the question based only on the following context:
 
-<guidance>
-[Citation rules...]
-</guidance>
-
 [Retrieved documents...]
+
+<guidance>
+[Citation rules and source catalog...]
+</guidance>
 </localSearch>
 
 <webSearch>
@@ -356,7 +357,8 @@ Answer the question based only on the following context:
 - Each tool wrapped as `<toolName>content</toolName>`
 - Prepended to user message via `renderCiCMessage()`
 - RAG instruction ("Answer based on context") included in localSearch output
-- Citation guidance travels with RAG results
+- Citation guidance included directly within each `<localSearch>` block (self-contained)
+- Multi-search turns: each localSearch has its own guidance block with source mappings
 - Turn-specific, never cached in L2
 
 ### Intent Analysis

docs/deprecating-intent-analyzer.md

@@ -0,0 +1,83 @@
+# Deprecating `IntentAnalyzer` and Broca
+
+This document captures the current responsibilities of the Copilot Plus intent analysis flow (`IntentAnalyzer` + Brevilabs “broca” API) and outlines a safe migration path to remove it while keeping Copilot Plus functionality aligned with the autonomous agent experience.
+
+## Current Responsibilities
+
+- **Tool orchestration via Broca** (`src/LLMProviders/intentAnalyzer.ts:34`\
+  – `BrevilabsClient.broca`): each chat turn sends the raw user message to `/broca` and receives:
+  - `tool_calls`: predefined tool names with argument payloads. In practice the only tools that still rely on Broca for automatic detection are the _utility tools_ (`getCurrentTime`, `convertTimeBetweenTimezones`, `getTimeRangeMs`, `getTimeInfoByEpoch`, and occasionally `getFileTree`). Feature toggles, explicit UI controls, and `@` commands already cover search, web search, composer, memory updates, and indexing.
+  - `salience_terms`: keywords Broca derives from the user message, passed untouched to the vault search tool.
+- **Tool registry bootstrap** (`IntentAnalyzer.initTools`): wires up the same Zod-described tools used by the agent (`localSearchTool`, `webSearchTool`, `getTimeRangeMs`, etc.) so Copilot Plus can execute them without the agent loop.
+- **Time-expression handling**: when Broca schedules `getTimeRangeMs`, `IntentAnalyzer` executes it first and stores the returned range so the subsequent `localSearch` call includes the `timeRange`.
+- **`@` command overrides** (`IntentAnalyzer.processAtCommands`): falls back to local heuristics for inline control commands even if Broca does not schedule a tool call.
+  - `@vault` → forces `localSearch`.
+  - `@websearch` / `@web` → forces `webSearch`.
+  - `@memory` → forces `updateMemory`.
+- **Plus-specific salient term injection**: any Broca `salience_terms` array is passed into `localSearch` unchanged, altering recall compared with the agent chain. Our discovery shows this causes divergence between Plus and agent results; we need both flows to end up using the same salient term set.
+- **Implicit license enforcement**: `/broca` is the only per-turn API call guaranteed to touch Brevilabs. Although license validation also uses `/license` (`checkIsPlusUser`), Broca acts as the continuous touch point that can detect expired keys mid-session.
+
+## Known Consumers and Side Effects
+
+- `CopilotPlusChainRunner.run` (`src/LLMProviders/chainRunner/CopilotPlusChainRunner.ts:488`) is the sole caller of `IntentAnalyzer.analyzeIntent`.
+- `BrevilabsClient` exposes `broca` and `/license` validation; multiple subsystems already call `validateLicenseKey` directly (e.g., `checkIsPlusUser`, `embeddingManager`, `plusUtils`).
+- Tests: there is no direct unit test coverage for `IntentAnalyzer`, but tool execution tests (`src/LLMProviders/chainRunner/utils/toolExecution.test.ts`) rely on the same registry.
+- Production telemetry/debug logging assumes the Broca payload; removal must not break logging expectations.
+
+## Constraints for Migration
+
+- **Feature parity**: Copilot Plus must keep working with vault search, timeline questions, web search, file tree lookup, and memory updates.
+- **License verification**: every chat turn must continue to check Plus eligibility (either by retaining a per-turn Brevilabs call or by adding an explicit `/license` check with caching and backoff).
+- **Minimal prompt drift**: Copilot Plus prompts currently do not include the aggressive tool-calling instructions used by the autonomous agent; the migration should not break existing prompt tuning until the replacement strategy is ready.
+- **Zero regression for `@` commands**: the existing inline overrides are user-facing affordances.
+- **Search recall**: any solution must surface the same or better salient term quality as the agent chain to avoid regressions highlighted in recent investigations.
+- **Plus/agent parity**: after migration, the Plus chain must feed the vault search pipeline with the same query string, expanded variants, and salient term list that the agent chain would generate for the identical user input.
+- **Scoped automatic detection**: only the utility tools without explicit UI affordances (`getCurrentTime`, `convertTimeBetweenTimezones`, `getTimeRangeMs`, `getTimeInfoByEpoch`, `getFileTree`) need automatic invocation; everything else can be triggered through toggles or `@tool` commands.
+
+## Migration Plan
+
+### Phase 0 – Discovery & Telemetry
+
+1. **Instrument current intent decisions**: add temporary logging (behind debug flag) to record Broca output, executed tools, and overrides. Purpose: baseline behavior before refactor.
+2. **Map tool usage**: capture how frequently each Broca tool is invoked to prioritise feature parity work.
+3. **Clarify license expectations**: confirm with product whether `/license` checks can replace Broca for per-turn validation, or if a lighter “heartbeat” endpoint is needed.
+
+### Phase 1 – Surface-Agnostic Tool Planning
+
+4. **Extract shared tool planner interface**: design a new planner (e.g., `PlusToolPlanner`) that returns the same `{ tool, args }[]` shape as `IntentAnalyzer`. Plan for multiple implementations (Broca, agent-driven, heuristic).
+5. **Port `@` command handling**: move `processAtCommands` logic into the new planner so overrides are planner-agnostic.
+6. **Adopt agent-style salient term generation**: introduce a reusable salience extractor (likely the agent instruction set or a deterministic tokenizer) so Plus mode produces the same salient terms the agent chain would compute.
+7. **Define utility auto-detection rules**: implement deterministic heuristics for the remaining auto-triggered tools (time/time-range/time-info/file-tree) so the planner can schedule them without LLM help.
+
+### Phase 2 – Replace Broca for Tool Scheduling
+
+8. **Reuse ModelAdapter-driven planning**: embed a constrained agent loop (single-iteration tool planner) that leverages the existing XML instructions used by autonomous agent models. Limit to deciding tool calls; keep Copilot Plus response streaming as-is.
+9. **Fallback heuristics**: provide deterministic patterns for the utility tools to guard against planner failures and avoid unneeded LLM calls for searches already controlled by UI toggles.
+10. **Feature flag & dogfood**: gate the new planner behind a runtime toggle (`settings.debugPlanner` or remote flag) to test against Broca in parallel, and compare Plus/agent query-expansion outputs in telemetry to ensure they match.
+
+### Phase 3 – License Enforcement Replacement
+
+11. **Introduce explicit per-turn license check**: call `validateLicenseKey` (or a new lightweight endpoint) at the start of each Plus turn. Cache results for the current conversation with TTL to avoid redundant traffic within a single turn.
+12. **Graceful degradation**: if validation fails or is unreachable, surface the same error handling as today (e.g., show invalid key notice, disable Plus features).
+
+### Phase 4 – Removal & Cleanup
+
+13. **Switch default planner**: once the new planner reaches parity, flip the feature flag so Copilot Plus no longer calls Broca. Keep Broca behind a hidden fallback flag for one release.
+14. **Delete `IntentAnalyzer`**: remove the class, its tests, and references. Update imports (`initializeBuiltinTools` already handles tool registration outside IntentAnalyzer).
+15. **Retire `BrevilabsClient.broca`**: delete the method and any unused types. Keep `/license` and other endpoints.
+16. **Documentation & migration notes**: update `AGENTS.md`, `TODO.md`, and any user-facing Plus documentation to note the new flow.
+
+## Risks and Mitigations
+
+- **Planner hallucination / regressions**: mitigate with deterministic overrides, strict XML parsing (already in `toolExecution`), and fallback to default responses if tool planning fails.
+- **License API outages**: add retry/backoff and degrade gracefully (read-only mode, user notice).
+- **Search recall differences**: unit test the new salience extractor against QueryExpander fixtures to guarantee improved recall vs Broca output and to keep Plus aligned with agent expansion behaviour.
+- **Timeline for removal**: schedule the cleanup after at least one release cycle of telemetry from the new planner.
+
+## Open Questions
+
+- Should Copilot Plus adopt the full autonomous agent loop (multiple tool turns) or stay single-shot with a tighter planner?
+- Do we need a dedicated Brevilabs endpoint for per-turn license heartbeat instead of reusing `/license`?
+- How will we migrate existing analytics dashboards that currently expect Broca telemetry?
+
+Document owner: _TBD_ (assign during implementation kickoff).

src/LLMProviders/chainRunner/AutonomousAgentChainRunner.ts

@@ -37,10 +37,7 @@ import {
   ToolExecutionResult,
 } from "./utils/toolExecution";
 
-import {
-  appendInlineCitationReminder,
-  ensureCiCOrderingWithQuestion,
-} from "./utils/cicPromptUtils";
+import { ensureCiCOrderingWithQuestion } from "./utils/cicPromptUtils";
 import { LayerToMessagesConverter } from "@/context/LayerToMessagesConverter";
 import { buildAgentPromptDebugReport } from "./utils/promptDebugService";
 import { recordPromptPayload } from "./utils/promptPayloadRecorder";
@@ -199,8 +196,9 @@ ${params}
 
   /**
    * Apply CiC ordering by appending the original user question after the local search payload.
+   * Guidance is now self-contained within each localSearch payload from prepareLocalSearchResult.
    *
-   * @param localSearchPayload - XML-wrapped local search payload prepared for the LLM.
+   * @param localSearchPayload - XML-wrapped local search payload prepared for the LLM (includes guidance).
    * @param originalPrompt - The original user prompt (before any enhancements).
    * @returns Payload with question appended using CiC ordering when needed.
    */
@@ -208,8 +206,7 @@ ${params}
     localSearchPayload: string,
     originalPrompt: string
   ): string {
-    const promptWithReminder = appendInlineCitationReminder(originalPrompt);
-    return ensureCiCOrderingWithQuestion(localSearchPayload, promptWithReminder);
+    return ensureCiCOrderingWithQuestion(localSearchPayload, originalPrompt);
   }
 
   private getTemporaryToolCallId(toolName: string, index: number): string {

src/LLMProviders/chainRunner/CopilotPlusChainRunner.ts

@@ -299,7 +299,8 @@ export class CopilotPlusChainRunner extends BaseChainRunner {
     userMessage: ChatMessage,
     allToolOutputs: any[],
     abortController: AbortController,
-    thinkStreamer: ThinkBlockStreamer
+    thinkStreamer: ThinkBlockStreamer,
+    originalUserQuestion: string
   ): Promise<void> {
     // Get chat history
     const memory = this.chainManager.memoryManager.getMemory();
@@ -350,18 +351,42 @@ export class CopilotPlusChainRunner extends BaseChainRunner {
       // All tools (including localSearch) are formatted uniformly and added to user message
       const hasTools = allToolOutputs.length > 0;
 
+      const ensureUserQueryLabel = (content: string): string => {
+        const userQueryLabel = "[User query]:";
+        if (content.includes(userQueryLabel)) {
+          return content;
+        }
+
+        const trimmedContent = content.trimEnd();
+        const sections: string[] = [];
+        if (trimmedContent.length > 0) {
+          sections.push(trimmedContent);
+        }
+
+        const trimmedQuestion =
+          originalUserQuestion.trim() ||
+          userMessage.message?.trim() ||
+          userMessage.originalMessage?.trim() ||
+          "";
+        if (trimmedQuestion.length > 0) {
+          sections.push(`${userQueryLabel}\n${trimmedQuestion}`);
+        } else {
+          sections.push(userQueryLabel);
+        }
+
+        return sections.join("\n\n");
+      };
+
       if (hasTools) {
         // Format all tool outputs and prepend to user content using CiC format
         const toolContext = this.formatAllToolOutputs(allToolOutputs);
 
-        finalUserContent = renderCiCMessage(
-          toolContext,
-          userMessageContent.content // L3 smart refs + L5 from converter
-        );
+        const userContentWithLabel = ensureUserQueryLabel(userMessageContent.content);
+        finalUserContent = renderCiCMessage(toolContext, userContentWithLabel);
       } else {
         // No tools - use converter's output as-is
         // Smart references are already properly formatted by LayerToMessagesConverter
-        finalUserContent = userMessageContent.content;
+        finalUserContent = ensureUserQueryLabel(userMessageContent.content);
       }
 
       // Add composer instructions if textContent has them
@@ -513,7 +538,8 @@ export class CopilotPlusChainRunner extends BaseChainRunner {
         userMessage,
         allToolOutputs,
         abortController,
-        thinkStreamer
+        thinkStreamer,
+        cleanedUserMessage
       );
     } catch (error: any) {
       // Reset loading message to default
@@ -576,7 +602,18 @@ export class CopilotPlusChainRunner extends BaseChainRunner {
     const toolOutputs = [];
     const allSources: { title: string; path: string; score: number; explanation?: any }[] = [];
 
+    // TODO: remove this hack until better solution in place (logan, wenzheng)
+    // Skip getFileTree if localSearch is already being called to avoid redundant work
+    const hasLocalSearch = toolCalls.some((tc) => tc.tool.name === "localSearch");
+
     for (const toolCall of toolCalls) {
+      // TODO: remove this hack until better solution in place (logan, wenzheng)
+      // Skip getFileTree when localSearch is present
+      if (toolCall.tool.name === "getFileTree" && hasLocalSearch) {
+        logInfo("Skipping getFileTree since localSearch is already active");
+        continue;
+      }
+
       logInfo(`Step 2: Calling tool: ${toolCall.tool.name}`);
       if (toolCall.tool.name === "localSearch") {
         updateLoadingMessage?.(LOADING_MESSAGES.READING_FILES);
@@ -672,17 +709,18 @@ export class CopilotPlusChainRunner extends BaseChainRunner {
       };
     });
 
-    const guidance = getLocalSearchGuidance(catalogLines);
+    // Build guidance block with citation rules and source catalog
+    const guidance = getLocalSearchGuidance(catalogLines).trim();
 
     // Add RAG instruction (like VaultQA) to ensure model uses the context
     const ragInstruction = "Answer the question based only on the following context:";
-    const innerContent = buildLocalSearchInnerContent(
-      `${ragInstruction}\n\n${guidance}`,
-      formattedContent
-    );
+    const documentsSection = buildLocalSearchInnerContent(ragInstruction, formattedContent);
+
+    // Include guidance directly in the payload, making it self-contained
+    const fullInnerContent = guidance ? `${documentsSection}\n\n${guidance}` : documentsSection;
 
     // Wrap in XML-like tags for better LLM understanding
-    return wrapLocalSearchPayload(innerContent, timeExpression);
+    return wrapLocalSearchPayload(fullInnerContent, timeExpression);
   }
 
   /**

src/LLMProviders/chainRunner/utils/cicPromptUtils.test.ts

@@ -1,20 +1,20 @@
 import {
-  appendInlineCitationReminder,
   buildLocalSearchInnerContent,
   ensureCiCOrderingWithQuestion,
+  injectGuidanceBeforeUserQuery,
   renderCiCMessage,
   wrapLocalSearchPayload,
 } from "./cicPromptUtils";
 
 describe("cicPromptUtils", () => {
   describe("buildLocalSearchInnerContent", () => {
-    it("orders guidance before documents and trims whitespace", () => {
-      const guidance = "\n<guidance>Rules</guidance>\n";
+    it("orders intro text before documents and trims whitespace", () => {
+      const intro = "\nIntro block\n";
       const documents = "\n<document>Doc</document>\n";
 
-      const inner = buildLocalSearchInnerContent(guidance, documents);
+      const inner = buildLocalSearchInnerContent(intro, documents);
 
-      expect(inner).toBe("<guidance>Rules</guidance>\n\n<document>Doc</document>");
+      expect(inner).toBe("Intro block\n\n<document>Doc</document>");
     });
 
     it("returns empty string when both inputs are blank", () => {
@@ -50,29 +50,6 @@ describe("cicPromptUtils", () => {
     });
   });
 
-  describe("appendInlineCitationReminder", () => {
-    it("appends reminder when question has content", () => {
-      const result = appendInlineCitationReminder("Summarize my notes.");
-
-      expect(result).toBe(
-        "Summarize my notes.\n\nHave inline citations according to the guidance."
-      );
-    });
-
-    it("avoids duplicating reminder when already present", () => {
-      const question = "Summarize. Have inline citations according to the guidance.";
-      const result = appendInlineCitationReminder(question);
-
-      expect(result).toBe("Summarize. Have inline citations according to the guidance.");
-    });
-
-    it("returns reminder alone when question is blank", () => {
-      const result = appendInlineCitationReminder("   ");
-
-      expect(result).toBe("Have inline citations according to the guidance.");
-    });
-  });
-
   describe("ensureCiCOrderingWithQuestion", () => {
     it("appends the trimmed question with [User query]: label after the payload when missing", () => {
       const payload = "<localSearch>\n<context/>\n</localSearch>";
@@ -97,4 +74,40 @@ describe("cicPromptUtils", () => {
       expect(ensureCiCOrderingWithQuestion(payload, "   ")).toBe(payload);
     });
   });
+
+  describe("injectGuidanceBeforeUserQuery", () => {
+    const guidance = "<guidance>\nRules\n</guidance>";
+
+    it("places guidance before user query label when present", () => {
+      const payload = "# Additional context:\n\n<context>\n</context>\n\n[User query]:\nWhat?";
+      const result = injectGuidanceBeforeUserQuery(payload, guidance);
+
+      expect(result).toBe(
+        "# Additional context:\n\n<context>\n</context>\n\n<guidance>\nRules\n</guidance>\n\n[User query]:\nWhat?"
+      );
+    });
+
+    it("appends guidance when user query label missing", () => {
+      const payload = "<context>\n</context>";
+      const result = injectGuidanceBeforeUserQuery(payload, guidance);
+
+      expect(result).toBe("<context>\n</context>\n\n<guidance>\nRules\n</guidance>");
+    });
+
+    it("leaves payload unchanged when guidance empty", () => {
+      const payload = "<context>\n</context>";
+      expect(injectGuidanceBeforeUserQuery(payload, "")).toBe(payload);
+      expect(injectGuidanceBeforeUserQuery(payload, null)).toBe(payload);
+      expect(injectGuidanceBeforeUserQuery(payload, undefined)).toBe(payload);
+    });
+
+    it("handles payloads with trailing whitespace before label", () => {
+      const payload = "# Additional context:\n\n<context>\n</context>\n  \n\n[User query]:\nWhat?";
+      const result = injectGuidanceBeforeUserQuery(payload, guidance);
+
+      expect(result).toBe(
+        "# Additional context:\n\n<context>\n</context>\n\n<guidance>\nRules\n</guidance>\n\n[User query]:\nWhat?"
+      );
+    });
+  });
 });

src/LLMProviders/chainRunner/utils/cicPromptUtils.ts

@@ -3,13 +3,14 @@
  */
 
 /**
- * Builds the ordered inner payload for a localSearch result, placing guidance before documents.
- * @param guidance Citation guidance or instructions associated with the search results.
+ * Builds the ordered inner payload for a localSearch result, placing optional lead-in
+ * instructions ahead of the serialized documents.
+ * @param introText Context-setting instructions to show before the documents.
  * @param formattedContent Serialized documents selected for inclusion.
  * @returns Combined payload string with minimal whitespace.
  */
-export function buildLocalSearchInnerContent(guidance: string, formattedContent: string): string {
-  const sections = [guidance, formattedContent]
+export function buildLocalSearchInnerContent(introText: string, formattedContent: string): string {
+  const sections = [introText, formattedContent]
     .map((section) => section?.trim())
     .filter((section): section is string => Boolean(section));
 
@@ -28,27 +29,6 @@ export function wrapLocalSearchPayload(innerContent: string, timeExpression: str
   return `<localSearch${timeAttribute}>${payload}</localSearch>`;
 }
 
-/**
- * Append an inline citation reminder to the user's question.
- *
- * @param question - The original user question.
- * @returns The question with the reminder appended if not already present.
- */
-export function appendInlineCitationReminder(question: string): string {
-  const reminder = "Have inline citations according to the guidance.";
-  const trimmedQuestion = question.trimEnd();
-
-  if (!trimmedQuestion) {
-    return reminder;
-  }
-
-  if (trimmedQuestion.toLowerCase().includes(reminder.toLowerCase())) {
-    return trimmedQuestion;
-  }
-
-  return `${trimmedQuestion}\n\n${reminder}`;
-}
-
 /**
  * Produces a CiC-aligned prompt by placing context first and the user question last.
  * @param contextSection Prepared instruction/context block.
@@ -89,3 +69,29 @@ export function ensureCiCOrderingWithQuestion(
   // Use same label format as LayerToMessagesConverter for consistency
   return renderCiCMessage(localSearchPayload, `[User query]:\n${trimmedQuestion}`);
 }
+
+/**
+ * Inserts citation guidance immediately before the user query label, keeping tool context intact.
+ * @param payload Serialized CiC payload containing tool outputs and user query.
+ * @param guidance Guidance block (typically <guidance>...</guidance>) to insert.
+ * @returns Payload with guidance positioned before `[User query]:` or appended when label missing.
+ */
+export function injectGuidanceBeforeUserQuery(payload: string, guidance?: string | null): string {
+  const trimmedGuidance = guidance?.trim();
+  if (!trimmedGuidance) {
+    return payload;
+  }
+
+  const userQueryLabel = "[User query]:";
+  const labelIndex = payload.indexOf(userQueryLabel);
+  if (labelIndex === -1) {
+    const trimmedPayload = payload.trimEnd();
+    const joiner = trimmedPayload.length > 0 ? "\n\n" : "";
+    return `${trimmedPayload}${joiner}${trimmedGuidance}`;
+  }
+
+  const prefix = payload.slice(0, labelIndex).trimEnd();
+  const suffix = payload.slice(labelIndex).trimStart();
+
+  return `${prefix}\n\n${trimmedGuidance}\n\n${suffix}`;
+}

src/components/chat-components/ToolCallBanner.test.tsx

@@ -0,0 +1,254 @@
+import React from "react";
+import { render, screen } from "@testing-library/react";
+import { ToolCallBanner } from "@/components/chat-components/ToolCallBanner";
+
+// Mock the Collapsible components from Radix UI
+jest.mock("@/components/ui/collapsible", () => ({
+  Collapsible: ({ children, open }: { children: React.ReactNode; open: boolean }) => (
+    <div data-testid="collapsible" data-open={open}>
+      {children}
+    </div>
+  ),
+  CollapsibleContent: ({ children }: { children: React.ReactNode }) => (
+    <div data-testid="collapsible-content">{children}</div>
+  ),
+  CollapsibleTrigger: ({ children }: { children: React.ReactNode }) => (
+    <div data-testid="collapsible-trigger">{children}</div>
+  ),
+}));
+
+// Mock lucide-react icons
+jest.mock("lucide-react", () => ({
+  Check: () => <div data-testid="check-icon">Check</div>,
+  X: () => <div data-testid="x-icon">X</div>,
+  ChevronRight: () => <div data-testid="chevron-icon">ChevronRight</div>,
+}));
+
+// Mock the ToolResultFormatter
+jest.mock("@/tools/ToolResultFormatter", () => ({
+  ToolResultFormatter: {
+    format: jest.fn((_toolName: string, result: string) => result),
+  },
+}));
+
+describe("ToolCallBanner", () => {
+  const defaultProps = {
+    toolName: "testTool",
+    displayName: "Test Tool",
+    emoji: "🔧",
+  };
+
+  describe("actuallyExecuting logic (defensive check)", () => {
+    it("should show animation when executing with no result", () => {
+      const { container } = render(
+        <ToolCallBanner {...defaultProps} isExecuting={true} result={null} />
+      );
+
+      // Check for shimmer animation container
+      const shimmerOverlay = container.querySelector(".tw-absolute.tw-inset-0.tw-z-\\[1\\]");
+      expect(shimmerOverlay).not.toBeNull();
+
+      // Check for "Calling" text
+      expect(screen.getByText(/Calling Test Tool/)).toBeTruthy();
+    });
+
+    it("should hide animation when not executing with result", () => {
+      const { container } = render(
+        <ToolCallBanner {...defaultProps} isExecuting={false} result="Success" />
+      );
+
+      // Shimmer animation should NOT be present
+      const shimmerOverlay = container.querySelector(".tw-absolute.tw-inset-0.tw-z-\\[1\\]");
+      expect(shimmerOverlay).toBeNull();
+
+      // Check for "Called" text (past tense)
+      expect(screen.getByText(/Called Test Tool/)).toBeTruthy();
+    });
+
+    it("should hide animation when executing=true but result is present (bug fix)", () => {
+      const { container } = render(
+        <ToolCallBanner {...defaultProps} isExecuting={true} result="Success" />
+      );
+
+      // This is the key test: even though isExecuting=true, we have a result,
+      // so the animation should NOT run (actuallyExecuting = false)
+      const shimmerOverlay = container.querySelector(".tw-absolute.tw-inset-0.tw-z-\\[1\\]");
+      expect(shimmerOverlay).toBeNull();
+
+      // Should show "Called" since we have a result
+      expect(screen.getByText(/Called Test Tool/)).toBeTruthy();
+    });
+
+    it("should hide animation when not executing with empty result", () => {
+      const { container } = render(
+        <ToolCallBanner {...defaultProps} isExecuting={false} result="" />
+      );
+
+      // Shimmer animation should NOT be present
+      const shimmerOverlay = container.querySelector(".tw-absolute.tw-inset-0.tw-z-\\[1\\]");
+      expect(shimmerOverlay).toBeNull();
+
+      // Check for "Called" text
+      expect(screen.getByText(/Called Test Tool/)).toBeTruthy();
+    });
+  });
+
+  describe("expansion behavior", () => {
+    it("should not allow expansion while executing without result", () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={true} result={null} />);
+
+      const collapsible = screen.getByTestId("collapsible");
+      expect(collapsible.getAttribute("data-open")).toBe("false");
+    });
+
+    it("should allow expansion when not executing with result", () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={false} result="Success" />);
+
+      const collapsible = screen.getByTestId("collapsible");
+      // Initially closed, but can be opened
+      expect(collapsible.getAttribute("data-open")).toBe("false");
+    });
+
+    it("should allow expansion when executing=true but has result (actuallyExecuting=false)", () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={true} result="Success" />);
+
+      const collapsible = screen.getByTestId("collapsible");
+      // Should be expandable since we have a result
+      expect(collapsible.getAttribute("data-open")).toBe("false");
+    });
+  });
+
+  describe("text rendering", () => {
+    it('should show "Calling" when executing without result', () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={true} result={null} />);
+      expect(screen.getByText(/Calling Test Tool/)).toBeTruthy();
+    });
+
+    it('should show "Called" when not executing with result', () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={false} result="Success" />);
+      expect(screen.getByText(/Called Test Tool/)).toBeTruthy();
+    });
+
+    it("should show confirmation message when executing and message provided", () => {
+      render(
+        <ToolCallBanner
+          {...defaultProps}
+          isExecuting={true}
+          result={null}
+          confirmationMessage="Processing data"
+        />
+      );
+      expect(screen.getByText(/Processing data/)).toBeTruthy();
+    });
+
+    it('should use "Reading/Read" for readNote tool', () => {
+      render(
+        <ToolCallBanner
+          {...defaultProps}
+          toolName="readNote"
+          displayName="MyNote.md"
+          isExecuting={true}
+          result={null}
+        />
+      );
+      expect(screen.getByText(/Reading MyNote.md/)).toBeTruthy();
+    });
+
+    it('should use "Read" for readNote tool with result', () => {
+      render(
+        <ToolCallBanner
+          {...defaultProps}
+          toolName="readNote"
+          displayName="MyNote.md"
+          isExecuting={false}
+          result="Note content"
+        />
+      );
+      expect(screen.getByText(/Read MyNote.md/)).toBeTruthy();
+    });
+  });
+
+  describe("result formatting", () => {
+    it("should format and display result in collapsible content", () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={false} result="Success result" />);
+
+      const content = screen.getByTestId("collapsible-content");
+      expect(content).toBeTruthy();
+      expect(content.textContent).toContain("Success result");
+    });
+
+    it("should handle very long results with truncation message", () => {
+      const longResult = "a".repeat(6000); // Exceeds MAX_DISPLAY_CHARS (5000)
+      render(<ToolCallBanner {...defaultProps} isExecuting={false} result={longResult} />);
+
+      const content = screen.getByTestId("collapsible-content");
+      expect(content.textContent).toMatch(/returned 6,000 characters.*preserved in chat history/);
+    });
+
+    it("should not show result while executing", () => {
+      render(<ToolCallBanner {...defaultProps} isExecuting={true} result={null} />);
+
+      const content = screen.getByTestId("collapsible-content");
+      expect(content.textContent).toContain("No result available");
+    });
+  });
+
+  describe("accept/reject buttons", () => {
+    it("should not show accept/reject buttons when executing", () => {
+      const onAccept = jest.fn();
+      const onReject = jest.fn();
+
+      render(
+        <ToolCallBanner
+          {...defaultProps}
+          isExecuting={true}
+          result={null}
+          onAccept={onAccept}
+          onReject={onReject}
+        />
+      );
+
+      // Buttons should not be visible during execution
+      expect(screen.queryByTitle("Accept")).toBeNull();
+      expect(screen.queryByTitle("Reject")).toBeNull();
+    });
+
+    it("should show accept/reject buttons when not executing with handlers", () => {
+      const onAccept = jest.fn();
+      const onReject = jest.fn();
+
+      render(
+        <ToolCallBanner
+          {...defaultProps}
+          isExecuting={false}
+          result="Success"
+          onAccept={onAccept}
+          onReject={onReject}
+        />
+      );
+
+      // Buttons should be visible when done
+      expect(screen.getByTitle("Accept")).toBeTruthy();
+      expect(screen.getByTitle("Reject")).toBeTruthy();
+    });
+
+    it("should not show buttons when executing=true but has result (actuallyExecuting=false)", () => {
+      const onAccept = jest.fn();
+      const onReject = jest.fn();
+
+      render(
+        <ToolCallBanner
+          {...defaultProps}
+          isExecuting={true}
+          result="Success"
+          onAccept={onAccept}
+          onReject={onReject}
+        />
+      );
+
+      // Buttons SHOULD be visible since actuallyExecuting=false
+      expect(screen.getByTitle("Accept")).toBeTruthy();
+      expect(screen.getByTitle("Reject")).toBeTruthy();
+    });
+  });
+});

src/components/chat-components/ToolCallBanner.tsx

@@ -73,8 +73,12 @@ export const ToolCallBanner: React.FC<ToolCallBannerProps> = ({
 
   const formattedResult = useMemo(() => formatToolResult(toolName, result), [toolName, result]);
 
+  // Defensive check: If we have a result, the tool is definitely done executing
+  // This prevents infinite rolling animation if marker update fails or is delayed
+  const actuallyExecuting = isExecuting && !result;
+
   // Don't allow expanding while executing
-  const canExpand = !isExecuting && formattedResult !== null;
+  const canExpand = !actuallyExecuting && formattedResult !== null;
 
   return (
     <Collapsible
@@ -87,11 +91,11 @@ export const ToolCallBanner: React.FC<ToolCallBannerProps> = ({
       <div
         className={cn(
           "tw-rounded-md tw-border tw-border-border tw-bg-secondary/50",
-          isExecuting && "tw-relative tw-overflow-hidden"
+          actuallyExecuting && "tw-relative tw-overflow-hidden"
         )}
       >
         {/* Shimmer effect overlay */}
-        {isExecuting && (
+        {actuallyExecuting && (
           <div className="tw-absolute tw-inset-0 tw-z-[1] tw-overflow-hidden">
             <div
               className="tw-absolute tw-inset-0 -tw-translate-x-full"
@@ -115,18 +119,18 @@ export const ToolCallBanner: React.FC<ToolCallBannerProps> = ({
             <span className="tw-text-base">{emoji}</span>
             <span className="tw-font-medium">
               {toolName === "readNote"
-                ? `${isExecuting ? "Reading" : "Read"} ${displayName}`
-                : `${isExecuting ? "Calling" : "Called"} ${displayName}`}
-              {isExecuting && toolName !== "readNote" && "..."}
+                ? `${actuallyExecuting ? "Reading" : "Read"} ${displayName}`
+                : `${actuallyExecuting ? "Calling" : "Called"} ${displayName}`}
+              {actuallyExecuting && toolName !== "readNote" && "..."}
             </span>
-            {isExecuting && confirmationMessage && (
+            {actuallyExecuting && confirmationMessage && (
               <span className="tw-text-xs tw-text-muted">• {confirmationMessage}...</span>
             )}
           </div>
 
           <div className="tw-flex tw-items-center tw-gap-2">
             {/* Future: Accept/Reject buttons */}
-            {!isExecuting && onAccept && onReject && (
+            {!actuallyExecuting && onAccept && onReject && (
               <>
                 <button
                   onClick={(e) => {