Conversation
Logger JSON.stringify crashed on Error objects with circular references, causing the CommitMsgHandler to fail silently and skip trailers. Added safeStringify with circular reference detection and Error serialization to the Logger. Also updated all five hook handlers to extract error message/stack before passing to logger context instead of raw Error objects. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> AI-Agent: Claude-Code/2.1.42
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
No actionable comments were generated in the recent review. 🎉 📝 WalkthroughWalkthroughThis PR standardizes error handling across multiple application handlers by normalizing caught errors into proper Error instances, enhancing logging to capture both message and stack trace, and ensuring returned error objects use the normalized Error. It also adds a safeStringify helper to handle circular references and Error serialization in logging. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes a critical bug where circular references in Error objects caused JSON.stringify to crash in the Logger, leading to silent failures in CommitMsgHandler and other handlers that prevented AI trailers from being added to commits.
Changes:
- Added
safeStringifyutility to Logger that detects circular references using WeakSet and serializes Error objects to plain objects with onlymessageandstackproperties - Updated all five hook handlers to extract
error.messageanderror.stackbefore passing to logger context, providing defense-in-depth against circular reference issues
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/infrastructure/logging/Logger.ts | Added safeStringify function with circular reference detection and Error object serialization; replaced JSON.stringify with safeStringify in logger context formatting |
| src/application/handlers/CommitMsgHandler.ts | Updated catch block to extract error message and stack before logging |
| src/application/handlers/PostCommitHandler.ts | Updated catch block to extract error message and stack before logging |
| src/application/handlers/PromptSubmitHandler.ts | Updated catch block to extract error message and stack before logging |
| src/application/handlers/SessionStartHandler.ts | Updated catch block to extract error message and stack before logging |
| src/application/handlers/SessionStopHandler.ts | Updated catch block to extract error message and stack before logging |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /** JSON.stringify that handles circular references and Error objects. */ | ||
| function safeStringify(obj: unknown): string { | ||
| const seen = new WeakSet(); | ||
| return JSON.stringify(obj, (_key, value) => { | ||
| if (value instanceof Error) { | ||
| return { message: value.message, stack: value.stack }; | ||
| } | ||
| if (typeof value === 'object' && value !== null) { | ||
| if (seen.has(value)) return '[Circular]'; | ||
| seen.add(value); | ||
| } | ||
| return value; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The new safeStringify function lacks test coverage. This is critical functionality that prevents silent logger failures, and should have tests covering:
- Circular reference detection (e.g.,
const obj = {}; obj.self = obj;) - Error object serialization (e.g.,
new Error('test')) - Error objects with circular references in their properties
- Nested objects with mixed circular references and Error objects
- Null and undefined values
- Primitives (strings, numbers, booleans)
Consider adding a test file at tests/unit/infrastructure/logging/safeStringify.test.ts or adding these tests to the existing Logger.test.ts file.
Summary
JSON.stringifycrashed onErrorobjects with circular references, causingCommitMsgHandlerto fail silently and skip AI trailers on commitssafeStringifyto Logger with circular reference detection andErrorobject serialization (message+stack)CommitMsgHandler,PostCommitHandler,PromptSubmitHandler,SessionStartHandler,SessionStopHandler) to extracterror.messageanderror.stackbefore passing to logger contextTest plan
🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes