fix(check): write the report to stdout so it can be piped

[djbclark]

Fixes #372, where the reasoning and the transcripts are laid out in full.

check emitted its entire report through eprintln!, so stdout was empty and
secretspec check | grep DATABASE_URL matched nothing while the command exited
0 — which reads as "no secrets found" rather than "the report went elsewhere".

Three things beyond the piping annoyance, in the order I'd weigh them:

• check already contradicts itself. check --json (cli/mod.rs:1507) and
  check --explain (:1509) write to stdout. Only the default human rendering
  did not, so which stream carried the report depended on a flag.

• The colour handling is currently wrong in both directions. colored
  decides on ANSI escapes by testing stdout (control.rs:108) while the
  report went to stderr. So check 2>log leaked raw escape bytes into the log
  file, and check >file stripped colour from output being read on a terminal.
  Routing the report to stdout makes the existing detection correct rather than
  needing new colour logic.

• A naive macro swap introduces a panic. Putting the report on stdout puts
  it on a stream a reader can close early, and println! panics on EPIPE.
  stdout is a LineWriter, so every report line is its own write syscall and no
  large output is needed:

  $ secretspec check --reason r | head -1
  Checking secrets in demo (profile: default)...
  thread 'main' panicked at library/std/src/io/stdio.rs:1165:9:
  failed printing to stdout: Broken pipe (os error 32)
  [exit 101]

  That pipeline exits 0 today only because stdout receives nothing. So this
  writes the report through a locked &mut dyn io::Write instead — the pattern
  write_export's doc comment already documents for exactly this reason ("turns
  a broken pipe into a returned error instead of a panic"). A closed pipe now
  behaves as export | head already does: a clean IO error: Broken pipe.

ensure_secrets is deliberately untouched — its output is prompts and
diagnostics, not report — and the lock is released before it, so prompting is
unaffected. No change to exit codes, to what is printed, or to dependencies.

Tests

secretspec/tests/check_report_stream.rs, three tests, each confirmed to fail
without the change:

• a_passing_check_writes_its_whole_report_to_stdout
• a_failing_check_reports_on_stdout_but_errors_on_stderr
• a_reader_that_closes_the_pipe_early_does_not_panic — #[cfg(unix)], drives
  a real sh/head pipeline

The first two fail against the pre-fix tree; the third fails against a bare
eprintln! → println! swap and passes here.

Compatibility

Filed under Changed rather than Fixed: anyone capturing via 2>&1 keeps
working, but a script capturing stderr specifically needs to capture stdout
instead. I won't claim that's rare — it caught me, and I had to update three
fixtures in my own downstream post-install suite.

Open questions, happy to go either way

• The constraint-violation lines in display_validation_errors are arguably
  diagnostics rather than report. I kept them with the rest of the report on
  stdout, on the grounds that splitting one human-readable report across two
  streams is the same class of bug — say the word and I'll move them.
• Secrets::check() is public, so this makes a library write to its consumer's
  stdout. It already printed unconditionally, just to the other stream, so no
  consumer is spared it today. If you'd rather a library not print at all, the
  cleaner shape is check taking an impl Write defaulting to io::stdout(),
  or moving rendering into the CLI layer — happy to send either instead.
• Two adjacent pre-existing things this deliberately does not touch:
  check --json | head panics the same way already (cli/mod.rs:1456), and
  import's summary is on stderr too (secrets.rs:4313/:4320/:4328). Glad
  to include either if you want them.

🤖 Generated with Claude Code

[domenkozar]

Reviewed this against a local checkout and ran the tests. The stream split is the right call, the colour argument holds up, and the description saved me most of the digging. Four things, one of which answers your second open question.

1. Take the sink as a parameter rather than reaching for stdout inside the library

Yes to the impl Write shape you offered. export already established it: cli/mod.rs:1466 acquires stdout in the CLI and passes &mut out into Secrets::export. check should look the same:

