Forced tool_choice ("any"/"required") makes both Anthropic and OpenAI
skip any text/thinking block before the tool call, which silently made
rationale and thinking always None despite the planner-reflection-history
change's capture code being correct. Switch the primary call to
tool_choice="auto" (Anthropic: type=auto, disable_parallel_tool_use=true;
OpenAI: "auto") so the model can emit its reflection text, and add a
one-time forced retry (Anthropic "any", OpenAI "required", thinking
disabled) if the model responds without a tool call, guaranteeing a step
never stalls. Also add OpenAI text_output capture from message.content,
which was never extracted before (Anthropic-only gap).
Update planner-reflection-history design.md/tasks.md to document the bug
found during the pending manual smoke test (task 8.5) and the fix (new
section 9).
This commit is contained in:
@@ -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.
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user