fix: preserve error details in task logs#1987
fix: preserve error details in task logs#1987zerone0x wants to merge 1 commit intoAndyMik90:developfrom
Conversation
…o-Authored-By: Claude <noreply@anthropic.com>
|
Clawdbot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account. You have signed the CLA already but the status is still pending? Let us recheck it. |
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
🎉 Thanks for your first PR!
A maintainer will review it soon. Please make sure:
- Your branch is synced with
develop - CI checks pass
- You've followed our contribution guide
Welcome to the Auto Claude community!
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces error message sanitization and extraction logic to the task log writer, ensuring sensitive data like API keys and Bearer tokens are redacted from logs. It also enhances error handling by robustly extracting messages from various error formats and causes. A review comment suggests improving type safety when inspecting unknown error causes by using the 'in' operator instead of type assertions.
| if (cause && typeof cause === 'object') { | ||
| const maybeMessage = (cause as { message?: unknown }).message; | ||
| if (typeof maybeMessage === 'string' && maybeMessage.trim()) { | ||
| return sanitizeErrorMessage(maybeMessage); | ||
| } | ||
| const maybeError = (cause as { error?: unknown }).error; | ||
| if (typeof maybeError === 'string' && maybeError.trim()) { | ||
| return sanitizeErrorMessage(maybeError); | ||
| } | ||
| } |
There was a problem hiding this comment.
While this approach works, using type assertions like (cause as { message?: unknown }) can be brittle. A more robust and type-safe way to check for properties on an unknown or object type is to use the in operator. This avoids unsafe casting and makes the code's intent clearer.
if (cause && typeof cause === 'object') {
if ('message' in cause && typeof cause.message === 'string' && cause.message.trim()) {
return sanitizeErrorMessage(cause.message);
}
if ('error' in cause && typeof cause.error === 'string' && cause.error.trim()) {
return sanitizeErrorMessage(cause.error);
}
}![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/desktop/src/main/ai/logging/task-log-writer.ts`:
- Around line 35-61: Add focused unit tests for extractErrorMessage to cover
every extraction branch and redaction: create tests that pass a SessionError
with an empty/blank message and (1) cause as an Error whose message should be
returned via sanitizeErrorMessage, (2) cause as a string, (3) cause as an object
with a message property, (4) cause as an object with an error property, and (5)
where message and cause are blank but error.code is present (expect "Error:
<code>"). Also add tests verifying sanitizeErrorMessage redacts tokens and
sensitive strings in returned messages; reference the extractErrorMessage
function and sanitizeErrorMessage to locate the code under test. Ensure each
test asserts the exact sanitized output for regressions.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4bc8a660-514b-4c5f-9f84-f24b30dc1e37
📒 Files selected for processing (1)
apps/desktop/src/main/ai/logging/task-log-writer.ts
| function extractErrorMessage(error: SessionError): string { | ||
| const direct = error?.message?.trim(); | ||
| if (direct) return sanitizeErrorMessage(direct); | ||
|
|
||
| const cause = error?.cause as unknown; | ||
| if (cause instanceof Error && cause.message?.trim()) { | ||
| return sanitizeErrorMessage(cause.message); | ||
| } | ||
|
|
||
| if (typeof cause === 'string' && cause.trim()) { | ||
| return sanitizeErrorMessage(cause); | ||
| } | ||
|
|
||
| if (cause && typeof cause === 'object') { | ||
| const maybeMessage = (cause as { message?: unknown }).message; | ||
| if (typeof maybeMessage === 'string' && maybeMessage.trim()) { | ||
| return sanitizeErrorMessage(maybeMessage); | ||
| } | ||
| const maybeError = (cause as { error?: unknown }).error; | ||
| if (typeof maybeError === 'string' && maybeError.trim()) { | ||
| return sanitizeErrorMessage(maybeError); | ||
| } | ||
| } | ||
|
|
||
| if (error?.code) return `Error: ${error.code}`; | ||
| return 'Unknown error'; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add focused regression tests for extraction and redaction branches.
Given this is a bug fix for lost diagnostics, please add tests for blank message with: cause as Error, string, object { message }, object { error }, plus token redaction cases to prevent regressions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/desktop/src/main/ai/logging/task-log-writer.ts` around lines 35 - 61,
Add focused unit tests for extractErrorMessage to cover every extraction branch
and redaction: create tests that pass a SessionError with an empty/blank message
and (1) cause as an Error whose message should be returned via
sanitizeErrorMessage, (2) cause as a string, (3) cause as an object with a
message property, (4) cause as an object with an error property, and (5) where
message and cause are blank but error.code is present (expect "Error: <code>").
Also add tests verifying sanitizeErrorMessage redacts tokens and sensitive
strings in returned messages; reference the extractErrorMessage function and
sanitizeErrorMessage to locate the code under test. Ensure each test asserts the
exact sanitized output for regressions.
|
We independently found and fixed the same issue in PR #1979. Our fix is in the same files (stream-handler.ts, task-log-writer.ts, worker.ts). The root cause is that the AI SDK error events have Cross-referencing so Andy can pick the best implementation when reviewing. |
3 participants
Base Branch
developbranch (required for all feature/fix PRs)main(hotfix only - maintainers)Description
Ensure task log error entries always include a meaningful message by falling back to the underlying cause when the SessionError message is blank. This keeps planning/coding errors visible in the Logs tab after the AI SDK migration.
Related Issue
Closes #1978
Type of Change
Area
Commit Message Format
Follow conventional commits:
<type>: <subject>Types: feat, fix, docs, style, refactor, test, chore
Example:
feat: add user authentication systemAI Disclosure
Tool(s) used: OpenAI Codex
Testing level:
Untested -- AI output not yet verified
Lightly tested -- ran the app / spot-checked key paths
Fully tested -- all tests pass, manually verified behavior
I understand what this PR does and how the underlying code works
Checklist
developbranchPlatform Testing Checklist
CRITICAL: This project supports Windows, macOS, and Linux. Platform-specific bugs are a common source of breakage.
platform/module instead of directprocess.platformchecksfindExecutable()or platform abstractions)If you only have access to one OS: CI now tests on all platforms. Ensure all checks pass before submitting.
CI/Testing Requirements
Screenshots
N/A
Feature Toggle
Breaking Changes
Breaking: No
Details:
N/A
Summary by CodeRabbit
Release Notes