Repository navigation
fix(cli): write JSON to stdout, always as an array - #73
Conversation
`sqlguard scan --format json > findings.json` produced an empty file: the JSON reporter was constructed with NewJSONReporter(), which targets stderr, so the one thing a machine-readable format is for did not work. Piping into jq saw nothing either. `explain --format json` had the same wiring. Both commands now build the reporter with NewJSONReporterTo(os.Stdout). The console format and the progress/summary lines stay on stderr, so stdout is pure JSON and `2>` still captures the human-readable report — the usual split between a program's product and its diagnostics. A clean run also used to emit nothing at all, handing a consumer an empty file rather than a parseable empty array. JSON now reports unconditionally and prints `[]` when there are no findings; the exit code is unchanged. The library defaults are deliberately left alone. NewJSONReporter() and NewConsoleReporter() still target stderr, which is right for a reporter wired into middleware inside someone else's process — writing findings to that application's stdout would corrupt its output.
Three follow-ups from review of the stdout change. explain accepted any --format value and fell back to console, so `explain --format jsonn … > plan.json` dialed the database, rendered to stderr and left an empty file — the same failure this branch fixes for the correctly spelled flag. It now shares scan's newReporter, which rejects unknown formats, and validates before connecting so a typo costs a message rather than a dial. Now that stdout carries the product, a failed write matters: an encode error only reached a warning on stderr, so a truncated array on a full disk exited 0 and the documented CI recipe would upload a corrupt artifact as green. reporter.Reporter cannot return an error, so the CLI wraps stdout in a writer that records the first failure and turns it into a non-zero exit. The stream-capture test helper read both pipes only after runScan returned, which deadlocks once either stream exceeds the 64 KiB pipe buffer. It now drains concurrently and closes the read ends; a 400-finding fixture covers the case, and asserts the fixture still exceeds the buffer so the guard cannot quietly stop testing anything.
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Repository: KARTIKrocks/sqlguard/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
| for _, f := range []string{"console", "json", ""} { | ||
| explainFormat = f | ||
| err := runExplain(&cobra.Command{}, []string{"SELECT 1"}) |
There was a problem hiding this comment.
Explain output lacks coverage Every accepted-format case stops at a connection failure, before explain reports anything. These tests cannot catch a regression in JSON stdout routing, clean
[] output, or the findings exit status. Add a successful-plan test that reaches the JSON reporting path.
Prompt To Fix With AI
This is a comment left during a code review.
Path: cmd/sqlguard/explain_test.go
Line: 43-45
Comment:
**Explain output lacks coverage** Every accepted-format case stops at a connection failure, before explain reports anything. These tests cannot catch a regression in JSON stdout routing, clean `[]` output, or the findings exit status. Add a successful-plan test that reaches the JSON reporting path.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.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!
…d policy Three follow-ups from review. The checked writer lived in a package-level var, so two invocations in one process could read each other's write result. newReporter now returns the write-error check alongside the reporter, and nothing is shared between calls. scan and explain carried the same reporting branch twice. It moves into one `report` helper — JSON always reports so a redirect gets an array, console stays quiet unless there is something to render, findings mean errIssuesFound either way, and a failed write outranks both. The callers keep only their own summary wording. The clean-run assertion could not fail: `null` unmarshals into a slice without error and leaves it nil with length 0, so a reporter that switched to a nil slice would emit `null` and still satisfy a len()-only check. It now asserts the literal `[]` and a non-nil decode — verified by making reporter/json.go emit `null`, which the old assertion passed and the new one catches. explain's reporting path cannot be reached without a live database, so the policy is pinned through the shared helper instead, plus a direct test that the JSON reporter targets stdout and one that two reporters keep their write results apart.
Summary
sqlguard scan --format json > findings.jsonproduced an empty file. The JSON reporter was built withNewJSONReporter(), which targets stderr, so the one thing a machine-readable format exists for did not work — and piping intojqsaw nothing either.explain --format jsonhad the same wiring.Reproducing it turned up a second defect in the same user story: a clean run emitted nothing at all, so even with the stream fixed a consumer got an empty file rather than a parseable empty array.
Closes #70
Type of change
Checklist
make cipasses (fmt-check, vet, lint, vuln, test-race, lint-docs) across all moduleswebsite/docs/with a version marker for anything new (_0.3+_,_Added in 0.3._,// 0.3+) — neverwebsite/versioned_docs/AGENTS.md/.sqlguard.example.ymlif a convention or config key changed## [Unreleased]inCHANGELOG.mdanalyzer/middleware/reporterResult)AGENTS.mdunticked deliberately: no convention or config key changed. The library reporter defaults are unchanged (see below), so nothingAGENTS.mddocuments moved.The design decision worth reviewing
Only the CLI changed. The library defaults are deliberately left alone.
reporter.NewJSONReporter()andNewConsoleReporter()still target stderr, because a reporter wired into middleware runs inside someone else's process — writing findings to that application's stdout would corrupt its output. So the split is product-vs-diagnostic rather than a blanket rule:--format json--format consoleIf you'd rather the library default move too, say so — it's a one-line change, but I think it's wrong for the middleware case.
Notes for reviewers
Docs were documenting the bug.
website/docs/scan.mdsaid "Findings go to stderr, so2>redirection captures them" as though it were intended. That paragraph is rewritten to describe the split by audience, with a_Changed in 0.3._marker and the 0.2 behaviour. I also added a CI recipe that uploadssqlguard.jsonas a build artifact, since that is the workflow the format exists for.Three follow-ups from review are in the second commit:
explainaccepted any--formatvalue and fell back to console — so--format jsonn > plan.jsondialled the database, rendered to stderr and left an empty file, re-creating this very bug for a typo. It now sharesscan'snewReporterand validates before connecting, so a mistake costs a message rather than a 30-second dial.reporter.Reportercannot return an error, so the CLI wraps stdout in a writer that records the first failure and turns it into a non-zero exit. I did not change theReporterinterface — that ripples throughmiddlewareand all six integrations and deserves its own decision.runScanreturned, which deadlocks once either stream exceeds the 64 KiB pipe buffer. It now drains concurrently and closes the read ends.Every test was checked against its own regression — each fails when the corresponding fix is reverted:
TestScan_JSONGoesToStdoutNewJSONReporterTo(os.Stdout)→NewJSONReporter()TestScan_JSONEmptyArrayWhenClean""instead of[]TestScan_JSONLargeOutputDoesNotDeadlockThe 400-finding fixture asserts it still exceeds 64 KiB, so that guard cannot quietly stop testing anything.
Verified:
make cigreen across all nine modules (lint-docsfails only on an untracked local scratch file outside this branch); the Docusaurus build passes, so the newscan#usagecross-link resolves underonBrokenLinks: 'throw'.