From 519cf58414668b92ab99f3d1d9034136980a4962 Mon Sep 17 00:00:00 2001 From: devlikebear Date: Tue, 25 Aug 2026 13:40:11 +0900 Subject: [PATCH] fix(agent): keep the turn's final call in the same cache lineage MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes the measurement question #933's test plan deferred, by removing the condition that made it ambiguous rather than by measuring it. The finalization call — the one that extracts an answer after the loop exhausts its iterations — went out with Tools: nil. Providers render tools -> system -> messages into one prefix-matched cache key, so a tools-absent request cannot read the prefix the tool-bearing iterations just wrote, and pays its own cache write at the 1.25x premium for an entry no following call in that turn can read back. Keeping the tool list and suppressing it with tool_choice=none puts the call back in the turn's lineage. This is also the first time ToolChoiceNone actually reaches the wire: every provider emits tool_choice only inside `if len(tools) > 0` (anthropic.go, openai_compat_client.go, gemini_native_convert.go all share the shape), so with Tools: nil the "none" was silently dropped and suppression relied entirely on there being no tools to call. Dropping the tools is what made a text answer structural rather than a request the provider might ignore, so that guarantee is kept as a fallback: if the finalization response comes back with tool calls and no content, retry once the old way. Without it that case degrades to the max-iterations error instead of an answer. Costs an extra round trip only when a provider misbehaves. TestLoop_Run_FinalizesWithoutToolsWhenMaxIterationsReached asserted the mechanism (tool count 0); it now asserts the outcome it was protecting, with the ignore-none path covered by its own test. Verified: make test green, make lint-diff 0 issues, diff coverage 100% (28/28). CLI providers are unaffected — claude-code-cli and antigravity-cli never read ChatOptions.Tools. --- CHANGELOG.md | 2 + internal/agent/loop.go | 30 +++++++++- internal/agent/loop_test.go | 115 +++++++++++++++++++++++++++++++++++- 3 files changed, 143 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cfa4d336..83eff6a8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,8 @@ The format is based on Keep a Changelog and the project follows Semantic Version ### Fixed +- **에이전트 턴의 마지막 호출이 캐시 계보를 벗어나던 문제 (#921)** — 루프가 반복을 소진한 뒤 답변을 받아내는 finalization 호출은 도구 목록을 통째로 뺀 채(`Tools: nil`) 나갔다. 프로바이더는 `tools` → `system` → `messages` 순으로 하나의 prefix 캐시 키를 만들기 때문에, 도구 없는 요청은 바로 앞 루프 반복들이 방금 쓴 prefix를 **읽을 수 없고**, 자기 캐시 write를 1.25배 요금으로 치르면서도 그 엔트리를 뒤이어 읽어줄 호출이 없었다. 이제 도구 목록을 유지하고 `tool_choice: none`으로 억제해 같은 계보에 머문다. 부수적으로 `ToolChoiceNone()`이 처음으로 실제 전송된다 — 모든 프로바이더가 `tool_choice`를 `len(tools) > 0`일 때만 실으므로 지금까지는 조용히 버려지고 있었다. 프로바이더가 `none`을 무시하고 도구 호출을 돌려주면 예전 방식(도구 제거)으로 한 번 재시도해, 텍스트 답변 보장은 그대로 유지한다. + - **온보딩 마법사 저장이 커스텀 tier를 지우던 문제 (#931)** — `buildConfigPayload`는 `llm_providers`를 디스크 기존값과 병합하면서 `llm_tiers`는 `heavy`/`standard`/`light`만으로 새로 만들어 통째로 반환했다. alias-keyed PATCH가 보낸 값으로 on-disk 집합을 대체하므로, `vision` 같은 커스텀 tier를 쓰는 사용자가 마법사에서 모델 하나만 바꿔 저장해도 그 tier가 설정에서 사라졌다. Config 페이지가 `llm_tiers` 편집을 마법사 딥링크로 넘기면서 이 경로가 일반 사용자에게 열렸다. 이제 tier도 provider와 동일하게 기존값 위에 덮어쓴다. - **Inspect 뷰가 저장되지 않은 값을 보여주던 문제 (#931)** — Quick Start 밖에서는 Save/Discard 버튼이 숨는데 `getDisplayValue`는 view mode와 무관하게 dirty 값을 우선해서, "Read-only inspection" 배너 아래에 커밋되지 않은 편집이 서버 상태처럼 보였다. 사용자가 반영됐다고 믿고 재시작하면 편집은 사라진다. 이제 Inspect는 서버가 실제로 로드한 값만 보여준다. - **DESIGN.md가 스스로 모순되던 문제 (#931)** — Quick Start 13개 게이트가 "stay interactive"라고 적힌 줄과, 같은 필드가 read-only가 된다고 적힌 줄이 공존했다. `embodiment_providers_json`의 실제 동작, 자격증명 7개 중 콘솔 입력란이 남은 2개와 YAML 전용이 된 5개(토큰 rotation에 호스트 파일 접근이 필요해진다는 결과 포함), 그리고 편집기 삭제 후 참조가 끊긴 `configStructured.ts` 드래프트 빌더 목록을 명시했다. diff --git a/internal/agent/loop.go b/internal/agent/loop.go index bef99fdb..18f5c07b 100644 --- a/internal/agent/loop.go +++ b/internal/agent/loop.go @@ -428,9 +428,35 @@ func (l *Loop) Run(ctx context.Context, initial []llm.ChatMessage, opts RunOptio finalResp, finalErr := l.client.Chat(ctx, messages, llm.ChatOptions{ OnDelta: opts.OnDelta, OnReasoningDelta: opts.OnReasoningDelta, - Tools: nil, - ToolChoice: llm.ToolChoiceNone(), + // Keep the tool list and suppress it with tool_choice=none rather than + // dropping it. Providers render tools ahead of messages into one + // prefix-matched cache key, so a tools-absent request lands in a + // different cache lineage than the loop iterations it follows: it + // cannot read the prefix they just wrote, and pays its own write + // premium for an entry those iterations can never read back. + // + // This is also the first time ToolChoiceNone reaches the wire. Every + // provider emits tool_choice only inside `if len(tools) > 0`, so with + // Tools=nil the "none" was silently dropped and suppression relied + // entirely on there being no tools to call. + Tools: llmTools, + ToolChoice: llm.ToolChoiceNone(), }) + // Dropping the tools is what used to make a text answer structural rather + // than a request the provider might ignore. Keep that guarantee as a + // fallback: if the provider honored neither tool_choice=none nor the + // implicit "answer now", retry the way this call used to be made. Costs an + // extra round trip only when a provider misbehaves, and without it that + // case degrades to the max-iterations error instead of an answer. + if finalErr == nil && len(finalResp.Message.ToolCalls) > 0 && strings.TrimSpace(finalResp.Message.Content) == "" { + l.emit(ctx, Event{Type: EventBeforeLLM, Iteration: finalIter, MessageCount: len(messages)}) + finalResp, finalErr = l.client.Chat(ctx, messages, llm.ChatOptions{ + OnDelta: opts.OnDelta, + OnReasoningDelta: opts.OnReasoningDelta, + Tools: nil, + ToolChoice: llm.ToolChoiceNone(), + }) + } if finalErr == nil { afterLLMEvent := Event{Type: EventAfterLLM, Iteration: finalIter, MessageCount: len(messages), SessionID: strings.TrimSpace(finalResp.SessionID)} if opts.AfterLLM != nil { diff --git a/internal/agent/loop_test.go b/internal/agent/loop_test.go index 5badad1d..18a4e173 100644 --- a/internal/agent/loop_test.go +++ b/internal/agent/loop_test.go @@ -816,14 +816,67 @@ func TestLoop_Run_FinalizesWithoutToolsWhenMaxIterationsReached(t *testing.T) { if len(client.seenToolCounts) != 3 { t.Fatalf("expected 3 llm calls, got %d", len(client.seenToolCounts)) } - if client.seenToolCounts[2] != 0 { - t.Fatalf("expected finalization call without tools, got %d", client.seenToolCounts[2]) + // The finalization call keeps the tool list so it stays in the same cache + // lineage as the loop iterations; suppression is tool_choice=none. What + // this test guards is the outcome — finalization yields text — not the + // mechanism. See the ignores-none fallback test below for the case where a + // provider does not honor the suppression. + if client.seenToolCounts[2] == 0 { + t.Fatalf("finalization should keep the tools it was called with, got %d", client.seenToolCounts[2]) } if client.seenToolChoice[2] != "none" { t.Fatalf("expected finalization tool_choice=none, got %q", client.seenToolChoice[2]) } } +// A provider that ignores tool_choice=none would hand back another tool call +// with no text, which the loop cannot use — before this fallback existed that +// turn degraded into the max-iterations error instead of an answer. +func TestLoop_Run_FinalizationRetriesWithoutToolsWhenProviderIgnoresNone(t *testing.T) { + reg := tool.NewRegistry() + reg.Register(tool.NewSessionStatusTool(func(_ context.Context) (tool.SessionStatus, error) { + return tool.SessionStatus{SessionID: "sess"}, nil + })) + + client := &scriptedLLMClient{ + responses: []llm.ChatResponse{ + {Message: llm.ChatMessage{Role: "assistant", ToolCalls: []llm.ToolCall{ + {ID: "call_1", Name: "session_status", Arguments: `{}`}, + }}}, + // Finalization attempt: provider ignores none and calls a tool. + {Message: llm.ChatMessage{Role: "assistant", ToolCalls: []llm.ToolCall{ + {ID: "call_2", Name: "session_status", Arguments: `{}`}, + }}}, + // Retry with the tools removed: now it answers. + {Message: llm.ChatMessage{Role: "assistant", Content: "fallback answer"}}, + }, + } + + loop := NewLoop(client, reg) + resp, err := loop.Run(context.Background(), []llm.ChatMessage{ + {Role: "user", Content: "go"}, + }, RunOptions{ + MaxIterations: 1, + Tools: reg.Schemas(), + ToolChoice: llm.ToolChoiceAuto(), + }) + if err != nil { + t.Fatalf("fallback should still produce an answer, got %v", err) + } + if resp.Message.Content != "fallback answer" { + t.Fatalf("unexpected final content: %q", resp.Message.Content) + } + if len(client.seenToolCounts) != 3 { + t.Fatalf("expected loop + finalization + retry = 3 calls, got %v", client.seenToolCounts) + } + if client.seenToolCounts[1] == 0 { + t.Fatalf("first finalization attempt should carry tools, got %v", client.seenToolCounts) + } + if client.seenToolCounts[2] != 0 { + t.Fatalf("retry must drop the tools to make a text answer structural, got %v", client.seenToolCounts) + } +} + type testRecordingEmitter struct { events []testEmittedLine } @@ -1103,3 +1156,61 @@ func TestLoop_Run_HonorsCallerResumeSessionID(t *testing.T) { t.Fatalf("expected iter 1 to receive caller resume id 'carried', got %v", client.seenResumeIDs) } } + +// The turn's final call used to drop the tool list. Providers render tools +// ahead of messages into one prefix-matched cache key, so a tools-absent +// request lands in a different cache lineage than the tool-bearing loop +// iterations it follows — it cannot read what they just wrote, and its own +// cache write is paid at a premium for an entry the next iteration cannot use. +// +// Keeping the tools and suppressing them with tool_choice=none puts the call +// back in the turn's lineage. It also makes ToolChoiceNone actually reach the +// wire: every provider emits tool_choice only when tools are present, so with +// Tools=nil the "none" was silently dropped. +func TestLoop_Run_FinalCallKeepsToolsAndSuppressesWithToolChoiceNone(t *testing.T) { + reg := tool.NewRegistry() + reg.Register(tool.NewSessionStatusTool(func(_ context.Context) (tool.SessionStatus, error) { + return tool.SessionStatus{SessionID: "sess"}, nil + })) + + client := &scriptedLLMClient{ + responses: []llm.ChatResponse{ + { + Message: llm.ChatMessage{ + Role: "assistant", + ToolCalls: []llm.ToolCall{ + {ID: "call-1", Name: "session_status", Arguments: "{}"}, + }, + }, + StopReason: "tool_use", + }, + {Message: llm.ChatMessage{Role: "assistant", Content: "done"}}, + }, + } + + loop := NewLoop(client, reg) + if _, err := loop.Run(context.Background(), []llm.ChatMessage{ + {Role: "user", Content: "status please"}, + }, RunOptions{ + Tools: reg.Schemas(), + ToolChoice: llm.ToolChoiceAuto(), + MaxIterations: 1, + }); err != nil { + t.Fatalf("run: %v", err) + } + + if len(client.seenToolCounts) < 2 { + t.Fatalf("expected the loop to reach its final call, got %v", client.seenToolCounts) + } + last := len(client.seenToolCounts) - 1 + if client.seenToolCounts[last] == 0 { + t.Fatalf("final call must still carry the tool list so it shares the turn's cache prefix, got %v", client.seenToolCounts) + } + if client.seenToolCounts[last] != client.seenToolCounts[0] { + t.Fatalf("final call tool count %d should match the loop's %d — a different tool set is a different cache prefix", + client.seenToolCounts[last], client.seenToolCounts[0]) + } + if got := client.seenToolChoice[last]; got != "none" { + t.Fatalf("final call must suppress tool use with tool_choice=none, got %q", got) + } +}