-
Notifications
You must be signed in to change notification settings - Fork 3k
fix(core): preserve every reasoning episode's signature during history consolidation #8260
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
c23d899
b7fe21c
2a76e3e
da4b3e3
8a2a7ed
c403fb3
f907a0f
a7f4df4
11b7458
9b84e79
13867dc
60d4103
ad1c54b
2fe2ee3
6b3e68a
14292a7
36cdca8
ce58a20
345f0f9
19dcebf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -802,6 +802,12 @@ export class AnthropicContentGenerator implements ContentGenerator { | |
| dropUnsignedAssistantThinking, | ||
| stripAssistantThinking, | ||
| stripTrailingAssistantPrefill, | ||
| // Manual (non-adaptive) extended thinking requires an assistant | ||
| // turn to begin with a thinking block whenever a tool_use remains | ||
| // in it; adaptive thinking relaxes this. Applied to every such turn | ||
| // in history, not just the latest -- see | ||
| // ensureLeadingAssistantThinking's doc in the converter. | ||
| ensureLeadingAssistantThinking: thinking?.type === 'enabled', | ||
|
netbrah marked this conversation as resolved.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [Suggestion] This gate is pinned by tests for only one of the two Failure scenario: such a refactor would silently disable normalization for manual-mode ladder models — the generation where the manual leading-thinking wire rule originated — and since this PR's removal of the Suggested fix: add a third generator-level test mirroring the manual-mode one with a pre-4.6 model (e.g. 中文说明该门控只有"显式 失败场景:这样的重构会静默关闭 manual 模式阶梯型号的归一化——而 manual leading-thinking 链路规则正是起源于这一代型号;且本 PR 移除了 — qwen3.8-max via Qwen Code /review (v0.21.10)
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Confirmed and fixed in Applied your exact gate mutation (deriving from Added a third generator-level test using |
||
| enableCacheControl, | ||
| useGlobalCacheScope, | ||
| cacheRetention, | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[Suggestion] This adaptive-mode fixture ends on a model turn with an unanswered
functionCall t1:stripTrailingAssistantPrefillkeeps the trailing assistant and appends a'Continue.'user turn, so the converted request carriestool_use t1followed by a text-only user message — the exact shapemergeConsecutiveAssistantMessages's own doc block quotes as HTTP 400 ("tool_use ids were found without tool_result blocks immediately after");cleanOrphanedToolCallsruns earlier in the pipeline and protects the trailing message. Probe-verified at this commit: the converted messages end[assistant [text, thinking, tool_use t1], user 'Continue.']. Also verified the assertion is not load-bearing on the unrealistic tail — appending thefunctionResponsefort1yields an API-valid request whose latest assistant is still['text','thinking','tool_use'].Failure scenario: the fixture models a request the real API rejects, passing only because the client is mocked — contradicting the realism standard its sibling manual-mode test documents in this same describe block; it would also resist a legitimate future fix (re-running
cleanOrphanedToolCallsafter the prefill pass would drop thetool_useand fail this assertion).Suggested fix: mirror the manual-mode test by appending the
functionResponseuser turn fort1after the tool turn; the assertion is unchanged.中文说明
该 adaptive 模式 fixture 以一个带未应答
functionCall t1的模型轮次结尾:stripTrailingAssistantPrefill会保留这条尾部 assistant 消息并追加'Continue.'用户轮,于是转换后的请求变成tool_use t1后面紧跟一条纯文本用户消息——正是mergeConsecutiveAssistantMessages文档块中引用的 HTTP 400 形状("tool_use ids were found without tool_result blocks immediately after");cleanOrphanedToolCalls在流水线更早处运行,且对尾部消息有保护。已在本 commit 上用探针验证:转换结果以[assistant [text, thinking, tool_use t1], user 'Continue.']结尾。另已验证该断言并不依赖这个不真实的尾部——为t1补上functionResponse后请求变为 API 合法形状,最新 assistant 仍为['text','thinking','tool_use']。失败场景:fixture 模拟的是真实 API 会拒绝的请求,只因客户端被 mock 才通过——与同一 describe 块中 manual 模式兄弟测试明确记录的真实性标准相矛盾;它还会阻碍未来合理的修复(若在 prefill 之后重跑
cleanOrphanedToolCalls,tool_use会被清掉,本断言随之失败)。— qwen3.8-max via Qwen Code /review (v0.21.10)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed and fixed in
2fe2ee32b.You were right that the assertion doesn't depend on the unrealistic tail. Appended the
functionResponseuser turn fort1, mirroring the manual-mode sibling: the request now ends on atool_result, no synthetic'Continue.'is appended, and the latest assistant is still['text','thinking','tool_use'].The assertion is byte-identical — the commit is 293 insertions with zero deletions, so nothing existing was weakened. Also agree on the forward-compatibility point: had
cleanOrphanedToolCallslater been re-run after the prefill pass, the old fixture would have started failing for a reason unrelated to what it tests.