fix: preserve error fields in $ai_error#3605
Open
carlos-marchal-ph wants to merge 1 commit into
Open
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/ai/tests/serializeError.test.ts:45-63
**Prefer parameterised tests for stack-truncation boundary**
The two stack tests ("truncates stacks longer than 20 lines" and "leaves stacks of 20 or fewer lines untouched") test the same code path with different input sizes and are the clearest candidates for an `it.each` / `test.each` block. The project's stated preference is parameterised tests — collapsing these two cases (and potentially a boundary case at exactly 21 lines) into a single table-driven test would reduce duplication and make it easy to add more boundary values later.
### Issue 2 of 2
packages/ai/src/serializeError.ts:1-6
**`DEFAULT_MAX_DEPTH = 3` serializes 4 levels, not 3**
Because the guard is `depth < 0` rather than `depth === 0`, calls at depths 3 → 2 → 1 → 0 all enter the serialisation branch — meaning four errors in a cause chain are fully expanded, and only the fifth is silently dropped (returned as a raw `Error`, which `JSON.stringify` coerces to `{}`). The constant name implies three levels but the runtime behaviour is four. Consider either renaming the constant to `DEFAULT_MAX_CAUSE_DEPTH = 4`, changing the guard to `depth <= 0`, or adding a short comment explaining that `depth = 0` still processes the current error but does not recurse into its `cause`.
Reviews (1): Last reviewed commit: "fix: preserve error fields in $ai_error" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+45
to
+63
| error.stack = Array.from({ length: 50 }, (_, i) => `line ${i}`).join('\n') | ||
|
|
||
| const result = serializeError(error) as { stack: string } | ||
| const lines = result.stack.split('\n') | ||
|
|
||
| expect(lines).toHaveLength(21) | ||
| expect(lines.slice(0, 20)).toEqual(Array.from({ length: 20 }, (_, i) => `line ${i}`)) | ||
| expect(lines[20]).toBe('... (truncated)') | ||
| }) | ||
|
|
||
| it('leaves stacks of 20 or fewer lines untouched', () => { | ||
| const error = new Error('short stack') | ||
| error.stack = Array.from({ length: 20 }, (_, i) => `line ${i}`).join('\n') | ||
|
|
||
| const result = serializeError(error) as { stack: string } | ||
| expect(result.stack).toBe(error.stack) | ||
| }) | ||
|
|
||
| it('expands a custom error class wrapping a chain of nested error causes', () => { |
Contributor
There was a problem hiding this comment.
The two stack tests ("truncates stacks longer than 20 lines" and "leaves stacks of 20 or fewer lines untouched") test the same code path with different input sizes and are the clearest candidates for an it.each / test.each block. The project's stated preference is parameterised tests — collapsing these two cases (and potentially a boundary case at exactly 21 lines) into a single table-driven test would reduce duplication and make it easy to add more boundary values later.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/tests/serializeError.test.ts
Line: 45-63
Comment:
**Prefer parameterised tests for stack-truncation boundary**
The two stack tests ("truncates stacks longer than 20 lines" and "leaves stacks of 20 or fewer lines untouched") test the same code path with different input sizes and are the clearest candidates for an `it.each` / `test.each` block. The project's stated preference is parameterised tests — collapsing these two cases (and potentially a boundary case at exactly 21 lines) into a single table-driven test would reduce duplication and make it easy to add more boundary values later.
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!
Comment on lines
+1
to
+6
| const DEFAULT_MAX_DEPTH = 3 | ||
| const MAX_STACK_LINES = 20 | ||
|
|
||
| export function serializeError(value: unknown, depth = DEFAULT_MAX_DEPTH): unknown { | ||
| if (depth < 0 || value === null || typeof value !== 'object') { | ||
| return value |
Contributor
There was a problem hiding this comment.
DEFAULT_MAX_DEPTH = 3 serializes 4 levels, not 3
Because the guard is depth < 0 rather than depth === 0, calls at depths 3 → 2 → 1 → 0 all enter the serialisation branch — meaning four errors in a cause chain are fully expanded, and only the fifth is silently dropped (returned as a raw Error, which JSON.stringify coerces to {}). The constant name implies three levels but the runtime behaviour is four. Consider either renaming the constant to DEFAULT_MAX_CAUSE_DEPTH = 4, changing the guard to depth <= 0, or adding a short comment explaining that depth = 0 still processes the current error but does not recurse into its cause.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/ai/src/serializeError.ts
Line: 1-6
Comment:
**`DEFAULT_MAX_DEPTH = 3` serializes 4 levels, not 3**
Because the guard is `depth < 0` rather than `depth === 0`, calls at depths 3 → 2 → 1 → 0 all enter the serialisation branch — meaning four errors in a cause chain are fully expanded, and only the fifth is silently dropped (returned as a raw `Error`, which `JSON.stringify` coerces to `{}`). The constant name implies three levels but the runtime behaviour is four. Consider either renaming the constant to `DEFAULT_MAX_CAUSE_DEPTH = 4`, changing the guard to `depth <= 0`, or adding a short comment explaining that `depth = 0` still processes the current error but does not recurse into its `cause`.
How can I resolve this? If you propose a fix, please make it concise.
Contributor
Contributor
|
Size Change: +11.7 kB (+0.07%) Total Size: 16 MB
ℹ️ View Unchanged
|
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.
Problem
$ai_errorwas set withJSON.stringify(error), which silently dropsmessage,stack, andcause. That made it impossible to debug.Changes
Uses a custom serialisation function to preserve these properties and capture nested errors through
cause. Has a maximum nested error depth and a maximum stack line count to avoid bloat.Closes #3556.
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file🤖 Agent context
Authored with Claude Code (Opus 4.7). Iterated on the serializer shape: started with a richer version (AggregateError handling, separate WeakSet cycle detection,
[Circular]/[Truncated]sentinels, plain-object container recursion) and trimmed it down to the current minimal version on Carlos's direction — depth-bound only, no cycle bookkeeping, plain objects pass through. Stack truncation at 20 lines was added afterwards to bound payload size on deep cause chains.$ai_errordeliberately remains a JSON-encoded string rather than a structured object so existing.toContain(...)test assertions and any downstream HogQL consumers keep working.