From 6a61829c3172b6db385db62b5a7f2fa578430f80 Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Mon, 13 Jul 2026 09:59:35 -0400 Subject: [PATCH 1/4] Add --skip/--skip-only flags and make group skip trim its dependency subtree `scope doctor run` could only select groups via `--only`; there was no way to deselect a group and prune its dependency subtree. Separately, a group's YAML `skip` config was resolved at execution time and only no-op'd that single node, so its dependencies ran anyway even though the group didn't. - Add `--skip`/`--skip-only` flags: `--skip` removes a group and any dependency not also required by another included group; `--skip-only` removes just the named group and keeps its dependencies. - Resolve a group's own `skip` config at planning time (in `resolve_skips`) so it trims its dependency subtree the same way `--skip` does, while still reporting the group as skipped in the run summary. - Fix the debug graph print to show group names instead of bare node indices (`Config::NodeIndexLabel` -> `Config::EdgeNoLabel`). - Guard `--skip-only` against force-adding dependencies of a group that was never going to run in the first place, and preserve `GroupReport` entries for planning-time-skipped groups. Co-Authored-By: Claude Opus 5 --- docs/docs/commands/doctor.md | 13 + docs/docs/models/ScopeDoctorGroup.mdx | 11 + src/doctor/commands/list.rs | 7 +- src/doctor/commands/run.rs | 180 +++++++- src/doctor/runner.rs | 406 +++++++++++++++--- tests/scope_doctor.rs | 77 ++++ .../.scope/needs-dep.yaml | 15 + .../.scope/skip-parent.yaml | 10 + tests/test-cases/skip-with-deps/.scope/a.yaml | 17 + tests/test-cases/skip-with-deps/.scope/b.yaml | 17 + tests/test-cases/skip-with-deps/.scope/c.yaml | 15 + tests/test-cases/skip-with-deps/.scope/d.yaml | 16 + .../skip-with-deps/.scope/shared.yaml | 15 + 13 files changed, 730 insertions(+), 69 deletions(-) create mode 100644 tests/test-cases/group-skip-boolean-trims-dependency/.scope/needs-dep.yaml create mode 100644 tests/test-cases/group-skip-boolean-trims-dependency/.scope/skip-parent.yaml create mode 100644 tests/test-cases/skip-with-deps/.scope/a.yaml create mode 100644 tests/test-cases/skip-with-deps/.scope/b.yaml create mode 100644 tests/test-cases/skip-with-deps/.scope/c.yaml create mode 100644 tests/test-cases/skip-with-deps/.scope/d.yaml create mode 100644 tests/test-cases/skip-with-deps/.scope/shared.yaml diff --git a/docs/docs/commands/doctor.md b/docs/docs/commands/doctor.md index 9fdd42e..2266819 100644 --- a/docs/docs/commands/doctor.md +++ b/docs/docs/commands/doctor.md @@ -23,6 +23,17 @@ Commands: `scope doctor run` is used to execute all the doctor steps. All checks will be run, if you want to only run specific checks, the `--only` flag with the name of the check to run. This option can be provided multiple times. +If you want to remove a group from the run instead, use `--skip`. Skipping a group also removes any +of its dependencies that aren't also required by another group still in the run — a dependency +shared with a non-skipped group keeps running. This option can be provided multiple times. + +If you only want to remove the named group itself, and let its dependencies keep running, use +`--skip-only` instead. + +A group can also skip itself via its [`skip` config](../models/ScopeDoctorGroup.mdx#skip) +(`skip: true` or a `skip` command); it's resolved the same way `--skip` is, trimming that group's +dependency subtree. + By default, any provided fix's will be run. If you don't want to run fixes add `--fix=false` to disable fixing issues. When using a [ScopeDoctorGroup](../models/ScopeDoctorGroup.mdx), the checksum of files are stored on disk. If you need to disable caching, add `--no-cache`. @@ -34,6 +45,8 @@ Usage: scope doctor run [OPTIONS] Options: -o, --only When set, only the checks listed will run + --skip When set, these groups are removed from the run, along with any dependency that isn't also required by a group that's still included + --skip-only When set, only the named groups are removed from the run; their dependencies still run -f, --fix When set, if a fix is specified it will also run [default: true] [possible values: true, false] -n, --no-cache When set cache will be disabled, forcing all file based checks to run --yolo Automatically approve all fix prompts without asking diff --git a/docs/docs/models/ScopeDoctorGroup.mdx b/docs/docs/models/ScopeDoctorGroup.mdx index 39b2f17..e4ee3ac 100644 --- a/docs/docs/models/ScopeDoctorGroup.mdx +++ b/docs/docs/models/ScopeDoctorGroup.mdx @@ -73,6 +73,17 @@ In the event there are no defined check, the fix will _always_ run. When the checks determine that something isn't correct, a fix is the way to automate the resolution. When provided, `scope` will run them in order. +## Skip + +`spec.skip` lets a group opt itself out of a run, either unconditionally (`skip: true`) or based +on a command's exit code (`skip: { command: ... }`, skipped when the command exits `0`). + +The skip decision is resolved during planning, before the run graph is built, so it behaves the +same as passing `--skip ` on the command line: the group is removed along with any +dependency that isn't also required by another group still in the run. A shared dependency (one +also required by a non-skipped group) is kept. The skipped group is still reported in the run +summary; its dependencies that get trimmed along with it are not. + ## Commands A command can either be relative, or use the PATH. diff --git a/src/doctor/commands/list.rs b/src/doctor/commands/list.rs index 3ae440f..91ed187 100644 --- a/src/doctor/commands/list.rs +++ b/src/doctor/commands/list.rs @@ -28,7 +28,12 @@ pub fn generate_doctor_list(found_config: &FoundConfig) -> Vec { .filter(|(_, v)| v.run_by_default) .map(|(k, _)| k.to_string()), ); - let group_order = compute_group_order(&found_config.doctor_group, all_keys); + let group_order = compute_group_order( + &found_config.doctor_group, + all_keys, + &BTreeSet::new(), + &BTreeSet::new(), + ); group_order .iter() diff --git a/src/doctor/commands/run.rs b/src/doctor/commands/run.rs index dc2d99b..8f86a41 100644 --- a/src/doctor/commands/run.rs +++ b/src/doctor/commands/run.rs @@ -21,6 +21,15 @@ pub struct DoctorRunArgs { /// When set, only the checks listed will run #[arg(short, long)] pub only: Option>, + /// When set, these groups are removed from the run, along with any dependency that isn't + /// also required by a group that's still included. This option can be provided multiple + /// times. + #[arg(long)] + pub skip: Option>, + /// When set, only the named groups are removed from the run; their dependencies still run. + /// This option can be provided multiple times. + #[arg(long)] + pub skip_only: Option>, /// When set, if a fix is specified it will also run. #[arg(long, short, default_value = "true")] fix: Option, @@ -92,18 +101,80 @@ fn migrate_old_cache(old_path: &PathBuf, new_path: &PathBuf) -> Result<()> { Ok(()) } +/// Resolves the CLI `--skip`/`--skip-only` names plus each candidate group's own `skip` config +/// (evaluated here, at planning time, so a `skip: true`/`skip: { command }` group can trim its +/// dependency subtree the same way `--skip` does) into the final skip sets used to build the run +/// order, along with the subset of those groups that would otherwise have run (for reporting). +async fn resolve_skips( + found_config: &FoundConfig, + transform: &RunTransform, + args: &DoctorRunArgs, +) -> Result<(BTreeSet, BTreeSet, BTreeSet)> { + let cli_skip: BTreeSet = args.skip.clone().unwrap_or_default().into_iter().collect(); + let skip_only: BTreeSet = args + .skip_only + .clone() + .unwrap_or_default() + .into_iter() + .collect(); + warn_on_unknown_group(found_config, &cli_skip, "--skip"); + warn_on_unknown_group(found_config, &skip_only, "--skip-only"); + + let candidate_order = compute_group_order( + &found_config.doctor_group, + transform.desired_groups.clone(), + &BTreeSet::new(), + &BTreeSet::new(), + ); + + let mut skip_subtree = cli_skip; + for name in &candidate_order { + if skip_subtree.contains(name) || skip_only.contains(name) { + continue; + } + if let Some(container) = transform.groups.get(name) + && container.should_skip_group().await? + { + skip_subtree.insert(name.clone()); + } + } + + let skipped_groups: BTreeSet = candidate_order + .into_iter() + .filter(|name| skip_subtree.contains(name) || skip_only.contains(name)) + .collect(); + + Ok((skip_subtree, skip_only, skipped_groups)) +} + +fn warn_on_unknown_group(found_config: &FoundConfig, names: &BTreeSet, flag: &str) { + for name in names { + if !found_config.doctor_group.contains_key(name) { + warn!(target: "user", "{flag} {name} does not match any known group, ignoring"); + } + } +} + #[instrument("scope doctor run", skip(found_config))] pub async fn doctor_run(found_config: &FoundConfig, args: &DoctorRunArgs) -> Result { let transform = transform_inputs(found_config, args); - - let all_paths = compute_group_order(&found_config.doctor_group, transform.desired_groups); - if all_paths.is_empty() { + let (skip_subtree, skip_only, skipped_groups) = + resolve_skips(found_config, &transform, args).await?; + + let all_paths = compute_group_order( + &found_config.doctor_group, + transform.desired_groups, + &skip_subtree, + &skip_only, + ); + if all_paths.is_empty() && skipped_groups.is_empty() { warn!(target: "user", "Could not find any tasks to execute"); } let run_groups = RunGroups { group_actions: transform.groups, all_paths, + skipped_groups, yolo: args.yolo, }; @@ -241,7 +312,7 @@ mod test { use crate::doctor::commands::DoctorRunArgs; use crate::doctor::commands::run::transform_inputs; use crate::doctor::tests::{group_noop, make_root_model_additional, meta_noop}; - use crate::prelude::FoundConfig; + use crate::prelude::{FoundConfig, SkipSpec}; #[test] fn test_will_include_by_default() { @@ -263,6 +334,107 @@ mod test { ); } + #[tokio::test] + async fn test_resolve_skips_cli_skip_trims_group_and_its_exclusive_dependency() { + let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); + fc.doctor_group.insert( + "a".to_string(), + make_root_model_additional( + vec![], + |meta| meta.name("a"), + |g| g.requires(vec!["b".to_string()]), + ), + ); + fc.doctor_group.insert( + "b".to_string(), + make_root_model_additional(vec![], |meta| meta.name("b"), |g| g.run_by_default(false)), + ); + let args = DoctorRunArgs { + skip: Some(vec!["a".to_string()]), + no_cache: true, + ..Default::default() + }; + + let transform = transform_inputs(&fc, &args); + let (skip_subtree, skip_only, skipped_groups) = + resolve_skips(&fc, &transform, &args).await.unwrap(); + + assert_eq!(BTreeSet::from(["a".to_string()]), skip_subtree); + assert!(skip_only.is_empty()); + assert_eq!(BTreeSet::from(["a".to_string()]), skipped_groups); + } + + #[tokio::test] + async fn test_resolve_skips_evaluates_group_skip_config_at_planning_time() { + let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); + fc.doctor_group.insert( + "configured-skip".to_string(), + make_root_model_additional( + vec![], + |meta| meta.name("configured-skip"), + |g| g.skip(SkipSpec::Skip(true)), + ), + ); + let args = DoctorRunArgs { + no_cache: true, + ..Default::default() + }; + + let transform = transform_inputs(&fc, &args); + let (skip_subtree, _, skipped_groups) = + resolve_skips(&fc, &transform, &args).await.unwrap(); + + assert_eq!( + BTreeSet::from(["configured-skip".to_string()]), + skip_subtree + ); + assert_eq!( + BTreeSet::from(["configured-skip".to_string()]), + skipped_groups + ); + } + + #[tokio::test] + async fn test_resolve_skips_does_not_report_groups_that_would_not_have_run() { + let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); + fc.doctor_group.insert( + "unreached".to_string(), + make_root_model_additional( + vec![], + |meta| meta.name("unreached"), + |g| g.run_by_default(false), + ), + ); + let args = DoctorRunArgs { + // `unreached` isn't desired (run_by_default = false and nothing requires it), so + // skipping it shouldn't be reported even though the name is valid. + skip: Some(vec!["unreached".to_string()]), + no_cache: true, + ..Default::default() + }; + + let transform = transform_inputs(&fc, &args); + let (_, _, skipped_groups) = resolve_skips(&fc, &transform, &args).await.unwrap(); + + assert!(skipped_groups.is_empty()); + } + + #[tokio::test] + async fn test_resolve_skips_ignores_unknown_group_names() { + let fc = FoundConfig::empty(PathBuf::from("/tmp")); + let args = DoctorRunArgs { + skip: Some(vec!["does-not-exist".to_string()]), + no_cache: true, + ..Default::default() + }; + + let transform = transform_inputs(&fc, &args); + // Should not panic on a name that matches no group; it's simply a no-op. + let (_, _, skipped_groups) = resolve_skips(&fc, &transform, &args).await.unwrap(); + + assert!(skipped_groups.is_empty()); + } + #[test] fn test_include_will_skip() { let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); diff --git a/src/doctor/runner.rs b/src/doctor/runner.rs index 362acb2..df93274 100644 --- a/src/doctor/runner.rs +++ b/src/doctor/runner.rs @@ -72,10 +72,6 @@ impl PathRunResult { self.skipped_group.insert(group_name); self.did_succeed = false; // User-denied fixes should cause failure } - GroupExecutionStatus::GroupSkipped => { - self.skipped_group.insert(group_name); - // Note: Group skips via configuration do not cause the overall command to fail - } }; self.group_reports.push(group.group_report.clone()); @@ -87,7 +83,6 @@ enum GroupExecutionStatus { Succeeded, Failed, Skipped, - GroupSkipped, } #[derive(Debug)] @@ -179,6 +174,10 @@ where { pub(crate) group_actions: BTreeMap>, pub(crate) all_paths: Vec, + /// Groups trimmed from `all_paths` during planning (via `--skip`, `--skip-only`, or a + /// group's own `skip` config) that would otherwise have run. Reported as skipped without + /// ever being executed. + pub(crate) skipped_groups: BTreeSet, pub(crate) yolo: bool, } @@ -194,7 +193,20 @@ where } } - self.run_path(full_path).await + let mut run_result = self.run_path(full_path).await?; + + for group_name in &self.skipped_groups { + debug_assert!( + !run_result.succeeded_groups.contains(group_name) + && !run_result.failed_group.contains(group_name), + "group {group_name} was both executed and planned-skipped" + ); + warn!(target: "always", "Group skipped, group: \"{}\"", group_name); + run_result.skipped_group.insert(group_name.to_string()); + run_result.group_reports.push(GroupReport::new(group_name)); + } + + Ok(run_result) } async fn run_path(&self, groups: Vec<&GroupActionContainer>) -> Result { @@ -264,13 +276,6 @@ where group_report: GroupReport::new(container.group_name()), }; - // Check if the group should be skipped - if container.should_skip_group().await? { - warn!(target: "always", "Group skipped, group: \"{}\"", container.group_name()); - results.status = GroupExecutionStatus::GroupSkipped; - return Ok(results); - } - for action in &container.actions { group_span.pb_inc(1); if results.skip_remaining { @@ -474,31 +479,109 @@ fn action_task_reports_for_display(action_report: &ActionReport) -> Vec, - desired_groups: BTreeSet, -) -> Vec { +/// Builds the dependency graph over every group not in `exclude`, with an edge from each +/// dependency to its dependent (per `requires`). +fn build_dependency_graph<'a>( + groups: &'a BTreeMap, + exclude: &BTreeSet, +) -> (DiGraph<&'a str, i32>, BTreeMap) { let mut graph = DiGraph::<&str, i32>::new(); let mut node_graph: BTreeMap = BTreeMap::new(); for name in groups.keys() { + if exclude.contains(name) { + continue; + } node_graph.insert(name.to_string(), graph.add_node(name)); } for (name, model) in groups { - let this = node_graph.get(name).unwrap(); + if exclude.contains(name) { + continue; + } + let this = node_graph[name]; for dep in &model.requires { + if exclude.contains(dep) { + continue; + } if let Some(other) = node_graph.get(dep) { - graph.add_edge(*other, *this, 1); + graph.add_edge(*other, this, 1); } else { warn!(target: "user", "{} needs {} but no such dependency found, ignoring dependency", name, dep); } } } + (graph, node_graph) +} + +/// Names of every group reachable from `desired_groups`, over the dependency graph with +/// `exclude` removed. Used to test "would this group have run at all" independent of any +/// `skip_only` decision. +fn reachable_group_names( + groups: &BTreeMap, + desired_groups: &BTreeSet, + exclude: &BTreeSet, +) -> BTreeSet { + let (mut graph, node_graph) = build_dependency_graph(groups, exclude); + + let start = graph.add_node("start"); + for name in desired_groups { + if let Some(this) = node_graph.get(name) { + graph.add_edge(*this, start, 1); + } + } + + graph.reverse(); + + DfsPostOrder::new(&graph, start) + .iter(&graph) + .filter(|&node| node != start) + .map(|node| graph.node_weight(node).unwrap().to_string()) + .collect() +} + +/// Computes the topologically-sorted set of groups to run, starting from `desired_groups` and +/// pulling in transitive dependencies (via `requires`). +/// +/// `skip_subtree` groups are removed along with any dependency not also required by a +/// non-skipped group (a shared dependency survives). `skip_only` groups are removed but their +/// dependencies are always kept — unless that group was never going to run in the first place, +/// in which case `--skip-only` on it is a no-op rather than pulling in unrelated work. +pub fn compute_group_order( + groups: &BTreeMap, + desired_groups: BTreeSet, + skip_subtree: &BTreeSet, + skip_only: &BTreeSet, +) -> Vec { + let all_skipped: BTreeSet = skip_subtree.union(skip_only).cloned().collect(); + + // Only force-keep a `skip_only` group's dependencies if that group would actually have run + // (reachable from `desired_groups` once force-removed `skip_subtree` groups are excluded). + let would_have_run = if skip_only.is_empty() { + BTreeSet::new() + } else { + reachable_group_names(groups, &desired_groups, skip_subtree) + }; + + let (mut graph, node_graph) = build_dependency_graph(groups, &all_skipped); + + let roots: BTreeSet = desired_groups + .into_iter() + .filter(|name| !all_skipped.contains(name)) + .chain( + skip_only + .iter() + .filter(|name| would_have_run.contains(*name)) + .filter_map(|name| groups.get(name)) + .flat_map(|model| model.requires.iter().cloned()) + .filter(|dep| !all_skipped.contains(dep)), + ) + .collect(); + let start = graph.add_node("start"); - for name in &desired_groups { + for name in &roots { if let Some(this) = node_graph.get(name) { graph.add_edge(*this, start, 1); } @@ -507,19 +590,30 @@ pub fn compute_group_order( debug!( format = "graphviz", "{:?}", - Dot::with_config(&graph, &[Config::NodeIndexLabel]) + Dot::with_config(&graph, &[Config::EdgeNoLabel]) ); graph.reverse(); - let mut order = Vec::new(); - for node in DfsPostOrder::new(&graph, start).iter(&graph) { - if node == start { - continue; + let order: Vec = DfsPostOrder::new(&graph, start) + .iter(&graph) + .filter(|&node| node != start) + .map(|node| graph.node_weight(node).unwrap().to_string()) + .collect(); + + debug!( + target: "user", + "Resolved doctor run order: [{}]{}", + order.join(", "), + if all_skipped.is_empty() { + String::new() + } else { + format!( + "; skipped/trimmed: [{}]", + all_skipped.iter().cloned().collect::>().join(", ") + ) } - let name = graph.node_weight(node).unwrap().to_string(); - order.push(name) - } + ); order } @@ -540,6 +634,10 @@ mod tests { use std::sync::Arc; use std::vec; + fn no_skip() -> BTreeSet { + BTreeSet::new() + } + #[tokio::test] async fn test_compute_group_order_with_no_dep_will_have_no_tasks() -> Result<()> { let action = build_run_fail_fix_succeed_action(); @@ -560,7 +658,10 @@ mod tests { ); groups.insert("step_2".to_string(), step_2); - assert_eq!(0, compute_group_order(&groups, BTreeSet::new()).len()); + assert_eq!( + 0, + compute_group_order(&groups, BTreeSet::new(), &no_skip(), &no_skip()).len() + ); Ok(()) } @@ -587,7 +688,12 @@ mod tests { assert_eq!( vec!["step_1", "step_2"], - compute_group_order(&groups, BTreeSet::from(["step_2".to_string()])) + compute_group_order( + &groups, + BTreeSet::from(["step_2".to_string()]), + &no_skip(), + &no_skip() + ) ); Ok(()) @@ -622,7 +728,12 @@ mod tests { assert_eq!( vec!["step_3", "step_2", "step_1"], - compute_group_order(&groups, BTreeSet::from(["step_1".to_string()])) + compute_group_order( + &groups, + BTreeSet::from(["step_1".to_string()]), + &no_skip(), + &no_skip() + ) ); Ok(()) @@ -657,7 +768,12 @@ mod tests { assert_eq!( vec!["step_1", "step_2", "step_3"], - compute_group_order(&groups, BTreeSet::from(["step_3".to_string()])) + compute_group_order( + &groups, + BTreeSet::from(["step_3".to_string()]), + &no_skip(), + &no_skip() + ) ); Ok(()) @@ -692,9 +808,138 @@ mod tests { assert_eq!( vec!["step_1", "step_3"], - compute_group_order(&groups, BTreeSet::from(["step_3".to_string()])) + compute_group_order( + &groups, + BTreeSet::from(["step_3".to_string()]), + &no_skip(), + &no_skip() + ) + ); + + Ok(()) + } + + fn make_dependency_chain() -> BTreeMap { + let action = build_run_fail_fix_succeed_action(); + + let mut groups = BTreeMap::new(); + groups.insert( + "a".to_string(), + make_root_model_additional( + vec![action.clone()], + |meta| meta.name("a"), + |group| group.requires(vec!["b".to_string()]), + ), + ); + groups.insert( + "b".to_string(), + make_root_model_additional( + vec![action.clone()], + |meta| meta.name("b"), + |group| group.requires(vec!["c".to_string()]), + ), + ); + groups.insert( + "c".to_string(), + make_root_model_additional(vec![action.clone()], |meta| meta.name("c"), group_noop), + ); + groups.insert( + "d".to_string(), + make_root_model_additional( + vec![action.clone()], + |meta| meta.name("d"), + |group| group.requires(vec!["shared".to_string()]), + ), + ); + groups.insert( + "shared".to_string(), + make_root_model_additional( + vec![action.clone()], + |meta| meta.name("shared"), + group_noop, + ), + ); + groups.insert( + "a-and-shared".to_string(), + make_root_model_additional( + vec![action.clone()], + |meta| meta.name("a-and-shared"), + |group| group.requires(vec!["a".to_string(), "shared".to_string()]), + ), + ); + + groups + } + + #[tokio::test] + async fn test_compute_group_order_skip_trims_exclusive_dependencies() -> Result<()> { + let groups = make_dependency_chain(); + + let order = compute_group_order( + &groups, + BTreeSet::from(["a".to_string(), "d".to_string()]), + &BTreeSet::from(["a".to_string()]), + &no_skip(), + ); + + // `a`, and its exclusive deps `b`/`c`, are trimmed; `d` and its dep are untouched. + assert_eq!(vec!["shared", "d"], order); + + Ok(()) + } + + #[tokio::test] + async fn test_compute_group_order_skip_force_removes_group_but_keeps_shared_dependency() + -> Result<()> { + let groups = make_dependency_chain(); + + // `a-and-shared` requires both `a` and `shared`. Skipping `a` force-removes it (and its + // exclusive deps `b`/`c`) even though `a-and-shared` — a non-skipped root — depends on + // it. `shared` survives because `a-and-shared` also depends on it directly. + let order = compute_group_order( + &groups, + BTreeSet::from(["a-and-shared".to_string()]), + &BTreeSet::from(["a".to_string()]), + &no_skip(), + ); + + assert_eq!(vec!["shared", "a-and-shared"], order); + + Ok(()) + } + + #[tokio::test] + async fn test_compute_group_order_skip_only_keeps_dependencies() -> Result<()> { + let groups = make_dependency_chain(); + + // `--skip-only=a` removes just `a`; its exclusive deps `b`/`c` still run. + let order = compute_group_order( + &groups, + BTreeSet::from(["a".to_string()]), + &no_skip(), + &BTreeSet::from(["a".to_string()]), + ); + + assert_eq!(vec!["c", "b"], order); + + Ok(()) + } + + #[tokio::test] + async fn test_compute_group_order_skip_only_on_unreached_group_is_a_noop() -> Result<()> { + let groups = make_dependency_chain(); + + // Only `d` is desired; `a` (and its exclusive deps `b`/`c`) were never going to run in + // the first place. `--skip-only=a` must not pull `b`/`c` in as unrelated new work. + let order = compute_group_order( + &groups, + BTreeSet::from(["d".to_string()]), + &no_skip(), + &BTreeSet::from(["a".to_string()]), ); + assert_eq!(vec!["shared", "d"], order); + Ok(()) } @@ -770,6 +1015,7 @@ mod tests { "group_2".to_string(), "group_3".to_string(), ], + skipped_groups: BTreeSet::new(), yolo: false, }; @@ -802,6 +1048,7 @@ mod tests { "skipped_1".to_string(), "skipped_2".to_string(), ], + skipped_groups: BTreeSet::new(), yolo: false, }; @@ -840,6 +1087,7 @@ mod tests { "user_denies".to_string(), "skipped".to_string(), ], + skipped_groups: BTreeSet::new(), yolo: false, }; @@ -888,6 +1136,7 @@ mod tests { "user_denies".to_string(), "succeeds_2".to_string(), ], + skipped_groups: BTreeSet::new(), yolo: false, }; @@ -932,6 +1181,7 @@ mod tests { "fails".to_string(), "succeeds_2".to_string(), ], + skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1010,53 +1260,52 @@ mod tests { } #[tokio::test] - async fn test_execute_group_skips_when_should_skip_group_returns_true() -> Result<()> { - let mut mock_action = MockDoctorActionRun::new(); - mock_action.expect_run_action().never(); // Should not be called - mock_action.expect_help_text().return_const(None); - mock_action.expect_help_url().return_const(None); - mock_action - .expect_name() - .returning(|| "test action".to_string()); - mock_action.expect_required().return_const(false); - mock_action - .expect_description() - .returning(|| "test description".to_string()); - - // Create a test group with skip = true + async fn test_should_skip_group_true_for_boolean_skip() -> Result<()> { let test_group = make_root_model_additional( vec![], |meta| meta.name("test-group"), |group| group.skip(SkipSpec::Skip(true)), ); - let container = GroupActionContainer { + let container: GroupActionContainer = GroupActionContainer { group: test_group, - actions: vec![mock_action], + actions: vec![], exec_provider: Arc::new(MockExecutionProvider::new()), exec_working_dir: Default::default(), sys_path: "".to_string(), }; - let run_groups = RunGroups { - group_actions: BTreeMap::new(), - all_paths: Vec::new(), - yolo: false, - }; + assert!(container.should_skip_group().await?); - let group_span = info_span!("test_group", "indicatif.pb_show" = true); - let result = run_groups.execute_group(&group_span, &container).await?; + Ok(()) + } - // Verify the group was skipped - assert_eq!(result.group_name, "test-group"); - assert!(matches!(result.status, GroupExecutionStatus::GroupSkipped)); - assert!(!result.skip_remaining); + #[tokio::test] + async fn test_should_skip_group_false_for_boolean_no_skip() -> Result<()> { + let test_group = make_root_model_additional( + vec![], + |meta| meta.name("test-group"), + |group| group.skip(SkipSpec::Skip(false)), + ); + + let container: GroupActionContainer = GroupActionContainer { + group: test_group, + actions: vec![], + exec_provider: Arc::new(MockExecutionProvider::new()), + exec_working_dir: Default::default(), + sys_path: "".to_string(), + }; + + assert!(!container.should_skip_group().await?); Ok(()) } #[tokio::test] - async fn test_execute_group_runs_actions_when_should_skip_group_returns_false() -> Result<()> { + async fn test_execute_runs_group_actions_regardless_of_group_skip_field() -> Result<()> { + // `execute_group` no longer consults `group.skip` — skip is resolved during planning + // (see doctor_run) and reflected in `RunGroups.skipped_groups` instead. A group reaching + // `execute_group` always runs its actions. let mut mock_action = MockDoctorActionRun::new(); mock_action.expect_run_action().returning(|_| { Ok(ActionRunResult::new( @@ -1077,11 +1326,10 @@ mod tests { .expect_description() .returning(|| "test description".to_string()); - // Create a test group with skip = false let test_group = make_root_model_additional( vec![], |meta| meta.name("test-group"), - |group| group.skip(SkipSpec::Skip(false)), + |group| group.skip(SkipSpec::Skip(true)), ); let container = GroupActionContainer { @@ -1095,17 +1343,47 @@ mod tests { let run_groups = RunGroups { group_actions: BTreeMap::new(), all_paths: Vec::new(), + skipped_groups: BTreeSet::new(), yolo: false, }; let group_span = info_span!("test_group", "indicatif.pb_show" = true); let result = run_groups.execute_group(&group_span, &container).await?; - // Verify the group was executed normally assert_eq!(result.group_name, "test-group"); assert!(matches!(result.status, GroupExecutionStatus::Succeeded)); assert!(!result.skip_remaining); Ok(()) } + + #[tokio::test] + async fn test_execute_reports_planned_skips_without_running_them() -> Result<()> { + let group_actions = BTreeMap::from([make_group_action( + "runs", + make_action_runs(ActionRunStatus::CheckSucceeded), + )]); + + let run_groups = RunGroups { + group_actions, + all_paths: vec!["runs".to_string()], + skipped_groups: BTreeSet::from(["trimmed".to_string()]), + yolo: false, + }; + + let exit_code = run_groups.execute().await?; + + // Groups trimmed during planning are reported as skipped without failing the run. + assert!(exit_code.did_succeed); + assert_eq!( + BTreeSet::from(["runs".to_string()]), + exit_code.succeeded_groups + ); + assert_eq!( + BTreeSet::from(["trimmed".to_string()]), + exit_code.skipped_group + ); + + Ok(()) + } } diff --git a/tests/scope_doctor.rs b/tests/scope_doctor.rs index 90569fb..abf9ae4 100644 --- a/tests/scope_doctor.rs +++ b/tests/scope_doctor.rs @@ -271,6 +271,83 @@ fn test_group_skip_subsequent_groups_run() { test_helper.clean_work_dir(); } +#[test] +fn test_group_skip_boolean_trims_dependency() { + let test_helper = ScopeTestHelper::new( + "test_group_skip_boolean_trims_dependency", + "group-skip-boolean-trims-dependency", + ); + let result = test_helper.doctor_run(None); + + result + .success() + .stdout(predicate::str::contains( + "Group skipped, group: \"skip-parent\"", + )) + .stdout(predicate::str::contains("should not run").not()) + .stdout(predicate::str::contains("group: \"needs-dep\"").not()) + .stdout(predicate::str::contains( + "Summary: 0 groups succeeded, 1 groups skipped", + )); + + test_helper.clean_work_dir(); +} + +#[test] +fn test_skip_flag_trims_exclusive_dependencies_but_keeps_shared_dependency() { + let test_helper = ScopeTestHelper::new( + "test_skip_flag_trims_exclusive_dependencies_but_keeps_shared_dependency", + "skip-with-deps", + ); + let result = test_helper.doctor_run(Some(&["--skip=a"])); + + result + .success() + .stdout(predicate::str::contains("Group skipped, group: \"a\"")) + .stdout(predicate::str::contains("group: \"b\"").not()) + .stdout(predicate::str::contains("group: \"c\"").not()) + .stdout(predicate::str::contains( + "Check was successful, group: \"d\", name: \"check-d\"", + )) + .stdout(predicate::str::contains( + "Check was successful, group: \"shared\", name: \"check-shared\"", + )) + .stdout(predicate::str::contains( + "Summary: 2 groups succeeded, 1 groups skipped", + )); + + test_helper.clean_work_dir(); +} + +#[test] +fn test_skip_only_flag_keeps_dependencies() { + let test_helper = + ScopeTestHelper::new("test_skip_only_flag_keeps_dependencies", "skip-with-deps"); + let result = test_helper.doctor_run(Some(&["--skip-only=a"])); + + result + .success() + .stdout(predicate::str::contains("Group skipped, group: \"a\"")) + .stdout(predicate::str::contains("group: \"a\", name: \"check-a\"").not()) + .stdout(predicate::str::contains( + "Check was successful, group: \"b\", name: \"check-b\"", + )) + .stdout(predicate::str::contains( + "Check was successful, group: \"c\", name: \"check-c\"", + )) + .stdout(predicate::str::contains( + "Check was successful, group: \"d\", name: \"check-d\"", + )) + .stdout(predicate::str::contains( + "Check was successful, group: \"shared\", name: \"check-shared\"", + )) + .stdout(predicate::str::contains( + "Summary: 4 groups succeeded, 1 groups skipped", + )); + + test_helper.clean_work_dir(); +} + #[test] fn test_yolo_flag_auto_approves_fix_prompts() { let test_helper = ScopeTestHelper::new( diff --git a/tests/test-cases/group-skip-boolean-trims-dependency/.scope/needs-dep.yaml b/tests/test-cases/group-skip-boolean-trims-dependency/.scope/needs-dep.yaml new file mode 100644 index 0000000..e36f2b9 --- /dev/null +++ b/tests/test-cases/group-skip-boolean-trims-dependency/.scope/needs-dep.yaml @@ -0,0 +1,15 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: needs-dep + description: Only required by skip-parent; should be trimmed along with it +spec: + include: when-required + actions: + - name: should-not-run + check: + commands: + - echo "This check should not run" + fix: + commands: + - echo "This fix should not run" diff --git a/tests/test-cases/group-skip-boolean-trims-dependency/.scope/skip-parent.yaml b/tests/test-cases/group-skip-boolean-trims-dependency/.scope/skip-parent.yaml new file mode 100644 index 0000000..3ef4522 --- /dev/null +++ b/tests/test-cases/group-skip-boolean-trims-dependency/.scope/skip-parent.yaml @@ -0,0 +1,10 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: skip-parent + description: Test group with skip boolean set to true and a dependency of its own +spec: + skip: true + needs: + - needs-dep + actions: [] diff --git a/tests/test-cases/skip-with-deps/.scope/a.yaml b/tests/test-cases/skip-with-deps/.scope/a.yaml new file mode 100644 index 0000000..dc65827 --- /dev/null +++ b/tests/test-cases/skip-with-deps/.scope/a.yaml @@ -0,0 +1,17 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: a + description: Root group that depends on b (exclusive) and shared (also used by d) +spec: + needs: + - b + - shared + actions: + - name: check-a + check: + commands: + - echo "checking a" + fix: + commands: + - echo "fixing a" diff --git a/tests/test-cases/skip-with-deps/.scope/b.yaml b/tests/test-cases/skip-with-deps/.scope/b.yaml new file mode 100644 index 0000000..3edf82d --- /dev/null +++ b/tests/test-cases/skip-with-deps/.scope/b.yaml @@ -0,0 +1,17 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: b + description: Only reachable through a; should be trimmed when a is skipped +spec: + include: when-required + needs: + - c + actions: + - name: check-b + check: + commands: + - echo "checking b" + fix: + commands: + - echo "fixing b" diff --git a/tests/test-cases/skip-with-deps/.scope/c.yaml b/tests/test-cases/skip-with-deps/.scope/c.yaml new file mode 100644 index 0000000..3ee2b6b --- /dev/null +++ b/tests/test-cases/skip-with-deps/.scope/c.yaml @@ -0,0 +1,15 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: c + description: Only reachable through b; should be trimmed when a is skipped +spec: + include: when-required + actions: + - name: check-c + check: + commands: + - echo "checking c" + fix: + commands: + - echo "fixing c" diff --git a/tests/test-cases/skip-with-deps/.scope/d.yaml b/tests/test-cases/skip-with-deps/.scope/d.yaml new file mode 100644 index 0000000..ead638a --- /dev/null +++ b/tests/test-cases/skip-with-deps/.scope/d.yaml @@ -0,0 +1,16 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: d + description: Independent root that also depends on shared +spec: + needs: + - shared + actions: + - name: check-d + check: + commands: + - echo "checking d" + fix: + commands: + - echo "fixing d" diff --git a/tests/test-cases/skip-with-deps/.scope/shared.yaml b/tests/test-cases/skip-with-deps/.scope/shared.yaml new file mode 100644 index 0000000..37999d6 --- /dev/null +++ b/tests/test-cases/skip-with-deps/.scope/shared.yaml @@ -0,0 +1,15 @@ +apiVersion: scope.github.com/v1alpha +kind: ScopeDoctorGroup +metadata: + name: shared + description: Required by both a and d; must survive skipping a +spec: + include: when-required + actions: + - name: check-shared + check: + commands: + - echo "checking shared" + fix: + commands: + - echo "fixing shared" From 25bf2dc5f90f19f73483b58dcd0ab7a7d925832d Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Mon, 13 Jul 2026 11:28:32 -0400 Subject: [PATCH 2/4] Refactor compute_group_order to take a struct, dedupe graph traversal, close QA gaps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit compute_group_order took 4 positional args (2 same-typed BTreeSets), and resolve_skips returned a bare 3-tuple of same-typed sets — both easy to transpose silently. build_dependency_graph returned an unnamed tuple and used continue-based exclusion loops. - compute_group_order now takes a single GroupOrderParams struct. - build_dependency_graph returns a named DependencyGraph struct and filters excluded groups declaratively instead of via continue. - resolve_skips returns a named SkipResolution struct instead of a tuple. - Extracted wire_start_node/traversal_order_from to remove duplicated graph-traversal logic between reachable_group_names and compute_group_order. - Closed two gaps found by coverage/mutation testing: execute_command had no test, and the skip_subtree/skip_only short-circuit guard in resolve_skips had a surviving mutant (no test proved a group's own skip command is skipped once already excluded via --skip/--skip-only). - Tightened the skipped-group debug_assert in RunGroups::execute to also cover skipped_group, not just succeeded/failed. Co-Authored-By: Claude Opus 5 --- src/doctor/commands/list.rs | 14 +- src/doctor/commands/run.rs | 188 ++++++++++++++++++++---- src/doctor/runner.rs | 279 ++++++++++++++++++++++-------------- 3 files changed, 337 insertions(+), 144 deletions(-) diff --git a/src/doctor/commands/list.rs b/src/doctor/commands/list.rs index 91ed187..a814738 100644 --- a/src/doctor/commands/list.rs +++ b/src/doctor/commands/list.rs @@ -4,7 +4,7 @@ use anyhow::Result; use clap::Args; use tracing::instrument; -use crate::doctor::runner::compute_group_order; +use crate::doctor::runner::{GroupOrderParams, compute_group_order}; use crate::report_stdout; use crate::shared::prelude::{DoctorGroup, FoundConfig}; use crate::shared::print_details; @@ -28,12 +28,12 @@ pub fn generate_doctor_list(found_config: &FoundConfig) -> Vec { .filter(|(_, v)| v.run_by_default) .map(|(k, _)| k.to_string()), ); - let group_order = compute_group_order( - &found_config.doctor_group, - all_keys, - &BTreeSet::new(), - &BTreeSet::new(), - ); + let group_order = compute_group_order(GroupOrderParams { + groups: &found_config.doctor_group, + desired_groups: &all_keys, + skip_subtree: &BTreeSet::new(), + skip_only: &BTreeSet::new(), + }); group_order .iter() diff --git a/src/doctor/commands/run.rs b/src/doctor/commands/run.rs index 8f86a41..dc33e97 100644 --- a/src/doctor/commands/run.rs +++ b/src/doctor/commands/run.rs @@ -8,7 +8,9 @@ use tracing::{info, instrument, warn}; use crate::doctor::check::{DefaultDoctorActionRun, DefaultGlobWalker}; use crate::doctor::file_cache::{FileBasedCache, FileCache, NoOpCache}; -use crate::doctor::runner::{GroupActionContainer, RunGroups, compute_group_order}; +use crate::doctor::runner::{ + GroupActionContainer, GroupOrderParams, RunGroups, compute_group_order, +}; use crate::prelude::{ DefaultGroupedReportBuilder, ExecutionProvider, GroupedReportBuilder, ReportRenderer, }; @@ -101,15 +103,22 @@ fn migrate_old_cache(old_path: &PathBuf, new_path: &PathBuf) -> Result<()> { Ok(()) } +/// The result of [`resolve_skips`]: the final skip sets used to build the run order, plus the +/// subset of those groups that would otherwise have run (for reporting). +struct SkipResolution { + skip_subtree: BTreeSet, + skip_only: BTreeSet, + skipped_groups: BTreeSet, +} + /// Resolves the CLI `--skip`/`--skip-only` names plus each candidate group's own `skip` config /// (evaluated here, at planning time, so a `skip: true`/`skip: { command }` group can trim its -/// dependency subtree the same way `--skip` does) into the final skip sets used to build the run -/// order, along with the subset of those groups that would otherwise have run (for reporting). +/// dependency subtree the same way `--skip` does) into a [`SkipResolution`]. async fn resolve_skips( found_config: &FoundConfig, transform: &RunTransform, args: &DoctorRunArgs, -) -> Result<(BTreeSet, BTreeSet, BTreeSet)> { +) -> Result { let cli_skip: BTreeSet = args.skip.clone().unwrap_or_default().into_iter().collect(); let skip_only: BTreeSet = args .skip_only @@ -120,12 +129,12 @@ async fn resolve_skips( warn_on_unknown_group(found_config, &cli_skip, "--skip"); warn_on_unknown_group(found_config, &skip_only, "--skip-only"); - let candidate_order = compute_group_order( - &found_config.doctor_group, - transform.desired_groups.clone(), - &BTreeSet::new(), - &BTreeSet::new(), - ); + let candidate_order = compute_group_order(GroupOrderParams { + groups: &found_config.doctor_group, + desired_groups: &transform.desired_groups, + skip_subtree: &BTreeSet::new(), + skip_only: &BTreeSet::new(), + }); let mut skip_subtree = cli_skip; for name in &candidate_order { @@ -144,7 +153,11 @@ async fn resolve_skips( .filter(|name| skip_subtree.contains(name) || skip_only.contains(name)) .collect(); - Ok((skip_subtree, skip_only, skipped_groups)) + Ok(SkipResolution { + skip_subtree, + skip_only, + skipped_groups, + }) } fn warn_on_unknown_group(found_config: &FoundConfig, names: &BTreeSet, flag: &str) { @@ -158,15 +171,18 @@ fn warn_on_unknown_group(found_config: &FoundConfig, names: &BTreeSet, f #[instrument("scope doctor run", skip(found_config))] pub async fn doctor_run(found_config: &FoundConfig, args: &DoctorRunArgs) -> Result { let transform = transform_inputs(found_config, args); - let (skip_subtree, skip_only, skipped_groups) = - resolve_skips(found_config, &transform, args).await?; - - let all_paths = compute_group_order( - &found_config.doctor_group, - transform.desired_groups, - &skip_subtree, - &skip_only, - ); + let SkipResolution { + skip_subtree, + skip_only, + skipped_groups, + } = resolve_skips(found_config, &transform, args).await?; + + let all_paths = compute_group_order(GroupOrderParams { + groups: &found_config.doctor_group, + desired_groups: &transform.desired_groups, + skip_subtree: &skip_subtree, + skip_only: &skip_only, + }); if all_paths.is_empty() && skipped_groups.is_empty() { warn!(target: "user", "Could not find any tasks to execute"); } @@ -312,7 +328,9 @@ mod test { use crate::doctor::commands::DoctorRunArgs; use crate::doctor::commands::run::transform_inputs; use crate::doctor::tests::{group_noop, make_root_model_additional, meta_noop}; - use crate::prelude::{FoundConfig, SkipSpec}; + use crate::prelude::{ + DoctorGroup, FoundConfig, MockExecutionProvider, OutputCaptureBuilder, SkipSpec, + }; #[test] fn test_will_include_by_default() { @@ -356,8 +374,11 @@ mod test { }; let transform = transform_inputs(&fc, &args); - let (skip_subtree, skip_only, skipped_groups) = - resolve_skips(&fc, &transform, &args).await.unwrap(); + let SkipResolution { + skip_subtree, + skip_only, + skipped_groups, + } = resolve_skips(&fc, &transform, &args).await.unwrap(); assert_eq!(BTreeSet::from(["a".to_string()]), skip_subtree); assert!(skip_only.is_empty()); @@ -381,8 +402,11 @@ mod test { }; let transform = transform_inputs(&fc, &args); - let (skip_subtree, _, skipped_groups) = - resolve_skips(&fc, &transform, &args).await.unwrap(); + let SkipResolution { + skip_subtree, + skipped_groups, + .. + } = resolve_skips(&fc, &transform, &args).await.unwrap(); assert_eq!( BTreeSet::from(["configured-skip".to_string()]), @@ -394,6 +418,114 @@ mod test { ); } + fn group_action_container_with_exec_provider( + group: DoctorGroup, + exec_provider: MockExecutionProvider, + ) -> GroupActionContainer { + GroupActionContainer { + group, + actions: Vec::new(), + exec_provider: Arc::new(exec_provider), + exec_working_dir: PathBuf::from("/tmp"), + sys_path: "".to_string(), + } + } + + fn transform_with_group( + name: &str, + group: DoctorGroup, + exec_provider: MockExecutionProvider, + ) -> RunTransform { + RunTransform { + groups: BTreeMap::from([( + name.to_string(), + group_action_container_with_exec_provider(group, exec_provider), + )]), + desired_groups: BTreeSet::from([name.to_string()]), + file_cache: Arc::new(NoOpCache::default()), + exec_runner: Arc::new(DefaultExecutionProvider::default()), + } + } + + #[tokio::test] + async fn test_resolve_skips_evaluates_command_skip_config_at_planning_time() { + let group = make_root_model_additional( + vec![], + |meta| meta.name("command-skip"), + |g| { + g.skip(SkipSpec::Command { + command: "should-skip".to_string(), + }) + }, + ); + + let mut mock_exec = MockExecutionProvider::new(); + mock_exec.expect_run_command().times(1).returning(|_| { + Ok(OutputCaptureBuilder::default() + .command("should-skip".to_string()) + .exit_code(Some(0)) + .build() + .unwrap()) + }); + + let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); + fc.doctor_group + .insert("command-skip".to_string(), group.clone()); + let transform = transform_with_group("command-skip", group, mock_exec); + let args = DoctorRunArgs { + no_cache: true, + ..Default::default() + }; + + let SkipResolution { + skip_subtree, + skipped_groups, + .. + } = resolve_skips(&fc, &transform, &args).await.unwrap(); + + assert_eq!(BTreeSet::from(["command-skip".to_string()]), skip_subtree); + assert_eq!(BTreeSet::from(["command-skip".to_string()]), skipped_groups); + } + + #[tokio::test] + async fn test_resolve_skips_does_not_run_own_skip_command_when_already_cli_skipped() { + // Regression test for a mutation-testing survivor: the `skip_subtree.contains(name) + // || skip_only.contains(name)` guard must short-circuit before evaluating a group's own + // `skip` command once that group is already excluded via `--skip`/`--skip-only` — + // otherwise a potentially expensive or side-effecting command runs needlessly for a + // group that was never going to execute. + let group = make_root_model_additional( + vec![], + |meta| meta.name("already-skipped"), + |g| { + g.skip(SkipSpec::Command { + command: "should-not-run".to_string(), + }) + }, + ); + + let mut mock_exec = MockExecutionProvider::new(); + mock_exec.expect_run_command().never(); + + let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); + fc.doctor_group + .insert("already-skipped".to_string(), group.clone()); + let transform = transform_with_group("already-skipped", group, mock_exec); + let args = DoctorRunArgs { + skip: Some(vec!["already-skipped".to_string()]), + no_cache: true, + ..Default::default() + }; + + let SkipResolution { skip_subtree, .. } = + resolve_skips(&fc, &transform, &args).await.unwrap(); + + assert_eq!( + BTreeSet::from(["already-skipped".to_string()]), + skip_subtree + ); + } + #[tokio::test] async fn test_resolve_skips_does_not_report_groups_that_would_not_have_run() { let mut fc = FoundConfig::empty(PathBuf::from("/tmp")); @@ -414,7 +546,8 @@ mod test { }; let transform = transform_inputs(&fc, &args); - let (_, _, skipped_groups) = resolve_skips(&fc, &transform, &args).await.unwrap(); + let SkipResolution { skipped_groups, .. } = + resolve_skips(&fc, &transform, &args).await.unwrap(); assert!(skipped_groups.is_empty()); } @@ -430,7 +563,8 @@ mod test { let transform = transform_inputs(&fc, &args); // Should not panic on a name that matches no group; it's simply a no-op. - let (_, _, skipped_groups) = resolve_skips(&fc, &transform, &args).await.unwrap(); + let SkipResolution { skipped_groups, .. } = + resolve_skips(&fc, &transform, &args).await.unwrap(); assert!(skipped_groups.is_empty()); } diff --git a/src/doctor/runner.rs b/src/doctor/runner.rs index df93274..fd3abe4 100644 --- a/src/doctor/runner.rs +++ b/src/doctor/runner.rs @@ -198,7 +198,8 @@ where for group_name in &self.skipped_groups { debug_assert!( !run_result.succeeded_groups.contains(group_name) - && !run_result.failed_group.contains(group_name), + && !run_result.failed_group.contains(group_name) + && !run_result.skipped_group.contains(group_name), "group {group_name} was both executed and planned-skipped" ); warn!(target: "always", "Group skipped, group: \"{}\"", group_name); @@ -479,68 +480,99 @@ fn action_task_reports_for_display(action_report: &ActionReport) -> Vec { + graph: DiGraph<&'a str, i32>, + node_graph: BTreeMap, +} + /// Builds the dependency graph over every group not in `exclude`, with an edge from each /// dependency to its dependent (per `requires`). fn build_dependency_graph<'a>( groups: &'a BTreeMap, exclude: &BTreeSet, -) -> (DiGraph<&'a str, i32>, BTreeMap) { - let mut graph = DiGraph::<&str, i32>::new(); - let mut node_graph: BTreeMap = BTreeMap::new(); +) -> DependencyGraph<'a> { + let included = |name: &String| !exclude.contains(name); - for name in groups.keys() { - if exclude.contains(name) { - continue; - } - node_graph.insert(name.to_string(), graph.add_node(name)); - } + let mut graph = DiGraph::<&str, i32>::new(); + let node_graph: BTreeMap = groups + .keys() + .filter(|name| included(name)) + .map(|name| (name.clone(), graph.add_node(name))) + .collect(); - for (name, model) in groups { - if exclude.contains(name) { - continue; - } + for (name, model) in groups.iter().filter(|(name, _)| included(name)) { let this = node_graph[name]; - for dep in &model.requires { - if exclude.contains(dep) { - continue; - } - if let Some(other) = node_graph.get(dep) { - graph.add_edge(*other, this, 1); - } else { - warn!(target: "user", "{} needs {} but no such dependency found, ignoring dependency", name, dep); + for dep in model.requires.iter().filter(|dep| included(dep)) { + match node_graph.get(dep) { + Some(other) => { + graph.add_edge(*other, this, 1); + } + None => { + warn!(target: "user", "{} needs {} but no such dependency found, ignoring dependency", name, dep) + } } } } - (graph, node_graph) + DependencyGraph { graph, node_graph } } -/// Names of every group reachable from `desired_groups`, over the dependency graph with -/// `exclude` removed. Used to test "would this group have run at all" independent of any -/// `skip_only` decision. -fn reachable_group_names( - groups: &BTreeMap, - desired_groups: &BTreeSet, - exclude: &BTreeSet, -) -> BTreeSet { - let (mut graph, node_graph) = build_dependency_graph(groups, exclude); - +/// Adds a synthetic "start" node to `graph`, wired from every name in `roots`, and returns its +/// index. +fn wire_start_node( + graph: &mut DiGraph<&str, i32>, + node_graph: &BTreeMap, + roots: &BTreeSet, +) -> NodeIndex { let start = graph.add_node("start"); - for name in desired_groups { + for name in roots { if let Some(this) = node_graph.get(name) { graph.add_edge(*this, start, 1); } } + start +} +/// Reverses `graph` and returns the dependency-first (post-order) traversal of everything +/// reachable from `start`, excluding `start` itself. +fn traversal_order_from(graph: &mut DiGraph<&str, i32>, start: NodeIndex) -> Vec { graph.reverse(); - DfsPostOrder::new(&graph, start) - .iter(&graph) + DfsPostOrder::new(&*graph, start) + .iter(&*graph) .filter(|&node| node != start) .map(|node| graph.node_weight(node).unwrap().to_string()) .collect() } +/// Names of every group reachable from `desired_groups`, over the dependency graph with +/// `exclude` removed. Used to test "would this group have run at all" independent of any +/// `skip_only` decision. +fn reachable_group_names( + groups: &BTreeMap, + desired_groups: &BTreeSet, + exclude: &BTreeSet, +) -> BTreeSet { + let DependencyGraph { + mut graph, + node_graph, + } = build_dependency_graph(groups, exclude); + + let start = wire_start_node(&mut graph, &node_graph, desired_groups); + traversal_order_from(&mut graph, start) + .into_iter() + .collect() +} + +/// Arguments for [`compute_group_order`]. +pub struct GroupOrderParams<'a> { + pub groups: &'a BTreeMap, + pub desired_groups: &'a BTreeSet, + pub skip_subtree: &'a BTreeSet, + pub skip_only: &'a BTreeSet, +} + /// Computes the topologically-sorted set of groups to run, starting from `desired_groups` and /// pulling in transitive dependencies (via `requires`). /// @@ -548,12 +580,14 @@ fn reachable_group_names( /// non-skipped group (a shared dependency survives). `skip_only` groups are removed but their /// dependencies are always kept — unless that group was never going to run in the first place, /// in which case `--skip-only` on it is a no-op rather than pulling in unrelated work. -pub fn compute_group_order( - groups: &BTreeMap, - desired_groups: BTreeSet, - skip_subtree: &BTreeSet, - skip_only: &BTreeSet, -) -> Vec { +pub fn compute_group_order(params: GroupOrderParams) -> Vec { + let GroupOrderParams { + groups, + desired_groups, + skip_subtree, + skip_only, + } = params; + let all_skipped: BTreeSet = skip_subtree.union(skip_only).cloned().collect(); // Only force-keep a `skip_only` group's dependencies if that group would actually have run @@ -561,14 +595,18 @@ pub fn compute_group_order( let would_have_run = if skip_only.is_empty() { BTreeSet::new() } else { - reachable_group_names(groups, &desired_groups, skip_subtree) + reachable_group_names(groups, desired_groups, skip_subtree) }; - let (mut graph, node_graph) = build_dependency_graph(groups, &all_skipped); + let DependencyGraph { + mut graph, + node_graph, + } = build_dependency_graph(groups, &all_skipped); let roots: BTreeSet = desired_groups - .into_iter() - .filter(|name| !all_skipped.contains(name)) + .iter() + .filter(|name| !all_skipped.contains(*name)) + .cloned() .chain( skip_only .iter() @@ -579,13 +617,7 @@ pub fn compute_group_order( ) .collect(); - let start = graph.add_node("start"); - - for name in &roots { - if let Some(this) = node_graph.get(name) { - graph.add_edge(*this, start, 1); - } - } + let start = wire_start_node(&mut graph, &node_graph, &roots); debug!( format = "graphviz", @@ -593,13 +625,7 @@ pub fn compute_group_order( Dot::with_config(&graph, &[Config::EdgeNoLabel]) ); - graph.reverse(); - - let order: Vec = DfsPostOrder::new(&graph, start) - .iter(&graph) - .filter(|&node| node != start) - .map(|node| graph.node_weight(node).unwrap().to_string()) - .collect(); + let order = traversal_order_from(&mut graph, start); debug!( target: "user", @@ -626,7 +652,9 @@ mod tests { use crate::doctor::check::{ ActionRunResult, ActionRunStatus, DoctorActionRun, MockDoctorActionRun, }; - use crate::doctor::runner::{GroupActionContainer, RunGroups, compute_group_order}; + use crate::doctor::runner::{ + GroupActionContainer, GroupOrderParams, RunGroups, compute_group_order, + }; use crate::doctor::tests::{group_noop, make_root_model_additional}; use crate::prelude::{ActionReport, ActionTaskReport, MockExecutionProvider}; use anyhow::Result; @@ -660,7 +688,13 @@ mod tests { assert_eq!( 0, - compute_group_order(&groups, BTreeSet::new(), &no_skip(), &no_skip()).len() + compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::new(), + skip_subtree: &no_skip(), + skip_only: &no_skip(), + }) + .len() ); Ok(()) @@ -688,12 +722,12 @@ mod tests { assert_eq!( vec!["step_1", "step_2"], - compute_group_order( - &groups, - BTreeSet::from(["step_2".to_string()]), - &no_skip(), - &no_skip() - ) + compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["step_2".to_string()]), + skip_subtree: &no_skip(), + skip_only: &no_skip(), + }) ); Ok(()) @@ -728,12 +762,12 @@ mod tests { assert_eq!( vec!["step_3", "step_2", "step_1"], - compute_group_order( - &groups, - BTreeSet::from(["step_1".to_string()]), - &no_skip(), - &no_skip() - ) + compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["step_1".to_string()]), + skip_subtree: &no_skip(), + skip_only: &no_skip(), + }) ); Ok(()) @@ -768,12 +802,12 @@ mod tests { assert_eq!( vec!["step_1", "step_2", "step_3"], - compute_group_order( - &groups, - BTreeSet::from(["step_3".to_string()]), - &no_skip(), - &no_skip() - ) + compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["step_3".to_string()]), + skip_subtree: &no_skip(), + skip_only: &no_skip(), + }) ); Ok(()) @@ -808,12 +842,12 @@ mod tests { assert_eq!( vec!["step_1", "step_3"], - compute_group_order( - &groups, - BTreeSet::from(["step_3".to_string()]), - &no_skip(), - &no_skip() - ) + compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["step_3".to_string()]), + skip_subtree: &no_skip(), + skip_only: &no_skip(), + }) ); Ok(()) @@ -875,12 +909,12 @@ mod tests { async fn test_compute_group_order_skip_trims_exclusive_dependencies() -> Result<()> { let groups = make_dependency_chain(); - let order = compute_group_order( - &groups, - BTreeSet::from(["a".to_string(), "d".to_string()]), - &BTreeSet::from(["a".to_string()]), - &no_skip(), - ); + let order = compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["a".to_string(), "d".to_string()]), + skip_subtree: &BTreeSet::from(["a".to_string()]), + skip_only: &no_skip(), + }); // `a`, and its exclusive deps `b`/`c`, are trimmed; `d` and its dep are untouched. assert_eq!(vec!["shared", "d"], order); @@ -896,12 +930,12 @@ mod tests { // `a-and-shared` requires both `a` and `shared`. Skipping `a` force-removes it (and its // exclusive deps `b`/`c`) even though `a-and-shared` — a non-skipped root — depends on // it. `shared` survives because `a-and-shared` also depends on it directly. - let order = compute_group_order( - &groups, - BTreeSet::from(["a-and-shared".to_string()]), - &BTreeSet::from(["a".to_string()]), - &no_skip(), - ); + let order = compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["a-and-shared".to_string()]), + skip_subtree: &BTreeSet::from(["a".to_string()]), + skip_only: &no_skip(), + }); assert_eq!(vec!["shared", "a-and-shared"], order); @@ -913,12 +947,12 @@ mod tests { let groups = make_dependency_chain(); // `--skip-only=a` removes just `a`; its exclusive deps `b`/`c` still run. - let order = compute_group_order( - &groups, - BTreeSet::from(["a".to_string()]), - &no_skip(), - &BTreeSet::from(["a".to_string()]), - ); + let order = compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["a".to_string()]), + skip_subtree: &no_skip(), + skip_only: &BTreeSet::from(["a".to_string()]), + }); assert_eq!(vec!["c", "b"], order); @@ -931,12 +965,12 @@ mod tests { // Only `d` is desired; `a` (and its exclusive deps `b`/`c`) were never going to run in // the first place. `--skip-only=a` must not pull `b`/`c` in as unrelated new work. - let order = compute_group_order( - &groups, - BTreeSet::from(["d".to_string()]), - &no_skip(), - &BTreeSet::from(["a".to_string()]), - ); + let order = compute_group_order(GroupOrderParams { + groups: &groups, + desired_groups: &BTreeSet::from(["d".to_string()]), + skip_subtree: &no_skip(), + skip_only: &BTreeSet::from(["a".to_string()]), + }); assert_eq!(vec!["shared", "d"], order); @@ -1259,6 +1293,31 @@ mod tests { assert_eq!(actual.output, Some("validate output".to_string())); } + #[tokio::test] + async fn test_execute_command_returns_command_output() -> Result<()> { + let test_group = + make_root_model_additional(vec![], |meta| meta.name("test-group"), group_noop); + + let mut mock_exec = MockExecutionProvider::new(); + mock_exec + .expect_run_for_output() + .times(1) + .withf(|_, _, command| command == "echo hi") + .returning(|_, _, _| "hi".to_string()); + + let container: GroupActionContainer = GroupActionContainer { + group: test_group, + actions: vec![], + exec_provider: Arc::new(mock_exec), + exec_working_dir: Default::default(), + sys_path: "".to_string(), + }; + + assert_eq!("hi", container.execute_command("echo hi").await?); + + Ok(()) + } + #[tokio::test] async fn test_should_skip_group_true_for_boolean_skip() -> Result<()> { let test_group = make_root_model_additional( From 123723801abedb0640259e75e4242a2eef09093e Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Mon, 13 Jul 2026 14:59:45 -0400 Subject: [PATCH 3/4] Report planning-time-skipped groups in their natural run position MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit RunGroups::execute ran everything in all_paths first, then looped over skipped_groups (a BTreeSet, alphabetical) and printed "Group skipped" warnings afterward — so skipped groups always appeared at the end of the run instead of where they'd naturally sit in the topological order. Thread the full (unpruned) candidate order through SkipResolution into RunGroups as `full_order`, and merge execute()/run_path() into a single pass over it: report a skip in its natural position, silently drop anything transitively pruned as an exclusive dependency, and otherwise run normally (preserving the existing skip_remaining cascade-on-failure behavior unchanged). Co-Authored-By: Claude Opus 5 --- src/doctor/commands/run.rs | 15 ++-- src/doctor/runner.rs | 144 +++++++++++++++++++++---------------- 2 files changed, 93 insertions(+), 66 deletions(-) diff --git a/src/doctor/commands/run.rs b/src/doctor/commands/run.rs index dc33e97..7b68fa7 100644 --- a/src/doctor/commands/run.rs +++ b/src/doctor/commands/run.rs @@ -103,12 +103,14 @@ fn migrate_old_cache(old_path: &PathBuf, new_path: &PathBuf) -> Result<()> { Ok(()) } -/// The result of [`resolve_skips`]: the final skip sets used to build the run order, plus the -/// subset of those groups that would otherwise have run (for reporting). +/// The result of [`resolve_skips`]: the final skip sets used to build the run order, the subset +/// of those groups that would otherwise have run (for reporting), and the full candidate order +/// (ignoring skip decisions) so skipped groups can be reported in their natural position. struct SkipResolution { skip_subtree: BTreeSet, skip_only: BTreeSet, skipped_groups: BTreeSet, + full_order: Vec, } /// Resolves the CLI `--skip`/`--skip-only` names plus each candidate group's own `skip` config @@ -149,14 +151,16 @@ async fn resolve_skips( } let skipped_groups: BTreeSet = candidate_order - .into_iter() - .filter(|name| skip_subtree.contains(name) || skip_only.contains(name)) + .iter() + .filter(|name| skip_subtree.contains(*name) || skip_only.contains(*name)) + .cloned() .collect(); Ok(SkipResolution { skip_subtree, skip_only, skipped_groups, + full_order: candidate_order, }) } @@ -175,6 +179,7 @@ pub async fn doctor_run(found_config: &FoundConfig, args: &DoctorRunArgs) -> Res skip_subtree, skip_only, skipped_groups, + full_order, } = resolve_skips(found_config, &transform, args).await?; let all_paths = compute_group_order(GroupOrderParams { @@ -190,6 +195,7 @@ pub async fn doctor_run(found_config: &FoundConfig, args: &DoctorRunArgs) -> Res let run_groups = RunGroups { group_actions: transform.groups, all_paths, + full_order, skipped_groups, yolo: args.yolo, }; @@ -378,6 +384,7 @@ mod test { skip_subtree, skip_only, skipped_groups, + .. } = resolve_skips(&fc, &transform, &args).await.unwrap(); assert_eq!(BTreeSet::from(["a".to_string()]), skip_subtree); diff --git a/src/doctor/runner.rs b/src/doctor/runner.rs index fd3abe4..ddd369b 100644 --- a/src/doctor/runner.rs +++ b/src/doctor/runner.rs @@ -174,6 +174,10 @@ where { pub(crate) group_actions: BTreeMap>, pub(crate) all_paths: Vec, + /// The full candidate order (topological, ignoring any skip decisions). Iterated instead of + /// `all_paths` purely so `skipped_groups` can be reported in their natural position in the + /// run rather than lumped together at the end. + pub(crate) full_order: Vec, /// Groups trimmed from `all_paths` during planning (via `--skip`, `--skip-only`, or a /// group's own `skip` config) that would otherwise have run. Reported as skipped without /// ever being executed. @@ -186,35 +190,11 @@ where T: DoctorActionRun, { pub async fn execute(&self) -> Result { - let mut full_path = Vec::new(); - for path in &self.all_paths { - if let Some(group_container) = self.group_actions.get(path) { - full_path.push(group_container); - } - } - - let mut run_result = self.run_path(full_path).await?; - - for group_name in &self.skipped_groups { - debug_assert!( - !run_result.succeeded_groups.contains(group_name) - && !run_result.failed_group.contains(group_name) - && !run_result.skipped_group.contains(group_name), - "group {group_name} was both executed and planned-skipped" - ); - warn!(target: "always", "Group skipped, group: \"{}\"", group_name); - run_result.skipped_group.insert(group_name.to_string()); - run_result.group_reports.push(GroupReport::new(group_name)); - } - - Ok(run_result) - } + let runnable: BTreeSet<&str> = self.all_paths.iter().map(String::as_str).collect(); - async fn run_path(&self, groups: Vec<&GroupActionContainer>) -> Result { let header_span = info_span!("doctor run", "indicatif.pb_show" = true); - header_span.pb_set_length(self.all_paths.len() as u64); + header_span.pb_set_length((self.all_paths.len() + self.skipped_groups.len()) as u64); header_span.pb_set_message("scope doctor run"); - let _span = header_span.enter(); let mut skip_remaining = false; @@ -226,13 +206,34 @@ where group_reports: Vec::new(), }; - for group_container in groups { - let group_name = group_container.group_name(); + for group_name in &self.full_order { + if self.skipped_groups.contains(group_name) { + debug_assert!( + !runnable.contains(group_name.as_str()), + "group {group_name} is both runnable and planned-skipped" + ); + header_span.pb_inc(1); + warn!(target: "always", "Group skipped, group: \"{}\"", group_name); + run_result.skipped_group.insert(group_name.clone()); + run_result.group_reports.push(GroupReport::new(group_name)); + continue; + } + + if !runnable.contains(group_name.as_str()) { + // Transitively pruned as an exclusive dependency of a skipped group — nothing + // to report, it simply never appears in `all_paths`. + continue; + } + + let Some(group_container) = self.group_actions.get(group_name) else { + continue; + }; + header_span.pb_inc(1); debug!(target: "user", "Running check {}", group_name); if skip_remaining { - run_result.skipped_group.insert(group_name.to_string()); + run_result.skipped_group.insert(group_name.clone()); continue; } @@ -240,7 +241,7 @@ where parent: &header_span, "group", "indicatif.pb_show" = true, - "group.name" = group_name, + "group.name" = group_name.as_str(), "otel.name" = format!("group {}", group_name) ); group_span.pb_set_length(group_container.actions.len() as u64); @@ -1042,13 +1043,15 @@ mod tests { make_group_action("group_3", make_action_runs(ActionRunStatus::CheckSucceeded)), ]); + let all_paths = vec![ + "group_1".to_string(), + "group_2".to_string(), + "group_3".to_string(), + ]; let run_groups = RunGroups { group_actions, - all_paths: vec![ - "group_1".to_string(), - "group_2".to_string(), - "group_3".to_string(), - ], + full_order: all_paths.clone(), + all_paths, skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1075,13 +1078,15 @@ mod tests { make_group_action("skipped_2", will_not_run()), ]); + let all_paths = vec![ + "fails".to_string(), + "skipped_1".to_string(), + "skipped_2".to_string(), + ]; let run_groups = RunGroups { group_actions, - all_paths: vec![ - "fails".to_string(), - "skipped_1".to_string(), - "skipped_2".to_string(), - ], + full_order: all_paths.clone(), + all_paths, skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1114,13 +1119,15 @@ mod tests { make_group_action("skipped", will_not_run()), ]); + let all_paths = vec![ + "succeeds".to_string(), + "user_denies".to_string(), + "skipped".to_string(), + ]; let run_groups = RunGroups { group_actions, - all_paths: vec![ - "succeeds".to_string(), - "user_denies".to_string(), - "skipped".to_string(), - ], + full_order: all_paths.clone(), + all_paths, skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1163,13 +1170,15 @@ mod tests { ), ]); + let all_paths = vec![ + "succeeds_1".to_string(), + "user_denies".to_string(), + "succeeds_2".to_string(), + ]; let run_groups = RunGroups { group_actions, - all_paths: vec![ - "succeeds_1".to_string(), - "user_denies".to_string(), - "succeeds_2".to_string(), - ], + full_order: all_paths.clone(), + all_paths, skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1208,13 +1217,15 @@ mod tests { ), ]); + let all_paths = vec![ + "succeeds_1".to_string(), + "fails".to_string(), + "succeeds_2".to_string(), + ]; let run_groups = RunGroups { group_actions, - all_paths: vec![ - "succeeds_1".to_string(), - "fails".to_string(), - "succeeds_2".to_string(), - ], + full_order: all_paths.clone(), + all_paths, skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1402,6 +1413,7 @@ mod tests { let run_groups = RunGroups { group_actions: BTreeMap::new(), all_paths: Vec::new(), + full_order: Vec::new(), skipped_groups: BTreeSet::new(), yolo: false, }; @@ -1417,15 +1429,23 @@ mod tests { } #[tokio::test] - async fn test_execute_reports_planned_skips_without_running_them() -> Result<()> { - let group_actions = BTreeMap::from([make_group_action( - "runs", - make_action_runs(ActionRunStatus::CheckSucceeded), - )]); + async fn test_execute_reports_planned_skips_in_their_natural_position() -> Result<()> { + let group_actions = BTreeMap::from([ + make_group_action("before", make_action_runs(ActionRunStatus::CheckSucceeded)), + make_group_action("after", make_action_runs(ActionRunStatus::CheckSucceeded)), + ]); + // "trimmed" sits between "before" and "after" in the full candidate order, even though + // it's absent from `all_paths` — proving the skip warning interleaves at that position + // instead of being reported only after every runnable group has finished. let run_groups = RunGroups { group_actions, - all_paths: vec!["runs".to_string()], + all_paths: vec!["before".to_string(), "after".to_string()], + full_order: vec![ + "before".to_string(), + "trimmed".to_string(), + "after".to_string(), + ], skipped_groups: BTreeSet::from(["trimmed".to_string()]), yolo: false, }; @@ -1435,7 +1455,7 @@ mod tests { // Groups trimmed during planning are reported as skipped without failing the run. assert!(exit_code.did_succeed); assert_eq!( - BTreeSet::from(["runs".to_string()]), + BTreeSet::from(["before".to_string(), "after".to_string()]), exit_code.succeeded_groups ); assert_eq!( From 5065a455a3bab9f2a6296ab31b1b4dcba76072b2 Mon Sep 17 00:00:00 2001 From: Chris McClellan Date: Mon, 13 Jul 2026 15:43:36 -0400 Subject: [PATCH 4/4] Close mutation-testing coverage gaps in skip resolution Three real mutants survived a mutation-testing pass on this branch, all in untested warning/error paths rather than core logic: - should_skip_group's run_command error path (SkipSpec::Command) was never exercised, so an Err was indistinguishable from "don't skip". Added a unit test asserting the error propagates. - warn_on_unknown_group's condition could be deleted or inverted with no test noticing. Added E2E tests (matching the existing stdout-substring pattern used throughout this suite) asserting the warning fires for an unknown --skip/--skip-only name. - The "Could not find any tasks to execute" guard's `&&` could become `||` with no test noticing, which would spuriously warn whenever something was skipped but other groups still ran. Added an E2E test covering the mixed case (everything runnable was explicitly skipped). Co-Authored-By: Claude Opus 5 --- src/doctor/runner.rs | 32 ++++++++++++++++++++++++++- tests/scope_doctor.rs | 51 +++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 82 insertions(+), 1 deletion(-) diff --git a/src/doctor/runner.rs b/src/doctor/runner.rs index ddd369b..f3c51ba 100644 --- a/src/doctor/runner.rs +++ b/src/doctor/runner.rs @@ -657,7 +657,7 @@ mod tests { GroupActionContainer, GroupOrderParams, RunGroups, compute_group_order, }; use crate::doctor::tests::{group_noop, make_root_model_additional}; - use crate::prelude::{ActionReport, ActionTaskReport, MockExecutionProvider}; + use crate::prelude::{ActionReport, ActionTaskReport, CaptureError, MockExecutionProvider}; use anyhow::Result; use std::collections::{BTreeMap, BTreeSet}; use std::sync::Arc; @@ -1371,6 +1371,36 @@ mod tests { Ok(()) } + #[tokio::test] + async fn test_should_skip_group_propagates_command_error() { + let test_group = make_root_model_additional( + vec![], + |meta| meta.name("test-group"), + |group| { + group.skip(SkipSpec::Command { + command: "boom".to_string(), + }) + }, + ); + + let mut mock_exec = MockExecutionProvider::new(); + mock_exec.expect_run_command().times(1).returning(|_| { + Err(CaptureError::MissingShExec { + name: "boom".to_string(), + }) + }); + + let container: GroupActionContainer = GroupActionContainer { + group: test_group, + actions: vec![], + exec_provider: Arc::new(mock_exec), + exec_working_dir: Default::default(), + sys_path: "".to_string(), + }; + + assert!(container.should_skip_group().await.is_err()); + } + #[tokio::test] async fn test_execute_runs_group_actions_regardless_of_group_skip_field() -> Result<()> { // `execute_group` no longer consults `group.skip` — skip is resolved during planning diff --git a/tests/scope_doctor.rs b/tests/scope_doctor.rs index abf9ae4..e031a6e 100644 --- a/tests/scope_doctor.rs +++ b/tests/scope_doctor.rs @@ -149,6 +149,57 @@ fn test_no_tasks_found() { )); } +#[test] +fn test_no_tasks_warning_suppressed_when_everything_runnable_was_skipped() { + // Regression test for a mutation-testing survivor: `all_paths.is_empty() && + // skipped_groups.is_empty()` must stay an `&&` — skipping every group that would otherwise + // have run is not the same as finding nothing to do, and shouldn't be reported as such. + let test_helper = ScopeTestHelper::new( + "test_no_tasks_warning_suppressed_when_everything_runnable_was_skipped", + "two-groups", + ); + let result = test_helper.doctor_run(Some(&["--skip=group-one", "--skip=group-two"])); + + result + .success() + .stdout(predicate::str::contains("Could not find any tasks to execute").not()) + .stdout(predicate::str::contains( + "Group skipped, group: \"group-one\"", + )) + .stdout(predicate::str::contains( + "Group skipped, group: \"group-two\"", + )) + .stdout(predicate::str::contains( + "Summary: 0 groups succeeded, 2 groups skipped", + )); + + test_helper.clean_work_dir(); +} + +#[test] +fn test_skip_unknown_group_name_warns() { + let test_helper = ScopeTestHelper::new("test_skip_unknown_group_name_warns", "two-groups"); + let result = test_helper.doctor_run(Some(&["--skip=does-not-exist"])); + + result.success().stdout(predicate::str::contains( + "--skip does-not-exist does not match any known group, ignoring", + )); + + test_helper.clean_work_dir(); +} + +#[test] +fn test_skip_only_unknown_group_name_warns() { + let test_helper = ScopeTestHelper::new("test_skip_only_unknown_group_name_warns", "two-groups"); + let result = test_helper.doctor_run(Some(&["--skip-only=does-not-exist"])); + + result.success().stdout(predicate::str::contains( + "--skip-only does-not-exist does not match any known group, ignoring", + )); + + test_helper.clean_work_dir(); +} + #[test] fn test_sub_command_works() { let test_helper = ScopeTestHelper::new("test_sub_command_works", "command-paths");