From 9e60a4d6531d71935f1a8a03fe79878d356aa03f Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Tue, 28 Jul 2026 13:19:21 -0400 Subject: [PATCH 1/2] Hard-fail on invalid ScopeKnownError config instead of silently dropping it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Config::new (now FoundConfig::new) discarded any raw config document that failed to convert into its typed model, with zero logging. A ScopeKnownError using a reserved capture group name, or shipping a broken fix template, would silently never register, giving false confidence scope was watching for it. ScopeKnownError conversion failures now abort config loading with a diagnosable error naming the file and cause (scope exits 2; scope-intercept falls back to an empty config after logging the error). Other kinds, chiefly ScopeDoctorGroup, keep the existing tolerant behavior from #68/PR #69 but now actually warn instead of failing silently — including a ScopeKnownError from an apiVersion this binary doesn't recognize, which is treated the same as any other not-yet-understood config rather than a hard failure. Also fixes ParsedConfig::try_from silently swallowing real deserialization errors behind a generic, typo'd message, and scope.rs's error log dropping the underlying cause by using Display instead of the alternate/chained form. Fixes #344 --- src/bin/scope.rs | 2 +- src/shared/config_load.rs | 217 +++++++++++++++++++++++++++++- src/shared/models/internal/mod.rs | 12 +- 3 files changed, 219 insertions(+), 12 deletions(-) diff --git a/src/bin/scope.rs b/src/bin/scope.rs index 979470a..92463ab 100644 --- a/src/bin/scope.rs +++ b/src/bin/scope.rs @@ -86,7 +86,7 @@ async fn main() { async fn run_subcommand(opts: Cli) -> i32 { let loaded_config = match opts.config.load_config().await { Err(e) => { - error!(target: "user", "Failed to load configuration: {}", e); + error!(target: "user", "Failed to load configuration: {:#}", e); return 2; } Ok(c) => c, diff --git a/src/shared/config_load.rs b/src/shared/config_load.rs index 4274dbd..2a14a99 100644 --- a/src/shared/config_load.rs +++ b/src/shared/config_load.rs @@ -1,5 +1,5 @@ -use crate::models::HelpMetadata; -use crate::models::prelude::ModelRoot; +use crate::models::prelude::{ModelRoot, V1AlphaKnownError}; +use crate::models::{HelpMetadata, InternalScopeModel}; use crate::shared::RUN_ID_ENV_VAR; use crate::shared::directories; use crate::shared::models::prelude::{DoctorGroup, KnownError, ParsedConfig, ReportUploadLocation}; @@ -70,7 +70,7 @@ impl ConfigOptions { }; let config_path = self.find_scope_paths(&working_dir); - let found_config = FoundConfig::new(self, working_dir, config_path).await; + let found_config = FoundConfig::new(self, working_dir, config_path).await?; debug!("Loaded config {:?}", found_config); @@ -132,7 +132,7 @@ impl FoundConfig { config_options: &ConfigOptions, working_dir: PathBuf, config_path: Vec, - ) -> Self { + ) -> Result { let default_path = std::env::var("PATH").unwrap_or_default(); let mut config_path = config_path.to_vec(); @@ -164,13 +164,31 @@ impl FoundConfig { run_id: config_options.get_run_id(), }; + let known_error_api_version = V1AlphaKnownError::int_api_version().to_lowercase(); + let known_error_kind = V1AlphaKnownError::int_kind().to_lowercase(); for raw_config in raw_config { - if let Ok(value) = raw_config.try_into() { - this.add_model(value); + // A recognized `ScopeKnownError` that fails to build can never work as written, so + // that's a hard failure. Anything else (including a `ScopeKnownError` from an + // apiVersion this binary doesn't recognize) keeps the tolerant warn-and-continue + // behavior from #68/PR #69, since scope needs to tolerate config it doesn't yet + // understand. + let is_known_error = raw_config.kind.to_lowercase() == known_error_kind + && raw_config.api_version.to_lowercase() == known_error_api_version; + let full_name = raw_config.full_name(); + let file_path = raw_config.file_path(); + + match raw_config.try_into() { + Ok(value) => this.add_model(value), + Err(e) if is_known_error => { + return Err(e.context(format!("Unable to load {full_name} from {file_path}"))); + } + Err(e) => { + warn!(target: "user", "Unable to load {} from {} because {}", full_name.bold(), file_path, e); + } } } - this + Ok(this) } pub fn write_raw_config_to_disk(&self) -> Result { @@ -508,4 +526,189 @@ mod tests { let nonexistent_path = Path::new("/this/path/does/not/exist/hopefully"); build_config_path(nonexistent_path); } + + /// Regression test for https://github.com/Gusto/scope/issues/344: a `ScopeKnownError` that + /// fails its own validation (here, a reserved capture group name) must abort config load with + /// a diagnosable error, rather than silently registering nothing. + #[tokio::test] + async fn invalid_known_error_aborts_config_load() { + let temp_dir = tempdir().unwrap(); + let scope_dir = temp_dir.path().join(".scope"); + fs::create_dir_all(&scope_dir).unwrap(); + fs::write( + scope_dir.join("known-error.yaml"), + r#" +apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: bad-reserved +spec: + pattern: "boom: (?.*)" + help: "should never load" +"#, + ) + .unwrap(); + + let config_options = ConfigOptions::parse_from(["scope"]); + let result = FoundConfig::new( + &config_options, + temp_dir.path().to_path_buf(), + vec![scope_dir], + ) + .await; + + let err = result.expect_err("invalid ScopeKnownError should abort config load"); + let message = format!("{err:#}"); + assert!(message.contains("reserved"), "unexpected error: {message}"); + assert!( + message.contains("ScopeKnownError/bad-reserved"), + "unexpected error: {message}" + ); + } + + /// Companion to `invalid_known_error_aborts_config_load`: a `ScopeDoctorGroup` that fails to + /// build (here, a broken fix template) should warn and be dropped, not abort the whole load — + /// that's the pre-existing, intentional behavior from #68/PR #69. A valid group in the same + /// directory must still load, proving this is "drop the bad one", not "load nothing". + #[tokio::test] + async fn invalid_doctor_group_warns_and_continues() { + let temp_dir = tempdir().unwrap(); + let scope_dir = temp_dir.path().join(".scope"); + fs::create_dir_all(&scope_dir).unwrap(); + fs::write( + scope_dir.join("broken-group.yaml"), + r#" +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: broken +spec: + actions: + - name: action1 + check: {} + fix: + commands: + - "echo {{ unterminated" +"#, + ) + .unwrap(); + fs::write( + scope_dir.join("good-group.yaml"), + r#" +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: good +spec: + actions: + - name: action1 + check: {} + fix: + commands: + - echo fine +"#, + ) + .unwrap(); + + let config_options = ConfigOptions::parse_from(["scope"]); + let found_config = FoundConfig::new( + &config_options, + temp_dir.path().to_path_buf(), + vec![scope_dir], + ) + .await + .expect("a broken ScopeDoctorGroup should warn and continue, not abort"); + + assert!(!found_config.doctor_group.contains_key("broken")); + assert!(found_config.doctor_group.contains_key("good")); + } + + /// A `ScopeKnownError` from an apiVersion this binary doesn't recognize must NOT be treated as + /// a "recognized but invalid" known error — it's the same "config we don't yet understand" + /// case as an unrecognized kind, and should warn-and-continue rather than aborting the load. + /// (A future scope version adding a new known-error apiVersion must not break older binaries + /// running against a fleet-wide shared config directory.) + #[tokio::test] + async fn unrecognized_api_version_known_error_warns_and_continues() { + let temp_dir = tempdir().unwrap(); + let scope_dir = temp_dir.path().join(".scope"); + fs::create_dir_all(&scope_dir).unwrap(); + fs::write( + scope_dir.join("known-error.yaml"), + r#" +apiVersion: scope.github.com/v1beta +kind: ScopeKnownError +metadata: + name: future-version +spec: + pattern: "boom" + help: "from a version this binary doesn't understand" +"#, + ) + .unwrap(); + + let config_options = ConfigOptions::parse_from(["scope"]); + let found_config = FoundConfig::new( + &config_options, + temp_dir.path().to_path_buf(), + vec![scope_dir], + ) + .await + .expect("an unrecognized apiVersion should warn and continue, not abort"); + + assert!(found_config.known_error.is_empty()); + } + + /// A genuinely invalid `ScopeKnownError` aborts the whole config load, even when a perfectly + /// valid `ScopeDoctorGroup` sits in the same directory — this is the documented tradeoff of + /// treating known-error validation failures as fatal (see #344). + #[tokio::test] + async fn invalid_known_error_aborts_load_of_otherwise_valid_config() { + let temp_dir = tempdir().unwrap(); + let scope_dir = temp_dir.path().join(".scope"); + fs::create_dir_all(&scope_dir).unwrap(); + fs::write( + scope_dir.join("good-group.yaml"), + r#" +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: good +spec: + actions: + - name: action1 + check: {} + fix: + commands: + - echo fine +"#, + ) + .unwrap(); + fs::write( + scope_dir.join("bad-known-error.yaml"), + r#" +apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: bad-reserved +spec: + pattern: "boom: (?.*)" + help: "should never load" +"#, + ) + .unwrap(); + + let config_options = ConfigOptions::parse_from(["scope"]); + let result = FoundConfig::new( + &config_options, + temp_dir.path().to_path_buf(), + vec![scope_dir], + ) + .await; + + assert!( + result.is_err(), + "an invalid known error must abort the whole load, even alongside valid config" + ); + } } diff --git a/src/shared/models/internal/mod.rs b/src/shared/models/internal/mod.rs index 0a53f3a..3d9b72a 100644 --- a/src/shared/models/internal/mod.rs +++ b/src/shared/models/internal/mod.rs @@ -1,3 +1,4 @@ +use crate::models::HelpMetadata; use crate::models::InternalScopeModel; use crate::models::prelude::{ ModelRoot, V1AlphaDoctorGroup, V1AlphaKnownError, V1AlphaReportLocation, @@ -63,18 +64,21 @@ impl TryFrom> for ParsedConfig { type Error = anyhow::Error; fn try_from(value: ModelRoot) -> Result { - if let Ok(Some(known)) = V1AlphaDoctorGroup::known_type(&value) { + if let Some(known) = V1AlphaDoctorGroup::known_type(&value)? { return Ok(ParsedConfig::DoctorGroup(DoctorGroup::try_from(known)?)); } - if let Ok(Some(known)) = V1AlphaKnownError::known_type(&value) { + if let Some(known) = V1AlphaKnownError::known_type(&value)? { return Ok(ParsedConfig::KnownError(KnownError::try_from(known)?)); } - if let Ok(Some(known)) = V1AlphaReportLocation::known_type(&value) { + if let Some(known) = V1AlphaReportLocation::known_type(&value)? { return Ok(ParsedConfig::ReportUpload(ReportUploadLocation::try_from( known, )?)); } - Err(anyhow!("Error was know a known type")) + Err(anyhow!( + "'{}' did not match the apiVersion/kind of any known resource", + value.full_name() + )) } } From a382475e81a396b84427da9eef7e5a2ae3e427e1 Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Tue, 28 Jul 2026 13:41:25 -0400 Subject: [PATCH 2/2] Warn instead of hard-failing on invalid ScopeKnownError config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Revisited the design from the previous commit after further discussion: a recognized-but-invalid ScopeKnownError now warns and is dropped, the same as any other kind, rather than aborting the entire config load. Aborting the whole load meant a single broken known-error file anywhere in a fleet-wide shared config directory would silently disable known-error matching (and scope-intercept's auto-fix) for every unrelated known error too, and would block scope doctor/list/report outright — too large a blast radius for one bad file. This also removes the hand-rolled kind/apiVersion matching that only existed to special-case ScopeKnownError, and consolidates the four config-load tests onto a shared found_config_from_files test helper. --- src/shared/config_load.rs | 215 +++++++++++--------------------------- 1 file changed, 60 insertions(+), 155 deletions(-) diff --git a/src/shared/config_load.rs b/src/shared/config_load.rs index 2a14a99..9125e8e 100644 --- a/src/shared/config_load.rs +++ b/src/shared/config_load.rs @@ -1,5 +1,5 @@ -use crate::models::prelude::{ModelRoot, V1AlphaKnownError}; -use crate::models::{HelpMetadata, InternalScopeModel}; +use crate::models::HelpMetadata; +use crate::models::prelude::ModelRoot; use crate::shared::RUN_ID_ENV_VAR; use crate::shared::directories; use crate::shared::models::prelude::{DoctorGroup, KnownError, ParsedConfig, ReportUploadLocation}; @@ -70,7 +70,7 @@ impl ConfigOptions { }; let config_path = self.find_scope_paths(&working_dir); - let found_config = FoundConfig::new(self, working_dir, config_path).await?; + let found_config = FoundConfig::new(self, working_dir, config_path).await; debug!("Loaded config {:?}", found_config); @@ -132,7 +132,7 @@ impl FoundConfig { config_options: &ConfigOptions, working_dir: PathBuf, config_path: Vec, - ) -> Result { + ) -> Self { let default_path = std::env::var("PATH").unwrap_or_default(); let mut config_path = config_path.to_vec(); @@ -164,31 +164,19 @@ impl FoundConfig { run_id: config_options.get_run_id(), }; - let known_error_api_version = V1AlphaKnownError::int_api_version().to_lowercase(); - let known_error_kind = V1AlphaKnownError::int_kind().to_lowercase(); for raw_config in raw_config { - // A recognized `ScopeKnownError` that fails to build can never work as written, so - // that's a hard failure. Anything else (including a `ScopeKnownError` from an - // apiVersion this binary doesn't recognize) keeps the tolerant warn-and-continue - // behavior from #68/PR #69, since scope needs to tolerate config it doesn't yet - // understand. - let is_known_error = raw_config.kind.to_lowercase() == known_error_kind - && raw_config.api_version.to_lowercase() == known_error_api_version; let full_name = raw_config.full_name(); let file_path = raw_config.file_path(); match raw_config.try_into() { Ok(value) => this.add_model(value), - Err(e) if is_known_error => { - return Err(e.context(format!("Unable to load {full_name} from {file_path}"))); - } Err(e) => { warn!(target: "user", "Unable to load {} from {} because {}", full_name.bold(), file_path, e); } } } - Ok(this) + this } pub fn write_raw_config_to_disk(&self) -> Result { @@ -527,17 +515,34 @@ mod tests { build_config_path(nonexistent_path); } - /// Regression test for https://github.com/Gusto/scope/issues/344: a `ScopeKnownError` that - /// fails its own validation (here, a reserved capture group name) must abort config load with - /// a diagnosable error, rather than silently registering nothing. - #[tokio::test] - async fn invalid_known_error_aborts_config_load() { + /// Writes each `(file_name, contents)` pair under a fresh temp `.scope` dir and loads it + /// through the real `FoundConfig::new` entrypoint. + async fn found_config_from_files(files: &[(&str, &str)]) -> FoundConfig { let temp_dir = tempdir().unwrap(); let scope_dir = temp_dir.path().join(".scope"); fs::create_dir_all(&scope_dir).unwrap(); - fs::write( - scope_dir.join("known-error.yaml"), - r#" + for (name, contents) in files { + fs::write(scope_dir.join(name), contents).unwrap(); + } + + let config_options = ConfigOptions::parse_from(["scope"]); + FoundConfig::new( + &config_options, + temp_dir.path().to_path_buf(), + vec![scope_dir], + ) + .await + } + + /// Regression test for https://github.com/Gusto/scope/issues/344: a `ScopeKnownError` that + /// fails its own validation (here, a reserved capture group name) must be logged instead of + /// silently registering nothing, while a valid known error in the same directory still loads. + #[tokio::test] + async fn invalid_known_error_warns_and_continues() { + let found_config = found_config_from_files(&[ + ( + "bad-known-error.yaml", + r#" apiVersion: scope.github.com/v1alpha kind: ScopeKnownError metadata: @@ -546,38 +551,36 @@ spec: pattern: "boom: (?.*)" help: "should never load" "#, - ) - .unwrap(); - - let config_options = ConfigOptions::parse_from(["scope"]); - let result = FoundConfig::new( - &config_options, - temp_dir.path().to_path_buf(), - vec![scope_dir], - ) + ), + ( + "good-known-error.yaml", + r#" +apiVersion: scope.github.com/v1alpha +kind: ScopeKnownError +metadata: + name: good +spec: + pattern: "boom" + help: "fine" +"#, + ), + ]) .await; - let err = result.expect_err("invalid ScopeKnownError should abort config load"); - let message = format!("{err:#}"); - assert!(message.contains("reserved"), "unexpected error: {message}"); - assert!( - message.contains("ScopeKnownError/bad-reserved"), - "unexpected error: {message}" - ); + assert!(!found_config.known_error.contains_key("bad-reserved")); + assert!(found_config.known_error.contains_key("good")); } - /// Companion to `invalid_known_error_aborts_config_load`: a `ScopeDoctorGroup` that fails to + /// Companion to `invalid_known_error_warns_and_continues`: a `ScopeDoctorGroup` that fails to /// build (here, a broken fix template) should warn and be dropped, not abort the whole load — - /// that's the pre-existing, intentional behavior from #68/PR #69. A valid group in the same - /// directory must still load, proving this is "drop the bad one", not "load nothing". + /// the pre-existing, intentional behavior from #68/PR #69. A valid group in the same directory + /// must still load, proving this is "drop the bad one", not "load nothing". #[tokio::test] async fn invalid_doctor_group_warns_and_continues() { - let temp_dir = tempdir().unwrap(); - let scope_dir = temp_dir.path().join(".scope"); - fs::create_dir_all(&scope_dir).unwrap(); - fs::write( - scope_dir.join("broken-group.yaml"), - r#" + let found_config = found_config_from_files(&[ + ( + "broken-group.yaml", + r#" apiVersion: scope.github.com/v1alpha kind: ScopeDoctorGroup metadata: @@ -590,11 +593,10 @@ spec: commands: - "echo {{ unterminated" "#, - ) - .unwrap(); - fs::write( - scope_dir.join("good-group.yaml"), - r#" + ), + ( + "good-group.yaml", + r#" apiVersion: scope.github.com/v1alpha kind: ScopeDoctorGroup metadata: @@ -607,108 +609,11 @@ spec: commands: - echo fine "#, - ) - .unwrap(); - - let config_options = ConfigOptions::parse_from(["scope"]); - let found_config = FoundConfig::new( - &config_options, - temp_dir.path().to_path_buf(), - vec![scope_dir], - ) - .await - .expect("a broken ScopeDoctorGroup should warn and continue, not abort"); + ), + ]) + .await; assert!(!found_config.doctor_group.contains_key("broken")); assert!(found_config.doctor_group.contains_key("good")); } - - /// A `ScopeKnownError` from an apiVersion this binary doesn't recognize must NOT be treated as - /// a "recognized but invalid" known error — it's the same "config we don't yet understand" - /// case as an unrecognized kind, and should warn-and-continue rather than aborting the load. - /// (A future scope version adding a new known-error apiVersion must not break older binaries - /// running against a fleet-wide shared config directory.) - #[tokio::test] - async fn unrecognized_api_version_known_error_warns_and_continues() { - let temp_dir = tempdir().unwrap(); - let scope_dir = temp_dir.path().join(".scope"); - fs::create_dir_all(&scope_dir).unwrap(); - fs::write( - scope_dir.join("known-error.yaml"), - r#" -apiVersion: scope.github.com/v1beta -kind: ScopeKnownError -metadata: - name: future-version -spec: - pattern: "boom" - help: "from a version this binary doesn't understand" -"#, - ) - .unwrap(); - - let config_options = ConfigOptions::parse_from(["scope"]); - let found_config = FoundConfig::new( - &config_options, - temp_dir.path().to_path_buf(), - vec![scope_dir], - ) - .await - .expect("an unrecognized apiVersion should warn and continue, not abort"); - - assert!(found_config.known_error.is_empty()); - } - - /// A genuinely invalid `ScopeKnownError` aborts the whole config load, even when a perfectly - /// valid `ScopeDoctorGroup` sits in the same directory — this is the documented tradeoff of - /// treating known-error validation failures as fatal (see #344). - #[tokio::test] - async fn invalid_known_error_aborts_load_of_otherwise_valid_config() { - let temp_dir = tempdir().unwrap(); - let scope_dir = temp_dir.path().join(".scope"); - fs::create_dir_all(&scope_dir).unwrap(); - fs::write( - scope_dir.join("good-group.yaml"), - r#" -apiVersion: scope.github.com/v1alpha -kind: ScopeDoctorGroup -metadata: - name: good -spec: - actions: - - name: action1 - check: {} - fix: - commands: - - echo fine -"#, - ) - .unwrap(); - fs::write( - scope_dir.join("bad-known-error.yaml"), - r#" -apiVersion: scope.github.com/v1alpha -kind: ScopeKnownError -metadata: - name: bad-reserved -spec: - pattern: "boom: (?.*)" - help: "should never load" -"#, - ) - .unwrap(); - - let config_options = ConfigOptions::parse_from(["scope"]); - let result = FoundConfig::new( - &config_options, - temp_dir.path().to_path_buf(), - vec![scope_dir], - ) - .await; - - assert!( - result.is_err(), - "an invalid known error must abort the whole load, even alongside valid config" - ); - } }