From a84534315ccceff557a33ab215ece5bef42e1e90 Mon Sep 17 00:00:00 2001 From: Thomas Kosiewski Date: Thu, 30 Apr 2026 14:08:59 +0200 Subject: [PATCH] feat(coderd/x/chatd/chatloop): add exclusive tool execution policy (#24619) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Introduce an **exclusive-tool execution policy** in `chatloop.Run()` so that a designated "planning-only" tool can never execute in the same batch as any other locally executed tool. This is the generic foundation for the advisor tool landed in the stacked PRs above. ## Motivation The advisor tool (stacked on top) is intentionally a pre-action planning step. It must be consulted *alone*, not interleaved with side-effecting tools, so the model has to reason strategically before committing to actions. Rather than building advisor-specific plumbing into `chatloop`, this PR adds a small, general-purpose policy hook that any future tool can opt into. ## Changes - `RunOptions.ExclusiveToolNames map[string]bool` declares which tool names must run exclusively within a single tool-execution batch. - `executeTools()` detects mixed batches: if any exclusive tool name appears next to any other locally executed tool, no tool runs. Instead, structured `ToolResultOutputContentError` entries are synthesized for every tool in the batch so the model can cleanly retry. - Deterministic, model-facing error copy: - Exclusive tool gets: "must be called by itself before action tools". - Sibling tools get: "skipped because `` must run alone". - New tests in `chatloop_test.go` cover: single exclusive batch runs normally, mixed batch returns policy errors and executes nothing, non-exclusive batches are untouched. ## Stack context This is **PR 1 of 6** in the advisor feature stack. ``` main └─ feat/advisor-01-chatloop-exclusive-policy ← this PR └─ feat/advisor-02-chatadvisor-pkg └─ feat/advisor-03-config-api ├─ feat/advisor-04-chatd-runtime │ └─ feat/advisor-05-chat-tool-renderer └─ feat/advisor-06-admin-settings-ui ``` ## Scope / non-goals - No advisor-specific code lives in this PR. - Zero behavior change when `ExclusiveToolNames` is empty (current default for every existing caller). ## Validation - `go test ./coderd/x/chatd/chatloop/... -run TestExclusive` - `make lint` ---
📋 Implementation Plan (shared across the advisor stack) # Plan: Add a Mux-style advisor tool to coder agents/chatd ## Outcome Add a first-class `advisor` tool to agent chats in `coderd/x/chatd` that feels native to Coder: - it is a built-in server-side tool, not an MCP/dynamic-tool workaround; - it performs a nested **tool-less** model call for strategic advice; - it is exposed only when eligible, and the prompt mentions it only when it is actually available; - it is treated as a **planning-only** tool so it does not run alongside action tools in the same batch; - it tracks usage/cost separately enough for operators to reason about it; - it has a minimally polished UI in the Agents page; - and it ships with explicit dogfooding evidence, including screenshots and repro videos. ## Design decisions to lock before coding 1. **Primary architecture:** native built-in tool in `chattool/`, backed by a small `chatadvisor` package. 2. **Nested model execution:** reuse chatd's existing model/provider stack for a one-step, tool-less advisor call rather than inventing a new provider pathway. 3. **Execution policy:** treat `advisor` as an exclusive/planning-only tool; mixed batches must return structured policy errors and force the model to retry cleanly. 4. **Availability:** initial rollout is for root agent chats only; disable for child/sub-agent chats until recursion/cost policy is proven. 5. **Prompt sync:** use one eligibility boolean to drive both tool registration and advisor guidance injection. 6. **Persistence/cost split:** MVP should keep advisor usage visible in result metadata and server metrics; only add DB schema if product/billing explicitly needs queryable advisor-specific cost. 7. **UI scope:** generic tool rendering is an acceptable temporary milestone during backend bring-up, but the release candidate should include a dedicated lightweight advisor renderer. ## Delivery model The work should be executed as coordinated workstreams with one integration owner and parallel contributors for low-conflict areas. The integration owner should own `coderd/x/chatd/chatd.go` because prompt assembly, tool registration, and model resolution all converge there. ## Detailed workstreams ### Repo evidence used for this plan
Mux reference and current chatd seams **Mux reference implementation** - `src/node/services/tools/advisor.ts` — native advisor tool implementation. - `src/common/constants/advisor.ts` — advisor prompt/constants and truncation policy. - `src/common/utils/tools/tools.ts` — conditional tool registration. - `src/node/services/streamContextBuilder.ts` — injects advisor guidance only when the tool is available. **Current chatd seams** - `coderd/x/chatd/chatd.go` - `processChat()` — tool assembly, prompt assembly, and chatloop invocation. - `resolveChatModel()` — current model/provider/key resolution seam. - `type Config struct` — server-level chatd configuration surface. - `coderd/x/chatd/chatloop/chatloop.go` - `Run()` — main streaming/model loop. - `executeTools()` — built-in tool execution/batching seam. - `coderd/x/chatd/chattool/` — built-in tool implementations. - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` — tool renderer dispatch. - `site/src/pages/AgentsPage/components/ChatConversation/messageParsing.ts` and `ConversationTimeline.tsx` — tool/result merge and rendering flow.
### Workstream map and ownership | Workstream | Primary owner | Main files | Can run in parallel? | Done when | |---|---|---|---|---| | 0. Integration + gating | Integration lead | `coderd/x/chatd/chatd.go` | No; central merge lane | Tool registration, prompt sync, and model selection are wired together | | 1. Advisor runtime + tool | Backend agent | new `coderd/x/chatd/chatadvisor/`, new `coderd/x/chatd/chattool/advisor.go` | Yes | Tool can perform a tool-less advisor call in memory and return structured results | | 2. Planning-only execution policy | Chatloop agent | `coderd/x/chatd/chatloop/chatloop.go`, related tests | Yes | Mixed `advisor` + action-tool batches are rejected cleanly and deterministically | | 3. Metrics/usage/config | Backend/telemetry agent | `chatd.go`, `chatloop/metrics.go`, optional config plumbing | Partially; coordinate with integration lead | Advisor usage is separately visible in metadata/metrics and limits are enforced | | 4. Frontend rendering | Frontend agent | `site/.../tools/Tool.tsx`, new `AdvisorTool.tsx`, stories | Yes after result schema stabilizes | Advisor renders as a readable card and story tests pass | | 5. Dogfood + QA evidence | QA agent | dev server, Storybook, dogfood output | After backend + UI are usable | Repro videos, screenshots, and a concise QA report exist | ### Parallelization rules - **Do not split `coderd/x/chatd/chatd.go` across multiple execution agents without an integration lead.** That file owns prompt building, tool registration, model resolution, and cost persistence. - Workstreams 1 and 2 can be developed in parallel and then stacked onto the integration branch. - Workstream 4 should begin once the backend result schema is agreed on, even if the backend is still behind a feature flag. - Any agent that needs to re-check Mux behavior should clone `coder/mux` into a temporary directory (for example, `$(mktemp -d)/mux`) and inspect it read-only; do not vendor or copy code from Mux directly. ## Phase 0 — Preflight and guardrails ### Goals - Align the team on the smallest shippable architecture. - Prevent scope creep into MCP/dynamic-tool/sub-agent variants. - Decide upfront what is MVP vs. follow-up. ### Tasks 1. **Confirm the MVP boundary.** - Ship a built-in advisor tool first. - Do **not** make MCP, dynamic tools, or sub-agents the primary implementation. - Do **not** add transient streaming phases in the first backend PR unless they fall out almost for free. 2. **Confirm local workflow hygiene before coding.** - Ensure the repo is using the project git hooks from `scripts/githooks`. - Do not bypass hooks with `--no-verify`. - Use `./scripts/develop.sh` for the full dev server rather than manual build/run commands. 3. **Lock the model-selection policy.** - **Recommended MVP:** advisor uses the same resolved provider/model/cost config as the current chat, with advisor-specific max-output and usage caps. - **Follow-up only if required:** add a separate `AdvisorModelConfigID`-style override that resolves through the existing `configCache`/model-config path. Do not invent a new free-form `provider:model` parser if chatd already stores provider/model separately. 4. **Lock the persistence policy.** - **Recommended MVP:** no DB migration. Persist advisor-visible metadata in the tool result and record separate metrics in memory/Prometheus. - **Only if product/billing explicitly asks for queryable advisor cost:** add a later DB migration or usage table, following the normal `queries/*.sql` + `make gen` workflow. 5. **Create an execution ADR note in the work item or tracking doc.** - Capture: built-in tool, tool-less nested call, root-chat-only rollout, exclusive execution policy, MVP no-DB-migration default. ### Quality gate - Everyone on the team can state the same answers to these questions: - Is advisor a built-in tool? **Yes.** - Can advisor run with action tools in the same batch? **No.** - Does advisor get tools of its own? **No.** - Is a DB migration required for MVP? **No, unless billing insists.** ## Phase 1 — Build the advisor runtime and tool wrapper ### Goals Create the core advisor implementation in a way that is easy to test and keeps `chattool/` thin. ### Files to add - `coderd/x/chatd/chatadvisor/types.go` - `coderd/x/chatd/chatadvisor/guidance.go` - `coderd/x/chatd/chatadvisor/handoff.go` - `coderd/x/chatd/chatadvisor/runtime.go` - `coderd/x/chatd/chatadvisor/runner.go` - `coderd/x/chatd/chattool/advisor.go` ### Responsibilities by file 1. **`types.go`** - Define the input/result schema used by the tool and UI. - Keep the result shape close to Mux so the UI and model both have predictable cases. - Recommended result variants: - `advice` - `limit_reached` - `error` Recommended shape: ```go type AdvisorArgs struct { Question string `json:"question"` } type AdvisorResult struct { Type string `json:"type"` Advice string `json:"advice,omitempty"` Error string `json:"error,omitempty"` AdvisorModel string `json:"advisor_model,omitempty"` RemainingUses int `json:"remaining_uses,omitempty"` Usage *AdvisorUsageResult `json:"usage,omitempty"` } ``` 2. **`guidance.go`** - Hold two strings: - the nested advisor system prompt; - the parent-agent guidance block to inject into the outer system prompt. - The nested advisor prompt must say, in plain language: - you are advising the parent agent; - you do not address the end user directly; - you do not claim actions happened; - you return concise strategic guidance and tradeoffs. 3. **`runtime.go`** - Define the per-run runtime state. - Recommended fields: - resolved model + model config; - provider keys/options reused from the outer chat; - `MaxUsesPerRun`; - `MaxOutputTokens`; - atomic/current call counter; - callback(s) to obtain the current prompt snapshot and current-step snapshot; - optional metrics/usage hook. - Add fail-fast validation for impossible config: nil model, non-positive limits, empty prompt builders, etc. 4. **`handoff.go`** - Build the advisor handoff message from: - the explicit question; - the exact prompt/messages the parent model just used; - the current step's text/reasoning snapshot, if available; - the most recent relevant tool outputs, if they are already in the prompt snapshot. - **Important:** use the already-prepared outer prompt tail, not a fresh DB reload. That keeps the advisor aligned with compaction and the exact context the outer model saw. - Apply hard truncation budgets with recent-context bias. 5. **`runner.go`** - Execute the nested advisor call. - **Recommended implementation:** call `chatloop.Run()` in an in-memory, one-step mode: - `Tools: nil` - `ProviderTools: nil` - `MaxSteps: 1` - `PersistStep`: capture the assistant output in memory instead of writing DB rows - Reuse the existing provider/model/cost path instead of building a second provider runner. - Assert that no tool definitions are passed to the nested call. 6. **`chattool/advisor.go`** - Keep this file thin and consistent with other built-ins. - Responsibilities: - decode `AdvisorArgs`; - validate `Question` is non-empty and bounded; - call the `chatadvisor` runner; - return a structured tool response. ### Defensive programming requirements - Assert `Question` is non-empty after trimming. - Assert runtime limits are positive. - Assert the nested advisor call runs with zero tools/provider tools. - Assert `AdvisorResult.Type` is one of the known variants before returning. - Assert remaining uses never goes negative. ### Acceptance criteria - A unit test can call the advisor tool with a fake model and receive a stable `advice` result. - The nested advisor call is impossible to run with tools accidentally attached. - The core logic lives in `chatadvisor/`, not embedded inside `chatd.go`. ## Phase 2 — Wire advisor into chatd and keep prompt/tool availability in sync ### Goals Register the tool in the right place, expose it only when eligible, and inject system guidance only when the tool is present. ### Files to modify - `coderd/x/chatd/chatd.go` - optionally a small helper file if `chatd.go` becomes too crowded ### Tasks 1. **Compute one eligibility boolean in `processChat()`.** Recommended inputs: - server-level advisor enabled flag; - root chat only (`chat.ParentChatID == uuid.Nil` or equivalent existing root/child check); - a usable resolved model/provider exists; - optional experiment/workspace/org gate if product wants staged rollout. 2. **Create the runtime once per outer chat run.** - Use the model/config/keys resolved by `resolveChatModel()`. - Reuse provider options from the current chat's `ChatModelCallConfig`. - Set `MaxUsesPerRun` and `MaxOutputTokens` from advisor config defaults. 3. **Register the tool in the built-in tool block.** - Insert after the skill tools and before MCP tools in `processChat()`. - Record `builtinToolNames["advisor"] = true` so metrics stay bounded. 4. **Inject advisor guidance into the outer system prompt using the same boolean.** - Use `chatprompt.InsertSystem()` in the same prompt assembly path that already injects user/system instructions. - Place the block near the existing instruction insertion, before plan-path/skill context blocks. - Wrap the guidance in an explicit tag like `` so it is easy to spot in tests and future refactors. 5. **Keep advisor out of child chats for the first release.** - That avoids recursion/cost blowups with `spawn_agent` / `wait_agent` flows. - Document this explicitly in the rollout notes and tests. ### Acceptance criteria - If advisor is disabled, neither the tool nor the prompt guidance appears. - If advisor is enabled, both the tool and the prompt guidance appear. - Root chats can use advisor; child chats cannot. - Built-in tool names include `advisor` so metrics do not collapse it into the generic `mcp` label. ## Phase 3 — Enforce planning-only execution policy in `chatloop` ### Goals Prevent the model from calling `advisor` and action tools in the same execution batch. ### Files to modify - `coderd/x/chatd/chatloop/chatloop.go` - related chatloop tests ### Recommended implementation Keep the MVP small; do **not** build a general policy engine yet. 1. Add a minimal field to `chatloop.RunOptions`, for example: ```go ExclusiveToolName *string ``` 2. In `Run()` / `executeTools()`, detect the case where the exclusive tool appears in the same local-tool batch as any other locally executed tool. 3. When that happens, synthesize structured tool-result errors for the affected calls instead of executing anything in the batch. - `advisor` should receive a clear error like: _advisor must be called by itself before action tools_. - The sibling action tools should receive a paired policy error like: _this tool was skipped because advisor must run alone_. 4. Let the outer model see those tool errors and retry cleanly. - This is simpler and safer than partial execution or hidden deferral. - It preserves deterministic transcript history for debugging. 5. Pass the just-finished step snapshot into the tool execution context. - The advisor runtime should be able to see the current step's text/reasoning content, because that is often the best hint about what the outer model is trying to decide. ### Why this is the right fit - It matches the intended semantics: advisor is consulted **before** taking action. - It avoids subtle race conditions caused by concurrent built-in tool execution. - It keeps the behavior easy to test with fake models. ### Acceptance criteria - A model-emitted batch containing only `advisor` succeeds. - A model-emitted batch containing `advisor` plus any other locally executed tool returns deterministic policy errors and executes nothing. - Non-advisor tool execution stays unchanged for normal chats. ## Phase 4 — Usage limits, metrics, and configuration ### Goals Make advisor safe to operate without over-designing billing/storage in the first release. ### Files to modify - `coderd/x/chatd/chatd.go` - `coderd/x/chatd/chatloop/metrics.go` as needed - `coderd/x/chatd/chatd.go` `Config` struct and constructor path - optional follow-up config/db files only if a separate advisor model or persistent billing is required ### Tasks 1. **Add explicit server config knobs for MVP.** Recommended fields on `chatd.Config` or a nested advisor config struct: - `AdvisorEnabled bool` - `AdvisorMaxUsesPerRun int` - `AdvisorMaxOutputTokens int64` 2. **Track usage per outer run.** - Reset the counter for each `processChat()` invocation. - Return `remaining_uses` in the tool result. - Return `limit_reached` when the cap is exhausted. 3. **Expose advisor usage metadata in the tool result.** - Include model name and token/cost summary if available. - Use the same `callConfig.Cost` calculation path as the outer chat for MVP if advisor reuses the same model. 4. **Record server-side metrics.** - Count advisor invocations, failures, and latency. - Ensure they show up under the built-in tool label `advisor`. 5. **Optional decision gate: separate advisor model.** - If product insists on a stronger/different advisor model, add a follow-up config hook that resolves another existing chat model config through the same `configCache` path. - Keep that out of the first landing PR unless it is required for acceptance. 6. **Optional decision gate: queryable advisor cost.** - If this becomes required, spin a follow-up DB task: - update `coderd/database/queries/*.sql`; - add migration files; - run `make gen`; - update audit mappings if a new auditable type/field is introduced. ### Acceptance criteria - Advisor calls are capped per outer run. - Limit exhaustion is user-visible in the tool result. - Metrics distinguish advisor calls from other built-in tools. - MVP does not require a schema migration unless explicitly approved. ## Phase 5 — Frontend rendering and Storybook coverage ### Goals Make advisor feel intentional in the Agents UI without blocking the backend on fancy streaming UI. ### Files to modify - `site/src/pages/AgentsPage/components/ChatElements/tools/Tool.tsx` - new `site/src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.tsx` - Storybook story file(s) in the same tools directory ### Delivery strategy 1. **Intermediate milestone during backend bring-up:** rely on the existing generic tool renderer if needed. - This is acceptable only as a short-lived integration checkpoint. 2. **Release milestone:** add a dedicated lightweight `AdvisorTool` renderer. - Reuse existing primitives: - `ToolCollapsible` - `ToolIcon` - `Response` for markdown/prose rendering - `ScrollArea` if the advice can be long - Keep styling light and consistent with the Agents page. - Do not add unnecessary React memoization in `site/src/pages/AgentsPage/`; that area is already React-Compiler aware. 3. **Render the structured result states cleanly.** - `advice` — readable prose/markdown with optional metadata footer. - `limit_reached` — warning-style message. - `error` — error state with visible fallback text. - `running` — existing tool loading state/spinner is enough for MVP. 4. **Add Storybook coverage instead of ad-hoc component tests.** Recommended stories: - successful advice; - running/loading; - limit reached; - error. 5. **Keep the UI contract narrow.** - Prefer one text field like `advice` plus small metadata rather than a deeply nested schema. - That keeps the UI resilient to prompt iteration. ### Acceptance criteria - The advisor tool card renders readable content rather than raw quoted JSON in the final release branch. - Running, limit, and error states are visibly distinct. - Storybook stories and play assertions cover the new states. - Existing tool rendering flows remain unchanged. ## Phase 6 — Automated tests and validation gates ### Backend tests to add 1. **Advisor runtime/tool tests** - question validation; - tool-less nested execution assertion; - success result shaping; - limit-reached result shaping; - error result shaping. 2. **Prompt/gating tests in chatd** - advisor disabled ⇒ no tool, no guidance; - advisor enabled/root chat ⇒ tool + guidance; - child chat ⇒ advisor absent. 3. **Chatloop policy tests** - advisor alone runs; - advisor + action tool mixed batch returns deterministic policy errors; - non-advisor tools still execute normally. 4. **Usage/metrics tests** - per-run cap resets correctly; - builtin tool labeling includes `advisor`; - returned metadata includes model/usage summary when available. ### Frontend tests to add - Storybook `play()` assertions for the advisor renderer states. - Verify expand/collapse behavior and visible fallback text. - Verify the message timeline still renders adjacent tools correctly. ### Recommended command sequence Run these as the implementation matures, not only at the end: 1. Backend-focused gate after phases 1–4: - `make test RUN=TestAdvisor` - `make test RUN=TestChatloopAdvisor` - `make lint` 2. Frontend-focused gate after phase 5: - `pnpm test:storybook src/pages/AgentsPage/components/ChatElements/tools/AdvisorTool.stories.tsx` - `pnpm lint` - `pnpm format` 3. Final repo gate before handoff: - `make pre-commit` - run any additional targeted `make test RUN=...` selections covering touched chatd paths > Use the exact new test names the implementing agents create; the names above are recommended anchors, not existing tests. ## Dogfooding plan ### Principle Dogfood the change as a real agent feature, not just a unit-tested backend. Per the dogfood and `agent-browser` skills, the reviewer should get **watchable repro videos** plus screenshots that make the behavior obvious without reading logs. ### Required setup 1. Start the full dev environment with: - `./scripts/develop.sh` 2. If the frontend renderer changes, also start Storybook from `site/` with: - `pnpm storybook --no-open` 3. Use `agent-browser` directly — **never `npx agent-browser`**. 4. Use named browser sessions and an output folder such as: - `./dogfood-output/advisor/` - with subfolders `screenshots/` and `videos/` ### Evidence protocol For every interactive scenario below: 1. Start video recording **before** the action. 2. Capture step-by-step screenshots at human pace. 3. Capture one annotated screenshot of the final state. 4. Stop the recording. 5. Note the exact pass/fail observation in the QA report. For static UI states (for example Storybook error/limit cards), an annotated screenshot is sufficient; video is optional but still encouraged by this project’s review preference. ### Dogfood scenarios #### Scenario A — Happy path in the real Agents UI **Goal:** prove that a root agent chat can invoke advisor and produce a readable recommendation before taking further action. Steps: 1. Open the Agents page with an advisor-enabled root chat. 2. Start a repro video. 3. Send a prompt that should reasonably trigger strategic planning, such as an architecture or multi-tradeoff question. 4. Capture screenshots of: - the prompt before send; - the running advisor state; - the completed advisor card and the assistant’s follow-up response. 5. Stop recording. Pass criteria: - advisor appears in the timeline; - the rendered result is readable; - the assistant can continue after consuming the advisor output. #### Scenario B — Advisor unavailable path **Goal:** prove the feature is truly gated. Suggested variants (at least one is required, both are better): - feature flag/config off; - child/sub-agent chat. Evidence: - annotated screenshot of the chat/tool state showing advisor is absent; - short video if toggling the gate live is part of the repro. Pass criteria: - no advisor tool is available; - no advisor-specific prompt behavior leaks through. #### Scenario C — UI states in Storybook **Goal:** prove the renderer handles non-happy states cleanly. Required story states: - success/advice; - running; - limit reached; - error. Evidence: - one screenshot per state; - at least one short video showing collapse/expand behavior. Pass criteria: - success renders readable advice; - limit/error have visible fallback text; - the component behaves like the other tool cards. #### Scenario D — Regression sweep of nearby tools **Goal:** ensure advisor does not break the surrounding chat timeline. Check at minimum: - another existing built-in tool still renders correctly near advisor; - sub-agent/tool cards still expand/collapse normally; - no obvious console errors appear in the Agents page during the advisor flow. Evidence: - screenshots of adjacent tool cards; - console/error capture if anything suspicious appears. ### `agent-browser` usage notes for the QA agent - Prefer `agent-browser batch` for 2+ sequential commands when no intermediate parsing is needed. - Use `snapshot -i` to discover interactive refs. - Re-snapshot after navigation or major DOM changes. - Avoid `wait --load networkidle` unless the page is known to go idle; prefer explicit element/text waits or short fixed waits. - Record videos at human pace and include pauses that a reviewer can follow. ## Rollout plan ### Initial rollout - Gate behind a server-side advisor-enabled flag. - Enable only for selected internal/root agent chats first. - Watch metrics for: - invocation count; - failure rate; - latency; - obvious retry loops. ### Expansion conditions Expand beyond the initial rollout only after the following are true: - mixed-batch policy behavior is stable; - cost impact is understood; - frontend UX is readable in production-like dogfood; - no recursion surprises have appeared with sub-agent flows. ### Explicit non-goals for the first release - advisor inside child/sub-agent chats; - provider-agnostic streaming phase UI; - MCP-based external advisor implementation; - mandatory DB-backed advisor cost reporting. ## Final acceptance checklist - [ ] `advisor` is a built-in chatd tool, not an MCP/dynamic-tool substitute. - [ ] The nested advisor call is tool-less and bounded to one in-memory step. - [ ] One eligibility boolean controls both tool registration and prompt guidance injection. - [ ] Root chats can use advisor; child chats cannot in the initial rollout. - [ ] Mixed advisor/action batches produce deterministic policy errors instead of partial execution. - [ ] Per-run usage caps and limit-reached behavior work. - [ ] Advisor usage is visible in metadata/metrics without forcing a DB migration for MVP. - [ ] The Agents UI has a readable advisor card and Storybook coverage. - [ ] Dogfooding produced screenshots and repro videos for the required scenarios. - [ ] Validation commands (`make lint`, targeted `make test`, Storybook tests, `make pre-commit`) passed before handoff. ## Suggested PR split 1. **PR 1 — Backend foundation** - `chatadvisor/` package - `chattool/advisor.go` - `chatloop` exclusive policy - chatd gating/prompt sync - backend tests 2. **PR 2 — Frontend + QA** - advisor renderer - stories/play assertions - dogfood artifacts and QA notes 3. **PR 3 — Optional follow-ups only if demanded by stakeholders** - separate advisor model override - persistent advisor billing/queryability - transient phase-stream UX
--- _Generated with [`mux`](https://github.com/coder/mux) • Model: `anthropic:claude-opus-4-7` • Thinking: `max`_ --- coderd/x/chatd/chatloop/chatloop.go | 375 +++++++++++---- coderd/x/chatd/chatloop/chatloop_test.go | 586 +++++++++++++++++++++++ 2 files changed, 865 insertions(+), 96 deletions(-) diff --git a/coderd/x/chatd/chatloop/chatloop.go b/coderd/x/chatd/chatloop/chatloop.go index 4e367e818b..34de2e4e74 100644 --- a/coderd/x/chatd/chatloop/chatloop.go +++ b/coderd/x/chatd/chatloop/chatloop.go @@ -128,6 +128,11 @@ type RunOptions struct { // the current step. This is used for plan turns where // propose_plan should terminate the run on success. StopAfterTools map[string]struct{} + // ExclusiveToolNames lists tool names that must be called + // alone in a batch. When any exclusive tool appears + // alongside other locally-executed tools, every tool in the + // batch receives a policy error and nothing executes. + ExclusiveToolNames map[string]bool // ModelConfig holds per-call LLM parameters (temperature, // max tokens, etc.) read from the chat model configuration. @@ -477,102 +482,10 @@ func Run(ctx context.Context, opts RunOptions) error { // blocks into separate database messages by role. var toolResults []fantasy.ToolResultContent if result.shouldContinue { - // Check for context cancellation before starting - // tool execution. If the chat was interrupted - // between stream completion and here, persist - // what we have and bail out. - if ctx.Err() != nil { - if errors.Is(context.Cause(ctx), ErrInterrupted) { - persistInterruptedStep(ctx, opts, &result) - return ErrInterrupted - } - return ctx.Err() - } - - // Partition tool calls into built-in and dynamic. - var builtinCalls, dynamicCalls []fantasy.ToolCallContent - if len(opts.DynamicToolNames) > 0 { - for _, tc := range result.toolCalls { - if opts.DynamicToolNames[tc.ToolName] { - dynamicCalls = append(dynamicCalls, tc) - } else { - builtinCalls = append(builtinCalls, tc) - } - } - } else { - builtinCalls = result.toolCalls - } - - // Execute only built-in tools. - toolResults = executeTools(ctx, opts.Tools, opts.ActiveTools, opts.ProviderTools, builtinCalls, opts.Metrics, opts.Logger, provider, modelName, opts.BuiltinToolNames, func(tr fantasy.ToolResultContent, completedAt time.Time) { - recordToolResultTimestamp(&result, tr.ToolCallID, completedAt) - publishToolAttachments(ctx, opts.Logger, tr, completedAt, publishMessagePart) - ssePart := chatprompt.PartFromContentWithLogger(ctx, opts.Logger, tr) - ssePart.CreatedAt = &completedAt - publishMessagePart(codersdk.ChatMessageRoleTool, ssePart) - }) - for _, tr := range toolResults { - result.content = append(result.content, tr) - } - - // If dynamic tools were called, persist what we - // have (assistant + built-in results) and exit so - // the caller can execute them externally. - if len(dynamicCalls) > 0 { - pending := make([]PendingToolCall, 0, len(dynamicCalls)) - for _, dc := range dynamicCalls { - pending = append(pending, PendingToolCall{ - ToolCallID: dc.ToolCallID, - ToolName: dc.ToolName, - Args: dc.Input, - }) - } - - contextLimit := extractContextLimitWithFallback( - result.providerMetadata, - opts.ContextLimitFallback, - ) - - result.content = chatsanitize.SanitizeAnthropicProviderToolStepContent( - ctx, opts.Logger, provider, modelName, - "dynamic_tool_persist", step, result.finishReason, result.content, - ) - if len(result.content) == 0 && len(pending) == 0 { - tryCompactOnExit(ctx, opts, result.usage, result.providerMetadata) - return ErrDynamicToolCall - } - - if err := opts.PersistStep(ctx, PersistedStep{ - Content: result.content, - Usage: result.usage, - ContextLimit: contextLimit, - ProviderResponseID: extractOpenAIResponseIDIfStored(opts.ProviderOptions, result.providerMetadata), - Runtime: time.Since(stepStart), - PendingDynamicToolCalls: pending, - }); err != nil { - if errors.Is(err, ErrInterrupted) { - persistInterruptedStep(ctx, opts, &result) - return ErrInterrupted - } - return xerrors.Errorf("persist step: %w", err) - } - - tryCompactOnExit(ctx, opts, result.usage, result.providerMetadata) - - return ErrDynamicToolCall - } - - // Check for interruption after tool execution. - // Tools that were canceled mid-flight produce error - // results via ctx cancellation. Persist the full - // step (assistant blocks + tool results) through - // the interrupt-safe path so nothing is lost. - if ctx.Err() != nil { - if errors.Is(context.Cause(ctx), ErrInterrupted) { - persistInterruptedStep(ctx, opts, &result) - return ErrInterrupted - } - return ctx.Err() + var err error + toolResults, err = executeToolsForStep(ctx, opts, &result, provider, modelName, step, stepStart, publishMessagePart) + if err != nil { + return err } } // Extract context limit from provider metadata. @@ -1154,6 +1067,276 @@ func executeTools( return results } +// executeToolsForStep runs the tool-execution phase of a single +// chatloop step. It enforces the exclusive-tool policy, partitions +// built-in versus dynamic tool calls, dispatches built-in tools, and +// when dynamic tool calls are present persists the step and returns +// ErrDynamicToolCall so the caller can execute them externally. +// Returns the tool results to append to the step, or an error that the +// caller must propagate (ErrInterrupted, ErrDynamicToolCall, ctx.Err(), +// or a persistence failure). +func executeToolsForStep( + ctx context.Context, + opts RunOptions, + result *stepResult, + provider, modelName string, + step int, + stepStart time.Time, + publishMessagePart func(codersdk.ChatMessageRole, codersdk.ChatMessagePart), +) ([]fantasy.ToolResultContent, error) { + // Check for context cancellation before starting tool + // execution. If the chat was interrupted between stream + // completion and here, persist what we have and bail out. + if ctx.Err() != nil { + if errors.Is(context.Cause(ctx), ErrInterrupted) { + persistInterruptedStep(ctx, opts, result) + return nil, ErrInterrupted + } + return nil, ctx.Err() + } + + // Enforce exclusivity across ALL locally-executable tool + // calls (both built-in and dynamic) before partitioning. + // Checking only the built-in partition would let the model + // bypass the policy by mixing an exclusive tool with a + // dynamic tool: the exclusive tool would still run and the + // dynamic call would still be handed to the caller for + // external execution, breaking the planning-only contract. + localCandidates := make([]fantasy.ToolCallContent, 0, len(result.toolCalls)) + for _, tc := range result.toolCalls { + if !tc.ProviderExecuted { + localCandidates = append(localCandidates, tc) + } + } + policyResults, exclusiveViolation := applyExclusiveToolPolicy( + localCandidates, + opts.ExclusiveToolNames, + opts.Metrics, + provider, + modelName, + ) + if exclusiveViolation { + now := dbtime.Now() + for _, tr := range policyResults { + recordToolResultTimestamp(result, tr.ToolCallID, now) + publishToolAttachments(ctx, opts.Logger, tr, now, publishMessagePart) + ssePart := chatprompt.PartFromContentWithLogger(ctx, opts.Logger, tr) + ssePart.CreatedAt = &now + publishMessagePart(codersdk.ChatMessageRoleTool, ssePart) + } + for _, tr := range policyResults { + result.content = append(result.content, tr) + } + // Mirror the post-execution interruption check used by the + // non-policy path: if the chat was interrupted while we + // synthesized policy errors, route through + // persistInterruptedStep so the synthesized results are not + // dropped when the regular PersistStep path fails on a + // canceled context. + if ctx.Err() != nil { + if errors.Is(context.Cause(ctx), ErrInterrupted) { + persistInterruptedStep(ctx, opts, result) + return nil, ErrInterrupted + } + return nil, ctx.Err() + } + // Fall through to the normal persistence path so the loop + // continues with error results that the model can observe + // and retry. Skip partitioning, execution, and + // pending-dynamic persistence. + return policyResults, nil + } + + // Partition tool calls into built-in and dynamic. + var builtinCalls, dynamicCalls []fantasy.ToolCallContent + if len(opts.DynamicToolNames) > 0 { + for _, tc := range result.toolCalls { + if opts.DynamicToolNames[tc.ToolName] { + dynamicCalls = append(dynamicCalls, tc) + } else { + builtinCalls = append(builtinCalls, tc) + } + } + } else { + builtinCalls = result.toolCalls + } + + // Execute only built-in tools. + toolResults := executeTools(ctx, opts.Tools, opts.ActiveTools, opts.ProviderTools, builtinCalls, opts.Metrics, opts.Logger, provider, modelName, opts.BuiltinToolNames, func(tr fantasy.ToolResultContent, completedAt time.Time) { + recordToolResultTimestamp(result, tr.ToolCallID, completedAt) + publishToolAttachments(ctx, opts.Logger, tr, completedAt, publishMessagePart) + ssePart := chatprompt.PartFromContentWithLogger(ctx, opts.Logger, tr) + ssePart.CreatedAt = &completedAt + publishMessagePart(codersdk.ChatMessageRoleTool, ssePart) + }) + for _, tr := range toolResults { + result.content = append(result.content, tr) + } + + // If dynamic tools were called, persist what we have + // (assistant + built-in results) and exit so the caller can + // execute them externally. + if len(dynamicCalls) > 0 { + // Strip Anthropic provider-executed tool calls without + // matching results before persisting so the action-required + // step does not carry a malformed tool-call history into + // downstream provider requests. + result.content = chatsanitize.SanitizeAnthropicProviderToolStepContent( + ctx, opts.Logger, provider, modelName, + "dynamic_tool_persist", step, result.finishReason, result.content, + ) + if err := persistPendingDynamicStep(ctx, opts, result, stepStart, dynamicCalls); err != nil { + return nil, err + } + tryCompactOnExit(ctx, opts, result.usage, result.providerMetadata) + return nil, ErrDynamicToolCall + } + + // Check for interruption after tool execution. Tools that + // were canceled mid-flight produce error results via ctx + // cancellation. Persist the full step (assistant blocks + + // tool results) through the interrupt-safe path so nothing + // is lost. + if ctx.Err() != nil { + if errors.Is(context.Cause(ctx), ErrInterrupted) { + persistInterruptedStep(ctx, opts, result) + return nil, ErrInterrupted + } + return nil, ctx.Err() + } + + return toolResults, nil +} + +// persistPendingDynamicStep persists a step that has pending dynamic +// tool calls awaiting external execution. Returns ErrInterrupted when +// persistence fails because the chat was interrupted. +func persistPendingDynamicStep( + ctx context.Context, + opts RunOptions, + result *stepResult, + stepStart time.Time, + dynamicCalls []fantasy.ToolCallContent, +) error { + pending := make([]PendingToolCall, 0, len(dynamicCalls)) + for _, dc := range dynamicCalls { + pending = append(pending, PendingToolCall{ + ToolCallID: dc.ToolCallID, + ToolName: dc.ToolName, + Args: dc.Input, + }) + } + + contextLimit := extractContextLimitWithFallback(result.providerMetadata, opts.ContextLimitFallback) + + if err := opts.PersistStep(ctx, PersistedStep{ + Content: result.content, + Usage: result.usage, + ContextLimit: contextLimit, + ProviderResponseID: extractOpenAIResponseIDIfStored(opts.ProviderOptions, result.providerMetadata), + Runtime: time.Since(stepStart), + PendingDynamicToolCalls: pending, + }); err != nil { + if errors.Is(err, ErrInterrupted) { + persistInterruptedStep(ctx, opts, result) + return ErrInterrupted + } + return xerrors.Errorf("persist step: %w", err) + } + return nil +} + +// applyExclusiveToolPolicy checks whether toolCalls violate the +// exclusive-tool policy declared by exclusiveToolNames. When a +// violation is detected it synthesizes deterministic policy-error +// results for every tool call and records size/error metrics so the +// exclusivity failure mode is visible to operators. Returns +// (results, true) on violation; (nil, false) otherwise. +func applyExclusiveToolPolicy( + toolCalls []fantasy.ToolCallContent, + exclusiveToolNames map[string]bool, + metrics *Metrics, + provider, model string, +) ([]fantasy.ToolResultContent, bool) { + blockingToolName, ok := firstExclusiveToolName(toolCalls, exclusiveToolNames) + if !ok { + return nil, false + } + results := exclusiveToolPolicyResults(toolCalls, exclusiveToolNames, blockingToolName) + for _, tr := range results { + recordToolResultMetrics(metrics, provider, model, tr) + } + return results, true +} + +// recordToolResultMetrics observes tool result size and increments +// tool_errors_total when the result carries an error output. Mirrors +// the metric-recording defer in executeSingleTool so that synthetic +// results (e.g. exclusive-tool policy errors) contribute to operator +// visibility. +func recordToolResultMetrics(metrics *Metrics, provider, model string, tr fantasy.ToolResultContent) { + if metrics == nil { + return + } + label := tr.ToolName + if label == "" { + label = "unknown" + } + metrics.ToolResultSizeBytes.WithLabelValues(provider, model, label).Observe( + float64(ToolResultSize(tr)), + ) + if _, ok := tr.Result.(fantasy.ToolResultOutputContentError); ok { + metrics.RecordToolError(provider, model, label) + } +} + +func firstExclusiveToolName( + toolCalls []fantasy.ToolCallContent, + exclusiveToolNames map[string]bool, +) (string, bool) { + if len(toolCalls) <= 1 || len(exclusiveToolNames) == 0 { + return "", false + } + + for _, tc := range toolCalls { + if exclusiveToolNames[tc.ToolName] { + return tc.ToolName, true + } + } + + return "", false +} + +func exclusiveToolPolicyResults( + toolCalls []fantasy.ToolCallContent, + exclusiveToolNames map[string]bool, + blockingToolName string, +) []fantasy.ToolResultContent { + results := make([]fantasy.ToolResultContent, len(toolCalls)) + for i, tc := range toolCalls { + message := exclusiveToolSkippedErrorMessage(blockingToolName) + if exclusiveToolNames[tc.ToolName] { + message = exclusiveToolMustRunAloneErrorMessage(tc.ToolName) + } + results[i] = fantasy.ToolResultContent{ + ToolCallID: tc.ToolCallID, + ToolName: tc.ToolName, + Result: fantasy.ToolResultOutputContentError{ + Error: xerrors.New(message), + }, + } + } + return results +} + +func exclusiveToolMustRunAloneErrorMessage(toolName string) string { + return toolName + " must be called alone, without other tools in the same batch. Retry with only the " + toolName + " call." +} + +func exclusiveToolSkippedErrorMessage(toolName string) string { + return "this tool was skipped because " + toolName + " must run alone in its batch. Retry your tool calls without " + toolName + ", or call " + toolName + " separately first." +} + // executeSingleTool executes one tool call and converts the // response into a ToolResultContent. func executeSingleTool( diff --git a/coderd/x/chatd/chatloop/chatloop_test.go b/coderd/x/chatd/chatloop/chatloop_test.go index 08489773ce..3b3a4483ed 100644 --- a/coderd/x/chatd/chatloop/chatloop_test.go +++ b/coderd/x/chatd/chatloop/chatloop_test.go @@ -15,6 +15,7 @@ import ( "charm.land/fantasy" fantasyanthropic "charm.land/fantasy/providers/anthropic" "github.com/prometheus/client_golang/prometheus" + promtestutil "github.com/prometheus/client_golang/prometheus/testutil" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/xerrors" @@ -1079,6 +1080,19 @@ func TestRun_InterruptedStepPersistsSyntheticToolResult(t *testing.T) { "interrupted tool should have no call timestamp (never reached StreamPartTypeToolCall)") } +func requireToolResultErrorMessage( + t *testing.T, + result fantasy.ToolResultContent, + expected string, +) { + t.Helper() + + output, ok := result.Result.(fantasy.ToolResultOutputContentError) + require.Truef(t, ok, "expected error tool result, got %T", result.Result) + require.Error(t, output.Error) + require.Equal(t, expected, output.Error.Error()) +} + func streamFromParts(parts []fantasy.StreamPart) fantasy.StreamResponse { return iter.Seq[fantasy.StreamPart](func(yield func(fantasy.StreamPart) bool) { for _, part := range parts { @@ -1643,6 +1657,578 @@ func TestRun_ParallelToolExecutionTimestamps(t *testing.T) { "tc-2 tool-result timestamp must be >= tool-call timestamp") } +// TestRun_ExclusiveToolPolicyViolation exercises the full Run() -> +// executeToolsForStep() -> applyExclusiveToolPolicy() wiring. When an +// exclusive tool is called alongside other locally-executable tools, +// neither runner must fire and every call in the batch must receive a +// synthesized policy error that is both persisted and published via +// SSE. This guards against a regression where +// executeToolsForStep's policy call is accidentally removed: the +// pure-unit tests cover the policy function in isolation, but only +// this test catches a broken wiring path. +func TestRun_ExclusiveToolPolicyViolation(t *testing.T) { + t.Parallel() + + var advisorRuns atomic.Int32 + advisorTool := fantasy.NewAgentTool( + "advisor", + "returns strategic guidance", + func(context.Context, struct{}, fantasy.ToolCall) (fantasy.ToolResponse, error) { + advisorRuns.Add(1) + return fantasy.NewTextResponse(`{"status":"ok"}`), nil + }, + ) + var readRuns atomic.Int32 + readTool := fantasy.NewAgentTool( + "read_file", + "reads a file", + func(context.Context, struct{}, fantasy.ToolCall) (fantasy.ToolResponse, error) { + readRuns.Add(1) + return fantasy.NewTextResponse(`{"contents":"main"}`), nil + }, + ) + + var mu sync.Mutex + var streamCalls int + model := &chattest.FakeModel{ + ProviderName: "fake", + StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + mu.Lock() + step := streamCalls + streamCalls++ + mu.Unlock() + + if step == 0 { + // Step 0: model emits an illegal mixed batch. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeToolInputStart, ID: "advisor-1", ToolCallName: "advisor"}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "advisor-1", Delta: `{}`}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "advisor-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "advisor-1", + ToolCallName: "advisor", + ToolCallInput: `{}`, + }, + {Type: fantasy.StreamPartTypeToolInputStart, ID: "read-1", ToolCallName: "read_file"}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "read-1", Delta: `{"path":"main.go"}`}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "read-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "read-1", + ToolCallName: "read_file", + ToolCallInput: `{"path":"main.go"}`, + }, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonToolCalls}, + }), nil + } + // Step 1: the loop re-streams after tool results; end the run. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeTextStart, ID: "text-1"}, + {Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "ok, retrying"}, + {Type: fantasy.StreamPartTypeTextEnd, ID: "text-1"}, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonStop}, + }), nil + }, + } + + var persistedSteps []PersistedStep + var publishedToolParts []codersdk.ChatMessagePart + err := Run(context.Background(), RunOptions{ + Model: model, + Messages: []fantasy.Message{ + textMessage(fantasy.MessageRoleUser, "please advise and read"), + }, + Tools: []fantasy.AgentTool{advisorTool, readTool}, + ExclusiveToolNames: map[string]bool{"advisor": true}, + MaxSteps: 5, + PersistStep: func(_ context.Context, step PersistedStep) error { + persistedSteps = append(persistedSteps, step) + return nil + }, + PublishMessagePart: func(role codersdk.ChatMessageRole, part codersdk.ChatMessagePart) { + if role != codersdk.ChatMessageRoleTool { + return + } + publishedToolParts = append(publishedToolParts, part) + }, + }) + require.NoError(t, err) + + // Neither runner must have fired: the policy short-circuits + // before partitioning and execution. + require.Equal(t, int32(0), advisorRuns.Load(), + "advisor runner must not fire on mixed batches") + require.Equal(t, int32(0), readRuns.Load(), + "read_file runner must not fire on mixed batches") + + // Two steps: the mixed-batch step plus the follow-up stream. + require.Len(t, persistedSteps, 2) + firstStep := persistedSteps[0] + + advisorErr, ok := findToolResultByID(firstStep.Content, "advisor-1") + require.True(t, ok, "persisted step must contain the advisor policy result") + requireToolResultErrorMessage(t, advisorErr, + "advisor must be called alone, without other tools in the same batch. Retry with only the advisor call.") + + readErr, ok := findToolResultByID(firstStep.Content, "read-1") + require.True(t, ok, "persisted step must contain the read_file policy result") + requireToolResultErrorMessage(t, readErr, + "this tool was skipped because advisor must run alone in its batch. Retry your tool calls without advisor, or call advisor separately first.") + + // Policy-error results must be SSE-published so the client + // can render them immediately. Confirm both tool-result parts + // reached PublishMessagePart with a non-nil CreatedAt, which + // is the dbtime.Now() stamp the policy branch sets. + var sawAdvisorPart, sawReadPart bool + for _, part := range publishedToolParts { + switch part.ToolCallID { + case "advisor-1": + sawAdvisorPart = true + require.NotNil(t, part.CreatedAt, + "policy result SSE part must carry the dbtime.Now() timestamp") + case "read-1": + sawReadPart = true + require.NotNil(t, part.CreatedAt, + "policy result SSE part must carry the dbtime.Now() timestamp") + } + } + require.True(t, sawAdvisorPart, "advisor policy result must be SSE-published") + require.True(t, sawReadPart, "read_file policy result must be SSE-published") +} + +func findToolResultByID( + content []fantasy.Content, + toolCallID string, +) (fantasy.ToolResultContent, bool) { + for _, block := range content { + tr, ok := fantasy.AsContentType[fantasy.ToolResultContent](block) + if !ok { + continue + } + if tr.ToolCallID == toolCallID { + return tr, true + } + } + return fantasy.ToolResultContent{}, false +} + +func TestExclusiveToolPolicy_MixedBatchErrors(t *testing.T) { + t.Parallel() + + results, violated := applyExclusiveToolPolicy( + []fantasy.ToolCallContent{ + {ToolCallID: "advisor-1", ToolName: "advisor", Input: `{}`}, + {ToolCallID: "read-1", ToolName: "read_file", Input: `{"path":"main.go"}`}, + }, + map[string]bool{"advisor": true}, + NopMetrics(), + "fake", + "", + ) + + require.True(t, violated) + require.Len(t, results, 2) + require.Equal(t, "advisor-1", results[0].ToolCallID) + require.Equal(t, "read-1", results[1].ToolCallID) + requireToolResultErrorMessage( + t, + results[0], + "advisor must be called alone, without other tools in the same batch. Retry with only the advisor call.", + ) + requireToolResultErrorMessage( + t, + results[1], + "this tool was skipped because advisor must run alone in its batch. Retry your tool calls without advisor, or call advisor separately first.", + ) +} + +func TestApplyExclusiveToolPolicy_RecordsErrorMetrics(t *testing.T) { + t.Parallel() + + reg := prometheus.NewPedanticRegistry() + m := NewMetrics(reg) + + _, violated := applyExclusiveToolPolicy( + []fantasy.ToolCallContent{ + {ToolCallID: "advisor-1", ToolName: "advisor", Input: `{}`}, + {ToolCallID: "read-1", ToolName: "read_file", Input: `{"path":"main.go"}`}, + }, + map[string]bool{"advisor": true}, + m, + "fake", + "claude-test", + ) + require.True(t, violated) + + require.Equal(t, 1.0, promtestutil.ToFloat64( + m.ToolErrorsTotal.WithLabelValues("fake", "claude-test", "advisor"), + )) + require.Equal(t, 1.0, promtestutil.ToFloat64( + m.ToolErrorsTotal.WithLabelValues("fake", "claude-test", "read_file"), + )) +} + +func TestExclusiveToolPolicy_MultipleExclusive(t *testing.T) { + t.Parallel() + + results, violated := applyExclusiveToolPolicy( + []fantasy.ToolCallContent{ + {ToolCallID: "advisor-1", ToolName: "advisor", Input: `{}`}, + {ToolCallID: "advisor-2", ToolName: "advisor", Input: `{"mode":"second-opinion"}`}, + }, + map[string]bool{"advisor": true}, + NopMetrics(), + "fake", + "", + ) + + require.True(t, violated) + require.Len(t, results, 2) + requireToolResultErrorMessage( + t, + results[0], + "advisor must be called alone, without other tools in the same batch. Retry with only the advisor call.", + ) + requireToolResultErrorMessage( + t, + results[1], + "advisor must be called alone, without other tools in the same batch. Retry with only the advisor call.", + ) +} + +// TestRun_ExclusiveToolPolicyBlocksMixedWithDynamicTool guards the +// exclusive-over-dynamic bypass: the policy must run before the +// built-in vs dynamic partition. If a future refactor moves the +// policy check beneath the partition (so only built-in calls are +// inspected), an exclusive builtin mixed with a dynamic tool would +// still execute locally while the dynamic call is handed off via +// ErrDynamicToolCall, breaking the planning-only contract. +// +// This test has the model emit an exclusive builtin (advisor) +// alongside a dynamic tool (mcp_tool) in the same batch and asserts +// that Run does NOT exit with ErrDynamicToolCall, the advisor +// runner never fires, and both calls receive a synthesized policy +// error. +func TestRun_ExclusiveToolPolicyBlocksMixedWithDynamicTool(t *testing.T) { + t.Parallel() + + var advisorRuns atomic.Int32 + advisorTool := fantasy.NewAgentTool( + "advisor", + "returns strategic guidance", + func(context.Context, struct{}, fantasy.ToolCall) (fantasy.ToolResponse, error) { + advisorRuns.Add(1) + return fantasy.NewTextResponse(`{"status":"ok"}`), nil + }, + ) + + var mu sync.Mutex + var streamCalls int + model := &chattest.FakeModel{ + ProviderName: "fake", + StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + mu.Lock() + step := streamCalls + streamCalls++ + mu.Unlock() + + if step == 0 { + // Step 0: model emits an illegal mixed batch + // combining an exclusive builtin with a + // dynamic tool. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeToolInputStart, ID: "advisor-1", ToolCallName: "advisor"}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "advisor-1", Delta: `{}`}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "advisor-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "advisor-1", + ToolCallName: "advisor", + ToolCallInput: `{}`, + }, + {Type: fantasy.StreamPartTypeToolInputStart, ID: "mcp-1", ToolCallName: "mcp_tool"}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "mcp-1", Delta: `{"q":"docs"}`}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "mcp-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "mcp-1", + ToolCallName: "mcp_tool", + ToolCallInput: `{"q":"docs"}`, + }, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonToolCalls}, + }), nil + } + // Step 1: after the policy error is fed back, + // terminate the run so the test assertions have a + // deterministic exit. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeTextStart, ID: "text-1"}, + {Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "retrying"}, + {Type: fantasy.StreamPartTypeTextEnd, ID: "text-1"}, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonStop}, + }), nil + }, + } + + var persistedSteps []PersistedStep + err := Run(context.Background(), RunOptions{ + Model: model, + Messages: []fantasy.Message{ + textMessage(fantasy.MessageRoleUser, "please advise and fetch"), + }, + Tools: []fantasy.AgentTool{advisorTool}, + DynamicToolNames: map[string]bool{"mcp_tool": true}, + ExclusiveToolNames: map[string]bool{"advisor": true}, + MaxSteps: 5, + PersistStep: func(_ context.Context, step PersistedStep) error { + persistedSteps = append(persistedSteps, step) + return nil + }, + }) + // Run must NOT exit with ErrDynamicToolCall: the policy + // short-circuits before the dynamic partition so the dynamic + // call is never handed off for external execution. + require.NoError(t, err) + + // The advisor runner must not fire on mixed batches; the + // policy blocks the whole batch including the exclusive tool + // itself. + require.Equal(t, int32(0), advisorRuns.Load(), + "advisor runner must not fire on mixed batches") + + // Two steps: the mixed-batch step with synthesized policy + // errors plus the follow-up stream that ends the run. + require.Len(t, persistedSteps, 2) + firstStep := persistedSteps[0] + + // The persisted step must not record the dynamic tool as + // pending: the policy-error path returns before + // persistPendingDynamicStep runs. + require.Empty(t, firstStep.PendingDynamicToolCalls, + "policy-rejected batches must not leak dynamic tool calls to the caller") + + advisorErr, ok := findToolResultByID(firstStep.Content, "advisor-1") + require.True(t, ok, "persisted step must contain the advisor policy result") + requireToolResultErrorMessage(t, advisorErr, + "advisor must be called alone, without other tools in the same batch. Retry with only the advisor call.") + + mcpErr, ok := findToolResultByID(firstStep.Content, "mcp-1") + require.True(t, ok, "persisted step must contain the mcp_tool policy result") + requireToolResultErrorMessage(t, mcpErr, + "this tool was skipped because advisor must run alone in its batch. Retry your tool calls without advisor, or call advisor separately first.") +} + +// TestRun_ExclusiveToolAloneSucceeds is the happy-path counterpart +// to TestRun_ExclusiveToolPolicyViolation: a single exclusive tool +// emitted alone must actually execute. The `len(toolCalls) <= 1` +// guard in firstExclusiveToolName is the sole mechanism that lets +// solo exclusive-tool calls proceed. If that guard regresses to +// `< 1`, every solo exclusive-tool call would enter an infinite +// policy-error/retry loop, and every unit test on the policy +// function in isolation would still pass. Only this Run()-level +// test catches that regression. +func TestRun_ExclusiveToolAloneSucceeds(t *testing.T) { + t.Parallel() + + var advisorRuns atomic.Int32 + advisorTool := fantasy.NewAgentTool( + "advisor", + "returns strategic guidance", + func(context.Context, struct{}, fantasy.ToolCall) (fantasy.ToolResponse, error) { + advisorRuns.Add(1) + return fantasy.NewTextResponse(`{"status":"ok"}`), nil + }, + ) + + var mu sync.Mutex + var streamCalls int + model := &chattest.FakeModel{ + ProviderName: "fake", + StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + mu.Lock() + step := streamCalls + streamCalls++ + mu.Unlock() + + if step == 0 { + // Step 0: model emits exactly one + // exclusive-tool call in isolation. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeToolInputStart, ID: "advisor-1", ToolCallName: "advisor"}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "advisor-1", Delta: `{}`}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "advisor-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "advisor-1", + ToolCallName: "advisor", + ToolCallInput: `{}`, + }, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonToolCalls}, + }), nil + } + // Step 1: the loop re-streams after the tool + // result; end the run. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeTextStart, ID: "text-1"}, + {Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "done"}, + {Type: fantasy.StreamPartTypeTextEnd, ID: "text-1"}, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonStop}, + }), nil + }, + } + + var persistedSteps []PersistedStep + err := Run(context.Background(), RunOptions{ + Model: model, + Messages: []fantasy.Message{ + textMessage(fantasy.MessageRoleUser, "please advise"), + }, + Tools: []fantasy.AgentTool{advisorTool}, + ExclusiveToolNames: map[string]bool{"advisor": true}, + MaxSteps: 5, + PersistStep: func(_ context.Context, step PersistedStep) error { + persistedSteps = append(persistedSteps, step) + return nil + }, + }) + require.NoError(t, err) + + // The solo exclusive tool must actually execute exactly once. + require.Equal(t, int32(1), advisorRuns.Load(), + "solo exclusive-tool call must execute") + + // The first persisted step must contain a non-error tool + // result for the advisor call, proving the policy did not + // synthesize an error and the real runner fired. + require.GreaterOrEqual(t, len(persistedSteps), 1) + result, ok := findToolResultByID(persistedSteps[0].Content, "advisor-1") + require.True(t, ok, "persisted step must contain the advisor tool result") + _, isErr := result.Result.(fantasy.ToolResultOutputContentError) + require.Falsef(t, isErr, + "solo exclusive-tool call must produce a real tool result, not a policy error: %+v", result.Result) +} + +// TestRun_ExclusiveToolWithProviderExecutedSucceeds guards the +// interaction between the ProviderExecuted filter and the +// exclusive-tool policy. executeToolsForStep builds localCandidates +// by dropping ProviderExecuted calls before passing them to +// applyExclusiveToolPolicy. That filter is the sole mechanism +// preventing a false policy violation when a solo exclusive tool +// appears in a batch where the provider also server-executed a tool +// (for example Anthropic web_search). +// +// If the filter is removed, localCandidates would contain both the +// provider-executed call and the exclusive call. firstExclusiveToolName +// would then see len > 1, find advisor, and return a violation. The +// advisor would never run and the retry loop would burn steps until +// MaxSteps. +// +// This test emits an advisor call alongside a provider-executed +// web_search call (with its provider-emitted result) and asserts the +// advisor runner actually fires. +func TestRun_ExclusiveToolWithProviderExecutedSucceeds(t *testing.T) { + t.Parallel() + + var advisorRuns atomic.Int32 + advisorTool := fantasy.NewAgentTool( + "advisor", + "returns strategic guidance", + func(context.Context, struct{}, fantasy.ToolCall) (fantasy.ToolResponse, error) { + advisorRuns.Add(1) + return fantasy.NewTextResponse(`{"status":"ok"}`), nil + }, + ) + + var mu sync.Mutex + var streamCalls int + model := &chattest.FakeModel{ + ProviderName: "fake", + StreamFn: func(_ context.Context, _ fantasy.Call) (fantasy.StreamResponse, error) { + mu.Lock() + step := streamCalls + streamCalls++ + mu.Unlock() + + if step == 0 { + // Step 0: provider server-executed web_search and + // returned its result inline, plus the model + // emitted an exclusive advisor call for local + // execution. The ProviderExecuted filter must + // drop web_search from the policy check so the + // advisor is treated as a solo exclusive call. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeToolInputStart, ID: "ws-1", ToolCallName: "web_search", ProviderExecuted: true}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "ws-1", Delta: `{"query":"coder"}`, ProviderExecuted: true}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "ws-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "ws-1", + ToolCallName: "web_search", + ToolCallInput: `{"query":"coder"}`, + ProviderExecuted: true, + }, + { + Type: fantasy.StreamPartTypeToolResult, + ID: "ws-1", + ToolCallName: "web_search", + ProviderExecuted: true, + }, + {Type: fantasy.StreamPartTypeToolInputStart, ID: "advisor-1", ToolCallName: "advisor"}, + {Type: fantasy.StreamPartTypeToolInputDelta, ID: "advisor-1", Delta: `{}`}, + {Type: fantasy.StreamPartTypeToolInputEnd, ID: "advisor-1"}, + { + Type: fantasy.StreamPartTypeToolCall, + ID: "advisor-1", + ToolCallName: "advisor", + ToolCallInput: `{}`, + }, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonToolCalls}, + }), nil + } + // Step 1: end the run after the advisor result is + // fed back. + return streamFromParts([]fantasy.StreamPart{ + {Type: fantasy.StreamPartTypeTextStart, ID: "text-1"}, + {Type: fantasy.StreamPartTypeTextDelta, ID: "text-1", Delta: "done"}, + {Type: fantasy.StreamPartTypeTextEnd, ID: "text-1"}, + {Type: fantasy.StreamPartTypeFinish, FinishReason: fantasy.FinishReasonStop}, + }), nil + }, + } + + var persistedSteps []PersistedStep + err := Run(context.Background(), RunOptions{ + Model: model, + Messages: []fantasy.Message{ + textMessage(fantasy.MessageRoleUser, "search and then advise"), + }, + Tools: []fantasy.AgentTool{advisorTool}, + ExclusiveToolNames: map[string]bool{"advisor": true}, + MaxSteps: 5, + PersistStep: func(_ context.Context, step PersistedStep) error { + persistedSteps = append(persistedSteps, step) + return nil + }, + }) + require.NoError(t, err) + + // The advisor must execute exactly once: the ProviderExecuted + // filter removes web_search from the exclusivity check, so the + // advisor is treated as a solo exclusive call. + require.Equal(t, int32(1), advisorRuns.Load(), + "advisor must execute when the only other call in the batch was provider-executed") + + // The advisor result must be a real tool result, not a + // synthesized policy error. + require.GreaterOrEqual(t, len(persistedSteps), 1) + advisorResult, ok := findToolResultByID(persistedSteps[0].Content, "advisor-1") + require.True(t, ok, "persisted step must contain the advisor tool result") + _, isErr := advisorResult.Result.(fantasy.ToolResultOutputContentError) + require.Falsef(t, isErr, + "advisor must produce a real tool result, not a policy error: %+v", advisorResult.Result) +} + func TestRun_PersistStepErrorPropagates(t *testing.T) { t.Parallel()