fix: surface cause details in error messages#139
Conversation
ToolExecutionError and CommandExecutionError were swallowing the underlying cause, producing generic messages like "Failed to execute tool review: Failed to execute code review" with no actionable detail. Now appends the cause's message when available, e.g.: "Failed to execute tool review: Failed to execute code review (model not found)" Closes tuannvm#91 (error message portion) Signed-off-by: TiM <tim.vdh@gmail.not> Signed-off-by: TiM <tim@nuimarkets.com>
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/errors.ts (1)
20-22: LGTM! Consider extracting shared logic.The implementation is correct and mirrors
ToolExecutionError. Lines 20-21 are identical to lines 7-8. For a file this small, the duplication is acceptable, but if you prefer DRY:♻️ Optional: Extract helper function
+function formatCauseDetail(message: string, cause?: unknown): string { + const causeMsg = cause instanceof Error ? cause.message : ''; + return causeMsg && causeMsg !== message ? ` (${causeMsg})` : ''; +} + export class ToolExecutionError extends Error { constructor( public readonly toolName: string, message: string, public readonly cause?: unknown ) { - const causeMsg = cause instanceof Error ? cause.message : ''; - const detail = causeMsg && causeMsg !== message ? ` (${causeMsg})` : ''; - super(`Failed to execute tool "${toolName}": ${message}${detail}`); + super(`Failed to execute tool "${toolName}": ${message}${formatCauseDetail(message, cause)}`); this.name = 'ToolExecutionError'; } } export class CommandExecutionError extends Error { constructor( public readonly command: string, message: string, public readonly cause?: unknown ) { - const causeMsg = cause instanceof Error ? cause.message : ''; - const detail = causeMsg && causeMsg !== message ? ` (${causeMsg})` : ''; - super(`Command execution failed for "${command}": ${message}${detail}`); + super(`Command execution failed for "${command}": ${message}${formatCauseDetail(message, cause)}`); this.name = 'CommandExecutionError'; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/errors.ts` around lines 20 - 22, Extract the duplicate cause/message formatting into a small helper (e.g., formatCauseDetail) and use it in both places where the lines are identical (the current constructor that builds the message and the ToolExecutionError constructor) so you avoid repeating the logic that computes causeMsg and detail; implement the helper to accept (cause, message) and return the trailing detail string (or empty string) and call it when constructing the super(...) message in both constructors.
🤖 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/errors.ts`:
- Around line 20-22: Extract the duplicate cause/message formatting into a small
helper (e.g., formatCauseDetail) and use it in both places where the lines are
identical (the current constructor that builds the message and the
ToolExecutionError constructor) so you avoid repeating the logic that computes
causeMsg and detail; implement the helper to accept (cause, message) and return
the trailing detail string (or empty string) and call it when constructing the
super(...) message in both constructors.
Summary
ToolExecutionErrorandCommandExecutionErrornow append the underlying cause's message when availableFailed to execute tool "review": Failed to execute code reviewgave no actionable detailFailed to execute tool "review": Failed to execute code review (model not found)Addresses the error message concern raised in #91.
Test plan
npm run build)Summary by CodeRabbit
Bug Fixes