diff --git a/openspec/changes/planner-reflection-history/design.md b/openspec/changes/planner-reflection-history/design.md index eff1bb4..19bfdd2 100644 --- a/openspec/changes/planner-reflection-history/design.md +++ b/openspec/changes/planner-reflection-history/design.md @@ -51,7 +51,15 @@ Constraints: **Why over method A (implicit)**: Method A relies on the AI organically reflecting without prompting, which is unreliable. Explicit instruction in the system prompt makes it consistent. -**Constraint**: `tool_choice: {type: "any"}` already allows text blocks before a tool use block. No API parameter changes needed for text output capture. +**Correction (post-implementation, found during manual smoke testing per task 8.5)**: The original assumption that `tool_choice: {type: "any"}` allows text blocks before `tool_use` was wrong. Per Anthropic's API behaviour, forced `tool_choice` (`any` or a specific tool) makes Claude skip any preceding text block entirely, and forced tool_choice is also incompatible with extended thinking. Under the original `tool_choice: {type: "any"}` call, `text_output`/`thinking` were therefore *always* `None` in practice — not merely "sometimes absent" as D1/Risks originally assumed. See D8 for the fix. + +### D8: `tool_choice` must be `"auto"` (with a forced retry fallback) to allow rationale/thinking capture + +**Decision**: `AnthropicToolCallingClient`/`OpenAIToolCallingClient` now call the API with `tool_choice: "auto"` first (Anthropic: `{"type": "auto", "disable_parallel_tool_use": True}`; OpenAI: `"auto"`). Extended thinking (when `thinking_budget_tokens` is set) is only ever requested alongside `"auto"`. If the model responds without any tool call at all, the client retries once with the original forced `tool_choice` (Anthropic `"any"`, OpenAI `"required"`) and no thinking parameter, guaranteeing a tool call is eventually returned. `OpenAIToolCallingClient` also now captures `message.content` as `text_output` (previously never extracted for any provider — OpenAI never had rationale capture at all, forced or not). + +**Why not just always force tool_choice with a "think first" instruction**: Confirmed via Anthropic's official docs/SDK guidance that this combination structurally suppresses the text/thinking Claude would otherwise produce — no amount of prompting fixes it while `tool_choice` stays forced. + +**Why a fallback retry rather than failing the step**: `PLANNER_SYSTEM_PROMPT` already instructs "you must then call exactly one tool", so `tool_choice: "auto"` calls overwhelmingly still return a tool_use block; the retry only guards the rare case where the model responds with pure text. Failing the task step outright on that rare case would regress reliability for a` cosmetic (`rationale`) improvement. The forced retry accepts losing rationale/thinking for that one step rather than losing task progress. ### D2: Extend `ToolCallDecision` with `thinking` and `text_output` @@ -106,7 +114,8 @@ Constraints: ## Risks / Trade-offs -- **AI may not always output a text block**: `tool_choice: {type: "any"}` does not guarantee a text block. When absent, `text_output` is `None` and `rationale` is `None`. History degrades gracefully to `{page, rationale: null, action, success}`. No retries or fallbacks needed. +- **AI may not always output a text block**: even with `tool_choice: "auto"` (D8), the model is not guaranteed to prefix a text block before the tool call. When absent, `text_output` is `None` and `rationale` is `None`. History degrades gracefully to `{page, rationale: null, action, success}`. +- **`tool_choice: "auto"` occasionally yields no tool call at all**: unlike forced `tool_choice`, `"auto"` permits the model to respond with text only and no tool call. D8's forced retry (no thinking, no rationale on that path) guards this case so a step never stalls; this trades away rationale/thinking for that single step, not overall reliability. - **Extended thinking increases latency**: `budget_tokens` directly adds to minimum response time. This is opt-in and accepted by the operator who enables it. - **`WorldEvent` schema divergence from stored data**: Existing `WorldEvent` instances in memory or serialised timelines lack `rationale`/`thinking`. The `to_dict()` method will emit `null` for these fields; downstream consumers should treat `null` as absent, not as a failure. - **Cloud-proxy transport never produces thinking/rationale at the client layer**: The proxy returns only `tool_name`/`arguments`. `ToolCallDecision.thinking` and `.text_output` will always be `None` for cloud-transport tasks. The `planner_decision_log` on the cloud side will be populated from the cloud-proxied call itself (D7), which does see the full LLM response. diff --git a/openspec/changes/planner-reflection-history/tasks.md b/openspec/changes/planner-reflection-history/tasks.md index cfec37d..8f1cb85 100644 --- a/openspec/changes/planner-reflection-history/tasks.md +++ b/openspec/changes/planner-reflection-history/tasks.md @@ -20,7 +20,7 @@ ## 4. Planner prompts — reflection instruction - [x] 4.1 Update `PLANNER_SYSTEM_PROMPT` in `runtime/planner_prompts.py` to instruct the AI to output a short text block (1–2 sentences) before each tool call: first assessing whether the previous action achieved its intended effect (or noting "first step" if no history), then stating the intent of the current action -- [x] 4.2 Verify via prompt review that the instruction is consistent with `tool_choice: {type: "any"}` (text blocks are already allowed before tool_use blocks — no API change needed) +- [x] 4.2 ~~Verify via prompt review that the instruction is consistent with `tool_choice: {type: "any"}` (text blocks are already allowed before tool_use blocks — no API change needed)~~ — **superseded, see section 9**: this assumption was wrong; forced `tool_choice` suppresses text/thinking entirely ## 5. History format — compact rationale-based representation @@ -48,4 +48,15 @@ - [x] 8.2 Run `ruff check` and `ruff format --check` on all modified files - [x] 8.3 Run `python -m compileall` on modified packages - [x] 8.4 Run `openspec validate --strict --change "planner-reflection-history"` and confirm all artifacts pass validation -- [ ] 8.5 Manual smoke test (optional, requires Host Agent + Appium + iPhone): verify that a live task run produces non-null `rationale` in `WorldEvent` history and that the history prompt passed to the LLM is in compact format +- [x] 8.5 Manual smoke test (requires Host Agent + Appium + iPhone): ran a live task and found `rationale`/`thinking` were **always** `None` in `WorldEvent` history — root cause diagnosed and fixed in section 9 below + +## 9. Bug fix — forced `tool_choice` was suppressing rationale/thinking (found via 8.5) + +- [x] 9.1 `AnthropicToolCallingClient._create_message()`: add required `forced: bool` param; `tool_choice` is `{"type": "auto", "disable_parallel_tool_use": True}` when `forced=False`, `{"type": "any", "disable_parallel_tool_use": True}` when `forced=True`; thinking/`betas` kwargs only added when `forced=False` +- [x] 9.2 `AnthropicToolCallingClient.decide()`: call with `forced=False` first; if `_anthropic_response_has_tool_use(response)` is `False`, retry once with `forced=True`; parse whichever response has the tool call +- [x] 9.3 `OpenAIToolCallingClient._create_completion()`: add required `forced: bool` param; `tool_choice` is `"auto"` when `forced=False`, `"required"` when `forced=True` +- [x] 9.4 `OpenAIToolCallingClient.decide()`: same auto-then-forced-retry pattern using `_openai_response_has_tool_call()` +- [x] 9.5 `_decision_from_openai_response()`: extract `message.content` into `text_output` (previously never captured for OpenAI, forced or not) +- [x] 9.6 Add/rename unit tests in `tests/test_tool_calling_client.py` covering: default request uses `tool_choice: "auto"`; retry sequence when first response has no tool call (Anthropic and OpenAI); thinking/`betas` dropped on the forced retry; OpenAI `text_output` capture +- [x] 9.7 Update `design.md` (D1 correction + new D8) and this file to document the bug and fix +- [x] 9.8 Re-run `uv run --all-packages pytest -m "not integration"`, `ruff check`/`ruff format --check`, `python -m compileall`, and `openspec validate --strict --change "planner-reflection-history"` after the fix diff --git a/runtime/tool_calling_client.py b/runtime/tool_calling_client.py index d8e446a..223423d 100644 --- a/runtime/tool_calling_client.py +++ b/runtime/tool_calling_client.py @@ -84,7 +84,24 @@ class AnthropicToolCallingClient: screenshot, tools, timeout=timeout, + forced=False, ) + if not _anthropic_response_has_tool_use(response): + # tool_choice="any"/"tool" forces Claude to skip any preceding + # text block, so the reflection instructed by the system + # prompt requires tool_choice="auto". That leaves a small + # chance the model responds without calling a tool at all; + # retry once with tool_choice="any" to guarantee progress. + # The forced retry cannot carry rationale/thinking (Anthropic + # rejects thinking combined with forced tool_choice). + response = self._create_message( + system_prompt, + user_prompt, + screenshot, + tools, + timeout=timeout, + forced=True, + ) return _decision_from_anthropic_response( response, system_prompt=system_prompt, @@ -103,13 +120,16 @@ class AnthropicToolCallingClient: tools: list[ToolSpec], *, timeout: float, + forced: bool, ) -> Any: client = self._client() - budget = self._thinking_budget_tokens + # Forced tool_choice is incompatible with extended thinking. + budget = self._thinking_budget_tokens if not forced else None # Enforce max_tokens >= budget + 1 when thinking is enabled. max_tokens = self.max_tokens if budget is not None: max_tokens = max(max_tokens, budget + 1) + tool_choice_type = "any" if forced else "auto" kwargs: dict[str, Any] = { "model": self.model, "max_tokens": max_tokens, @@ -128,7 +148,10 @@ class AnthropicToolCallingClient: } ], "tools": [_anthropic_tool(spec) for spec in tools], - "tool_choice": {"type": "any", "disable_parallel_tool_use": True}, + "tool_choice": { + "type": tool_choice_type, + "disable_parallel_tool_use": True, + }, } if budget is not None: kwargs["thinking"] = {"type": "enabled", "budget_tokens": budget} @@ -188,7 +211,21 @@ class OpenAIToolCallingClient: screenshot, tools, timeout=timeout, + forced=False, ) + if not _openai_response_has_tool_call(response): + # tool_choice="required" forces a function call and suppresses + # any reflection text, so capturing rationale requires + # tool_choice="auto". Retry once, forcing tool use, if the + # model responds without calling a tool at all. + response = self._create_completion( + system_prompt, + user_prompt, + screenshot, + tools, + timeout=timeout, + forced=True, + ) return _decision_from_openai_response( response, system_prompt=system_prompt, @@ -207,6 +244,7 @@ class OpenAIToolCallingClient: tools: list[ToolSpec], *, timeout: float, + forced: bool, ) -> Any: client = self._client() kwargs = { @@ -218,7 +256,7 @@ class OpenAIToolCallingClient: {"role": "user", "content": _openai_content(user_prompt, screenshot)}, ], "tools": [_openai_tool(spec) for spec in tools], - "tool_choice": "required", + "tool_choice": "required" if forced else "auto", "parallel_tool_calls": False, } chat = getattr(client, "chat", None) @@ -282,6 +320,13 @@ def _anthropic_tool(spec: ToolSpec) -> dict[str, Any]: } +def _anthropic_response_has_tool_use(response: Any) -> bool: + content = _value(response, "content") + if not isinstance(content, list): + return False + return any(_value(block, "type") == "tool_use" for block in content) + + def _decision_from_anthropic_response( response: Any, *, @@ -346,6 +391,15 @@ def _openai_tool(spec: ToolSpec) -> dict[str, Any]: } +def _openai_response_has_tool_call(response: Any) -> bool: + choices = _value(response, "choices") + if not isinstance(choices, list) or not choices: + return False + message = _value(choices[0], "message") + tool_calls = _value(message, "tool_calls") + return isinstance(tool_calls, list) and len(tool_calls) > 0 + + def _decision_from_openai_response( response: Any, *, @@ -367,6 +421,10 @@ def _decision_from_openai_response( # Extract reasoning_content from o-series models when present. raw_reasoning = _value(message, "reasoning_content") thinking: str | None = raw_reasoning if isinstance(raw_reasoning, str) else None + # Pre-tool reflection text (rationale), when the model emits content + # alongside the tool call under tool_choice="auto". + raw_content = _value(message, "content") + text_output: str | None = raw_content if isinstance(raw_content, str) else None return ToolCallDecision( tool_name=name, arguments=arguments, @@ -374,6 +432,7 @@ def _decision_from_openai_response( system_prompt=system_prompt, user_prompt=user_prompt, thinking=thinking, + text_output=text_output, ) diff --git a/tests/test_tool_calling_client.py b/tests/test_tool_calling_client.py index 5e1172c..99cff71 100644 --- a/tests/test_tool_calling_client.py +++ b/tests/test_tool_calling_client.py @@ -21,9 +21,14 @@ from tests.fakes import PNG_10X20 class FakeMessages: def __init__( - self, *, response: object | None = None, error: Exception | None = None + self, + *, + response: object | None = None, + responses: list[object] | None = None, + error: Exception | None = None, ) -> None: self.response = response + self._responses = list(responses) if responses is not None else None self.error = error self.calls: list[dict[str, Any]] = [] @@ -31,6 +36,8 @@ class FakeMessages: self.calls.append(kwargs) if self.error: raise self.error + if self._responses is not None: + return self._responses.pop(0) return self.response @@ -41,9 +48,14 @@ class FakeTransport: class FakeCompletions: def __init__( - self, *, response: object | None = None, error: Exception | None = None + self, + *, + response: object | None = None, + responses: list[object] | None = None, + error: Exception | None = None, ) -> None: self.response = response + self._responses = list(responses) if responses is not None else None self.error = error self.calls: list[dict[str, Any]] = [] @@ -51,6 +63,8 @@ class FakeCompletions: self.calls.append(kwargs) if self.error: raise self.error + if self._responses is not None: + return self._responses.pop(0) return self.response @@ -67,7 +81,7 @@ class FakeOpenAITransport: # --- Anthropic --------------------------------------------------------- -def test_anthropic_tool_calling_client_sends_forced_single_tool_call_request() -> None: +def test_anthropic_tool_calling_client_sends_auto_tool_choice_request() -> None: messages = FakeMessages( response={ "content": [{"type": "tool_use", "name": "tap", "input": {"x": 1, "y": 2}}] @@ -95,7 +109,7 @@ def test_anthropic_tool_calling_client_sends_forced_single_tool_call_request() - call = messages.calls[0] assert call["model"] == "test-model" assert call["timeout"] == 2.5 - assert call["tool_choice"] == {"type": "any", "disable_parallel_tool_use": True} + assert call["tool_choice"] == {"type": "auto", "disable_parallel_tool_use": True} assert call["tools"] == [ { "name": "tap", @@ -224,10 +238,72 @@ def test_anthropic_tool_calling_client_wraps_malformed_responses( ) +def test_anthropic_client_retries_with_forced_tool_choice_when_model_omits_tool_call() -> ( + None +): + messages = FakeMessages( + responses=[ + {"content": [{"type": "text", "text": "just thinking out loud"}]}, + { + "content": [ + {"type": "tool_use", "name": "tap", "input": {"x": 1, "y": 2}} + ] + }, + ] + ) + client = AnthropicToolCallingClient( + model="test-model", transport=FakeTransport(messages) + ) + + decision = client.decide( + system_prompt="s", user_prompt="u", screenshot=None, tools=[TAP_SPEC], timeout=1 + ) + + assert decision.tool_name == "tap" + assert len(messages.calls) == 2 + assert messages.calls[0]["tool_choice"] == { + "type": "auto", + "disable_parallel_tool_use": True, + } + assert messages.calls[1]["tool_choice"] == { + "type": "any", + "disable_parallel_tool_use": True, + } + + +def test_anthropic_client_retry_drops_thinking_param_since_incompatible_with_forced_tool_choice() -> ( + None +): + messages = FakeMessages( + responses=[ + {"content": [{"type": "thinking", "thinking": "hmm, no tool yet"}]}, + { + "content": [ + {"type": "tool_use", "name": "tap", "input": {"x": 1, "y": 2}} + ] + }, + ] + ) + client = AnthropicToolCallingClient( + model="test-model", + transport=FakeTransport(messages), + thinking_budget_tokens=1024, + ) + + client.decide( + system_prompt="s", user_prompt="u", screenshot=None, tools=[TAP_SPEC], timeout=1 + ) + + assert "thinking" in messages.calls[0] + assert "betas" in messages.calls[0] + assert "thinking" not in messages.calls[1] + assert "betas" not in messages.calls[1] + + # --- OpenAI -------------------------------------------------------------- -def test_openai_tool_calling_client_sends_forced_single_tool_call_request() -> None: +def test_openai_tool_calling_client_sends_auto_tool_choice_request() -> None: completions = FakeCompletions( response={ "choices": [ @@ -270,7 +346,7 @@ def test_openai_tool_calling_client_sends_forced_single_tool_call_request() -> N assert call["timeout"] == 2.5 assert call["max_completion_tokens"] == 1024 assert "max_tokens" not in call - assert call["tool_choice"] == "required" + assert call["tool_choice"] == "auto" assert call["parallel_tool_calls"] is False assert call["tools"] == [ { @@ -419,6 +495,44 @@ def test_openai_tool_calling_client_wraps_malformed_responses(response: object) ) +def test_openai_client_retries_with_forced_tool_choice_when_model_omits_tool_call() -> ( + None +): + completions = FakeCompletions( + responses=[ + {"choices": [{"message": {"content": "just chatting, no tool"}}]}, + { + "choices": [ + { + "message": { + "tool_calls": [ + { + "function": { + "name": "tap", + "arguments": '{"x": 1, "y": 2}', + } + } + ] + } + } + ] + }, + ] + ) + client = OpenAIToolCallingClient( + model="test-model", transport=FakeOpenAITransport(completions) + ) + + decision = client.decide( + system_prompt="s", user_prompt="u", screenshot=None, tools=[TAP_SPEC], timeout=1 + ) + + assert decision.tool_name == "tap" + assert len(completions.calls) == 2 + assert completions.calls[0]["tool_choice"] == "auto" + assert completions.calls[1]["tool_choice"] == "required" + + # --- build_client ---------------------------------------------------------- @@ -498,7 +612,9 @@ def test_anthropic_client_captures_both_thinking_and_text_output() -> None: assert decision.text_output == "Step succeeded. Tapping next." -def test_anthropic_client_tool_only_response_has_none_thinking_and_text_output() -> None: +def test_anthropic_client_tool_only_response_has_none_thinking_and_text_output() -> ( + None +): messages = FakeMessages( response={ "content": [ @@ -524,7 +640,12 @@ def test_openai_client_captures_reasoning_content() -> None: "message": { "reasoning_content": "I reasoned about this step.", "tool_calls": [ - {"function": {"name": "tap", "arguments": '{"x": 1, "y": 2}'}} + { + "function": { + "name": "tap", + "arguments": '{"x": 1, "y": 2}', + } + } ], } } @@ -541,6 +662,35 @@ def test_openai_client_captures_reasoning_content() -> None: assert decision.text_output is None +def test_openai_client_captures_message_content_as_text_output() -> None: + completions = FakeCompletions( + response={ + "choices": [ + { + "message": { + "content": "Previous step succeeded. Now tapping login.", + "tool_calls": [ + { + "function": { + "name": "tap", + "arguments": '{"x": 5, "y": 5}', + } + } + ], + } + } + ] + } + ) + client = OpenAIToolCallingClient( + model="test-model", transport=FakeOpenAITransport(completions) + ) + decision = client.decide( + system_prompt="s", user_prompt="u", screenshot=None, tools=[TAP_SPEC], timeout=1 + ) + assert decision.text_output == "Previous step succeeded. Now tapping login." + + def test_openai_client_no_reasoning_content_gives_none_thinking() -> None: completions = FakeCompletions( response={ @@ -548,7 +698,12 @@ def test_openai_client_no_reasoning_content_gives_none_thinking() -> None: { "message": { "tool_calls": [ - {"function": {"name": "tap", "arguments": '{"x": 1, "y": 2}'}} + { + "function": { + "name": "tap", + "arguments": '{"x": 1, "y": 2}', + } + } ] } }