# Claude Review: PR #1330

- PR: https://github.com/KestrelSovereignAI/kestrel-sovereign/pull/1330
- Title: fix: replay provider reasoning through tool loops
- Reviewed: 2026-05-21T15:23:32Z

## PR Review: Provider Reasoning Roundtrip

### No blocking findings.

The diff is clean, well-scoped, and the refactor eliminates real duplication (6 identical inline blocks → 1 method). Tests cover the key matrix. A few residual observations:

---

### Low severity

**1. Exact-pin on `kestrel-llms[all]==0.1.4` may cause install conflicts downstream**
`pyproject.toml:129` — pinning `==0.1.4` means any bugfix release (0.1.5) requires a coordinated sovereign bump. The lockfile already pins transitives, so `~=0.1.4` (compatible release) would give the same lockfile resolution today while allowing patch-level fixes without a sovereign PR. Not blocking — you clearly chose this deliberately for reproducibility — but worth noting the maintenance cost.

**2. `_extract_response_reasoning_content` silently swallows non-dict, non-OpenAI `raw` shapes**
`orchestrator_engine.py:710-718` — If a future provider sets `raw` to something truthy but neither a dict nor an object with `.choices`, the method returns `None` silently. That's fine today (DeepSeek/Kimi/xAI all hit one of the two branches), but a debug log on the fallthrough would save future debugging time. Not blocking.

**3. `reasoning_content` key is provider-convention, not OpenAI-standard**
The key `reasoning_content` in the assistant message dict is a DeepSeek/Kimi convention. OpenAI's own reasoning models use a different structure (reasoning tokens in `reasoning` or opaque). If the orchestrator ever sends these messages back to a provider that doesn't understand that key, it'll be ignored (harmless) or rejected (provider-specific). The guard `if reasoning_content and response.tool_calls` correctly limits blast radius. Fine as-is.

---

### Positive observations

- **Streaming/non-streaming parity**: All 6 call sites (3 non-streaming, 3 streaming) now use `_build_assistant_tool_history_msg`. Good.
- **Reasoning gated on tool_calls**: The comment at line 730-731 correctly explains why text-only turns omit reasoning — avoids polluting follow-up context. Test `test_assistant_tool_history_omits_reasoning_without_tool_calls` covers this.
- **Test matrix**: 4 tests cover raw-dict path, OpenAI-response-object path, empty-string rejection, and no-tool-calls exclusion. The two extraction shapes match the two provider families.
- **Lockfile**: `uv.lock` additions are consistent with `pyproject.toml`. Hashes present, upload timestamps recent.
- **No regressions to existing tests**: The existing `test_no_tool_continuation_gets_one_repair_step` test is untouched and the helper binding it uses (`_build_tool_calls_msg`) is preserved.

**Verdict: Ship it.**
