From b8decbc0bec21505737cf00d9639effdcc2450d3 Mon Sep 17 00:00:00 2001 From: Denis Kisselev Date: Mon, 8 Jun 2026 16:56:05 -0700 Subject: [PATCH] feat: surface stale-CODEOWNERS diff as info output instead of inline error The diff added in #107 was rendered inline under the "CODEOWNERS out of date" headline, so a long diff pushed the actionable line up and out of view in CI logs, and the wrapping code_ownership gem raised the entire diff as part of the error. Route the diff through RunResult.info_messages instead. main.rs prints info_messages ahead of validation errors and still exits 1, so the diff prints first as informational output while the terse, actionable headline lands at the bottom of the log where it's seen. Errors::messages() no longer emits the diff; a new Errors::info_messages() extracts it. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/ownership/validator.rs | 30 +++++++++++++++++++++------- src/runner.rs | 3 +++ tests/executable_name_config_test.rs | 8 ++++---- tests/invalid_project_test.rs | 4 ++-- 4 files changed, 32 insertions(+), 13 deletions(-) diff --git a/src/ownership/validator.rs b/src/ownership/validator.rs index 1ff1f79..664de3a 100644 --- a/src/ownership/validator.rs +++ b/src/ownership/validator.rs @@ -208,18 +208,34 @@ impl Error { vec![messages.join("\n")] } - Error::CodeownershipFileIsStale { executable_name: _, diff } => { - if diff.is_empty() { - vec![] - } else { - vec![format!("The following changes are required (- current, + expected):\n{diff}")] - } - } + // The diff is intentionally *not* rendered as part of the error. It is + // surfaced separately as an informational message (see `Errors::info_messages`) + // so that a long diff doesn't bury the actionable headline. + Error::CodeownershipFileIsStale { .. } => vec![], Error::InvalidTeam { name, path } => vec![format!("- {} is referencing an invalid team - '{}'", path.to_string_lossy(), name)], } } } +impl Errors { + /// Supplementary detail that explains *what* is wrong without itself being an error. + /// The stale-CODEOWNERS diff is surfaced here, as informational output, rather than + /// inline with the error so that a long diff doesn't bury the actionable headline in + /// CI logs (and so the wrapping `code_ownership` gem raises only the headline rather + /// than the entire diff). + pub fn info_messages(&self) -> Vec { + self.0 + .iter() + .filter_map(|error| match error { + Error::CodeownershipFileIsStale { diff, .. } if !diff.is_empty() => { + Some(format!("The following changes are required (- current, + expected):\n{diff}")) + } + _ => None, + }) + .collect() + } +} + impl Display for Errors { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let grouped_errors = self.0.iter().into_group_map_by(|error| error.category()); diff --git a/src/runner.rs b/src/runner.rs index 6f6c688..e7704b4 100644 --- a/src/runner.rs +++ b/src/runner.rs @@ -128,6 +128,9 @@ impl Runner { match self.ownership.validate() { Ok(_) => RunResult::default(), Err(err) => RunResult { + // The stale-CODEOWNERS diff (if any) rides along as informational output, + // printed ahead of the errors, so the actionable headline isn't buried. + info_messages: err.info_messages(), validation_errors: vec![format!("{}", err)], ..Default::default() }, diff --git a/tests/executable_name_config_test.rs b/tests/executable_name_config_test.rs index e71da1d..4dc135d 100644 --- a/tests/executable_name_config_test.rs +++ b/tests/executable_name_config_test.rs @@ -41,8 +41,6 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box false, OutputStream::Stdout, predicate::eq(indoc! {r#" - - CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file The following changes are required (- current, + expected): -# Outdated content to trigger validation error -/app/old.rb @FooTeam @@ -57,6 +55,8 @@ fn test_custom_executable_name_full_error_message() -> Result<(), Box +/config/teams/foo.yml @FooTeam +/config/teams/payments.yml @PaymentTeam + CODEOWNERS out of date. Run `bin/codeownership validate` to update the CODEOWNERS file + "#}), )?; Ok(()) @@ -71,8 +71,6 @@ fn test_default_executable_name_full_error_message() -> Result<(), Box Result<(), Box Result<(), Box> { false, OutputStream::Stdout, predicate::eq(indoc! {" - - CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file The following changes are required (- current, + expected): +# STOP! - DO NOT EDIT THIS FILE MANUALLY +# This file was automatically generated by \"bin/codeownership validate\". @@ -47,6 +45,8 @@ fn test_validate() -> Result<(), Box> { +# Team owned gems +/gems/payroll_calculator/**/** @PayrollTeam + CODEOWNERS out of date. Run `codeowners generate` to update the CODEOWNERS file + Code ownership should only be defined for each file in one way. The following files have declared ownership in multiple ways gems/payroll_calculator/calculator.rb