Repository: mozilla/cargo-vet (rust)

Issue:
Do a better job of formatting error messages as json
Right now when cargo-vet spits out an error (in this case `some first-party packages match published crates.io versions`), it doesn't format it as json even if `--output-format=json` is specified. This can cause opaque errors from infrastructure that's expecting json.

Here's another example, which stems from a toml parse error: https://treeherder.mozilla.org/jobs?repo=try&revision=8a3a904d1fc5eedc4fc349af5fcc18767b35144d

CC @glandium

Pull request description:
print errors as JSON with --output-format=json
Fixes #216 

Several caveats that we can potentially hammer out now or over time:

* The errors will however still go to stderr rather than stdout, as a current artifact of the order we set things up in.
* Two major classes of errors, Audit Violations and Unvetted Dependencies, are currently treated
* Very early errors like CLI parsing will not have this format, as we cannot at that point "know" what you want
* Panics will not have this format
* I don't really know the full schema of this format at the moment, but some examples can be found in the checked in snapshots


Patch:
diff --git a/Cargo.lock b/Cargo.lock
index 2a30e63..8ef8399 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -721,9 +721,9 @@ checksum = "2dffe52ecf27772e601905b7522cb4ef790d2cc203488bbd0e2fe85fcb74566d"
 
 [[package]]
 name = "miette"
