Add json output format option to check command (#24)

* Implement json output format option. Validate against the included schema for check json output

* Control exit status for `check` command

This follows the convention used by linters like eslint, rubocop, and shellcheck.
Builds on clap's existing behavior.

All output formats (packwerk, json, csv) behave consistently:
- Exit 0 when no violations
- Exit 1 when violations found
- Exit 2 on internal errors

---------
Co-authored-by: Ivy Evans <ivy.evans@gusto.com>
Co-authored-by: Sweta Sanghavi <sweta.sanghavi@gusto.com>

Author: 3 people

Cargo.toml

@@ -24,34 +24,35 @@ name = "packs"
 path = "src/lib.rs"
 
 [dependencies]
-anyhow = { version = "1.0.75", features = [] } # for error handling
-clap = { version = "4.2.1", features = ["derive"] } # cli
-clap_derive = "4.2.0" # cli
-csv = "1.3.0" # csv de/serialize
-itertools = "0.13.0" # tools for iterating over iterable things
-jwalk = "0.8.1" # for walking the file tree
-path-clean = "1.0.1" # Pathname#cleaname in Ruby
-rayon = "1.7.0" # for parallel iteration
+anyhow = { version = "1.0.75", features = [] }                         # for error handling
+clap = { version = "4.2.1", features = ["derive"] }                    # cli
+clap_derive = "4.2.0"                                                  # cli
+csv = "1.3.0"                                                          # csv de/serialize
+itertools = "0.13.0"                                                   # tools for iterating over iterable things
+jwalk = "0.8.1"                                                        # for walking the file tree
+path-clean = "1.0.1"                                                   # Pathname#cleaname in Ruby
+rayon = "1.7.0"                                                        # for parallel iteration
 regex = "1.7.3"
-serde = { version = "~1", features = ["derive"] } # de(serialization)
-serde_yaml = "0.9.19" # de(serialization)
-serde_json = "1.0.96" # de(serialization)
-serde_magnus = "0.7.0" # permits a ruby gem to interface with this library
-tracing = "0.1.37" # logging
+serde = { version = "~1", features = ["derive"] }                      # de(serialization)
+serde_yaml = "0.9.19"                                                  # de(serialization)
+serde_json = "1.0.96"                                                  # de(serialization)
+serde_magnus = "0.7.0"                                                 # permits a ruby gem to interface with this library
+tracing = "0.1.37"                                                     # logging
 tracing-subscriber = { version = "0.3.16", features = ["env-filter"] } # logging
-glob = "0.3.1" # globbing
-globset = "0.4.10" # globbing
-lib-ruby-parser = "4.0.6" # ruby parser
-md5 = "0.7.0" # md5 hashing to take and compare md5 digests of file contents to ensure cache validity
-line-col = "0.2.1" # for creating source maps of violations
-ruby_inflector = '0.0.8' # for inflecting strings, e.g. turning `has_many :companies` into `Company`
-petgraph = "0.6.3" # for running graph algorithms (e.g. does the dependency graph contain a cycle?)
+glob = "0.3.1"                                                         # globbing
+globset = "0.4.10"                                                     # globbing
+lib-ruby-parser = "4.0.6"                                              # ruby parser
+md5 = "0.7.0"                                                          # md5 hashing to take and compare md5 digests of file contents to ensure cache validity
+line-col = "0.2.1"                                                     # for creating source maps of violations
+ruby_inflector = '0.0.8'                                               # for inflecting strings, e.g. turning `has_many :companies` into `Company`
+petgraph = "0.6.3"                                                     # for running graph algorithms (e.g. does the dependency graph contain a cycle?)
 fnmatch-regex2 = "0.3.0"
 strip-ansi-escapes = "0.2.0"
 
 [dev-dependencies]
-assert_cmd = "2.1.1" # testing CLI
-rusty-hook = "^0.11.2" # git hooks
-predicates = "3.0.2" # kind of like rspec assertions
+assert_cmd = "2.1.1"        # testing CLI
+jsonschema = "0.27"         # JSON schema validation
+rusty-hook = "^0.11.2"      # git hooks
+predicates = "3.0.2"        # kind of like rspec assertions
 pretty_assertions = "1.3.0" # Shows a more readable diff when comparing objects
-serial_test = "3.1.1" # Run specific tests in serial
+serial_test = "3.1.1"       # Run specific tests in serial

schema/check-output.json

@@ -0,0 +1,87 @@
+{
+  "$schema": "http://json-schema.org/draft-07/schema#",
+  "title": "pks check JSON output",
+  "type": "object",
+  "required": ["violations", "stale_todos", "summary"],
+  "additionalProperties": false,
+  "properties": {
+    "violations": {
+      "type": "array",
+      "items": { "$ref": "#/$defs/Violation" }
+    },
+    "stale_todos": {
+      "type": "array",
+      "items": { "$ref": "#/$defs/StaleTodo" }
+    },
+    "summary": {
+      "type": "object",
+      "required": [
+        "violation_count",
+        "stale_todo_count",
+        "strict_violation_count",
+        "success"
+      ],
+      "additionalProperties": false,
+      "properties": {
+        "violation_count": { "type": "integer", "minimum": 0 },
+        "stale_todo_count": { "type": "integer", "minimum": 0 },
+        "strict_violation_count": {
+          "type": "integer",
+          "minimum": 0,
+          "description": "Count of violations where strict=true (subset of violation_count)"
+        },
+        "success": { "type": "boolean" }
+      }
+    }
+  },
+  "$defs": {
+    "ViolationType": {
+      "type": "string",
+      "enum": ["dependency", "privacy", "visibility", "layer", "folder_privacy"]
+    },
+    "Violation": {
+      "type": "object",
+      "required": [
+        "violation_type",
+        "file",
+        "line",
+        "column",
+        "constant_name",
+        "referencing_pack_name",
+        "defining_pack_name",
+        "strict",
+        "message"
+      ],
+      "additionalProperties": false,
+      "properties": {
+        "violation_type": { "$ref": "#/$defs/ViolationType" },
+        "file": { "type": "string" },
+        "line": { "type": "integer", "minimum": 1 },
+        "column": { "type": "integer", "minimum": 0 },
+        "constant_name": { "type": "string" },
+        "referencing_pack_name": { "type": "string" },
+        "defining_pack_name": { "type": "string" },
+        "strict": { "type": "boolean" },
+        "message": { "type": "string" }
+      }
+    },
+    "StaleTodo": {
+      "type": "object",
+      "required": [
+        "violation_type",
+        "file",
+        "constant_name",
+        "referencing_pack_name",
+        "defining_pack_name"
+      ],
+      "additionalProperties": false,
+      "properties": {
+        "violation_type": { "$ref": "#/$defs/ViolationType" },
+        "file": { "type": "string" },
+        "constant_name": { "type": "string" },
+        "referencing_pack_name": { "type": "string" },
+        "defining_pack_name": { "type": "string" }
+      }
+    }
+  }
+}

src/main.rs

@@ -1,5 +1,18 @@
 use packs::packs::cli;
+use std::process::ExitCode;
 
-pub fn main() -> anyhow::Result<()> {
-    cli::run()
+pub fn main() -> ExitCode {
+    match cli::run() {
+        Ok(()) => ExitCode::SUCCESS,
+        Err(e) => {
+            if e.downcast_ref::<cli::ViolationsFound>().is_some() {
+                // ViolationsFound already printed its output; exit 1 for violations
+                ExitCode::from(1)
+            } else {
+                // Other errors (IO, config, etc.) exit 2; usage errors handled by clap
+                eprintln!("Error: {e:#}");
+                ExitCode::from(2)
+            }
+        }
+    }
 }

src/packs.rs

@@ -13,6 +13,7 @@ pub(crate) mod creator;
 pub(crate) mod csv;
 pub(crate) mod dependencies;
 pub(crate) mod ignored;
+pub(crate) mod json;
 pub(crate) mod monkey_patch_detection;
 pub(crate) mod pack;
 pub(crate) mod parsing;
@@ -40,6 +41,7 @@ pub(crate) use self::parsing::ParsedDefinition;
 pub(crate) use self::parsing::UnresolvedReference;
 use anyhow::bail;
 use cli::OutputFormat;
+use cli::ViolationsFound;
 pub(crate) use configuration::Configuration;
 pub(crate) use package_todo::PackageTodo;
 
@@ -79,13 +81,17 @@ pub fn check(
     match output_format {
         OutputFormat::Packwerk => {
             println!("{}", result);
-            if result.has_violations() {
-                bail!("Violations found!")
-            }
         }
         OutputFormat::CSV => {
             csv::write_csv(&result, std::io::stdout())?;
         }
+        OutputFormat::JSON => {
+            json::write_json(&result, std::io::stdout())?;
+        }
+    }
+
+    if result.has_violations() {
+        return Err(ViolationsFound.into());
     }
 
     Ok(())
@@ -230,10 +236,12 @@ pub struct ProcessedFile {
     pub definitions: Vec<ParsedDefinition>,
 }
 
-#[derive(Debug, PartialEq, Serialize, Deserialize, Default, Eq, Clone)]
+#[derive(
+    Debug, PartialEq, Serialize, Deserialize, Default, Eq, Clone, Hash,
+)]
 pub struct SourceLocation {
-    line: usize,
-    column: usize,
+    pub line: usize,
+    pub column: usize,
 }
 
 pub(crate) fn list_definitions(

src/packs/checker.rs

@@ -16,6 +16,7 @@ use crate::packs::pack::write_pack_to_disk;
 use crate::packs::pack::Pack;
 use crate::packs::package_todo;
 use crate::packs::Configuration;
+use crate::packs::SourceLocation;
 
 use anyhow::bail;
 // External imports
@@ -42,10 +43,20 @@ pub struct ViolationIdentifier {
     pub referencing_pack_name: String,
     pub defining_pack_name: String,
 }
+/// A violation combines an identifier with display metadata.
+///
+/// `source_location` is intentionally separate from `ViolationIdentifier` because:
+/// - The identifier defines "sameness" for deduplication and comparison with
+///   recorded violations in `package_todo.yml`, which doesn't store line/column
+/// - Multiple references to the same constant in the same file are considered
+///   one violation, even if they occur at different lines
+/// - Keeping line/column out of the identity makes violations stable across
+///   minor code movements that shift line numbers
 #[derive(PartialEq, Clone, Eq, Hash, Debug)]
 pub struct Violation {
     pub message: String,
     pub identifier: ViolationIdentifier,
+    pub source_location: SourceLocation,
 }
 
 pub(crate) trait CheckerInterface {
@@ -513,6 +524,7 @@ mod tests {
     use crate::packs::checker::{
         CheckAllResult, Violation, ViolationIdentifier,
     };
+    use crate::packs::SourceLocation;
 
     #[test]
     fn test_write_violations() {
@@ -526,7 +538,8 @@ mod tests {
                         constant_name: "::Foo::PrivateClass".to_string(),
                         referencing_pack_name: "bar".to_string(),
                         defining_pack_name: "foo".to_string(),
-                    }
+                    },
+                    source_location: SourceLocation { line: 10, column: 5 },
                 },
                 Violation {
                     message: "foo/bar/file2.rb:15:3\nDependency violation: `::Foo::AnotherClass` is not allowed to depend on `::Bar::SomeClass`".to_string(),
@@ -537,7 +550,8 @@ mod tests {
                         constant_name: "::Foo::AnotherClass".to_string(),
                         referencing_pack_name: "foo".to_string(),
                         defining_pack_name: "bar".to_string(),
-                    }
+                     },
+                     source_location: SourceLocation { line: 15, column: 3 },
                 }].iter().cloned().collect(),
             stale_violations: Vec::new(),
             strict_mode_violations: HashSet::new(),

src/packs/checker/common_test.rs

@@ -54,6 +54,7 @@ pub mod tests {
                 referencing_pack_name: String::from("packs/foo"),
                 defining_pack_name: String::from("packs/bar"),
             },
+            source_location: SourceLocation { line: 3, column: 1 },
         }
     }
 

