diff --git a/Cargo.lock b/Cargo.lock index a5b55c2..b850ede 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -266,6 +266,27 @@ version = "0.8.7" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" +[[package]] +name = "csv" +version = "1.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "52cd9d68cf7efc6ddfaaee42e7288d3a99d613d4b50f76ce9827ae0c6e14f938" +dependencies = [ + "csv-core", + "itoa", + "ryu", + "serde_core", +] + +[[package]] +name = "csv-core" +version = "0.1.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "704a3c26996a80471189265814dbc2c257598b96b8a7feae2d31ace646bb9782" +dependencies = [ + "memchr", +] + [[package]] name = "deadpool" version = "0.12.3" @@ -972,6 +993,7 @@ dependencies = [ "anyhow", "assert_cmd", "clap", + "csv", "globset", "owo-colors", "predicates", diff --git a/Cargo.toml b/Cargo.toml index a39b2e3..da1a616 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,7 @@ tokio = { version = "1.52.3", features = ["rt-multi-thread", "macros", "process" [dev-dependencies] assert_cmd = "2.2.2" +csv = "1.4.0" predicates = "3.1.4" tempfile = "3.27.0" wiremock = "0.6.5" diff --git a/README.md b/README.md index 8eb0591..3f9892a 100644 --- a/README.md +++ b/README.md @@ -185,7 +185,7 @@ postil doctor POSTIL_API_BASE=http://localhost:8000/v1 \ MODEL_API_KEY=local \ REVIEW_MODEL= \ -postil review --staged --output-json +postil review --staged --output json ``` Use the live benchmark harness before standardizing on a model: @@ -203,7 +203,7 @@ would change — which findings would be suppressed and which gate outcomes woul Deterministic, no model calls. ```sh -postil review --staged --output-json > .cache/envelopes/r1.json +postil review --staged --output json > .cache/envelopes/r1.json postil plan --envelopes .cache/envelopes --config .postil.candidate.yaml ``` @@ -220,9 +220,12 @@ Bitbucket incremental reviews are disabled unless ## The envelope -`--output-json` prints a stable versioned envelope (`summary`, `silent`, `findings`, +`--output json` prints a stable versioned envelope (`summary`, `silent`, `findings`, `resolved`, `counts`, `confidenceBuckets`, `gate`, `modelUsed`, `usage`, SHAs) consumed -by the hosted platform and `postil plan`. Schema: [postil.dev/docs/envelope](https://postil.dev/docs/envelope). +by the hosted platform and `postil plan`. `--output yaml` and `--output csv` print the +same review result in YAML or CSV. `--output-file ` writes the selected format to +a file instead of stdout. `--output-json` is deprecated in v0.2.1 as an alias for +`--output json` and emits a stderr warning. Schema: [postil.dev/docs/envelope](https://postil.dev/docs/envelope). Exit codes: `0` clean or below gate threshold, `1` gate-failing findings, `2` operational error. diff --git a/bench/fixtures/cases.test.ts b/bench/fixtures/cases.test.ts new file mode 100644 index 0000000..ad05725 --- /dev/null +++ b/bench/fixtures/cases.test.ts @@ -0,0 +1,17 @@ +import { describe, expect, test } from "bun:test"; +import { makeDiff } from "./cases"; + +describe("makeDiff", () => { + test("uses one-line hunk counts for one-line fixtures", () => { + const diff = makeDiff("src/example.ts", [{ line: 10, before: "return old;", after: "return next;" }]); + expect(diff).toContain("@@ -10,1 +10,1 @@"); + }); + + test("uses real hunk line counts for multiline fixtures", () => { + const diff = makeDiff("src/example.ts", [ + { line: 10, before: "const a = 1;\nreturn a;", after: "const a = 1;\nconst b = 2;\nreturn a + b;" }, + ]); + expect(diff).toContain("@@ -10,2 +10,3 @@"); + expect(diff).toContain("- const a = 1;\n- return a;\n+ const a = 1;\n+ const b = 2;\n+ return a + b;"); + }); +}); diff --git a/bench/fixtures/cases.ts b/bench/fixtures/cases.ts index 2e90bd4..126159a 100644 --- a/bench/fixtures/cases.ts +++ b/bench/fixtures/cases.ts @@ -49,17 +49,21 @@ function makeHeadSha(seed: number): string { return seed.toString(16).padStart(2, "0").repeat(20).slice(0, 40); } -function makeDiff(path: string, hunks: FixtureHunk[]): string { +export function makeDiff(path: string, hunks: FixtureHunk[]): string { return [ `diff --git a/${path} b/${path}`, "index 1111111..2222222 100644", `--- a/${path}`, `+++ b/${path}`, - ...hunks.flatMap((hunk) => [ - `@@ -${hunk.line},1 +${hunk.line},1 @@`, - `- ${hunk.before}`, - `+ ${hunk.after}`, - ]), + ...hunks.flatMap((hunk) => { + const beforeLines = hunk.before.split("\n"); + const afterLines = hunk.after.split("\n"); + return [ + `@@ -${hunk.line},${beforeLines.length} +${hunk.line},${afterLines.length} @@`, + ...beforeLines.map((line) => `- ${line}`), + ...afterLines.map((line) => `+ ${line}`), + ]; + }), "", ].join("\n"); } diff --git a/bench/src/livemodels.test.ts b/bench/src/livemodels.test.ts new file mode 100644 index 0000000..ef68169 --- /dev/null +++ b/bench/src/livemodels.test.ts @@ -0,0 +1,45 @@ +import { describe, expect, test } from "bun:test"; +import { formatLiveModelsReport, type LiveModelsReport } from "./livemodels"; + +describe("formatLiveModelsReport", () => { + test("formats dollar values to four decimal places", () => { + const cost = 0.123456; + const report: LiveModelsReport = { + generatedAt: "2026-07-11T00:00:00.000Z", + cliVersion: "postil 0.2.1", + apiBase: "https://example.test/v1", + models: [], + modelAggregates: [ + { + id: "test/model", + provider: null, + detectionRate: 1, + falsePositives: 0, + casesRun: 1, + defectCases: 1, + cleanCases: 0, + detections: 1, + misses: 0, + gateCorrect: 1, + gateIncorrect: 0, + severityExact: 1, + severityWithinOne: 1, + totalCostUsd: cost, + meanCostUsdPerReview: cost, + pricingKnown: true, + meanDurationMs: 1200, + errors: 0, + }, + ], + totalRunCostUsd: cost, + cases: [], + }; + + const output = formatLiveModelsReport(report); + expect(output).toContain("$0.1235"); + expect(output).not.toContain("$0.123456"); + + const displayed = Number(output.match(/Total run cost: \$(\d+\.\d{4})/)?.[1]); + expect(displayed).toBeCloseTo(cost, 4); + }); +}); diff --git a/bench/src/livemodels.ts b/bench/src/livemodels.ts index ffff4b2..c6d4b4e 100644 --- a/bench/src/livemodels.ts +++ b/bench/src/livemodels.ts @@ -352,7 +352,7 @@ function pct(v: number): string { } function usd(v: number): string { - return `$${v.toFixed(6)}`; + return `$${v.toFixed(4)}`; } function pad(s: string, width: number): string { diff --git a/src/cli.rs b/src/cli.rs index e93f62d..e5bc298 100644 --- a/src/cli.rs +++ b/src/cli.rs @@ -4,6 +4,8 @@ use std::path::PathBuf; use clap::{Parser, Subcommand, ValueEnum}; +use crate::output::OutputFormat; + #[derive(Parser)] #[command( name = "postil", @@ -66,8 +68,14 @@ pub enum Command { /// Previous review envelope for finding reconciliation. #[arg(long)] baseline: Option, - /// Print the envelope JSON on stdout (machine consumers). + /// Print the review envelope in this format on stdout: json, yaml, or csv. + #[arg(long, value_enum)] + output: Option, + /// Write --output or --output-json data to this path instead of stdout. #[arg(long)] + output_file: Option, + /// Deprecated in v0.2.1: use --output json. Prints the envelope JSON. + #[arg(long, conflicts_with = "output")] output_json: bool, /// Write SARIF 2.1.0 to this path for code-scanning ingestion. #[arg(long)] diff --git a/src/main.rs b/src/main.rs index 3c49866..15e326e 100644 --- a/src/main.rs +++ b/src/main.rs @@ -32,6 +32,8 @@ async fn dispatch(cli: Cli) -> anyhow::Result { gate_check_run_id, since_sha, baseline, + output, + output_file, output_json, sarif, fail_on, @@ -62,6 +64,8 @@ async fn dispatch(cli: Cli) -> anyhow::Result { gate_check_run_id, since_sha, baseline, + output, + output_file, output_json, sarif, fail_on, diff --git a/src/output.rs b/src/output.rs index 082dd58..2eb3a24 100644 --- a/src/output.rs +++ b/src/output.rs @@ -1,17 +1,122 @@ -//! Terminal rendering. JSON mode prints the envelope alone on stdout; -//! human chatter goes to stderr so pipelines stay clean. +//! Terminal rendering. Machine output prints the envelope alone on stdout or +//! writes it to the requested file; human chatter goes to stderr so pipelines +//! stay clean. use std::io::IsTerminal; +use std::path::Path; +use anyhow::Context; +use clap::ValueEnum; use owo_colors::OwoColorize; use crate::envelope::{Envelope, Severity}; -pub fn print_envelope_json(envelope: &Envelope) -> anyhow::Result<()> { - println!("{}", serde_json::to_string_pretty(envelope)?); +#[derive(Debug, Clone, Copy, PartialEq, Eq, ValueEnum)] +pub enum OutputFormat { + Json, + Yaml, + Csv, +} + +pub fn write_envelope( + envelope: &Envelope, + format: OutputFormat, + path: Option<&Path>, +) -> anyhow::Result<()> { + let rendered = render_envelope(envelope, format)?; + if let Some(path) = path { + std::fs::write(path, rendered) + .with_context(|| format!("writing {} output to {}", format.as_str(), path.display()))?; + } else { + print!("{rendered}"); + } Ok(()) } +impl OutputFormat { + pub fn as_str(self) -> &'static str { + match self { + OutputFormat::Json => "json", + OutputFormat::Yaml => "yaml", + OutputFormat::Csv => "csv", + } + } +} + +fn render_envelope(envelope: &Envelope, format: OutputFormat) -> anyhow::Result { + Ok(match format { + OutputFormat::Json => format!("{}\n", serde_json::to_string_pretty(envelope)?), + OutputFormat::Yaml => serde_yaml::to_string(envelope)?, + OutputFormat::Csv => render_csv(envelope), + }) +} + +fn render_csv(envelope: &Envelope) -> String { + let mut out = String::from( + "version,silent,summary,path,line,endLine,severity,kind,confidence,title,body,\ + gateFailOn,gateFailing,modelUsed,promptTokens,completionTokens,durationMs,baseSha,\ + headSha,sinceSha\n", + ); + if envelope.findings.is_empty() { + push_csv_row(&mut out, envelope, None); + } else { + for finding in &envelope.findings { + push_csv_row(&mut out, envelope, Some(finding)); + } + } + out +} + +fn push_csv_row(out: &mut String, envelope: &Envelope, finding: Option<&crate::envelope::Finding>) { + let mut fields = vec![ + envelope.version.to_string(), + envelope.silent.to_string(), + envelope.summary.clone(), + ]; + if let Some(finding) = finding { + fields.extend([ + finding.path.clone(), + finding.line.to_string(), + finding.end_line.map(|l| l.to_string()).unwrap_or_default(), + finding.severity.as_str().to_string(), + finding.kind.as_str().to_string(), + finding.confidence.to_string(), + finding.title.clone(), + finding.body.clone(), + ]); + } else { + fields.extend(std::iter::repeat_n(String::new(), 8)); + } + fields.extend([ + envelope.gate.fail_on.clone(), + envelope.gate.failing.to_string(), + envelope.model_used.clone(), + envelope.usage.prompt_tokens.to_string(), + envelope.usage.completion_tokens.to_string(), + envelope.duration_ms.to_string(), + envelope.base_sha.clone().unwrap_or_default(), + envelope.head_sha.clone().unwrap_or_default(), + envelope.since_sha.clone().unwrap_or_default(), + ]); + out.push_str( + &fields + .into_iter() + .map(csv_field) + .collect::>() + .join(","), + ); + out.push('\n'); +} + +fn csv_field(field: String) -> String { + let field = sanitize(&field); + if field.contains([',', '"', '\n', '\r']) { + format!("\"{}\"", field.replace('"', "\"\"")) + } else { + field + } +} + pub fn print_pretty(envelope: &Envelope) { let color = std::io::stderr().is_terminal(); let mut out = String::new(); @@ -181,4 +286,11 @@ mod tests { assert!(out.contains("hijacked title")); assert!(out.contains("FAKE ALL CLEAR")); } + + #[test] + fn csv_field_strips_terminal_controls_before_quoting() { + let field = csv_field("\x1b[31mred\x1b[0m,\"quoted\"\nnext".into()); + assert_eq!(field, "\"[31mred[0m,\"\"quoted\"\"\nnext\""); + assert!(!field.contains('\x1b')); + } } diff --git a/src/review.rs b/src/review.rs index 02736be..044b91c 100644 --- a/src/review.rs +++ b/src/review.rs @@ -14,7 +14,7 @@ use crate::forge::{ }; use crate::llm::LlmClient; use crate::local::{self, LocalSource}; -use crate::output; +use crate::output::{self, OutputFormat}; use crate::prompt::{self, PrContext}; const MAX_DIFF_BYTES: usize = 400_000; @@ -46,6 +46,8 @@ pub struct ReviewArgs { pub gate_check_run_id: Option, pub since_sha: Option, pub baseline: Option, + pub output: Option, + pub output_file: Option, pub output_json: bool, pub sarif: Option, pub fail_on: Option, @@ -54,6 +56,16 @@ pub struct ReviewArgs { pub no_post: bool, } +impl ReviewArgs { + fn resolved_output_format(&self) -> Option { + if self.output_json { + Some(OutputFormat::Json) + } else { + self.output + } + } +} + struct ReviewInput<'a> { diff_text: &'a str, meta: Option<&'a PrMeta>, @@ -66,6 +78,12 @@ struct ReviewInput<'a> { pub async fn run(args: ReviewArgs) -> Result { let cwd = std::env::current_dir()?; + if args.output_json { + eprintln!("warning: --output-json is deprecated; use --output json instead"); + } + if args.output_file.is_some() && args.resolved_output_format().is_none() { + return Err(anyhow!("--output-file requires --output or --output-json")); + } let mut cfg = Config::load(&cwd, args.config.as_deref())?; if let Some(m) = &args.model { cfg.model = m.clone(); @@ -565,9 +583,10 @@ async fn finish( .with_context(|| format!("writing SARIF to {}", path.display()))?; } - if args.output_json { - output::print_envelope_json(&envelope)?; - } else { + if let Some(format) = args.resolved_output_format() { + output::write_envelope(&envelope, format, args.output_file.as_deref())?; + } + if args.resolved_output_format().is_none() || args.output_file.is_some() { output::print_pretty(&envelope); } diff --git a/tests/e2e.rs b/tests/e2e.rs index 5afe37d..e1f1030 100644 --- a/tests/e2e.rs +++ b/tests/e2e.rs @@ -1,5 +1,7 @@ //! End-to-end tests: the real binary against mocked LLM and forge endpoints. +use std::collections::BTreeMap; + use assert_cmd::Command; use serde_json::{Value, json}; use wiremock::matchers::{header, method, path, path_regex, query_param}; @@ -62,6 +64,18 @@ fn finding_at(line: u32, severity: &str, confidence: f64) -> Value { }) } +fn finding_with_text(line: u32, severity: &str, confidence: f64, title: &str, body: &str) -> Value { + json!({ + "path": "src/auth.rs", + "line": line, + "severity": severity, + "kind": "risk", + "confidence": confidence, + "title": title, + "body": body + }) +} + fn postil() -> Command { let mut cmd = Command::cargo_bin("postil").unwrap(); // Isolate from developer environment and repo config discovery. @@ -84,17 +98,33 @@ fn write_diff(dir: &std::path::Path) -> std::path::PathBuf { p } -#[tokio::test] -async fn local_review_reports_grounded_finding_and_gates() { - let server = MockServer::start().await; +fn parse_csv_rows(csv: &str) -> Vec> { + let mut reader = csv::Reader::from_reader(csv.as_bytes()); + let headers = reader.headers().unwrap().clone(); + reader + .records() + .map(|record| { + headers + .iter() + .zip(record.unwrap().iter()) + .map(|(header, value)| (header.to_string(), value.to_string())) + .collect() + }) + .collect() +} + +async fn mock_review(server: &MockServer, findings: Value) { Mock::given(method("POST")) .and(path("/chat/completions")) - .respond_with( - ResponseTemplate::new(200) - .set_body_json(llm_content(json!([finding_at(41, "error", 0.92)]))), - ) - .mount(&server) + .respond_with(ResponseTemplate::new(200).set_body_json(llm_content(findings))) + .mount(server) .await; +} + +#[tokio::test] +async fn local_review_reports_grounded_finding_and_gates() { + let server = MockServer::start().await; + mock_review(&server, json!([finding_at(41, "error", 0.92)])).await; let dir = tempfile::tempdir().unwrap(); let diff = write_diff(dir.path()); @@ -103,7 +133,7 @@ async fn local_review_reports_grounded_finding_and_gates() { .env("POSTIL_API_BASE", server.uri()) .args(["review", "--diff-file"]) .arg(&diff) - .arg("--output-json") + .args(["--output", "json"]) .assert() .code(1); // gate fails on error severity let stdout = String::from_utf8(out.get_output().stdout.clone()).unwrap(); @@ -121,6 +151,172 @@ async fn local_review_reports_grounded_finding_and_gates() { assert_eq!(request["messages"].as_array().unwrap().len(), 2); } +#[tokio::test] +async fn local_review_prints_yaml_output() { + let server = MockServer::start().await; + mock_review(&server, json!([])).await; + + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "yaml"]) + .assert() + .success(); + + let stdout = String::from_utf8(out.get_output().stdout.clone()).unwrap(); + let env: serde_yaml::Value = serde_yaml::from_str(&stdout).unwrap(); + assert_eq!(env["silent"], true); + assert_eq!(env["gate"]["failing"], false); +} + +#[tokio::test] +async fn local_review_writes_csv_output_file_with_multiple_escaped_findings() { + let server = MockServer::start().await; + mock_review( + &server, + json!([ + finding_with_text( + 41, + "warn", + 0.88, + "Comma, quote \"and\" newline\nin title", + "First body has a comma, a \"quote\", and a newline\nsecond line." + ), + finding_with_text( + 42, + "info", + 0.77, + "Second finding", + "Body without CSV punctuation" + ) + ]), + ) + .await; + + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + let output = dir.path().join("review.csv"); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "csv", "--output-file"]) + .arg(&output) + .assert() + .success(); + + assert!(out.get_output().stdout.is_empty()); + let csv = std::fs::read_to_string(output).unwrap(); + let rows = parse_csv_rows(&csv); + assert_eq!(rows.len(), 2); + assert_eq!(rows[0]["version"], "1"); + assert_eq!(rows[0]["silent"], "false"); + assert_eq!(rows[0]["summary"], "SQL injection risk in auth path."); + assert_eq!(rows[0]["path"], "src/auth.rs"); + assert_eq!(rows[0]["line"], "41"); + assert_eq!(rows[0]["endLine"], ""); + assert_eq!(rows[0]["severity"], "warn"); + assert_eq!(rows[0]["kind"], "risk"); + assert_eq!(rows[0]["confidence"], "0.88"); + assert_eq!(rows[0]["title"], "Comma, quote \"and\" newline\nin title"); + assert_eq!( + rows[0]["body"], + "First body has a comma, a \"quote\", and a newline\nsecond line." + ); + assert_eq!(rows[0]["gateFailOn"], "error"); + assert_eq!(rows[0]["gateFailing"], "false"); + assert_eq!(rows[0]["promptTokens"], "100"); + assert_eq!(rows[0]["completionTokens"], "50"); + + assert_eq!(rows[1]["path"], "src/auth.rs"); + assert_eq!(rows[1]["line"], "42"); + assert_eq!(rows[1]["severity"], "info"); + assert_eq!(rows[1]["title"], "Second finding"); + assert_eq!(rows[1]["body"], "Body without CSV punctuation"); +} + +#[tokio::test] +async fn local_review_writes_output_file_in_selected_format() { + let server = MockServer::start().await; + mock_review(&server, json!([])).await; + + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + let output = dir.path().join("review.json"); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .args(["--output", "json", "--output-file"]) + .arg(&output) + .assert() + .success(); + + assert!(out.get_output().stdout.is_empty()); + let env: Value = serde_json::from_str(&std::fs::read_to_string(output).unwrap()).unwrap(); + assert_eq!(env["silent"], true); + assert_eq!(env["gate"]["failing"], false); +} + +#[tokio::test] +async fn deprecated_output_json_alias_prints_json_with_warning() { + let server = MockServer::start().await; + mock_review(&server, json!([])).await; + + let dir = tempfile::tempdir().unwrap(); + let diff = write_diff(dir.path()); + let out = postil() + .current_dir(dir.path()) + .env("POSTIL_API_BASE", server.uri()) + .args(["review", "--diff-file"]) + .arg(&diff) + .arg("--output-json") + .assert() + .success(); + + let stdout = String::from_utf8(out.get_output().stdout.clone()).unwrap(); + let stderr = String::from_utf8(out.get_output().stderr.clone()).unwrap(); + let env: Value = serde_json::from_str(&stdout).unwrap(); + assert_eq!(env["silent"], true); + assert!( + stderr.contains("warning: --output-json is deprecated; use --output json instead"), + "expected deprecation warning, got: {stderr}" + ); +} + +#[test] +fn review_help_documents_machine_output_flags() { + let out = Command::cargo_bin("postil") + .unwrap() + .args(["review", "--help"]) + .assert() + .success(); + + let stdout = String::from_utf8(out.get_output().stdout.clone()).unwrap(); + assert!(stdout.contains("--output ")); + assert!(stdout.contains("--output-file ")); + assert!(stdout.contains("--output-json")); + assert!(stdout.contains("Deprecated in v0.2.1: use --output json")); +} + +#[test] +fn review_rejects_unknown_output_format() { + Command::cargo_bin("postil") + .unwrap() + .args(["review", "--output", "xml"]) + .assert() + .failure() + .stderr(predicates::str::contains( + "possible values: json, yaml, csv", + )); +} + #[tokio::test] async fn full_remote_review_fetches_metadata_and_diff_concurrently() { let server = MockServer::start().await;