Repository navigation
fix(logging): preserve JSON logs on serialization errors - #8173
Vidit-Ostwal merged 2 commits into
Conversation
Strix Security ReviewWarning This pull request has 57 commits after the last Strix review ( No security issues found. Review summaryReviewed the two changed files: Updated for Reviewed by Strix |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
Bugbot is paused — on-demand spend limit reachedBugbot uses usage-based billing for this team and has hit its on-demand spend limit. A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue. |
16d4670 to
f37eebc
Compare
Related issue
Fixes #8172.
Summary
A failed JSON log attempt can destroy earlier log records: FileHandler opens the destination with
wbefore json.dump discovers unsupported or circular metadata. The partially written file is then treated as malformed by the next call, which replaces the history.Serialize the complete payload before opening the destination. Serialization errors still surface through the existing ValueError wrapper, with the previous file untouched and no partial file created for a failed first record. The file lock, JSON indentation and trailing newline remain in place. This change addresses serialization failures, without introducing crash-atomic writes for filesystem failures.
Four regression cases use real temporary files and cover unsupported/circular metadata with and without existing history, then verify a later valid log retains earlier records. A normal JSON metadata control and existing pickle tests also pass.
Verification
Python 3.13.12 / macOS arm64, repository-lockfile dependencies:
Tests ran with CREWAI_TELEMETRY_DISABLED=true and OTEL_SDK_DISABLED=true. Full monorepo tests were not run. No provider/model calls. Test-file formatting follows its existing style; repository Ruff configuration excludes tests.
Additional context
AI assistance: Codex investigated the problem, prepared the fix and tests, and ran the checks above.
Note
Low Risk
Narrow change to JSON file logging in
FileHandler; successful writes stay the same format, with lower risk of accidental log data loss on bad metadata.Overview
Fixes JSON log history loss when a log entry cannot be serialized.
FileHandlernow runsjson.dumpson the full append payload before opening the log file for write, so unsupported or circularmetadatastill raises the existingValueErrorwithout truncating prior records or leaving a partial new file.Adds regression tests for serialization failures with and without existing history, plus a happy-path check that serializable metadata appends correctly and the trailing newline is preserved.
Reviewed by Cursor Bugbot for commit 8aa19d4. Bugbot is set up for automated code reviews on this repo. Configure here.