Feat/api error - #5
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds project formatting/lint configs and Makefile changes; marks several message and core predicate methods as ChangesConfiguration & Linting Enforcement
API Error Infrastructure
API Surface Small Adjustments
Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Review rate limit: 9/10 reviews remaining, refill in 6 minutes. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/api_error.rs (1)
571-573: ⚡ Quick win
HttpandIovariants always map to a single code regardless of actual error type.The
code()method maps allApiError::Httperrors toHttpConnectionErrorand allApiError::Ioerrors toIoReadError, even though theErrorCodeenum defines more specific codes likeHttpRequestError,HttpResponseError,IoFileNotFound, andIoWriteError.Consider applying similar keyword-based classification for these variants, or documenting that callers should use specific constructors to get accurate codes.
♻️ Example: Add keyword matching for Io errors
- Self::Io(_) => ErrorCode::IoReadError, + Self::Io(e) => { + let msg = e.to_string().to_lowercase(); + if e.kind() == std::io::ErrorKind::NotFound { + ErrorCode::IoFileNotFound + } else if msg.contains("write") || msg.contains("permission denied") { + ErrorCode::IoWriteError + } else { + ErrorCode::IoReadError + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api_error.rs` around lines 571 - 573, The code path in the ApiError::code() method currently maps ApiError::Http and ApiError::Io to generic ErrorCode::HttpConnectionError and ErrorCode::IoReadError; update code() so it inspects the inner error to pick a more specific ErrorCode (for example: for ApiError::Io inspect the std::io::ErrorKind to return IoFileNotFound, IoWriteError, etc.; for ApiError::Http inspect the inner HTTP error type/message (request vs response vs connection) or known keywords to return HttpRequestError, HttpResponseError, etc.), falling back to the generic codes if no specific match is found; reference the ApiError::Http and ApiError::Io variants and the code() method and map to the ErrorCode enum values accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/api_error.rs`:
- Around line 571-573: The code path in the ApiError::code() method currently
maps ApiError::Http and ApiError::Io to generic ErrorCode::HttpConnectionError
and ErrorCode::IoReadError; update code() so it inspects the inner error to pick
a more specific ErrorCode (for example: for ApiError::Io inspect the
std::io::ErrorKind to return IoFileNotFound, IoWriteError, etc.; for
ApiError::Http inspect the inner HTTP error type/message (request vs response vs
connection) or known keywords to return HttpRequestError, HttpResponseError,
etc.), falling back to the generic codes if no specific match is found;
reference the ApiError::Http and ApiError::Io variants and the code() method and
map to the ErrorCode enum values accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 00866bea-477b-4e31-a71a-b0d2dd2537ff
📒 Files selected for processing (8)
.clippy.tomlCargo.tomlMakefilerustfmt.tomlsrc/api_error.rssrc/core/error.rssrc/lib.rssrc/message.rs
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
Makefile (1)
1-1: ⚡ Quick winAdd conventional
allandcleantargets for better Makefile interoperability.Line 1 updates
.PHONY, and this is a good place to also includeallandcleansince some tooling expects them (also matches the static-analysis warning).Proposed patch
-.PHONY: check test clippy fmt docs ci lint +.PHONY: all clean check test clippy fmt docs ci lint + +all: ci + +clean: + cargo clean🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile` at line 1, Update the Makefile to advertise and implement conventional targets: add "all" and "clean" to the .PHONY list on the existing .PHONY line and add corresponding "all:" (typically defaulting to the main build or test target such as "test" or "check") and "clean:" targets that remove generated artifacts (e.g., build outputs, temp files) so tools expecting standard targets can operate; ensure the "all" target depends on the appropriate default target (e.g., "check" or "test") and "clean" runs the cleanup commands used elsewhere in the repo.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api_error.rs`:
- Around line 548-573: The match arm in ApiError::code() currently collapses all
HTTP errors into ErrorCode::HttpConnectionError, making
HttpRequestError/HttpResponseError unreachable (e.g., errors created via
ApiError::http_with_status) and misclassifying retryability; update
ApiError::code() to inspect the Http variant payload (or parse the "HTTP
{status}:" shape) inside the Self::Http(_) arm so that status-based mapping
returns ErrorCode::HttpRequestError for 4xx (client) responses and
ErrorCode::HttpResponseError for 5xx (server) responses (and preserve
HttpConnectionError for transport-level failures), ensuring is_retryable() sees
the correct ErrorCode; reference ApiError::http_with_status, the Self::Http(_)
match arm, ErrorCode::HttpRequestError, ErrorCode::HttpResponseError, and
is_retryable() when implementing.
- Around line 574-586: The error classification in the ApiError::Tool arm
(Self::Tool(msg)) uses substring matches on msg.to_lowercase(), causing
misclassification (e.g., "file not found" treated as ToolNotFound); update the
matching to check for constructor-shaped prefixes with starts_with on the
lowercased string (e.g., starts_with("tool not found:"), starts_with("permission
denied:"), starts_with("timeout:"), starts_with("invalid input:") ) and fall
back to ErrorCode::ToolExecutionFailed when none of those prefixes match; keep
the lowercase normalization (msg_lower) and only replace contains(...) checks
with starts_with(...) using the specific prefix patterns in the Self::Tool match
arm.
- Around line 97-98: The ErrorCode enum is currently derived with serde
Serialize/Deserialize so it serializes to variant names; change it to serialize
as numeric discriminants by adding #[repr(u16)] to the enum and replacing serde
derives with serde_repr ones: remove Serialize and Deserialize from the derive
and instead add #[derive(Serialize_repr, Deserialize_repr, Debug, Clone, Copy,
PartialEq, Eq, Hash)] (from the serde_repr crate) so ErrorCode (the enum)
serializes/deserializes as its numeric value (e.g., 1000) rather than the
variant name.
- Around line 437-443: The Io variant currently converts any std::io::Error into
a single ErrorCode (IoReadError) making IoFileNotFound and IoWriteError
unreachable; add explicit constructors on ApiError such as
ApiError::io_file_not_found(std::io::Error), ApiError::io_read(std::io::Error),
and ApiError::io_write(std::io::Error) (or alternately accept an operation-kind
enum alongside the source error) and ensure each constructor sets the correct
error code (ErrorCode::IoFileNotFound, ErrorCode::IoReadError,
ErrorCode::IoWriteError) while storing the source std::io::Error so tests that
assert ApiError::IoFileNotFound or IoWriteError can construct those cases;
update places that relied on the #[from] conversion to call the appropriate
constructor or provide a converting function that maps ErrorKind::NotFound to
ApiError::io_file_not_found when context indicates a file access.
---
Nitpick comments:
In `@Makefile`:
- Line 1: Update the Makefile to advertise and implement conventional targets:
add "all" and "clean" to the .PHONY list on the existing .PHONY line and add
corresponding "all:" (typically defaulting to the main build or test target such
as "test" or "check") and "clean:" targets that remove generated artifacts
(e.g., build outputs, temp files) so tools expecting standard targets can
operate; ensure the "all" target depends on the appropriate default target
(e.g., "check" or "test") and "clean" runs the cleanup commands used elsewhere
in the repo.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 8b8d0ddf-171d-4f98-8d7a-4e4396309fb8
📒 Files selected for processing (5)
Makefilesrc/api_error.rssrc/core/error.rssrc/lib.rssrc/message.rs
✅ Files skipped from review due to trivial changes (2)
- src/lib.rs
- src/message.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/error.rs
Comment on lines
+574
to
+586
| Self::Tool(msg) => { | ||
| let msg_lower = msg.to_lowercase(); | ||
| if msg_lower.contains("not found") { | ||
| ErrorCode::ToolNotFound | ||
| } else if msg_lower.contains("permission") || msg_lower.contains("denied") { | ||
| ErrorCode::ToolPermissionDenied | ||
| } else if msg_lower.contains("timeout") { | ||
| ErrorCode::ToolTimeout | ||
| } else if msg_lower.contains("invalid") { | ||
| ErrorCode::ToolInputInvalid | ||
| } else { | ||
| ErrorCode::ToolExecutionFailed | ||
| } |
Contributor
There was a problem hiding this comment.
Tighten tool classification to the constructor prefixes instead of any substring match.
A generic execution failure like ApiError::tool_with_name("Read", "file not found") will currently classify as ToolNotFound, even though the tool exists and the missing thing is the file. Matching exact constructor-shaped prefixes such as starts_with("tool not found:") avoids turning arbitrary tool stderr into the wrong error code.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api_error.rs` around lines 574 - 586, The error classification in the
ApiError::Tool arm (Self::Tool(msg)) uses substring matches on
msg.to_lowercase(), causing misclassification (e.g., "file not found" treated as
ToolNotFound); update the matching to check for constructor-shaped prefixes with
starts_with on the lowercased string (e.g., starts_with("tool not found:"),
starts_with("permission denied:"), starts_with("timeout:"), starts_with("invalid
input:") ) and fall back to ErrorCode::ToolExecutionFailed when none of those
prefixes match; keep the lowercase normalization (msg_lower) and only replace
contains(...) checks with starts_with(...) using the specific prefix patterns in
the Self::Tool match arm.
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
No description provided.