-
Notifications
You must be signed in to change notification settings - Fork 1
fix: correct thinking block handling to prevent 400 errors #35
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
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Fixes issue #12 where thinkingBudget usage caused 400 errors from Anthropic's API due to improper thinking block handling. Changes: - Fix BetaThinkingBlock type: thinking field is string (content), not config object - Add BetaRedactedThinkingBlock type for redacted thinking responses - Update responseToParams to properly handle thinking and redacted_thinking blocks - Add explicit block ordering when pushing assistant messages to ensure thinking blocks always come first (API requirement) - Add test examples for extended thinking (single and multi-turn) The root cause was conflating the thinking CONFIG (what you send to enable thinking) with the thinking BLOCK (what the API returns with actual content).
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.
Important
Looks good to me! 👍
Reviewed everything up to 3947312 in 2 minutes and 19 seconds. Click for details.
- Reviewed
328lines of code in6files - Skipped
0files when reviewing. - Skipped posting
5draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. .changeset/fix-thinking-block-handling.md:1
- Draft comment:
Documentation is clear and detailed. The explanation of the fix and usage instructions are well described. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. examples/test-extended-thinking-multi-turn.ts:55
- Draft comment:
The use of the expression"=".repeat(70)is clever but the extra space before.repeatmight impact readability. Consider removing the space for clarity. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
3. loop.ts:277
- Draft comment:
The explicit block ordering for assistant messages is a good solution. However, any new block types should be explicitly added to theorderdictionary to avoid unexpected ordering. Also, verify that the extra request payload (extraBody.thinkingset as a config object) aligns with the updated response type that now expects a string. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =50%<= threshold50%The comment suggests adding new block types to a dictionary to avoid unexpected ordering, which is a specific code suggestion. However, the second part of the comment asks to verify alignment between request payload and response type, which is not allowed as it asks for verification. The first part is useful, but the second part violates the rules.
4. types/beta.ts:48
- Draft comment:
BetaThinkingBlock now requires a string for the 'thinking' field and a mandatory 'signature'. Ensure that all components generating these blocks supply a valid signature; otherwise, a default (like an empty string) might be needed. - Reason this comment was not posted:
Comment did not seem useful. Confidence is useful =20%<= threshold50%The comment is informative and suggests ensuring that components supply a valid signature or use a default. It doesn't provide a specific code suggestion or ask for a test to be written. It seems to be more of a reminder or caution, which violates the rules.
5. utils/message-processing.ts:18
- Draft comment:
When converting 'thinking' blocks in responseToParams, consider preserving additional properties (e.g. id and cache_control) so that metadata isn’t lost for later processing. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% The comment is speculative - it says "consider preserving" and mentions properties "for later processing" without showing evidence that these properties exist or are needed. The old code did use...restwhich would preserve extra properties, but the new code explicitly doesn't. However, I don't see any evidence in the file thatidorcache_controlproperties are actually used on thinking blocks. The comment is making an assumption about what might be needed without demonstrating that it's actually a problem. This violates the rule about not making speculative comments. The old code did use...restto preserve additional properties, so the author intentionally changed this behavior. Maybe there's a reason they wanted to be more explicit and only include specific properties. Without seeing the type definitions or usage elsewhere, I can't be certain this isn't a real issue. While the old code did preserve extra properties, the fact that the author explicitly changed this suggests it was intentional. The comment is speculative because it doesn't show evidence thatidorcache_controlare actually present on thinking blocks or needed elsewhere. Without strong evidence of a problem, this should be deleted. Delete this comment. It's speculative and doesn't provide strong evidence that the suggested properties are actually needed. The change from...restto explicit properties appears intentional, and the comment doesn't demonstrate a concrete issue.
Workflow ID: wflow_RmCbR9KYpcKlqUeL
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Important
Fixes thinking block handling to prevent 400 errors by correcting type definitions and ensuring proper block ordering.
BetaThinkingBlocktype to usestringforthinking.BetaRedactedThinkingBlocktype for redacted thinking responses.loop.tsandutils/message-processing.ts.test-extended-thinking-multi-turn.tsandtest-extended-thinking.tsto validate multi-turn and single-turn thinking block handling.responseToParamsinutils/message-processing.tsto handle new block types.samplingLoopinloop.tsto include block ordering logic.This description was created by
for 3947312. You can customize this summary. It will automatically update as commits are pushed.