pub fn check(&self, no_prompt: bool, out: &mut dyn io::Write) -> Result<ValidatedSecrets>

with cli/mod.rs:1519 handing it the stream. One caller in the tree and nothing in secretspec-derive touches check, so the churn is small, and a published library then stays out of its consumer's data channel.

One detail when you move it: pass &mut std::io::stdout() rather than a held .lock(). Holding the lock across self.validate() is the part of the current shape I would not keep. validate() fans out onto std::thread::scope workers whenever a check spans more than one provider group (secrets.rs:5412), so a global lock is held across arbitrary provider I/O running on other threads. Nothing in the tree writes to stdout from those workers today (no provider prints, none use Stdio::inherit), so this is latent rather than a reachable hang, but the lock buys no atomicity when nothing else writes to the stream, and an unlocked handle re-acquires per line exactly like println! does. export has the same pattern already; pre-existing, not yours to fix here.

2. The pipeline test earns its place, and I confirmed it is not vacuous

The report is small enough to fit in a 64KB pipe buffer, so I wanted to be sure head -1 really does close the pipe before the writer is done rather than the whole report landing in the buffer first. Measured against a build of this branch:

• 100 runs of that exact pipeline: 99 produced Broken pipe, so the post-header writes really do hit a closed pipe.
• Substituting a panicking sink for yours (a stand-in for the naive println! swap): the test failed 10 out of 10 runs.

The timing works because the header is flushed before validate() runs, and validate() then does a provider read plus audit writes, which is the whole window head needs to print and exit. Buffer capacity never comes into it. Roughly 1 run in 100 will not exercise the regression path, which I am happy with; making it deterministic would mean a manifest large enough to fill the pipe buffer, and that is not worth the test's runtime.

3. A closed pipe still prints an error on the pipeline this change exists to enable

check | head now exits 1 with Failed to check secrets: IO error: Broken pipe on stderr. Nothing in the repo touches SIGPIPE, so restoring the default disposition in the CLI entry point would quiet that for check, check --json, and export in one place, and would cover the pre-existing --json | head panic you flagged without visiting each call site. It needs a unix-only libc dependency (already in the lock file transitively), so a follow-up PR is the right size for it. Not blocking this one.

4. Two small ones

• tests/check_report_stream.rs:181: env is interpolated raw while bin and manifest go through shell_quote. The surrounding single quotes cover it in practice, but the helper is right there and the comment above it makes the case.
• Changelog: "goes through a locked sink" is implementation detail in a user-facing entry, and it stops being accurate after point 1. The observable half, a closed pipe reporting an error instead of panicking, already carries the meaning.

On your first open question: keeping the constraint violations on stdout with the rest of the report is right, they are report lines rather than diagnostics. On the third: leave import's summary alone, it is a separate decision and point 3 above covers the --json half.

---

🤖 Review generated with Claude Code. Noted here because the GitHub API has no field for it: cli/cli#13904

[djbclark]

Thanks for the thorough review — all addressed in 1b1783c:

1. check now takes out: &mut dyn io::Write instead of locking stdout internally, matching export's shape exactly (cli/mod.rs:1466). The CLI passes &mut std::io::stdout(), not a held lock — you're right that it bought no atomicity and would have serialized against anything the validate() worker threads might someday write.
2. No action needed — thanks for actually measuring it.
3. Agreed this is separate-PR sized; leaving SIGPIPE disposition alone here.
4. Fixed: tests/check_report_stream.rs now shell_quotes the env path like its neighbors, and the changelog no longer says "locked sink" now that the lock is gone.

[djbclark]

The SIGPIPE follow-up from point 3 is now open as #377.

It turned out to be worth doing on its own: check --json doesn't just report a broken pipe, it panics out of println! (exit 101). Resetting the disposition at the entry point covers that and export together, and will cover check here once this PR lands.

[domenkozar]

This one is still needed?