-
Notifications
You must be signed in to change notification settings - Fork 9
chore(storage): log "already in history" and thus expected conflicts as info #1736
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR updates conflict error handling to log "already in history" conflicts as informational messages rather than errors. The change modifies the condition that determines when conflicts should be treated as expected/informational.
- Changes the condition from checking
existsInHistoryto checking ifexpectedis null - Removes unnecessary line breaks in error message formatting
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| packages/runner/src/storage/cache.ts | Updates conflict detection logic to use expected == null condition |
| packages/memory/error.ts | Cleans up formatting by removing unnecessary line breaks in error message construction |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| result.error.conflict.existsInHistory | ||
| result.error.conflict.expected == null | ||
| ) { | ||
| logger.info(() => ["Transaction failed (aready exists)", result.error]); |
Copilot
AI
Sep 8, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a typo in the log message: 'aready' should be 'already'.
| logger.info(() => ["Transaction failed (aready exists)", result.error]); | |
| logger.info(() => ["Transaction failed (already exists)", result.error]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
|
I had some confusion in discord, but perhaps this is viable. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we're abandoning this, but don't want to close your PR.
Summary by cubic
Downgraded logging for expected “already exists” conflicts in storage to info by checking conflict.expected == null, reducing noisy error logs. Also cleaned up TheConflictError message formatting with no behavior change.