Skip to content

Conversation

jxnl
Copy link
Contributor

@jxnl jxnl commented May 14, 2025

Summary

  • Replace ruby-anthropic with the official anthropic-sdk-beta
  • Update code to use the new SDK client structure
  • Add better handling of response objects
  • Add test helpers that properly mock the SDK

Test plan

  • All OpenAI tests continue to pass
  • Anthropic tests are now passing with mock SDK
  • Manual verification of the SDK integration works with real API calls

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.

  • Integration:
    • Replace ruby-anthropic with anthropic-sdk-beta in instructor-rb.gemspec.
    • Update lib/instructor.rb to require 'anthropic' instead of 'ruby-anthropic'.
    • Modify lib/instructor/anthropic/patch.rb to match new SDK API, including message creation and client initialization.
  • Response Handling:
    • Update lib/instructor/anthropic/response.rb to handle new response format, including content and tool_calls methods.
  • Testing:
    • Add test helpers in spec/anthropic/patch_spec.rb for mocking SDK.
    • Update VCR configuration in spec/spec_helper.rb to handle new SDK requests.
  • Misc:
    • Add ANTHROPIC_TEST_NOTES.md for integration and testing notes.

This description was created by Ellipsis for f7aaf5a. You can customize this summary. It will automatically update as commits are pushed.

- 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>
Copy link

cloudflare-workers-and-pages bot commented May 14, 2025

Deploying instructor-rb-docs with  Cloudflare Pages  Cloudflare Pages

Latest commit: f7aaf5a
Status:🚫  Build failed.

View logs

Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 10 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 Ellipsis 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')
Copy link
Contributor

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.

Suggested change
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>
Copy link
Contributor

@ellipsis-dev ellipsis-dev bot left a 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 in 2 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 Ellipsis 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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant