mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
Michael SuchaczandPaul Frybarger
19e2b0415a
Supersedes #28883 by @pfrybar. His commit is carried with authorship intact (rebased onto main as32d261f94f); the second commit adds the overflow fallback found while testing it. ## Problem Compaction rebuilt the summary request without the conversation's tool definitions, so the summary prompt no longer shared the turn's prefix and providers could not reuse the cached context (measurements in #28883). Sending the tools with the summary makes that request at least as large as the generation it summarizes. At a model's real context window (gpt-3.5-turbo-0125, 16385 tokens) the summary request was rejected with `Your input exceeds the context window of this model.` on the same overflow compaction exists to recover from. The chat stayed in `error`, and both manual `/compact` and the automatic retrigger failed the same way, so the chat was stuck. ## Fix `generateCompactionSummary` sends the prepared tool definitions with the summary request. When the provider rejects that request for size, it regenerates the summary once without tools, which is the request shape compaction used before and still fits whenever the history alone does. The context-window check reuses fantasy's classification and also matches bad-request wordings fantasy does not parse (OpenAI Responses API, Anthropic, `context_length_exceeded`). `BuildToolDefinitions` moves into its own file in `chatloop` so the turn and the summary share one construction path. On Anthropic the summary prompt now carries the same `cache_control` breakpoints as the turn; without them Anthropic never reads the cache, so the shared tool and history prefix would have been pure cost there. A compaction override that resolves to a different model shares no prompt cache and may not support the chat model's provider-native tools, so its summary request carries no tool definitions; an override resolving to the chat model itself keeps them. HTTP 413 counts as a size rejection for the tool-less retry. ## Testing - Unit: `TestGenerateCompactionSummaryRetriesWithoutToolsWhenContextTooLarge`, `TestGenerateCompactionSummaryDoesNotRetryOtherFailures`, `TestGenerateCompactionSummaryAppliesAnthropicPromptCaching`, `TestIsContextTooLargeError`, `TestBuildToolDefinitions*`. - End to end: `TestActiveServer_ManualCompaction/retries the summary without tools when the prompt exceeds the context window` asserts two summary requests (with tools, then none), the chat returns to `waiting` with `last_error` cleared, and the summary is persisted. - Red-green: with the retry disabled the unit test fails on the returned provider error and the e2e chat never leaves `error`; with the phrase matching disabled the Responses, Anthropic, and error-code classifier cases fail. ## Review record - Local standards and spec review at 99f7e80a0a9 (pre-rebase content of8799db2f0e): switched `errors.As` to `errors.AsType`, removed two comments restating test names, trimmed the phrase list to provider-attested wordings. Rebase onto main resolved one test-file conflict (both sides deleted different tests) and inlined the `chattest.AnthropicErrorResponse` helper that main removed in #29008. - Codex round 1 on8799db2f0e: one P1, parent tool definitions sent to a compaction override on a different provider. Mechanism verified partially (fantasy skips foreign provider-defined tools with a warning; `web_search` translates across providers and schemas can be rejected). Fixed ina7fa7a61e4by dropping tool definitions when the override is a separate provider instance, with a red-green case in `TestActiveServer_CompactionModelOverride`. Thread resolved. - Codex round 2 ona7fa7a61e4: P1 override tools should drop for any different model (agreed, guard replaced by an unconditional drop); P2 HTTP 413 should take the tool-less retry (agreed); P2 Anthropic summary request had no cache breakpoints so the tools bought no cache hit (confirmed in code, matches earlier dogfood `cache_read=0`). All three fixed ine7bd72390cwith red-green unit and e2e assertions. Threads resolved. - Codex round 3 one7bd72390c: P2 keep tools when the override resolves to the chat model (agreed, fixed in6d8d8d6f71with a red-green e2e case); P1 missing ARCHITECTURE.md TODO items for the changed compaction flow (agreed, added in6d8d8d6f71per the chatd rule); P2 summary tool set may differ from the triggering request after a `find_tools` activation (mechanism confirmed, declined as a cache-efficiency edge case with the overflow path covered by the retry). Threads resolved. - Codex round 4 on6d8d8d6f71: "Didn't find any major issues", reviewed commit `6d8d8d6f71`, zero unresolved threads. CI green on the same head (Storybook rerun once after Pixel returned HTTP 500 while uploading snapshots; this branch has no `site/` changes). > Xum, an AI coding agent, prepared this PR on behalf of @ibetitsmike. --------- Co-authored-by: Paul Frybarger <pfrybar@frymail.net>
97 lines
3.2 KiB
Go
97 lines
3.2 KiB
Go
package chatloop
|
|
|
|
import (
|
|
"encoding/json"
|
|
"testing"
|
|
|
|
"charm.land/fantasy"
|
|
fantasyopenai "charm.land/fantasy/providers/openai"
|
|
"github.com/stretchr/testify/require"
|
|
)
|
|
|
|
// staticParametersTool returns a fixed ToolInfo, letting a test control
|
|
// Parameters directly. fantasy.NewAgentTool always generates a non-nil
|
|
// Parameters map, so it cannot reproduce the nil Parameters that MCP tool
|
|
// wrappers report for a schema with an empty "properties" object.
|
|
type staticParametersTool struct {
|
|
fantasy.AgentTool
|
|
info fantasy.ToolInfo
|
|
providerOptions fantasy.ProviderOptions
|
|
}
|
|
|
|
func (t staticParametersTool) Info() fantasy.ToolInfo { return t.info }
|
|
|
|
func (t staticParametersTool) ProviderOptions() fantasy.ProviderOptions { return t.providerOptions }
|
|
|
|
// TestBuildToolDefinitionsNilPropertiesBecomesEmptyObject verifies that a
|
|
// tool whose input schema has no properties (for example an MCP tool
|
|
// reporting {"type": "object", "properties": {}}) still serializes
|
|
// "properties" as an empty JSON object. A nil Parameters map would serialize
|
|
// to null, which OpenAI rejects with "Invalid schema for function ... None is
|
|
// not of type 'object'".
|
|
func TestBuildToolDefinitionsNilPropertiesBecomesEmptyObject(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
tool := staticParametersTool{
|
|
info: fantasy.ToolInfo{
|
|
Name: "document_graphql_schema",
|
|
Description: "Run a GraphQL query",
|
|
Parameters: nil,
|
|
},
|
|
}
|
|
|
|
defs := BuildToolDefinitions([]fantasy.AgentTool{tool}, nil, nil)
|
|
require.Len(t, defs, 1)
|
|
|
|
ft, ok := defs[0].(fantasy.FunctionTool)
|
|
require.True(t, ok, "expected a fantasy.FunctionTool")
|
|
|
|
properties, ok := ft.InputSchema["properties"].(map[string]any)
|
|
require.True(t, ok, "properties must be a map, got %T", ft.InputSchema["properties"])
|
|
require.NotNil(t, properties, "properties must not be nil")
|
|
|
|
// Verify it serializes to {} not null.
|
|
bs, err := json.Marshal(ft.InputSchema)
|
|
require.NoError(t, err)
|
|
require.JSONEq(t, `{"type":"object","properties":{}}`, string(bs))
|
|
}
|
|
|
|
func TestBuildToolDefinitionsFiltersInactiveToolsAndAppendsProviderTools(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
active := staticParametersTool{info: fantasy.ToolInfo{Name: "active"}}
|
|
inactive := staticParametersTool{info: fantasy.ToolInfo{Name: "inactive"}}
|
|
provider := fantasy.ProviderDefinedTool{ID: "web_search", Name: "web_search"}
|
|
|
|
defs := BuildToolDefinitions(
|
|
[]fantasy.AgentTool{active, inactive},
|
|
[]string{"active"},
|
|
[]ProviderTool{{Definition: provider}},
|
|
)
|
|
|
|
require.Len(t, defs, 2)
|
|
function, ok := defs[0].(fantasy.FunctionTool)
|
|
require.True(t, ok)
|
|
require.Equal(t, "active", function.Name)
|
|
require.Equal(t, provider, defs[1])
|
|
}
|
|
|
|
func TestBuildToolDefinitionsPreservesFunctionToolOptions(t *testing.T) {
|
|
t.Parallel()
|
|
|
|
providerOptions := fantasy.ProviderOptions{
|
|
fantasyopenai.Name: &fantasyopenai.ProviderOptions{},
|
|
}
|
|
tool := staticParametersTool{
|
|
info: fantasy.ToolInfo{Name: "test_tool"},
|
|
providerOptions: providerOptions,
|
|
}
|
|
|
|
defs := BuildToolDefinitions([]fantasy.AgentTool{tool}, nil, nil)
|
|
require.Len(t, defs, 1)
|
|
function, ok := defs[0].(fantasy.FunctionTool)
|
|
require.True(t, ok)
|
|
require.Equal(t, "test_tool", function.Name)
|
|
require.Equal(t, providerOptions, function.ProviderOptions)
|
|
}
|