-
Notifications
You must be signed in to change notification settings - Fork 6
Add Anthropic SDK integration #26
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?
Conversation
- Replace ruby-anthropic with the official anthropic-sdk-beta - Update code to use the new SDK client structure - Add support for tools in the Anthropic client using beta header - Add comprehensive test helpers for Anthropic integration - Update error handling and response parsing - Implement better response object handling 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
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.
Caution
Changes requested ❌
Reviewed everything up to 5aa272a in 2 minutes and 6 seconds. Click for details.
- Reviewed
802
lines of code in10
files - Skipped
0
files when reviewing. - Skipped posting
4
draft 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. ANTHROPIC_TEST_NOTES.md:53
- Draft comment:
Please add a newline at the end of the file for proper POSIX formatting. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% While having a trailing newline is a common convention, this is a minor formatting issue that doesn't affect functionality. Many editors automatically add trailing newlines, and this kind of comment doesn't add significant value. It's more of a stylistic preference than a critical issue requiring attention in a PR review. The comment is technically correct - POSIX text files should end with newlines. This could cause issues with some tools or text processing. While technically correct, this is too minor of an issue to warrant a PR comment. Many modern tools handle files without trailing newlines gracefully, and editors often add them automatically. Delete this comment as it's too minor and obvious of an issue to warrant attention in a PR review.
2. lib/instructor/anthropic/patch.rb:36
- Draft comment:
The validation context interpolation logic is duplicated here. Consider refactoring this block to reuse a helper (e.g., similar to prepare_parameters in base patch) to adhere to DRY principles. - 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% While the comment suggests there's duplication with a method in base patch, I can't see the base patch code to verify this claim. The validation context handling code is part of the new changes, but without seeing the allegedly duplicated code, I can't be certain this is actually duplicated logic. Making assumptions about code in other files violates the "ignore cross-file issues" rule. I might be too strict in applying the cross-file rule. If there's obvious duplication with a well-known base class method, maybe that's worth pointing out. No, the rules are clear - we should only comment on what we can see in this diff. Without seeing the other code, we can't be certain this is actually duplicate logic or that the suggested refactor would be appropriate. Delete the comment because we cannot verify the claimed duplication without seeing the base patch code, and we should ignore cross-file issues per the rules.
3. lib/instructor/base/patch.rb:132
- Draft comment:
The apply_validation_context method assumes the validation_context can be wrapped into an array and then mapped one-to-one with messages. Ensure that the number of messages aligns with the provided context to avoid mismatches. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
4. lib/instructor/anthropic/patch.rb:90
- Draft comment:
In the comment on this line, "invalid_model" is written in lowercase while the condition checks for 'InvalidModel'. For consistency and clarity, consider using 'InvalidModel' in the comment as well. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% This is a very minor stylistic suggestion about comment wording. It doesn't affect functionality, doesn't require any code changes, and is purely informational. According to our rules, we should not make purely informative comments that don't require code changes. While consistency is good, this level of nitpicking about comment text is not valuable enough to warrant a PR comment. The inconsistency could potentially cause confusion for future developers reading the code. Maybe maintaining strict consistency in naming is important for this codebase? Even if consistency is important, this is too minor of an issue to warrant a PR comment. The meaning is still clear, and this kind of nitpicking can slow down PR reviews. Delete this comment as it is purely informational and doesn't require any actual code changes.
Workflow ID: wflow_ZwtyP2YkKI4gIhaN
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
request_options = { extra_headers: { 'anthropic-beta' => 'tools-2024-04-04' } } | ||
|
||
# Get the API key from the environment or the initialized client | ||
api_key = ENV.fetch('ANTHROPIC_API_KEY', 'test-key-for-vcr') |
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.
Avoid re-fetching the API key from ENV
and instantiating a new client inside the messages
method. Consider using the instance's @api_key
(set during initialization) or simply self
to preserve configuration.
api_key = ENV.fetch('ANTHROPIC_API_KEY', 'test-key-for-vcr') | |
api_key = @api_key |
- Keep the simpler patch_spec.rb implementation - Remove helper files for cleaner test implementation - Remove code that handled mock objects - Update spec to use VCR fixtures directly 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
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 f7aaf5a in 2 minutes and 9 seconds. Click for details.
- Reviewed
269
lines of code in2
files - Skipped
0
files when reviewing. - Skipped posting
2
draft 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. spec/helpers/anthropic_mocks.rb:1
- Draft comment:
Legacy mocking module removed. Ensure that all tests previously relying on these stubs (e.g., for JSON parsing errors or specific response patterns) are updated to use the new official SDK mocks. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
2. spec/helpers/anthropic_test_helpers.rb:1
- Draft comment:
Removal of legacy Anthropic test helpers (MockAnthropicSDK) requires confirmation that the new SDK integration provides equivalent functionality for tests. Verify that any customization or method chaining (e.g., the patched messages method) is properly handled in the new setup. - Reason this comment was not posted:
Comment was not on a location in the diff, so it can't be submitted as a review comment.
Workflow ID: wflow_me6QK3kpf58flwjY
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Summary
Test plan
Note: Some example tests require updated VCR cassettes as they were testing with the old SDK structure.
🤖 Generated with Claude Code
Important
This PR integrates the
anthropic-sdk-beta
into the project, updating the codebase and tests to accommodate the new SDK structure and response format.ruby-anthropic
withanthropic-sdk-beta
ininstructor-rb.gemspec
.lib/instructor.rb
to require 'anthropic' instead of 'ruby-anthropic'.lib/instructor/anthropic/patch.rb
to match new SDK API, including message creation and client initialization.lib/instructor/anthropic/response.rb
to handle new response format, includingcontent
andtool_calls
methods.spec/anthropic/patch_spec.rb
for mocking SDK.spec/spec_helper.rb
to handle new SDK requests.ANTHROPIC_TEST_NOTES.md
for integration and testing notes.This description was created by
for f7aaf5a. You can customize this summary. It will automatically update as commits are pushed.