Skip to content

feat(cli): add lint output file flag - #163

Merged
aram-devdocs merged 1 commit into
mainfrom
issue-45-sarif-output-flag
Apr 30, 2026
Merged

feat(cli): add lint output file flag#163
aram-devdocs merged 1 commit into
mainfrom
issue-45-sarif-output-flag

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Summary

  • add --output <PATH> to plumb lint and thread it through the CLI into the lint command
  • keep formatter calls pure and write the exact rendered output to stdout or the requested file path
  • cover JSON/SARIF output-file behavior and write failures in CLI integration tests

Testing

  • cargo test -p plumb-cli --test cli_integration
  • cargo check --no-default-features
  • cargo fmt --all -- --check
  • cargo clippy -p plumb-cli --all-targets -- -D warnings

Fixes #45

@github-actions

Copy link
Copy Markdown
Contributor

Review: feat(cli): add lint output file flag

Scope: crates/plumb-cli/src/commands/lint.rs, crates/plumb-cli/src/main.rs, crates/plumb-cli/tests/cli_integration.rs, docs/src/cli.md, docs/src/quickstart.md, CHANGELOG.md.

No other crates touched. This is purely a plumb-cli change.


1. Determinism — PASS

No wall-clock, no HashMap/HashSet in observable output, no env reads introduced. std::fs::write is a pure side-effect on an input-derived value. The existing output path (out: String) is computed before the branch, so the format/sort pipeline is unchanged.

2. Workspace layering — PASS

std::fs::write lives entirely in plumb-cli. No library crate was modified. The run() signature change is internal to the crate — no public library API was perturbed.

3. Error handling — PASS

The write error is wrapped with .with_context(|| format!("write lint output to {}", path.display()))? and propagates through anyhow to main, which maps it to exit code 2 (infra failure). No unwrap/expect added anywhere.

4. Test coverage — WARNING

Tests added for: JSON happy path, SARIF happy path, missing-parent-dir error path. All three check exit codes, stdout suppression, and (for the error case) that the serialized payload doesn't leak to stderr.

Gap: --format pretty --output is untested. The pretty formatter returns a String containing ANSI escape sequences when the terminal advertises color support. Because plumb is invoked here via Command::cargo_bin (stdout is a pipe, not a TTY), the formatter likely strips codes in tests — but a user running plumb lint url --output report.txt interactively gets a file with raw escape codes. This isn't a correctness bug, but it is a user-visible surprise and the behavior is unspecified.

Suggested fix (warning, not a blocker): add one test pinning that --format pretty --output produces the same bytes as plumb lint … --format pretty with stdout captured (same pattern as the JSON test), so any future TTY-detection change in the formatter doesn't silently change file output.

5. Non-atomic write — WARNING

std::fs::write truncates and fills in a single call. A partial write (disk full mid-write) leaves a corrupt file with no recovery path. For a lint tool this is an acceptable trade-off, but it is worth noting because CI consumers may parse the file immediately after the run without checking exit code. A temp-file + rename pattern (tempfile::NamedTempFile) would give atomic semantics at the cost of one extra dep.

Not a blocker given the tool's scope; documenting here for the record.

6. Documentation — PASS

docs/src/cli.md row is clear and concise. docs/src/quickstart.md correctly swaps the shell-redirect example for --output, which is better CI hygiene (file is only created after the full output is computed). CHANGELOG entry is accurate. No AI-tell phrases detected.

Punch list

File Line Class Note
crates/plumb-cli/tests/cli_integration.rs after line 143 Warning No test for --format pretty --output; ANSI-in-file behavior is unspecified.
crates/plumb-cli/src/commands/lint.rs 104 Warning std::fs::write is non-atomic; partial write on disk-full is undetected. Consider tempfile::NamedTempFile + rename if robustness matters.

No blockers. Both warnings are acceptable for the current milestone.


Verdict: APPROVE

@aram-devdocs
aram-devdocs merged commit 3ee72d2 into main Apr 30, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

feat(cli): --format sarif + --output <path>

1 participant