fix(output): route formatter errors through an injectable sink

[OwenYWT]

Summary

Stop output helpers from writing internal formatting failures directly to os.Stderr.

Changes

• introduce an injectable ErrorSink for best-effort formatter errors
• route JSON, NDJSON, and CSV internal write failures through ErrorSink
• add focused tests covering marshal failures and CSV flush failures

Test Plan

• GOCACHE=/tmp/go-build-cache GOMODCACHE=/tmp/go-mod-cache /opt/homebrew/opt/go/bin/go test ./cmd/... ./internal/output

Summary by CodeRabbit

• Bug Fixes

  • Improved error reporting and handling for CSV, JSON, and NDJSON output operations.

• Tests

  • Added test coverage for output formatting error scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 4e518957-deeb-4b74-a6f1-afc2726b4e78

📥 Commits

Reviewing files that changed from the base of the PR and between d3d92e3 and c9fbcca.

📒 Files selected for processing (4)

• internal/output/csv.go
• internal/output/csv_test.go
• internal/output/print.go
• internal/output/print_test.go

---

📝 Walkthrough

Walkthrough

Introduced a package-level, injectable ErrorSink and writeInternalError helper function to unify internal error reporting across CSV, JSON, and NDJSON output formats, replacing direct os.Stderr writes with the new error sink mechanism.

Changes

Cohort / File(s)	Summary
Error Handling Infrastructure internal/output/print.go, internal/output/csv.go	Added injectable ErrorSink variable (defaults to os.Stderr) and writeInternalError helper function; updated CSV, JSON, and NDJSON formatters to route marshal/write errors through writeInternalError instead of directly writing to os.Stderr.
Test Coverage internal/output/csv_test.go, internal/output/print_test.go	Added tests validating error handling: TestFormatAsCSV_WriteError_UsesErrorSink checks CSV write errors; TestPrintJson_MarshalError_UsesErrorSink and TestPrintNdjson_MarshalError_UsesErrorSink verify JSON/NDJSON marshal errors are correctly routed to ErrorSink.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related issues

• output helpers should not write failures directly to os.Stderr #49: The PR directly implements the suggested fix from this issue by introducing an injectable ErrorSink mechanism and routing internal CSV/JSON/NDJSON errors through writeInternalError, with comprehensive test coverage for the new error handling behavior.

Poem

🐰 Errors now flow through ErrorSink streams,
No more stderr's direct dreams,
With tests to catch each fumbled byte,
Our output shines oh so bright! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 40.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely summarizes the main change: routing formatter errors through an injectable sink instead of direct os.Stderr writes.
Description check	✅ Passed	The description covers all main sections: it provides a clear summary, lists the key changes (ErrorSink introduction and routing through it), and specifies a test plan with test command execution confirmation.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[greptile-apps]

Greptile Summary

This PR replaces hardcoded os.Stderr writes in the output formatters (PrintJson, PrintNdjson, flushCSV) with a package-level injectable ErrorSink io.Writer, making error routing testable and overridable by embedders. The implementation is clean and focused tests cover all three modified error paths.

Confidence Score: 5/5

Safe to merge; all findings are minor style/best-practice suggestions that do not affect correctness.

No P0 or P1 issues found. The change is a clean refactor with focused tests covering every modified error path. The two P2 notes (concurrency safety of the global var and the silent discard of fmt.Fprintf errors) do not affect correctness under current usage patterns.

No files require special attention.

Important Files Changed

Filename	Overview
internal/output/print.go	Introduces injectable ErrorSink and writeInternalError helper; replaces direct os.Stderr writes in PrintJson and PrintNdjson
internal/output/csv.go	Removes os import; routes flushCSV errors through writeInternalError instead of a direct os.Stderr write
internal/output/print_test.go	Adds two focused tests verifying JSON and NDJSON marshal errors land in the overridden ErrorSink
internal/output/csv_test.go	Adds failingCSVWriter stub and test confirming CSV flush errors reach ErrorSink

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[PrintJson] -->|marshal error| W[writeInternalError]
    B[PrintNdjson] -->|marshal error| W
    C[flushCSV] -->|flush error| W
    W --> S{ErrorSink}
    S -->|default| D[os.Stderr]
    S -->|test override| E[bytes.Buffer]
    S -->|embedder override| F[custom io.Writer]

Loading

_{Reviews (1): Last reviewed commit: "fix(output): route formatter errors thro..." | Re-trigger Greptile}

[greptile-apps]

ErrorSink not safe for concurrent access

The exported ErrorSink variable is an interface value read inside writeInternalError (which can be called from any goroutine processing output) and written by callers or test setup. Concurrent reads and writes of an interface variable are a data race in Go's memory model. The current tests avoid this by not calling t.Parallel(), but the exported variable invites future callers to mutate it without synchronization. Consider using a sync/atomic.Pointer[io.Writer] or a small mutex-protected accessor to make this safe for concurrent use.

[greptile-apps]

Silently ignored ErrorSink write error

fmt.Fprintf returns (int, error) and the helper discards it. Given the "best-effort" framing this is intentional, but a brief //nolint or comment like // best-effort; ignore write errors would make it clear the discard is deliberate rather than accidental.

_{Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!}