fix: keep the images found around a document that fails - #103
Merged
Conversation
ProcessReader returned nil on the first document it could not process, so one unparseable object discarded every image already found in that stream. A malformed document at the end of a file cost the whole file, and because `kubectl get pod -A -o yaml | kir -` arrives as a single stream, one bad object in a cluster dump left an error and no images as the only visible result. Failures are now collected per document: the documents after a bad one are still read, every failure is reported rather than only the first, and the images found are returned alongside. cmd prints them even when an input reported an error, and unwraps the joined error so stderr stays one failure per line. ADR 0008 already promised kir prints every image it finds and surfaces failures through the exit code; it said so about inputs, and this makes it true of the documents inside one. The ADR now states that explicitly.
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.
The N1 finding from the hardening review.
fix:, so it also bumps the version.Problem
ProcessReaderreturnednilon the first document it couldn't process, throwing away everything already collected. Run againstmaster(e4ba37d) with a file whose middle document is malformed:The valid Pods either side of the bad one are gone. The exit code is honest, so this isn't silent — but the loss is total where partial was available, and the operator's only visible result is an error and no images.
It matters most where a whole cluster arrives as one input.
kubectl get pod -A -o yaml | kir -is a single stream, so one unparseable object costs every image in the dump.Change
Failures are collected per document instead of aborting: documents after a bad one are still read, every failure is reported rather than only the first, and the images found come back alongside the joined error.
cmdprints them even when an input reported an error, and unwraps the joined error so stderr stays oneerror:line per failure.One distinction the fix respects: a
reader.Read()failure means the stream can no longer be split into documents, so there's nothing further to read and it stops — but still returns what it found. Only per-document decode failures continue. I checked which layer actually raises the error in this case, and it'sProcessData, so the documents after the bad one really are recoverable rather than theoretically so.Why this isn't a new decision
ADR 0008 already promises
kir"processes every input, prints every image it finds, logs each failure to stderr, and returns a non-zero exit code if any input failed." It said that about inputs; a stream is a batch of documents, and the same reasoning applies one level down. The ADR now says so explicitly rather than leaving it to be inferred — no new ADR, per AGENTS.md.Tests
Both fail on unmodified code — verified by reverting
yamlparser.goandcmd.goand re-running:TestFailure/PartialStream: this input is ordinary text, not bytes a fixture would normalise, so per AGENTS.md it belongs inapprovals/rather than in Go source. It pins the whole triple — both images on stdout, one error on stderr, exit 1. Reverting givesreceived does not match approved. The existingBadYAMLcase can't pin this: its entire file is unparseable, so it passes whether or not partial results survive.yamlparserunit tests: one asserting the images either side of a bad document survive (reverting givesimages = [], want [before-the-break after-the-break]), one asserting a stream with two bad documents reports two failures rather than one.Every pre-existing golden passes untouched — the change doesn't shift
BadYAML, whose output is identical because there was nothing to salvage.Checklist
gofmt -l .empty ·go vet ./...clean ·go test ./...and-racegreen ·go mod tidyno drift · no new dependencies.Merge order
Both this and #102 change
printImages' call sites incmd.go, so whichever lands second needs a rebase — happy to do it. #102 first would be my suggestion, since it's the one already reviewed.Generated by Claude Code