-version = "4.7.1"
+version = "5.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "1c90329e44f9208b55f45711f9558cec15d7ef8295cc65ecd6d4188ae8edc58c"
+checksum = "837623c1a94ac99c95b1011a31df16d122cd686525bb01da4d484528c1a22be7"
 dependencies = [
  "atty",
  "backtrace",
@@ -741,9 +741,9 @@ dependencies = [
 
 [[package]]
 name = "miette-derive"
-version = "4.7.1"
+version = "5.0.0"
 source = "registry+https://github.com/rust-lang/crates.io-index"
-checksum = "6b5bc45b761bcf1b5e6e6c4128cd93b84c218721a8d9b894aa0aff4ed180174c"
+checksum = "dab7137e5ad19fc41e71dff006fd0497f3b5c84a51cfd07224c7214a19036ea1"
 dependencies = [
  "proc-macro2",
  "quote",
diff --git a/Cargo.toml b/Cargo.toml
index 6a047fa..734ead0 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -39,7 +39,7 @@ toml_edit = { version = "0.14.4", features = ["serde"] }
 tokio = { version = "1.12.0", features = ["fs", "macros", "process", "rt-multi-thread"] }
 tracing = { version = "0.1.34", features = ["log"] }
 tracing-subscriber = "0.3.11"
-miette = { version = "4.7.1", features = ["fancy"] }
+miette = { version = "5.0.0", features = ["fancy"] }
 thiserror = "1.0.31"
 url = "2.2.2"
 
diff --git a/src/errors.rs b/src/errors.rs
index 933e530..3c46c35 100644
--- a/src/errors.rs
+++ b/src/errors.rs
@@ -43,7 +43,7 @@ pub struct NeedsAuditAsErrors {
 
 impl Display for NeedsAuditAsErrors {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.write_str("Some first-party packages match published crates.io versions")?;
+        f.write_str("Some non-crates.io-fetched packages match published crates.io versions")?;
         for e in &self.errors {
             f.write_fmt(format_args!("\n  {}", e))?
         }
diff --git a/src/main.rs b/src/main.rs
index d468bc0..2580bdd 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -149,7 +149,8 @@ fn main() -> Result<(), ()> {
         }
     };
     main_result.map_err(|e| {
-        eprintln!("{:?}", e);
+        let out: &mut dyn Out = &mut Term::stderr();
+        writeln!(out, "{:?}", e).unwrap();
         std::process::exit(-1);
     })
 }
@@ -212,18 +213,24 @@ fn real_main() -> Result<(), miette::Report> {
     }));
 
     // FIXME: we should have separate configs for errors but this works for now
-    let error_colors_enabled = cli.output_file.is_none();
+    let error_colors_enabled = cli.output_file.is_none() && console::colors_enabled_stderr();
+    let output_json = cli.output_format == OutputFormat::Json;
     miette::set_hook(Box::new(move |_| {
-        let graphical_theme = if error_colors_enabled {
-            miette::GraphicalTheme::unicode()
+        if output_json {
+            let json_handler = miette::JSONReportHandler;
+            Box::new(json_handler)
         } else {
-            miette::GraphicalTheme::unicode_nocolor()
-        };
-        Box::new(
-            miette::MietteHandlerOpts::new()
-                .graphical_theme(graphical_theme)
-                .build(),
-        )
+            let graphical_theme = if error_colors_enabled {
+                miette::GraphicalTheme::unicode()
+            } else {
+                miette::GraphicalTheme::unicode_nocolor()
+            };
+            Box::new(
+                miette::MietteHandlerOpts::new()
+                    .graphical_theme(graphical_theme)
+                    .build(),
+            )
+        }
     }))
     .expect("Failed to initialize error handler");
 
@@ -684,7 +691,7 @@ fn do_cmd_certify(
                     .collect::<Vec<_>>()
             )?;
             writeln!(out, "(press ENTER to accept the current criteria)")?;
-            let input = out.read_line_initial_text("> ")?;
+            let input = out.read_line_with_prompt("> ")?;
             let input = input.trim();
             if input.is_empty() {
                 if chosen_criteria.is_empty() {
@@ -1242,7 +1249,14 @@ fn cmd_vet(out: &mut dyn Out, cfg: &Config) -> Result<(), miette::Report> {
             ResolveDepth::Deep
         },
     );
-    let suggest = report.compute_suggest(cfg, network.as_ref(), true)?;
+
+    // Bare `cargo vet` shouldn't suggest in CI
+    let suggest = if !cfg.cli.locked {
+        report.compute_suggest(cfg, network.as_ref(), true)?
+    } else {
+        None
+    };
+
     match cfg.cli.output_format {
         OutputFormat::Human => report
             .print_human(out, cfg, suggest.as_ref())
diff --git a/src/out.rs b/src/out.rs
index 8ecda3c..1f57091 100644
--- a/src/out.rs
+++ b/src/out.rs
@@ -25,7 +25,7 @@ pub trait Out: io::Write {
 
     /// Ask the user a question, and read in a line with the user's response. If
     /// there's no user able to respond, `None` will be returned instead.
-    fn read_line_initial_text(&mut self, _initial: &str) -> io::Result<String> {
+    fn read_line_with_prompt(&mut self, _prompt: &str) -> io::Result<String> {
         Err(io::ErrorKind::Unsupported.into())
     }
 
@@ -53,8 +53,10 @@ impl Out for Term {
         (&*self).clear_screen()
     }
 
-    fn read_line_initial_text(&mut self, initial: &str) -> io::Result<String> {
-        (&*self).read_line_initial_text(initial)
+    fn read_line_with_prompt(&mut self, prompt: &str) -> io::Result<String> {
+        self.write_str(prompt)?;
+        self.flush()?;
+        (&*self).read_line()
     }
 
     fn style(&self) -> Style {


Test patch:
diff --git a/src/tests/audit_as_crates_io.rs b/src/tests/audit_as_crates_io.rs
index f5b9cfd..827d5b6 100644
--- a/src/tests/audit_as_crates_io.rs
+++ b/src/tests/audit_as_crates_io.rs
@@ -8,6 +8,19 @@ fn get_audit_as_crates_io(cfg: &Config, store: &Store) -> String {
     }
 }
 
+fn get_audit_as_crates_io_json(cfg: &Config, store: &Store) -> String {
+    let res = crate::check_audit_as_crates_io(cfg, store);
+    match res {
+        Ok(()) => String::new(),
+        Err(e) => {
+            let handler = miette::JSONReportHandler::new();
+            let mut output = String::new();
+            handler.render_report(&mut output, &e).unwrap();
+            output
+        }
+    }
+}
+
 #[test]
 fn simple_audit_as_crates_io() {
     let _enter = TEST_RUNTIME.enter();
@@ -142,6 +155,28 @@ fn complex_audit_as_crates_io_max_wrong() {
     insta::assert_snapshot!("complex-audit-as-crates-io-max-wrong", output);
 }
 
+#[test]
+fn complex_audit_as_crates_io_max_wrong_json() {
+    let _enter = TEST_RUNTIME.enter();
+
+    let mock = MockMetadata::complex();
+    let metadata = mock.metadata();
+    let (mut config, audits, imports) = builtin_files_full_audited(&metadata);
+
+    config
+        .policy
+        .insert("rootA".to_owned(), audit_as_policy(Some(true)));
+    config
+        .policy
+        .insert("rootB".to_owned(), audit_as_policy(Some(true)));
+
+    let store = Store::mock(config, audits, imports);
+    let cfg = mock_cfg(&metadata);
+
+    let output = get_audit_as_crates_io_json(&cfg, &store);
+    insta::assert_snapshot!("complex-audit-as-crates-io-max-wrong-json", output);
+}
+
 #[test]
 fn simple_deps_audit_as_crates_io() {
     let _enter = TEST_RUNTIME.enter();
diff --git a/src/tests/mod.rs b/src/tests/mod.rs
index a00c279..8b820ab 100644
--- a/src/tests/mod.rs
+++ b/src/tests/mod.rs
@@ -1098,7 +1098,7 @@ impl<'a> Out for BasicTestOutput<'a> {
         Ok(())
     }
 
-    fn read_line_initial_text(&mut self, initial: &str) -> io::Result<String> {
+    fn read_line_with_prompt(&mut self, initial: &str) -> io::Result<String> {
         write!(self, "{}", initial)?;
         if let Some(on_read_line) = &mut self.on_read_line {
             let response = on_read_line(initial)?;
diff --git a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong-json.snap b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong-json.snap
new file mode 100644
index 0000000..a100fcf
--- /dev/null
+++ b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong-json.snap
@@ -0,0 +1,5 @@
+---
+source: src/tests/audit_as_crates_io.rs
+expression: output
+---
+{"message": "There are some issues with your policy.audit-as-crates-io entries","severity": "error","labels": [],"related": [{"message": "Some first-party packages match published crates.io versions\n  firstA:10.0.0\n  firstAB:10.0.0\n  firstB:10.0.0\n  firstB-nodeps:10.0.0","severity": "error","help": "Add a `policy.*.audit-as-crates-io` entry for them","labels": [],"related": []},{"message": "some audit-as-crates-io packages don't match published crates.io versions\n  rootA:10.0.0\n  rootB:10.0.0","severity": "error","help": "Remove the audit-as-crates-io entries or make them `false`","labels": [],"related": []}]}
diff --git a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong.snap b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong.snap
index 7453e3d..d6d1040 100644
--- a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong.snap
+++ b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io-max-wrong.snap
@@ -6,7 +6,7 @@ expression: output
   × There are some issues with your policy.audit-as-crates-io entries
 
 Error: 
-  × Some first-party packages match published crates.io versions
+  × Some non-crates.io-fetched packages match published crates.io versions
   │   firstA:10.0.0
   │   firstAB:10.0.0
   │   firstB:10.0.0
diff --git a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io.snap b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io.snap
index 1dfad47..24aa646 100644
--- a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io.snap
+++ b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__complex-audit-as-crates-io.snap
@@ -6,7 +6,7 @@ expression: output
   × There are some issues with your policy.audit-as-crates-io entries
 
 Error: 
-  × Some first-party packages match published crates.io versions
+  × Some non-crates.io-fetched packages match published crates.io versions
   │   firstA:10.0.0
   │   firstAB:10.0.0
   │   firstB:10.0.0
diff --git a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__simple-audit-as-crates-io.snap b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__simple-audit-as-crates-io.snap
index 90263bd..3ca89fe 100644
--- a/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__simple-audit-as-crates-io.snap
+++ b/src/tests/snapshots/cargo_vet__tests__audit_as_crates_io__simple-audit-as-crates-io.snap
@@ -6,7 +6,7 @@ expression: output
   × There are some issues with your policy.audit-as-crates-io entries
 
 Error: 
-  × Some first-party packages match published crates.io versions
+  × Some non-crates.io-fetched packages match published crates.io versions
   │   root-package:10.0.0
   │   first-party:10.0.0
   help: Add a `policy.*.audit-as-crates-io` entry for them
diff --git a/tests/snapshots/test_cli__test-project-bad-certify-human.snap b/tests/snapshots/test_cli__test-project-bad-certify-human.snap
new file mode 100644
index 0000000..784f9b8
--- /dev/null
+++ b/tests/snapshots/test_cli__test-project-bad-certify-human.snap
@@ -0,0 +1,10 @@
+---
+source: tests/test-cli.rs
+expression: format_outputs(&output)
+---
+stdout:
+
+stderr:
+
+  × 'asdfsdfs' isn't one of your foreign packages
+
diff --git a/tests/snapshots/test_cli__test-project-bad-certify-json.snap b/tests/snapshots/test_cli__test-project-bad-certify-json.snap
new file mode 100644
index 0000000..c2c97de
--- /dev/null
+++ b/tests/snapshots/test_cli__test-project-bad-certify-json.snap
@@ -0,0 +1,8 @@
+---
+source: tests/test-cli.rs
+expression: format_outputs(&output)
+---
+stdout:
+
+stderr:
+{"message": "'asdfsdfs' isn't one of your foreign packages","severity": "error","labels": [],"related": []}
diff --git a/tests/test-cli.rs b/tests/test-cli.rs
index cdca482..70e4f95 100644
--- a/tests/test-cli.rs
+++ b/tests/test-cli.rs
@@ -32,7 +32,8 @@ fn format_outputs(output: &Output) -> String {
         .unwrap()
         .lines()
         .filter(|line| !line.starts_with("Blocking: waiting for file lock"))
-        .collect::<String>();
+        .collect::<Vec<_>>()
+        .join("\n");
     format!("stdout:\n{stdout}\nstderr:\n{stderr}")
 }
 
@@ -296,3 +297,48 @@ fn test_project_dump_graph_full() {
     insta::assert_snapshot!("test-project-dump-graph-full", format_outputs(&output));
     assert!(output.status.success(), "{}", output.status);
 }
+
+#[test]
+fn test_project_bad_certify_human() {
+    let project = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
+        .join("tests")
+        .join("test-project");
+    let bin = env!("CARGO_BIN_EXE_cargo-vet");
+    let output = Command::new(bin)
+        .current_dir(&project)
+        .arg("vet")
+        .arg("--manifest-path")
+        .arg("Cargo.toml")
+        .arg("certify")
+        .arg("asdfsdfs")
+        .stdout(Stdio::piped())
+        .stderr(Stdio::piped())
+        .output()
+        .unwrap();
+
+    insta::assert_snapshot!("test-project-bad-certify-human", format_outputs(&output));
+    assert!(!output.status.success(), "{}", output.status);
+}
+
+#[test]
+fn test_project_bad_certify_json() {
+    let project = PathBuf::from(env!("CARGO_MANIFEST_DIR"))
+        .join("tests")
+        .join("test-project");
+    let bin = env!("CARGO_BIN_EXE_cargo-vet");
+    let output = Command::new(bin)
+        .current_dir(&project)
+        .arg("vet")
+        .arg("--output-format=json")
+        .arg("--manifest-path")
+        .arg("Cargo.toml")
+        .arg("certify")
+        .arg("asdfsdfs")
+        .stdout(Stdio::piped())
+        .stderr(Stdio::piped())
+        .output()
+        .unwrap();
+
+    insta::assert_snapshot!("test-project-bad-certify-json", format_outputs(&output));
+    assert!(!output.status.success(), "{}", output.status);
+}