feat: carry reasoning across turns with per-backend replay policy#1374
Conversation
ajbozarth
left a comment
There was a problem hiding this comment.
Reviewed the full diff and traced the capture → _parse → policy → serialize path. Solid, conservative change — no correctness blockers. All 72 new/changed tests pass locally. Action items below.
Before merge
- Complete the live-model verification. The e2e test (
test_reasoning_replay_e2e.py) is env-gated and CI-skipped, so nothing in CI exercisesreasoning_content/thinkingagainst a real endpoint. The PR body's own "still needs verification" note stands — this is the one thing that would catch a provider rejecting the injected wire key. - Tighten the PR body to say this lands the consensus rule only. The title says "per-backend replay policy" and
provideris a parameter, but the policy is currently provider-agnostic (provideris unused, documented as a reserved hook). The divergent contracts from #1201's table — DeepSeek hard-400 without tools, Anthropic signed blocks — are not implemented here. State that explicitly so reviewers/users don't assume DeepSeek/Anthropic are covered.
Non-blocking
- HF chat path is silently uncovered (see inline comment). Functionally fine given HF's capture gap, but a one-liner would prevent a future reader assuming coverage.
Verified separately
should_replay_reasoningreturns exactlylen(messages)flags, so thezipin each backend never truncates.- The adjacency check correctly maps to "this turn issued a tool call" —
_acall_toolsappendsToolMessage(role="tool")right after the tool-issuing turn. - Default
replay_reasoning=Falsepreserves historical behavior. - Real OpenAI Chat Completions never populates
thinking, so the non-standard key is never sent there.
|
|
|
This LGTM as far as Claude reviews go , but I'd prefer to defer final approval to @jakelorocco after he returns since this is a decently sized change |
jakelorocco
left a comment
There was a problem hiding this comment.
I think the should_replay_reasoning function is a good addition and makes sense. However, I worry that the re-serialization will continue to be problematic.
I keep pushing for this solution (and I understand that there are continuing complexities), but I feel that we should just resend the received message in cases like this. It will cause problems with streaming and for cross backend/model requests, but I think those are worth fixing to get that solution shape.
| # The prior turn "had a tool call" iff the next message is a tool result. | ||
| prior_turn_had_tool_call = ( | ||
| i + 1 < len(messages) and messages[i + 1].role == "tool" | ||
| ) |
There was a problem hiding this comment.
What happens if the assistant requests a tool call but we don't call the tool (leading to no message with role == tool)?
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
|
Rebased to fix conflicts. |
87167d6
…nerative-computing#1374) * feat: carry reasoning across turns with per-backend replay policy Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com> * add note about hf reasoning Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com> * handle tool request instead of direct usage Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com> * update reasoning replay tests Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com> --------- Signed-off-by: AngeloDanducci <angelo.danducci.ii@ibm.com>
Pull Request
Issue
Fixes #1201
Description
Carry reasoning across turns with per-backend replay policy for better thinking handling.
The policy is currently provider-agnostic (provider is unused, documented as a reserved hook). The divergent contracts from #1201 table — DeepSeek hard-400 without tools, Anthropic signed blocks — are not implemented here.
Testing
Attribution
Adding a new component, requirement, sampling strategy, or tool?
If your PR adds or modifies one of the types below, check the matching box. A checklist of type-specific review items will be posted as a comment.
NOTE: Please ensure you have an issue that has been acknowledged by a core contributor and routed you to open a pull request against this repository. Otherwise, please open an issue before continuing with this pull request.