Fix model access errors unhelpful #43
Conversation
a7874d4 to
377d6a6
Compare
|
Looks pretty good, add a test please that hits those errors? |
377d6a6 to
d043935
Compare
|
@dblock Sorry about that. I was unsure of how best to stub/mock in order to trigger these specific errors but I think I've got it figured out now. Have a look and let me know what you think. |
d043935 to
445319e
Compare
There was a problem hiding this comment.
This is looking good.
I really dislike catching NoMethodError though - we should prevent this from happening in code before the error happens. Why are we ever calling .chat on a client that could not be initialized? (you can totally split this PR if you want and add the NoResourceFound handling first).
lib/roast/workflow/base_workflow.rb
Outdated
| rescue NoMethodError => e | ||
| execution_time = Time.now - start_time | ||
|
|
||
| if e.message.include?("undefined method 'chat' for nil") |
There was a problem hiding this comment.
Can we catch this one earlier? IMO it should never get here.
There was a problem hiding this comment.
I've moved the logic to much earlier in the code for catching this sort of misconfiguration.
259038c to
25943d5
Compare
25943d5 to
9f3372c
Compare
|
@dblock I've added a test that should cover the OpenRouter api token missing. I think this should be enough to ensure we've tested |
In order to make errors more descriptive when things go wrong, this commit adds a few error types to handle api interactions more gracefully when things go wrong. Specifically, it currently handles when a model can't be found or is innaccesible with the api token provided. Also when an api token isn't provided, instead of an unhelpful NoMethodError, we let end users know the token wasn't provided or isn't valid.
9f3372c to
ca8e563
Compare
|
I've rebased against the latest main branch so there shouldn't be any more conflicts. |
This implements the error handling improvements originally proposed in PR #43 by @endoze, updated to work with the current codebase architecture. - Add custom error classes for ResourceNotFoundError and AuthenticationError - Catch Faraday 404 errors and convert to descriptive ResourceNotFoundError that includes the actual error message from the API response body - Validate API tokens during initialization with a lightweight API call - Provide clear error messages for authentication failures - Add comprehensive test coverage for error scenarios This fixes #13 by surfacing helpful error messages when models don't exist or users lack access, instead of generic Faraday errors. Co-authored-by: endoze <endoze@users.noreply.github.com> 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
|
Hi @endoze! 👋 Thank you so much for this contribution! Your implementation for better error handling was exactly what we needed to fix issue #13. Since this PR was based on an older version of our codebase (before some significant refactoring), I've taken your excellent work and updated it to be compatible with our current architecture in PR #82, which has now been merged. Your original implementation has been preserved with full credit as co-author on the commit. The core improvements you made are all there:
I'm closing this PR now that the functionality has been merged, but please know that your contribution is very much appreciated and is now part of Roast! 🎉 Thanks again for helping make Roast better! |
In order to make errors more descriptive when things go wrong, this commit adds a few error types to handle api interactions more gracefully when things go wrong. Specifically, it currently handles when a model can't be found or is innaccesible with the api token provided. Also when an api token isn't provided, instead of an unhelpful NoMethodError, we let end users know the token wasn't provided or isn't valid.
This resolves #13