Introduce Centralized Typed Error Model Across SDKs#436
Introduce Centralized Typed Error Model Across SDKs#436Shalini828 wants to merge 1 commit intoRunanywhereAI:mainfrom
Conversation
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 7261465 in 11 seconds. Click for details.
- Reviewed
130lines of code in4files - Skipped
0files when reviewing. - Skipped posting
0draft 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.
Workflow ID: wflow_D0Zbnbe3v1dYtNDQ
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
📝 WalkthroughWalkthroughA centralized error model system is introduced for the RunAnywhere SDK with a structured type containing error code, message, and category. Functions for constructing error models and mapping error codes to categories are added. The logger is updated to use this model for enhanced error reporting. Changes
Sequence DiagramsequenceDiagram
participant Logger as Logger
participant ErrorModel as Error Model System
participant CategoryMap as Category Mapping
Logger->>ErrorModel: rac_make_error_model(error_code)
ErrorModel->>CategoryMap: rac_error_category(error_code)
CategoryMap-->>ErrorModel: category string
ErrorModel->>ErrorModel: Fetch message via rac_error_message()
ErrorModel-->>Logger: rac_error_model_t {code, message, category}
Logger->>Logger: Log structured error details
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
sdk/runanywhere-commons/include/rac/core/rac_error_model.h (1)
16-20: Document pointer ownership/lifecycle formessageandcategory.As per coding guidelines, public C API headers must document lifecycle requirements. The
messageandcategorypointers are returned byrac_error_message()andrac_error_category()respectively, which return static string literals. Callers need to know they should not free these pointers and that they remain valid indefinitely.📝 Suggested documentation enhancement
typedef struct { rac_result_t code; /**< Numeric error code */ - const char* message; /**< Human-readable error message */ - const char* category; /**< Error category (e.g., Model, Network, Validation) */ + const char* message; /**< Human-readable error message (static string, do not free) */ + const char* category; /**< Error category (static string, do not free) */ } rac_error_model_t;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/include/rac/core/rac_error_model.h` around lines 16 - 20, Update the public header docs for rac_error_model_t to state ownership/lifetime for the pointers: clarify that the fields message and category (and the values returned by rac_error_message() and rac_error_category()) point to static string literals, must not be freed by callers, and remain valid for the lifetime of the process (i.e., indefinitely); add this brief note alongside the typedef comment for message and category so users know they do not own or need to free those pointers.sdk/runanywhere-commons/src/core/rac_error_model.cpp (2)
6-8: Misleading comment:rac_error_categoryis part of the public API.The comment labels this as an "Internal Helper," but
rac_error_categoryis declared in the public header and is part of the public API.📝 Proposed fix
-// ------------------------------------------------------------ -// Internal Helper: Determine Category from Error Code Range -// ------------------------------------------------------------ +// ------------------------------------------------------------ +// Public API: Determine Category from Error Code Range +// ------------------------------------------------------------🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/src/core/rac_error_model.cpp` around lines 6 - 8, The comment preceding rac_error_category incorrectly calls it an "Internal Helper" even though rac_error_category is part of the public API; update the comment near the rac_error_category function to reflect that it is public API (e.g., "Public API: Determine Category from Error Code Range" or similar) so the header and implementation comments are consistent with its public declaration, and ensure no wording implies it is internal/private.
4-4: Remove unused include.
<string.h>is included but no string functions are used in this file.🧹 Proposed fix
`#include` "rac/core/rac_error_model.h" `#include` "rac/core/rac_error.h" - -#include <string.h>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@sdk/runanywhere-commons/src/core/rac_error_model.cpp` at line 4, Remove the unused C header include by deleting the line that includes <string.h> from rac_error_model.cpp; ensure no string.h-dependent code remains (there are none), so simply remove the `#include` <string.h> directive to clean up the translation unit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@sdk/runanywhere-commons/include/rac/core/rac_error_model.h`:
- Around line 16-20: Update the public header docs for rac_error_model_t to
state ownership/lifetime for the pointers: clarify that the fields message and
category (and the values returned by rac_error_message() and
rac_error_category()) point to static string literals, must not be freed by
callers, and remain valid for the lifetime of the process (i.e., indefinitely);
add this brief note alongside the typedef comment for message and category so
users know they do not own or need to free those pointers.
In `@sdk/runanywhere-commons/src/core/rac_error_model.cpp`:
- Around line 6-8: The comment preceding rac_error_category incorrectly calls it
an "Internal Helper" even though rac_error_category is part of the public API;
update the comment near the rac_error_category function to reflect that it is
public API (e.g., "Public API: Determine Category from Error Code Range" or
similar) so the header and implementation comments are consistent with its
public declaration, and ensure no wording implies it is internal/private.
- Line 4: Remove the unused C header include by deleting the line that includes
<string.h> from rac_error_model.cpp; ensure no string.h-dependent code remains
(there are none), so simply remove the `#include` <string.h> directive to clean up
the translation unit.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
sdk/runanywhere-commons/CMakeLists.txtsdk/runanywhere-commons/include/rac/core/rac_error_model.hsdk/runanywhere-commons/src/core/rac_error_model.cppsdk/runanywhere-commons/src/core/rac_logger.cpp
| const char* rac_error_category(rac_result_t code) { | ||
| if (code >= -109 && code <= -100) return "Initialization"; | ||
| if (code >= -129 && code <= -110) return "Model"; | ||
| if (code >= -149 && code <= -130) return "Generation"; | ||
| if (code >= -179 && code <= -150) return "Network"; | ||
| if (code >= -219 && code <= -180) return "Storage"; | ||
| if (code >= -229 && code <= -220) return "Hardware"; | ||
| if (code >= -249 && code <= -230) return "ComponentState"; | ||
| if (code >= -279 && code <= -250) return "Validation"; | ||
| if (code >= -299 && code <= -280) return "Audio"; | ||
| if (code >= -319 && code <= -300) return "LanguageVoice"; | ||
| if (code >= -329 && code <= -320) return "Authentication"; | ||
| if (code >= -349 && code <= -330) return "Security"; | ||
| if (code >= -369 && code <= -350) return "Extraction"; | ||
| if (code >= -379 && code <= -370) return "Calibration"; | ||
| if (code >= -499 && code <= -400) return "ModuleService"; | ||
| if (code >= -599 && code <= -500) return "PlatformAdapter"; | ||
| if (code >= -699 && code <= -600) return "Backend"; | ||
| if (code >= -799 && code <= -700) return "Event"; | ||
| if (code >= -899 && code <= -800) return "Other"; |
There was a problem hiding this comment.
Missing category mapping for cancellation error range (-380 to -389)
The cancellation range is defined in rac_error.h but not handled here, causing RAC_ERROR_CANCELLED (-380) to return "Unknown" instead of "Cancellation".
| const char* rac_error_category(rac_result_t code) { | |
| if (code >= -109 && code <= -100) return "Initialization"; | |
| if (code >= -129 && code <= -110) return "Model"; | |
| if (code >= -149 && code <= -130) return "Generation"; | |
| if (code >= -179 && code <= -150) return "Network"; | |
| if (code >= -219 && code <= -180) return "Storage"; | |
| if (code >= -229 && code <= -220) return "Hardware"; | |
| if (code >= -249 && code <= -230) return "ComponentState"; | |
| if (code >= -279 && code <= -250) return "Validation"; | |
| if (code >= -299 && code <= -280) return "Audio"; | |
| if (code >= -319 && code <= -300) return "LanguageVoice"; | |
| if (code >= -329 && code <= -320) return "Authentication"; | |
| if (code >= -349 && code <= -330) return "Security"; | |
| if (code >= -369 && code <= -350) return "Extraction"; | |
| if (code >= -379 && code <= -370) return "Calibration"; | |
| if (code >= -499 && code <= -400) return "ModuleService"; | |
| if (code >= -599 && code <= -500) return "PlatformAdapter"; | |
| if (code >= -699 && code <= -600) return "Backend"; | |
| if (code >= -799 && code <= -700) return "Event"; | |
| if (code >= -899 && code <= -800) return "Other"; | |
| const char* rac_error_category(rac_result_t code) { | |
| if (code >= -109 && code <= -100) return "Initialization"; | |
| if (code >= -129 && code <= -110) return "Model"; | |
| if (code >= -149 && code <= -130) return "Generation"; | |
| if (code >= -179 && code <= -150) return "Network"; | |
| if (code >= -219 && code <= -180) return "Storage"; | |
| if (code >= -229 && code <= -220) return "Hardware"; | |
| if (code >= -249 && code <= -230) return "ComponentState"; | |
| if (code >= -279 && code <= -250) return "Validation"; | |
| if (code >= -299 && code <= -280) return "Audio"; | |
| if (code >= -319 && code <= -300) return "LanguageVoice"; | |
| if (code >= -329 && code <= -320) return "Authentication"; | |
| if (code >= -349 && code <= -330) return "Security"; | |
| if (code >= -369 && code <= -350) return "Extraction"; | |
| if (code >= -379 && code <= -370) return "Calibration"; | |
| if (code >= -389 && code <= -380) return "Cancellation"; | |
| if (code >= -499 && code <= -400) return "ModuleService"; | |
| if (code >= -599 && code <= -500) return "PlatformAdapter"; | |
| if (code >= -699 && code <= -600) return "Backend"; | |
| if (code >= -799 && code <= -700) return "Event"; | |
| if (code >= -899 && code <= -800) return "Other"; |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/core/rac_error_model.cpp
Line: 9-28
Comment:
Missing category mapping for cancellation error range (-380 to -389)
The cancellation range is defined in `rac_error.h` but not handled here, causing `RAC_ERROR_CANCELLED` (-380) to return "Unknown" instead of "Cancellation".
```suggestion
const char* rac_error_category(rac_result_t code) {
if (code >= -109 && code <= -100) return "Initialization";
if (code >= -129 && code <= -110) return "Model";
if (code >= -149 && code <= -130) return "Generation";
if (code >= -179 && code <= -150) return "Network";
if (code >= -219 && code <= -180) return "Storage";
if (code >= -229 && code <= -220) return "Hardware";
if (code >= -249 && code <= -230) return "ComponentState";
if (code >= -279 && code <= -250) return "Validation";
if (code >= -299 && code <= -280) return "Audio";
if (code >= -319 && code <= -300) return "LanguageVoice";
if (code >= -329 && code <= -320) return "Authentication";
if (code >= -349 && code <= -330) return "Security";
if (code >= -369 && code <= -350) return "Extraction";
if (code >= -379 && code <= -370) return "Calibration";
if (code >= -389 && code <= -380) return "Cancellation";
if (code >= -499 && code <= -400) return "ModuleService";
if (code >= -599 && code <= -500) return "PlatformAdapter";
if (code >= -699 && code <= -600) return "Backend";
if (code >= -799 && code <= -700) return "Event";
if (code >= -899 && code <= -800) return "Other";
```
How can I resolve this? If you propose a fix, please make it concise.| #include "rac/core/rac_error_model.h" | ||
| #include "rac/core/rac_error.h" | ||
|
|
||
| #include <string.h> |
There was a problem hiding this comment.
Unused include - <string.h> is not used anywhere in this file
| #include <string.h> |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/src/core/rac_error_model.cpp
Line: 4
Comment:
Unused include - `<string.h>` is not used anywhere in this file
```suggestion
```
How can I resolve this? If you propose a fix, please make it concise.| } | ||
| #endif | ||
|
|
||
| #endif // RAC_ERROR_MODEL_H No newline at end of file |
There was a problem hiding this comment.
Missing newline at end of file
| #endif // RAC_ERROR_MODEL_H | |
| #endif // RAC_ERROR_MODEL_H |
Prompt To Fix With AI
This is a comment left during a code review.
Path: sdk/runanywhere-commons/include/rac/core/rac_error_model.h
Line: 36
Comment:
Missing newline at end of file
```suggestion
#endif // RAC_ERROR_MODEL_H
```
How can I resolve this? If you propose a fix, please make it concise.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Siddhesh2377
left a comment
There was a problem hiding this comment.
The PR works but has a confirmed bug: add the missing range to rac_error_model.cpp:
if (code >= -389 && code <= -380) return "Cancellation";
Summary
Introduced a structured typed error model (
rac_error_model_t) to standardize error handling across SDK layers.Changes
rac_error_model.handrac_error_model.cpprac_result_tcodes into structured modelResult
Closes #423
Important
Introduces a centralized structured error model for consistent error handling and reporting across SDKs, enhancing debugging and telemetry support.
rac_error_model_tinrac_error_model.handrac_error_model.cppfor structured error handling.rac_result_tcodes to error categories inrac_error_model.cpp.log_to_stderr()inrac_logger.cppto userac_error_model_tfor detailed error logging.CMakeLists.txtto includerac_error_model.cppin the build process.This description was created by
for 7261465. You can customize this summary. It will automatically update as commits are pushed.
Summary by CodeRabbit
Greptile Summary
This PR introduces a centralized typed error model (
rac_error_model_t) that enhances error handling across SDKs by wrapping existingrac_result_tcodes with structured metadata (category and human-readable messages).Key Changes:
rac_error_model.hdefining the structured error model with code, message, and category fieldsrac_error_category()to map error codes to semantic categories based on rangesIssues Found:
RAC_ERROR_CANCELLEDto return "Unknown"The implementation correctly integrates with existing error infrastructure and provides a clean API for structured error reporting.
Confidence Score: 4/5
sdk/runanywhere-commons/src/core/rac_error_model.cppto ensure the cancellation range is addedImportant Files Changed
rac_error_model_tstruct and API for structured error handling; clean interface design, missing newline at EOF<string.h>headerrac_error_model.cppto build sources; correct placement in RAC_CORE_SOURCESLast reviewed commit: 7261465
(3/5) Reply to the agent's comments like "Can you suggest a fix for this @greptileai?" or ask follow-up questions!