From 117e7c4e69d43143d8a97724b7e2773fdb3afabf Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 16 Dec 2025 18:00:53 -0800 Subject: [PATCH 1/3] Implement json output format option Validate against the included schema for check json output Co-authored-by: Ivy Evans --- Cargo.toml | 49 ++++----- schema/check-output.json | 83 +++++++++++++++ src/packs.rs | 4 + src/packs/cli.rs | 1 + src/packs/json.rs | 101 ++++++++++++++++++ tests/check_test.rs | 214 +++++++++++++++++++++++++++++++++++++++ 6 files changed, 428 insertions(+), 24 deletions(-) create mode 100644 schema/check-output.json create mode 100644 src/packs/json.rs diff --git a/Cargo.toml b/Cargo.toml index 009362c..666ac5f 100644 --- a/Cargo.toml +++ b/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 diff --git a/schema/check-output.json b/schema/check-output.json new file mode 100644 index 0000000..155d409 --- /dev/null +++ b/schema/check-output.json @@ -0,0 +1,83 @@ +{ + "$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", + "constant_name", + "referencing_pack_name", + "defining_pack_name", + "strict", + "message" + ], + "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" }, + "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" } + } + } + } +} diff --git a/src/packs.rs b/src/packs.rs index b10f66d..fe76603 100644 --- a/src/packs.rs +++ b/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; @@ -86,6 +87,9 @@ pub fn check( OutputFormat::CSV => { csv::write_csv(&result, std::io::stdout())?; } + OutputFormat::JSON => { + json::write_json(&result, std::io::stdout())?; + } } Ok(()) diff --git a/src/packs/cli.rs b/src/packs/cli.rs index a9fec5e..cd5cb04 100644 --- a/src/packs/cli.rs +++ b/src/packs/cli.rs @@ -159,6 +159,7 @@ enum Command { pub enum OutputFormat { Packwerk, CSV, + JSON, } #[derive(Debug, Args)] diff --git a/src/packs/json.rs b/src/packs/json.rs new file mode 100644 index 0000000..9ddb2c7 --- /dev/null +++ b/src/packs/json.rs @@ -0,0 +1,101 @@ +use itertools::chain; +use serde::Serialize; + +use super::checker::{build_strict_violation_message, CheckAllResult}; + +#[derive(Serialize)] +struct JsonOutput<'a> { + violations: Vec>, + stale_todos: Vec>, + summary: JsonSummary, +} + +#[derive(Serialize)] +struct JsonViolation<'a> { + violation_type: &'a str, + file: &'a str, + constant_name: &'a str, + referencing_pack_name: &'a str, + defining_pack_name: &'a str, + strict: bool, + message: String, +} + +#[derive(Serialize)] +struct JsonStaleTodo<'a> { + violation_type: &'a str, + file: &'a str, + constant_name: &'a str, + referencing_pack_name: &'a str, + defining_pack_name: &'a str, +} + +#[derive(Serialize)] +struct JsonSummary { + violation_count: usize, + stale_todo_count: usize, + strict_violation_count: usize, + success: bool, +} + +pub fn write_json( + result: &CheckAllResult, + writer: W, +) -> anyhow::Result<()> { + let all_violations = chain!( + &result.reportable_violations, + &result.strict_mode_violations + ); + + let violations: Vec = all_violations + .map(|v| { + let message = if v.identifier.strict { + build_strict_violation_message(&v.identifier) + } else { + v.message.clone() + }; + JsonViolation { + violation_type: &v.identifier.violation_type, + file: &v.identifier.file, + constant_name: &v.identifier.constant_name, + referencing_pack_name: &v.identifier.referencing_pack_name, + defining_pack_name: &v.identifier.defining_pack_name, + strict: v.identifier.strict, + message, + } + }) + .collect(); + + let stale_todos: Vec = result + .stale_violations + .iter() + .map(|v| JsonStaleTodo { + violation_type: &v.violation_type, + file: &v.file, + constant_name: &v.constant_name, + referencing_pack_name: &v.referencing_pack_name, + defining_pack_name: &v.defining_pack_name, + }) + .collect(); + + let violation_count = violations.len(); + let stale_todo_count = stale_todos.len(); + let strict_violation_count = result.strict_mode_violations.len(); + let success = violation_count == 0 + && stale_todo_count == 0 + && strict_violation_count == 0; + + let output = JsonOutput { + violations, + stale_todos, + summary: JsonSummary { + violation_count, + stale_todo_count, + strict_violation_count, + success, + }, + }; + + serde_json::to_writer(writer, &output)?; + Ok(()) +} diff --git a/tests/check_test.rs b/tests/check_test.rs index f3d8448..ba71797 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -1,9 +1,26 @@ use assert_cmd::cargo::cargo_bin_cmd; +use jsonschema::Validator; use predicates::prelude::*; use std::{error::Error, fs}; mod common; +fn validate_check_output_schema(json_value: &serde_json::Value) { + let schema_str = fs::read_to_string("schema/check-output.json") + .expect("Failed to read schema file"); + let schema: serde_json::Value = + serde_json::from_str(&schema_str).expect("Schema should be valid JSON"); + let validator = + Validator::new(&schema).expect("Schema should be valid JSON Schema"); + if !validator.is_valid(json_value) { + let errors: Vec = validator + .iter_errors(json_value) + .map(|e| format!(" - {}", e)) + .collect(); + panic!("JSON output does not match schema:\n{}", errors.join("\n")); + } +} + pub fn stripped_output(output: Vec) -> String { String::from_utf8_lossy(&strip_ansi_escapes::strip(output)).to_string() } @@ -399,3 +416,200 @@ fn test_check_contents_ignoring_recorded_violations( common::teardown(); Ok(()) } + +#[test] +fn test_check_with_json_output_format_violations() -> Result<(), Box> +{ + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/simple_app") + .arg("check") + .arg("-o") + .arg("json") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["violation_count"], 2); + assert_eq!(json_output["summary"]["stale_todo_count"], 0); + assert_eq!(json_output["summary"]["strict_violation_count"], 0); + assert_eq!(json_output["summary"]["success"], false); + + let violations = json_output["violations"].as_array().unwrap(); + assert_eq!(violations.len(), 2); + + let dependency_violation = violations + .iter() + .find(|v| v["violation_type"].as_str().unwrap() == "dependency") + .expect("Should have a dependency violation"); + + assert_eq!( + dependency_violation["file"].as_str().unwrap(), + "packs/foo/app/services/foo.rb" + ); + assert_eq!( + dependency_violation["constant_name"].as_str().unwrap(), + "::Bar" + ); + assert_eq!( + dependency_violation["referencing_pack_name"] + .as_str() + .unwrap(), + "packs/foo" + ); + assert_eq!( + dependency_violation["defining_pack_name"].as_str().unwrap(), + "packs/bar" + ); + assert_eq!(dependency_violation["strict"].as_bool().unwrap(), false); + assert!(dependency_violation["message"] + .as_str() + .unwrap() + .contains("Dependency violation")); + + let privacy_violation = violations + .iter() + .find(|v| v["violation_type"].as_str().unwrap() == "privacy") + .expect("Should have a privacy violation"); + + assert_eq!( + privacy_violation["file"].as_str().unwrap(), + "packs/foo/app/services/foo.rb" + ); + assert_eq!( + privacy_violation["constant_name"].as_str().unwrap(), + "::Bar" + ); + assert_eq!( + privacy_violation["referencing_pack_name"].as_str().unwrap(), + "packs/foo" + ); + assert_eq!( + privacy_violation["defining_pack_name"].as_str().unwrap(), + "packs/bar" + ); + assert_eq!(privacy_violation["strict"].as_bool().unwrap(), false); + assert!(privacy_violation["message"] + .as_str() + .unwrap() + .contains("Privacy violation")); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_json_output_format_stale_todos() -> Result<(), Box> +{ + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_stale_violations") + .arg("check") + .arg("-o") + .arg("json") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + // The fixture has multiple stale todos + assert_eq!(json_output["summary"]["stale_todo_count"], 3); + assert_eq!(json_output["summary"]["violation_count"], 0); + assert_eq!(json_output["summary"]["strict_violation_count"], 0); + assert_eq!(json_output["summary"]["success"], false); + + let stale_todos = json_output["stale_todos"].as_array().unwrap(); + assert_eq!(stale_todos.len(), 3); + + // Find the specific stale todo for ::Bar dependency violation + let bar_dependency_stale = stale_todos + .iter() + .find(|t| { + t["constant_name"].as_str().unwrap() == "::Bar" + && t["violation_type"].as_str().unwrap() == "dependency" + }) + .expect("Should have stale dependency todo for ::Bar"); + + assert_eq!( + bar_dependency_stale["file"].as_str().unwrap(), + "packs/foo/app/services/foo.rb" + ); + assert_eq!( + bar_dependency_stale["referencing_pack_name"] + .as_str() + .unwrap(), + "packs/foo" + ); + assert_eq!( + bar_dependency_stale["defining_pack_name"].as_str().unwrap(), + "packs/bar" + ); + + // Find the stale todo for ::Foo privacy violation + let foo_privacy_stale = stale_todos + .iter() + .find(|t| { + t["constant_name"].as_str().unwrap() == "::Foo" + && t["violation_type"].as_str().unwrap() == "privacy" + }) + .expect("Should have stale privacy todo for ::Foo"); + + assert_eq!( + foo_privacy_stale["file"].as_str().unwrap(), + "packs/bar/app/services/bar.rb" + ); + assert_eq!( + foo_privacy_stale["referencing_pack_name"].as_str().unwrap(), + "packs/bar" + ); + assert_eq!( + foo_privacy_stale["defining_pack_name"].as_str().unwrap(), + "packs/foo" + ); + + common::teardown(); + Ok(()) +} + +#[test] +fn test_check_with_json_output_format_empty() -> Result<(), Box> { + let output = cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/contains_package_todo") + .arg("check") + .arg("-o") + .arg("json") + .assert() + .success() + .get_output() + .stdout + .clone(); + + let json_output: serde_json::Value = + serde_json::from_slice(&output).expect("Output should be valid JSON"); + + validate_check_output_schema(&json_output); + + assert_eq!(json_output["summary"]["violation_count"], 0); + assert_eq!(json_output["summary"]["stale_todo_count"], 0); + assert_eq!(json_output["summary"]["strict_violation_count"], 0); + assert_eq!(json_output["summary"]["success"], true); + assert!(json_output["violations"].as_array().unwrap().is_empty()); + assert!(json_output["stale_todos"].as_array().unwrap().is_empty()); + + common::teardown(); + Ok(()) +} From ac909dd951dcb882793b4d7811cebf71257e894a Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Tue, 16 Dec 2025 18:23:04 -0800 Subject: [PATCH 2/3] Add line and column to json output for violations --- schema/check-output.json | 4 ++++ src/packs.rs | 8 +++++--- src/packs/checker.rs | 18 ++++++++++++++++-- src/packs/checker/common_test.rs | 1 + src/packs/checker/pack_checker.rs | 1 + src/packs/json.rs | 4 ++++ 6 files changed, 31 insertions(+), 5 deletions(-) diff --git a/schema/check-output.json b/schema/check-output.json index 155d409..bc63f20 100644 --- a/schema/check-output.json +++ b/schema/check-output.json @@ -44,6 +44,8 @@ "required": [ "violation_type", "file", + "line", + "column", "constant_name", "referencing_pack_name", "defining_pack_name", @@ -54,6 +56,8 @@ "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" }, diff --git a/src/packs.rs b/src/packs.rs index fe76603..afe056f 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -234,10 +234,12 @@ pub struct ProcessedFile { pub definitions: Vec, } -#[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( diff --git a/src/packs/checker.rs b/src/packs/checker.rs index cad1cb4..58544af 100644 --- a/src/packs/checker.rs +++ b/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(), diff --git a/src/packs/checker/common_test.rs b/src/packs/checker/common_test.rs index 098c574..f090f70 100644 --- a/src/packs/checker/common_test.rs +++ b/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 }, } } diff --git a/src/packs/checker/pack_checker.rs b/src/packs/checker/pack_checker.rs index 3ba886c..d2eaed4 100644 --- a/src/packs/checker/pack_checker.rs +++ b/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(), })) } diff --git a/src/packs/json.rs b/src/packs/json.rs index 9ddb2c7..96902ac 100644 --- a/src/packs/json.rs +++ b/src/packs/json.rs @@ -14,6 +14,8 @@ struct JsonOutput<'a> { struct JsonViolation<'a> { violation_type: &'a str, file: &'a str, + line: usize, + column: usize, constant_name: &'a str, referencing_pack_name: &'a str, defining_pack_name: &'a str, @@ -57,6 +59,8 @@ pub fn write_json( JsonViolation { violation_type: &v.identifier.violation_type, file: &v.identifier.file, + line: v.source_location.line, + column: v.source_location.column, constant_name: &v.identifier.constant_name, referencing_pack_name: &v.identifier.referencing_pack_name, defining_pack_name: &v.identifier.defining_pack_name, From b72253485bf8ab8cc7c55136c16276b0d02ac55c Mon Sep 17 00:00:00 2001 From: Martin Emde Date: Wed, 17 Dec 2025 17:02:28 -0800 Subject: [PATCH 3/3] 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 --- src/main.rs | 17 +++++++++-- src/packs.rs | 8 +++-- src/packs/cli.rs | 18 +++++++++++- tests/check_test.rs | 71 ++++++++++++++++++++++++++++++++------------- 4 files changed, 88 insertions(+), 26 deletions(-) diff --git a/src/main.rs b/src/main.rs index b3ca5a6..f4521ba 100644 --- a/src/main.rs +++ b/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) => { + // Exit 1: Violations found (application-specific failure) + // Exit 2: Internal errors (config, IO, etc.) + if e.downcast_ref::().is_some() { + ExitCode::from(1) + } else { + eprintln!("Error: {e:#}"); + ExitCode::from(2) + } + } + } } diff --git a/src/packs.rs b/src/packs.rs index afe056f..3cc16a9 100644 --- a/src/packs.rs +++ b/src/packs.rs @@ -41,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; @@ -80,9 +81,6 @@ 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())?; @@ -92,6 +90,10 @@ pub fn check( } } + if result.has_violations() { + return Err(ViolationsFound.into()); + } + Ok(()) } diff --git a/src/packs/cli.rs b/src/packs/cli.rs index cd5cb04..3084a76 100644 --- a/src/packs/cli.rs +++ b/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)] @@ -194,7 +210,7 @@ pub fn run() -> anyhow::Result<()> { let args = Args::parse(); let absolute_root = args .absolute_project_root() - .expect("Issue getting absolute_project_root!"); + .map_err(|e| anyhow::anyhow!("Issue getting absolute_project_root: {}", e))?; install_logger(args.debug); diff --git a/tests/check_test.rs b/tests/check_test.rs index ba71797..f332940 100644 --- a/tests/check_test.rs +++ b/tests/check_test.rs @@ -34,7 +34,7 @@ fn test_check_with_privacy_dependency_error_template_overrides( .arg("--debug") .arg("check") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -56,7 +56,7 @@ fn test_check() -> Result<(), Box> { .arg("--debug") .arg("check") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -80,7 +80,7 @@ fn test_check_enforce_privacy_disabled() -> Result<(), Box> { .arg("--disable-enforce-privacy") .arg("check") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -103,7 +103,7 @@ fn test_check_enforce_dependency_disabled() -> Result<(), Box> { .arg("--disable-enforce-dependencies") .arg("check") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -126,7 +126,7 @@ fn test_check_with_single_file() -> Result<(), Box> { .arg("check") .arg("packs/foo/app/services/foo.rb") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -152,7 +152,7 @@ fn test_check_with_single_file_experimental_parser( .arg("check") .arg("packs/foo/app/services/foo.rb") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -175,7 +175,7 @@ fn test_check_with_package_todo_file() -> Result<(), Box> { .arg("--debug") .arg("check") .assert() - .success() + .code(0) .stdout(predicate::str::contains("No violations detected!")); common::teardown(); @@ -193,7 +193,7 @@ fn test_check_with_package_todo_file_csv() -> Result<(), Box> { .arg("-o") .arg("csv") .assert() - .success() + .code(0) .stdout(predicate::str::contains("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message")) .stdout(predicate::str::contains("No violations detected!")); @@ -212,7 +212,7 @@ fn test_check_with_package_todo_file_ignoring_recorded_violations( .arg("check") .arg("--ignore-recorded-violations") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -236,7 +236,7 @@ fn test_check_with_experimental_parser() -> Result<(), Box> { .arg("--debug") .arg("check") .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -258,7 +258,7 @@ fn test_check_with_stale_violations() -> Result<(), Box> { .arg("tests/fixtures/contains_stale_violations") .arg("check") .assert() - .failure() + .code(1) .stdout(predicate::str::contains( "There were stale violations found, please run `packs update`", )); @@ -275,7 +275,7 @@ fn test_check_with_stale_violations_when_file_no_longer_exists( .arg("tests/fixtures/contains_stale_violations_no_file") .arg("check") .assert() - .failure() + .code(1) .stdout(predicate::str::contains( "There were stale violations found, please run `packs update`", )); @@ -291,7 +291,7 @@ fn test_check_with_relationship_violations() -> Result<(), Box> { .arg("tests/fixtures/app_with_rails_relationships") .arg("check") .assert() - .failure() + .code(1) .stdout(predicate::str::contains("2 violation(s) detected:")) .stdout(predicate::str::contains("Privacy violation: `::Taco` is private to `packs/baz`, but referenced from `packs/bar`")) .stdout(predicate::str::contains("Privacy violation: `::Census` is private to `packs/baz`, but referenced from `packs/bar`")); @@ -307,7 +307,7 @@ fn test_check_without_stale_violations() -> Result<(), Box> { .arg("tests/fixtures/contains_package_todo") .arg("check") .assert() - .success() + .code(0) .stdout( predicate::str::contains( "There were stale violations found, please run `packs update`", @@ -326,7 +326,7 @@ fn test_check_with_strict_mode() -> Result<(), Box> { .arg("tests/fixtures/uses_strict_mode") .arg("check") .assert() - .failure() + .code(1) .stdout(predicate::str::contains( "packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack's package.yml file", )) @@ -347,6 +347,7 @@ fn test_check_with_strict_mode_output_csv() -> Result<(), Box> { .arg("-o") .arg("csv") .assert() + .code(1) .stdout(predicate::str::contains("Violation,Strict?,File,Constant,Referencing Pack,Defining Pack,Message")) .stdout(predicate::str::contains("privacy,true,packs/foo/app/services/foo.rb,::Bar,packs/foo,packs/bar,packs/foo cannot have privacy violations on packs/bar because strict mode is enabled for privacy violations in the enforcing pack\'s package.yml file")) .stdout(predicate::str::contains( @@ -372,7 +373,7 @@ fn test_check_contents() -> Result<(), Box> { .arg(relative_path) .write_stdin(format!("\n\n\n{}", foo_rb_contents)) .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -404,7 +405,7 @@ fn test_check_contents_ignoring_recorded_violations( .arg(relative_path) .write_stdin(format!("\n\n\n{}", foo_rb_contents)) .assert() - .failure() + .code(1) .get_output() .stdout .clone(); @@ -427,7 +428,7 @@ fn test_check_with_json_output_format_violations() -> Result<(), Box> .arg("-o") .arg("json") .assert() - .success() + .code(1) .get_output() .stdout .clone(); @@ -515,7 +516,7 @@ fn test_check_with_json_output_format_stale_todos() -> Result<(), Box .arg("-o") .arg("json") .assert() - .success() + .code(1) // Stale todos are violations that should fail .get_output() .stdout .clone(); @@ -593,7 +594,7 @@ fn test_check_with_json_output_format_empty() -> Result<(), Box> { .arg("-o") .arg("json") .assert() - .success() + .code(0) .get_output() .stdout .clone(); @@ -613,3 +614,33 @@ fn test_check_with_json_output_format_empty() -> Result<(), Box> { common::teardown(); Ok(()) } + +#[test] +fn test_check_with_nonexistent_project_root() -> Result<(), Box> { + // Exit code 2 for internal errors (non-existent project root) + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/does_not_exist") + .arg("check") + .assert() + .code(2) + .stderr(predicate::str::contains("Error:")); + + Ok(()) +} + +#[test] +fn test_check_with_invalid_output_format() -> Result<(), Box> { + // Exit code 2 for argument parsing errors (clap's default) + cargo_bin_cmd!("pks") + .arg("--project-root") + .arg("tests/fixtures/simple_app") + .arg("check") + .arg("-o") + .arg("invalid_format") + .assert() + .code(2) + .stderr(predicate::str::contains("invalid value 'invalid_format'")); + + Ok(()) +}