feat: serialize error payloads - #61
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
JSON serialization currently breaks the established exception pass-through path.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Automatically serializes non-string error payloads in MessageWrapper.
Changes:
- Serializes error payloads as JSON.
- Updates hash-payload test coverage.
- Ignores
.ideafiles.
File summaries
| File | Summary |
|---|---|
test/lib/message_wrapper.rb |
Updates hash serialization coverage. |
lib/leopard/message_wrapper.rb |
Serializes error responses; exception pass-through must be preserved. |
.gitignore |
Adds .idea to ignored files. |
Review details
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
be5eebf to
7fdc19f
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Structured NATS error metadata and payload preservation regress, and related documentation requires updating.
Get a fresh assessment by requesting another Copilot review.
Review details
Suppressed comments (1)
lib/leopard/message_wrapper.rb:42
- This changes the public request/reply contract, but the current documentation still says that
respond_with_errorreturns the same failure payload (documents/enable-injectable-request-reply-callbacks.adoc:41-42andReadme.adoc:110-111). Hash and other non-Exceptionfailures now reach the consumer as JSON strings, so please update those docs to describe serialization and the exception passthrough exception.
raw.respond_with_error(err.is_a?(Exception) ? err : serialize(err), &)
- Files reviewed: 2/3 changed files
- Comments generated: 1
- Review effort level: Lite
There was a problem hiding this comment.
🟡 Changes recommended
Address the incomplete error serialization, metrics content-type mismatch, and inconsistent release version sources.
Get a fresh assessment by requesting another Copilot review.
Review details
- Files reviewed: 3/5 changed files
- Comments generated: 3
- Review effort level: Lite
Co-authored-by: gabeodess <65213+gabeodess@users.noreply.github.com>
Head branch was pushed to by a user without write access
Co-authored-by: gabeodess <65213+gabeodess@users.noreply.github.com>
There was a problem hiding this comment.
🔵 Needs a closer look
Content type is determined after serialization, causing the added JSON response test to fail.
Review details
Suppressed comments (1)
lib/leopard/metrics_server.rb:67
bodyhas already been converted to aStringon the preceding line, so this condition is always true and hash payloads are sent with the Prometheustext/plaincontent type. The added test expectsapplication/jsonand currently fails; determine the content type before serializing the body.
body = JSON.generate(body) unless body.is_a?(String)
content_type = body.is_a?(String) ? 'text/plain; version=0.0.4' : 'application/json'
- Files reviewed: 5/7 changed files
- Comments generated: 0 new
- Review effort level: Lite
Automatically serializes error payloads so the consumer doesn't need to think about it.