src/packs/checker/pack_checker.rs

@@ -161,6 +161,7 @@ impl<'a> PackChecker<'a> {
         Ok(Some(Violation {
             message: self.interpolate_violation_message(extra_template_fields),
             identifier: self.violation_identifier(),
+            source_location: self.reference.source_location.clone(),
         }))
     }
 

src/packs/cli.rs

@@ -8,6 +8,22 @@ use tracing::debug;
 
 use super::logger::install_logger;
 
+/// Error returned when violations are found during check.
+///
+/// This is a marker type used to distinguish "violations found" (exit 1)
+/// from internal errors (exit 2) following linter conventions (eslint, rubocop, shellcheck).
+#[derive(Debug)]
+pub struct ViolationsFound;
+
+impl std::fmt::Display for ViolationsFound {
+    fn fmt(&self, _f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        // Output is already printed; this marker carries no additional message
+        Ok(())
+    }
+}
+
+impl std::error::Error for ViolationsFound {}
+
 /// A CLI to interact with packs
 #[derive(Parser, Debug)]
 #[command(author, version, about, long_about = None)]
@@ -159,6 +175,7 @@ enum Command {
 pub enum OutputFormat {
     Packwerk,
     CSV,
+    JSON,
 }
 
 #[derive(Debug, Args)]
@@ -191,9 +208,9 @@ impl Args {
 
 pub fn run() -> anyhow::Result<()> {
     let args = Args::parse();
-    let absolute_root = args
-        .absolute_project_root()
-        .expect("Issue getting absolute_project_root!");
+    let absolute_root = args.absolute_project_root().map_err(|e| {
+        anyhow::anyhow!("Issue getting absolute_project_root: {}", e)
+    })?;
 
     install_logger(args.debug);