Skip to content
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

Fix tool name method to strip unsupported characters for OpenAI #28

Merged
merged 5 commits into from
Mar 23, 2025

Conversation

stevegeek
Copy link
Contributor

Updates the name method in the Tool class to strip unsupported characters and replace them with '-' characters as OpenAI only supports tool names with /a-zA-Z0-9_-/+

Updates the `name` method in the `Tool` class to strip unsupported characters and replace them with '-' characters as OpenAI only supports tool names with `/a-zA-Z0-9_-/+`

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/stevegeek/ruby_llm?shareId=XXXX-XXXX-XXXX-XXXX).
@crmne
Copy link
Owner

crmne commented Mar 17, 2025

This is a great first contribution! Small, focused, and tested - exactly the kind of PR that's easy to review and merge.

One small suggestion: instead of simply replacing invalid characters with hyphens, could we implement something that converts to a more semantically appropriate alternative? For example, SampleTòol would become sample_tool rather than sample_t-ol.

If there's no straightforward way to implement this, I'm inclined to accept the PR as-is since it fixes a real issue. Let me know your thoughts!

@stevegeek
Copy link
Contributor Author

Ok will find some time soon to update to transliterate appropriately!

Copy link
Owner

@crmne crmne left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty good overall. Just waiting on this .unicode_normalize(:nfkd).encode('ASCII', replace: '') and the other few comments to be addressed.

@crmne crmne added the bug Something isn't working label Mar 21, 2025
…n supported characters.

Also updates to test file to remove some pointless test cases
@stevegeek stevegeek requested a review from crmne March 21, 2025 14:06
@crmne
Copy link
Owner

crmne commented Mar 21, 2025

Looks like overcommit was not installed and therefore rubocop complained. Here's how to fix both issues:

overcommit --install
rubocop -A

@stevegeek
Copy link
Contributor Author

doh sorry

@crmne
Copy link
Owner

crmne commented Mar 21, 2025

no problem! :)

@stevegeek stevegeek requested a review from crmne March 21, 2025 14:32
@crmne
Copy link
Owner

crmne commented Mar 21, 2025

@stevegeek
Copy link
Contributor Author

@crmne ok sure, if I can be of any help, let me know.

@crmne
Copy link
Owner

crmne commented Mar 21, 2025

I was checking into the permissions for running tests safely, and I think we need to take a different approach.

Instead of trying to dance around GitHub Actions security model with labels and pull_request_target, let's do this right by implementing VCR to record our API interactions. This will let us run the test suite against PRs without exposing our real API keys, and as a bonus make the tests faster and more reliable.

I'm going to work on adding VCR support before proceeding with this PR. Doing proper testing without compromising security is worth the small delay.

Will get back to you soon with the updated approach.

Copy link

codecov bot commented Mar 23, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.75%. Comparing base (8fce3ac) to head (582ac34).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main      #28      +/-   ##
==========================================
+ Coverage   92.67%   92.75%   +0.07%     
==========================================
  Files          54       55       +1     
  Lines        1665     1683      +18     
  Branches      284      284              
==========================================
+ Hits         1543     1561      +18     
  Misses        122      122              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@crmne crmne merged commit d34592e into crmne:main Mar 23, 2025
4 checks passed
@stevegeek stevegeek deleted the fix-tool-name-method branch March 23, 2025 20:28
OriPekelman pushed a commit to OriPekelman/ruby_llm that referenced this pull request Mar 24, 2025
…mne#28)

Updates the `name` method in the `Tool` class to strip unsupported
characters and replace them with the closest characters as OpenAI only supports
tool names with `/a-zA-Z0-9_-/+`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants