fix(cli): fail lint when rule violations are reported

docs/src/commands/buffrs-lint.md

@@ -5,7 +5,14 @@ Rules](../reference/protocol-buffer-rules.md))
 
 ### Synopsis
 
-`buffrs lint`
+`buffrs lint [--deny warnings | --allow warnings]`
+
+### Exit status
+
+Every violation is printed. The command exits with a non-zero status when any
+error-level violation is reported. Warning-level violations are printed and do
+not change the exit status unless `--deny warnings` is passed. `--allow
+warnings` selects the default, which reports warnings without failing.
 
 ### Description
 

src/command.rs

@@ -397,8 +397,11 @@ pub async fn list() -> miette::Result<()> {
 }
 
 /// Parses current package and validates rules.
+///
+/// Every violation is printed. The command fails when an error-level violation
+/// is present, and also when `deny_warnings` is set and a warning is present.
 #[cfg(feature = "validation")]
-pub async fn lint() -> miette::Result<()> {
+pub async fn lint(deny_warnings: bool) -> miette::Result<()> {
     let manifest_path = PathBuf::from(MANIFEST_FILE);
     let manifest = Manifest::require_package_manifest(&manifest_path).await?;
     let store = PackageStore::current().await?;
@@ -409,12 +412,20 @@ pub async fn lint() -> miette::Result<()> {
 
     store.populate(&pkg).await?;
     let violations = store.validate(&pkg).await?;
+    let fatal = violations
+        .iter()
+        .filter(|violation| violation.level.causes_lint_failure(deny_warnings))
+        .count();
 
     violations
         .into_iter()
         .map(miette::Report::new)
         .for_each(|r| eprintln!("{r:?}"));
 
+    if fatal > 0 {
+        bail!("found {fatal} rule violation(s)");
+    }
+
     Ok(())
 }
 

src/main.rs

@@ -70,7 +70,19 @@ enum Command {
     },
 
     /// Check rule violations for this package.
-    Lint,
+    Lint {
+        /// Fail the command when violations of this severity are reported.
+        ///
+        /// Error-level violations always fail the command. Warnings are printed
+        /// either way, and fail the command only when denied.
+        #[arg(long, value_name = "LEVEL", conflicts_with = "allow")]
+        deny: Option<LintSeverity>,
+        /// Do not fail the command when violations of this severity are reported.
+        ///
+        /// This is the default for warnings.
+        #[arg(long, value_name = "LEVEL", conflicts_with = "deny")]
+        allow: Option<LintSeverity>,
+    },
 
     /// Adds dependencies to a manifest file
     Add {
@@ -272,10 +284,14 @@ async fn main() -> miette::Result<()> {
         .wrap_err(miette!(
             "failed to publish `{package}` to `{registry}:{repository}`",
         )),
-        Command::Lint => command::lint().await.wrap_err(miette!(
-            "failed to lint protocol buffers in `{}`",
-            PackageStore::PROTO_PATH
-        )),
+        Command::Lint { deny, allow: _ } => {
+            let deny_warnings = matches!(deny, Some(LintSeverity::Warnings));
+
+            command::lint(deny_warnings).await.wrap_err(miette!(
+                "failed to lint protocol buffers in `{}`",
+                PackageStore::PROTO_PATH
+            ))
+        }
         Command::Install {
             preserve_local_mtime,
             offline,
@@ -303,6 +319,12 @@ async fn main() -> miette::Result<()> {
     }
 }
 
+#[derive(Copy, Clone, Debug, PartialEq, Eq, clap::ValueEnum)]
+enum LintSeverity {
+    /// Warning-level rule violations.
+    Warnings,
+}
+
 fn infer_package_type(lib: bool, api: bool) -> Option<PackageType> {
     if lib {
         Some(PackageType::Lib)

src/validation/rules/package_hierarchy.rs

@@ -48,7 +48,7 @@ impl Rule for PackageHierarchy {
                     for (expected, component) in expected_components.iter().zip(components) {
                         if component != *expected {
                             return Some(self.to_violation(violation::Message {
-                                message: format!("expected file {} to live in {}.", path.to_string_lossy(), &expected_path),
+                                message: format!("expected file {} to live in {}.", path.to_string_lossy(), expected_path),
                                 help: "Package names should be mirrored in folder structure, eg mypackage.subpackage should live in proto/subpackage.".into(),
                             }));
                         }

src/validation/violation.rs

@@ -28,6 +28,20 @@ pub enum Level {
     Error,
 }
 
+impl Level {
+    /// Whether `buffrs lint` should exit with a failure for this severity.
+    ///
+    /// Errors always fail the command. Warnings fail only when `deny_warnings`
+    /// is set. Informational violations are reported and never fail the command.
+    pub fn causes_lint_failure(self, deny_warnings: bool) -> bool {
+        match self {
+            Self::Error => true,
+            Self::Warning => deny_warnings,
+            Self::Info => false,
+        }
+    }
+}
+
 /// Location of violation.
 #[derive(Default, PartialEq, Clone, Eq, Debug)]
 pub struct Location {
@@ -73,7 +87,7 @@ impl std::error::Error for Violation {}
 
 impl Display for Violation {
     fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
-        write!(f, "{}", &self.info)
+        write!(f, "{}", self.info)
     }
 }
 
@@ -134,3 +148,26 @@ impl SourceCode for Location {
         )))
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::Level;
+
+    #[test]
+    fn errors_fail_lint_regardless_of_warning_policy() {
+        assert!(Level::Error.causes_lint_failure(false));
+        assert!(Level::Error.causes_lint_failure(true));
+    }
+
+    #[test]
+    fn warnings_fail_lint_only_when_denied() {
+        assert!(!Level::Warning.causes_lint_failure(false));
+        assert!(Level::Warning.causes_lint_failure(true));
+    }
+
+    #[test]
+    fn info_never_fails_lint() {
+        assert!(!Level::Info.causes_lint_failure(false));
+        assert!(!Level::Info.causes_lint_failure(true));
+    }
+}

tests/cmd/lint/mod.rs

@@ -12,3 +12,74 @@ fn fixture() {
         .stdout(include_str!("stdout.log"))
         .stderr(include_str!("stderr.log"));
 }
+
+#[test]
+fn exits_with_failure_when_a_rule_is_violated() {
+    let vfs = VirtualFileSystem::empty();
+    let root = vfs.root();
+
+    crate::cli!()
+        .args(["init", "--lib", "my-protos"])
+        .current_dir(&root)
+        .assert()
+        .success();
+
+    std::fs::write(
+        root.join("proto/my_protos.proto"),
+        "syntax = \"proto3\";\npackage wrongpkg;\nmessage Foo { string bar = 1; }\n",
+    )
+    .unwrap();
+
+    let output = crate::cli!()
+        .arg("lint")
+        .current_dir(&root)
+        .assert()
+        .failure();
+
+    let stderr = String::from_utf8_lossy(&output.get_output().stderr);
+    assert!(
+        stderr.contains("wrongpkg"),
+        "expected the package-name violation in stderr, got:\n{stderr}"
+    );
+    assert!(
+        stderr.contains("found 1 rule violation(s)"),
+        "expected a failing lint summary in stderr, got:\n{stderr}"
+    );
+}
+
+#[test]
+fn allow_warnings_still_fails_on_errors() {
+    let vfs = VirtualFileSystem::empty();
+    let root = vfs.root();
+
+    crate::cli!()
+        .args(["init", "--lib", "my-protos"])
+        .current_dir(&root)
+        .assert()
+        .success();
+
+    std::fs::write(
+        root.join("proto/my_protos.proto"),
+        "syntax = \"proto3\";\npackage wrongpkg;\nmessage Foo { string bar = 1; }\n",
+    )
+    .unwrap();
+
+    crate::cli!()
+        .args(["lint", "--allow", "warnings"])
+        .current_dir(&root)
+        .assert()
+        .failure();
+}
+
+#[test]
+fn deny_warnings_keeps_a_clean_package_successful() {
+    let vfs = VirtualFileSystem::copy(crate::parent_directory!().join("in"));
+
+    crate::cli!()
+        .args(["lint", "--deny", "warnings"])
+        .current_dir(vfs.root())
+        .assert()
+        .success()
+        .stdout(include_str!("stdout.log"))
+        .stderr(include_str!("stderr.log"));
+}