fix(reporter): contain JSONReporter writer panics as transport failures - #77
Merged
Conversation
A panic raised by the underlying io.Writer during the primary JSON encode was mislabeled as a Details encoding error (routing it through the encoding-error fallback), and the same panic during the fallback encode had no recovery at all and could escape to the caller. writerPanicGuard converts a Write panic into an ordinary error before it reaches emit(), so both paths latch the reporter's existing failed state like any other writer error. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
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
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.
Summary
JSONReporter.emitrecovered a panic from the underlyingio.Writerduringthe primary
json.Encoder.Encodecall, but mislabeled it as aDetailsencoding failure and routed it through the encoding-error fallback — and the
fallback's own
Encodecall had no panic recovery at all, so a writer panicthere could escape
emit()and crash the caller.Adds
writerPanicGuard, a thinio.Writerwrapper installed around thewriter at
NewJSONReporterconstruction, that recovers a panic fromWriteand converts it into an ordinary error before it ever reaches the encoder.
Both the primary and fallback encode paths already treat a returned writer
error correctly (discard it, latch
failed, drop later events), so thewriter-panic case now flows through that same existing, tested path instead
of a separate one. A panicking
Detailsjson.Marshaleris unaffected: thatpanic happens during in-memory marshaling, before
Writeis ever called, soencodeRecoverstill owns it.Checks
make check— gofmt leaves no diff,golangci-lint run(
.golangci.yml) clean,go test -v ./...green (unit +tests/e2e)go test -race -short ./...greengo.modstill declares no dependencies (stdlib only)exact output changes update
docs/specs/reporter-package.mdDocs housekeeping
AGENTS.md,docs/design/overview.md,docs/specs/*updated forbehavior or convention changes (
docs/specs/reporter-package.md)TEMPLATE.mdand indexed indocs/README.mdinstead
Protected boundaries
N/A — no
.github/workflows/**,.goreleaser.yaml,.svu.yaml, orpolicies/agent-governance.jsontouched.Verification
node scripts/check-docs.mjsgreen:PR review rubric
Risk classification
Moderate. This changes the failure-handling behavior of a shipped public
constructor (
NewJSONReporter): a writer panic is now contained and reportedidentically to a writer error instead of being mislabeled or, on the
fallback path, potentially escaping to the caller. No public API signature
changed, no protected boundary touched, and the change is narrowly scoped to
reporter/json.gowith focused new tests covering both the primary andfallback panic paths.