diff --git a/crates/no-mistakes/src/check_tasks/filesystem.rs b/crates/no-mistakes/src/check_tasks/filesystem.rs index 463f05d9f..1ec2eebe2 100644 --- a/crates/no-mistakes/src/check_tasks/filesystem.rs +++ b/crates/no-mistakes/src/check_tasks/filesystem.rs @@ -31,6 +31,7 @@ const FILESYSTEM_RULE_IDS: &[&str] = &[ rules::NEXTJS_REDIRECT_DESTINATIONS, rules::NO_EMPTY_OR_COMMENTS_ONLY_FILES, rules::NO_GIT_IDENTITY_MUTATION, + rules::NO_MISTAKES_CONFIG, rules::NO_RAW_EPHEMERAL_PORT, rules::PACKAGE_JSON_REGISTRY_ONLY, rules::PACKAGE_JSON_WORKSPACE_COVERAGE, @@ -65,6 +66,7 @@ const FILESYSTEM_RULE_IDS: &[&str] = &[ rules::VITEST_CI_PATH_COVERAGE, rules::VITEST_PROJECT_MAPPING, rules::VITEST_TEST_CORRESPONDENCE, + rules::WORKFLOW_TOPOLOGY_POLICY, rules::WORKSPACE_PACKAGE_CYCLES, ]; diff --git a/crates/no-mistakes/src/codebase/rules/filesystem_dispatch.rs b/crates/no-mistakes/src/codebase/rules/filesystem_dispatch.rs index 5289480f1..4d23e39ee 100644 --- a/crates/no-mistakes/src/codebase/rules/filesystem_dispatch.rs +++ b/crates/no-mistakes/src/codebase/rules/filesystem_dispatch.rs @@ -10,17 +10,18 @@ use super::{ markdown_child_links, markdown_eval_tests, markdown_link_display_text, markdown_mermaid_validation, markdown_reachability, markdown_structure_budget, nextjs_redirect_destinations, no_empty_or_comments_only_files, no_git_identity_mutation, - no_raw_ephemeral_port, package_json_registry_only, package_json_workspace_coverage, - postgres_constraint_validate, postgres_fk_index, postgres_lock_ordering, - postgres_no_add_column, postgres_no_generated_column_writes, postgres_no_offset, - postgres_redundant_index, postgres_require_fk_on_delete, postgres_require_named_constraints, - postgres_require_query_annotation, postgres_sql_statement_policy, - production_dependency_declarations, require_files_in_subdirs, require_test_per_subdir, - required_companion_imports, required_local_docs, rust_rules_combined, shellcheck_runner, - strict_package_layout, structured_config_policy, test_email_domain_policy, + no_mistakes_config, no_raw_ephemeral_port, package_json_registry_only, + package_json_workspace_coverage, postgres_constraint_validate, postgres_fk_index, + postgres_lock_ordering, postgres_no_add_column, postgres_no_generated_column_writes, + postgres_no_offset, postgres_redundant_index, postgres_require_fk_on_delete, + postgres_require_named_constraints, postgres_require_query_annotation, + postgres_sql_statement_policy, production_dependency_declarations, require_files_in_subdirs, + require_test_per_subdir, required_companion_imports, required_local_docs, rust_rules_combined, + shellcheck_runner, strict_package_layout, structured_config_policy, test_email_domain_policy, test_no_dependency_pins, tsconfig_alias_folder_mapping, tsconfig_file_coverage, tsconfig_gate_coverage, version_pin_consistency, vitest_ci_path_coverage, - vitest_project_mapping, vitest_test_correspondence, workspace_package_cycles, + vitest_project_mapping, vitest_test_correspondence, workflow_topology_policy, + workspace_package_cycles, }; mod candidate_helpers; @@ -42,18 +43,19 @@ use super::{ INTEGRATION_TEST_NO_MOCKS, LOCKFILE_ALLOWLIST, MARKDOWN_CHILD_LINKS, MARKDOWN_EVAL_TESTS, MARKDOWN_LINK_DISPLAY_TEXT, MARKDOWN_MERMAID_VALIDATION, MARKDOWN_REACHABILITY, MARKDOWN_STRUCTURE_BUDGET, NEXTJS_REDIRECT_DESTINATIONS, NO_EMPTY_OR_COMMENTS_ONLY_FILES, - NO_GIT_IDENTITY_MUTATION, NO_RAW_EPHEMERAL_PORT, PACKAGE_JSON_REGISTRY_ONLY, - PACKAGE_JSON_WORKSPACE_COVERAGE, POSTGRES_CONSTRAINT_VALIDATE, POSTGRES_FK_INDEX, - POSTGRES_LOCK_ORDERING, POSTGRES_NO_ADD_COLUMN, POSTGRES_NO_GENERATED_COLUMN_WRITES, - POSTGRES_NO_OFFSET, POSTGRES_REDUNDANT_INDEX, POSTGRES_REQUIRE_FK_ON_DELETE, - POSTGRES_REQUIRE_NAMED_CONSTRAINTS, POSTGRES_REQUIRE_QUERY_ANNOTATION, - POSTGRES_SQL_STATEMENT_POLICY, PRODUCTION_DEPENDENCY_DECLARATIONS, REQUIRED_COMPANION_IMPORTS, - REQUIRED_DOC_SECTION, REQUIRED_LOCAL_DOCS, REQUIRE_FILES_IN_SUBDIRS, REQUIRE_TEST_PER_SUBDIR, + NO_GIT_IDENTITY_MUTATION, NO_MISTAKES_CONFIG, NO_RAW_EPHEMERAL_PORT, + PACKAGE_JSON_REGISTRY_ONLY, PACKAGE_JSON_WORKSPACE_COVERAGE, POSTGRES_CONSTRAINT_VALIDATE, + POSTGRES_FK_INDEX, POSTGRES_LOCK_ORDERING, POSTGRES_NO_ADD_COLUMN, + POSTGRES_NO_GENERATED_COLUMN_WRITES, POSTGRES_NO_OFFSET, POSTGRES_REDUNDANT_INDEX, + POSTGRES_REQUIRE_FK_ON_DELETE, POSTGRES_REQUIRE_NAMED_CONSTRAINTS, + POSTGRES_REQUIRE_QUERY_ANNOTATION, POSTGRES_SQL_STATEMENT_POLICY, + PRODUCTION_DEPENDENCY_DECLARATIONS, REQUIRED_COMPANION_IMPORTS, REQUIRED_DOC_SECTION, + REQUIRED_LOCAL_DOCS, REQUIRE_FILES_IN_SUBDIRS, REQUIRE_TEST_PER_SUBDIR, RUST_MAX_LINES_PER_FILE, RUST_NO_INLINE_ALLOWS, RUST_NO_INLINE_TESTS, SHELLCHECK_RUNNER, STRICT_PACKAGE_LAYOUT, STRUCTURED_CONFIG_POLICY, TEST_EMAIL_DOMAIN_POLICY, TEST_NO_DEPENDENCY_PINS, TSCONFIG_ALIAS_FOLDER_MAPPING, TSCONFIG_FILE_COVERAGE, TSCONFIG_GATE_COVERAGE, VITEST_CI_PATH_COVERAGE, VITEST_PROJECT_MAPPING, - VITEST_TEST_CORRESPONDENCE, WORKSPACE_PACKAGE_CYCLES, + VITEST_TEST_CORRESPONDENCE, WORKFLOW_TOPOLOGY_POLICY, WORKSPACE_PACKAGE_CYCLES, }; pub use entrypoints::{ run_filesystem_rules, run_filesystem_rules_with_config, diff --git a/crates/no-mistakes/src/codebase/rules/filesystem_dispatch/registry.rs b/crates/no-mistakes/src/codebase/rules/filesystem_dispatch/registry.rs index 14eb848bf..705da129c 100644 --- a/crates/no-mistakes/src/codebase/rules/filesystem_dispatch/registry.rs +++ b/crates/no-mistakes/src/codebase/rules/filesystem_dispatch/registry.rs @@ -17,6 +17,7 @@ macro_rules! filesystem_rules { TSCONFIG_ALIAS_FOLDER_MAPPING => tsconfig_alias_folder_mapping::check_with_files, TSCONFIG_FILE_COVERAGE => tsconfig_file_coverage::check_with_files, NO_GIT_IDENTITY_MUTATION => no_git_identity_mutation::check_with_files, + NO_MISTAKES_CONFIG => no_mistakes_config::check_with_files, NO_RAW_EPHEMERAL_PORT => no_raw_ephemeral_port::check_with_files, MARKDOWN_EVAL_TESTS => markdown_eval_tests::check_with_files, PACKAGE_JSON_REGISTRY_ONLY => package_json_registry_only::check_with_files, @@ -32,6 +33,7 @@ macro_rules! filesystem_rules { NO_EMPTY_OR_COMMENTS_ONLY_FILES => no_empty_or_comments_only_files::check_with_files, NEXTJS_REDIRECT_DESTINATIONS => nextjs_redirect_destinations::check_with_files, VITEST_TEST_CORRESPONDENCE => vitest_test_correspondence::check_with_files, + WORKFLOW_TOPOLOGY_POLICY => workflow_topology_policy::check_with_files, FILE_EXTENSION_POLICY => file_extension_policy::check_with_files, BANNED_PATHS => banned_paths::check_with_files, BANNED_RENAMED_FILES => banned_renamed_files::check_with_files, diff --git a/crates/no-mistakes/src/codebase/rules/ids.rs b/crates/no-mistakes/src/codebase/rules/ids.rs index a7ca7bd76..2b9983459 100644 --- a/crates/no-mistakes/src/codebase/rules/ids.rs +++ b/crates/no-mistakes/src/codebase/rules/ids.rs @@ -25,6 +25,7 @@ pub use super::nextjs_no_caching::RULE_ID as NEXTJS_NO_CACHING; pub use super::nextjs_redirect_destinations::RULE_ID as NEXTJS_REDIRECT_DESTINATIONS; pub use super::no_empty_or_comments_only_files::RULE_ID as NO_EMPTY_OR_COMMENTS_ONLY_FILES; pub use super::no_git_identity_mutation::RULE_ID as NO_GIT_IDENTITY_MUTATION; +pub use super::no_mistakes_config::RULE_ID as NO_MISTAKES_CONFIG; pub use super::no_raw_ephemeral_port::RULE_ID as NO_RAW_EPHEMERAL_PORT; pub use super::package_json_registry_only::RULE_ID as PACKAGE_JSON_REGISTRY_ONLY; pub use super::package_json_workspace_coverage::RULE_ID as PACKAGE_JSON_WORKSPACE_COVERAGE; @@ -64,4 +65,5 @@ pub use super::version_pin_consistency::RULE_ID as VERSION_PIN_CONSISTENCY; pub use super::vitest_ci_path_coverage::RULE_ID as VITEST_CI_PATH_COVERAGE; pub use super::vitest_project_mapping::RULE_ID as VITEST_PROJECT_MAPPING; pub use super::vitest_test_correspondence::RULE_ID as VITEST_TEST_CORRESPONDENCE; +pub use super::workflow_topology_policy::RULE_ID as WORKFLOW_TOPOLOGY_POLICY; pub use super::workspace_package_cycles::RULE_ID as WORKSPACE_PACKAGE_CYCLES; diff --git a/crates/no-mistakes/src/codebase/rules/mod.rs b/crates/no-mistakes/src/codebase/rules/mod.rs index 6accc3aa7..2e1995fb3 100644 --- a/crates/no-mistakes/src/codebase/rules/mod.rs +++ b/crates/no-mistakes/src/codebase/rules/mod.rs @@ -30,6 +30,7 @@ pub mod nextjs_no_caching; pub mod nextjs_redirect_destinations; pub mod no_empty_or_comments_only_files; pub mod no_git_identity_mutation; +pub mod no_mistakes_config; pub mod no_raw_ephemeral_port; pub mod package_json_registry_only; pub mod package_json_workspace_coverage; @@ -70,6 +71,7 @@ pub mod vitest_ci_path_coverage; mod vitest_project_catalog; pub mod vitest_project_mapping; pub mod vitest_test_correspondence; +pub mod workflow_topology_policy; pub mod workspace_package_cycles; pub mod filesystem_dispatch; diff --git a/crates/no-mistakes/src/codebase/rules/no_mistakes_config.rs b/crates/no-mistakes/src/codebase/rules/no_mistakes_config.rs new file mode 100644 index 000000000..c1f24d780 --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/no_mistakes_config.rs @@ -0,0 +1,72 @@ +use super::RuleFinding; +use crate::codebase::ts_source::relative_slash_path; +use crate::config::v2::NoMistakesConfig; +use anyhow::Result; +use std::collections::BTreeSet; +use std::path::{Path, PathBuf}; + +mod globs; +mod limits; +mod paths; + +pub const RULE_ID: &str = "no-mistakes-config"; + +pub(crate) fn check_with_files( + root: &Path, + config: &NoMistakesConfig, + all_files: &[PathBuf], +) -> Result> { + let sources = super::source_store_for_files(all_files); + check_with_files_and_sources(root, config, all_files, &sources) +} + +pub(crate) fn check_with_files_and_sources( + root: &Path, + config: &NoMistakesConfig, + all_files: &[PathBuf], + _sources: &crate::codebase::ts_source::SourceStore, +) -> Result> { + if !config.rule_configured(RULE_ID) { + return Ok(Vec::new()); + } + let tracked = tracked_rels(root, all_files); + let config_file = config_rel(root, all_files); + let mut findings = paths::lint(config, &tracked, &config_file)?; + findings.extend(globs::lint(config, &tracked, &config_file)?); + findings.extend(limits::lint(config, &config_file)); + super::sort_findings(&mut findings); + Ok(findings) +} + +fn tracked_rels(root: &Path, all_files: &[PathBuf]) -> BTreeSet { + all_files + .iter() + .map(|path| relative_slash_path(root, path)) + .filter(|rel| !rel.is_empty()) + .collect() +} + +fn config_rel(root: &Path, all_files: &[PathBuf]) -> String { + all_files + .iter() + .find_map(|path| { + let name = path.file_name()?.to_str()?; + (name == ".no-mistakes.yml" || name == ".no-mistakes.yaml") + .then(|| relative_slash_path(root, path)) + }) + .unwrap_or_else(|| ".no-mistakes.yml".to_string()) +} + +pub(super) fn finding(config_file: &str, message: String) -> RuleFinding { + RuleFinding { + rule: RULE_ID.to_string(), + file: config_file.to_string(), + line: 1, + message, + import: None, + target: None, + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/no-mistakes/src/codebase/rules/no_mistakes_config/globs.rs b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/globs.rs new file mode 100644 index 000000000..3a84890e7 --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/globs.rs @@ -0,0 +1,92 @@ +use super::finding; +use super::paths::frameworks; +use crate::codebase::rules::path_filter::GlobMatcher; +use crate::codebase::rules::RuleFinding; +use crate::config::v2::NoMistakesConfig; +use anyhow::Result; +use std::collections::BTreeSet; + +pub(super) fn lint( + config: &NoMistakesConfig, + tracked: &BTreeSet, + config_file: &str, +) -> Result> { + let mut findings = Vec::new(); + for (framework, plan) in frameworks(config) { + for (env_name, env) in &plan.environments { + lint_patterns( + &format!("testPlan.{framework}.environments.{env_name}.include"), + &env.include, + tracked, + config_file, + &mut findings, + )?; + lint_patterns( + &format!("testPlan.{framework}.environments.{env_name}.exclude"), + &env.exclude, + tracked, + config_file, + &mut findings, + )?; + } + } + for (name, project) in &config.projects { + lint_patterns( + &format!("projects.{name}.include"), + &project.include, + tracked, + config_file, + &mut findings, + )?; + lint_patterns( + &format!("projects.{name}.exclude"), + &project.exclude, + tracked, + config_file, + &mut findings, + )?; + } + for (index, rule) in config.rules.iter().enumerate() { + lint_patterns( + &format!("rules[{index}].include"), + &rule.include, + tracked, + config_file, + &mut findings, + )?; + lint_patterns( + &format!("rules[{index}].exclude"), + &rule.exclude, + tracked, + config_file, + &mut findings, + )?; + } + Ok(findings) +} + +fn lint_patterns( + field: &str, + patterns: &[String], + tracked: &BTreeSet, + config_file: &str, + findings: &mut Vec, +) -> Result<()> { + for (index, pattern) in patterns.iter().enumerate() { + if pattern.trim().is_empty() || !looks_like_glob(pattern) { + continue; + } + let matcher = GlobMatcher::new(std::slice::from_ref(pattern), field)?; + if !tracked.iter().any(|rel| matcher.is_match(rel)) { + findings.push(finding( + config_file, + format!("{field}[{index}]: glob `{pattern}` matches no tracked files"), + )); + } + } + Ok(()) +} + +fn looks_like_glob(pattern: &str) -> bool { + pattern.contains('*') || pattern.contains('?') || pattern.contains('{') +} diff --git a/crates/no-mistakes/src/codebase/rules/no_mistakes_config/limits.rs b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/limits.rs new file mode 100644 index 000000000..2dd3175cc --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/limits.rs @@ -0,0 +1,43 @@ +use super::finding; +use super::paths::frameworks; +use crate::codebase::rules::RuleFinding; +use crate::config::v2::schema::{TestPlanEnvironment, TestPlanGroupType, TestPlanLimit}; +use crate::config::v2::NoMistakesConfig; + +pub(super) fn lint(config: &NoMistakesConfig, config_file: &str) -> Vec { + let mut findings = Vec::new(); + for (framework, plan) in frameworks(config) { + for (env_name, env) in &plan.environments { + if has_effective_limit(&env.limit) && !env.all && has_direct_group(env) { + findings.push(finding( + config_file, + format!( + "testPlan.{framework}.environments.{env_name} has a limit while a direct group exists; \ +scope the budget onto non-direct groups so changed tests are not dropped (regression #9440)" + ), + )); + } + } + } + findings +} + +fn has_direct_group(env: &TestPlanEnvironment) -> bool { + env.groups.is_empty() + || env + .groups + .iter() + .any(|group| group.type_ == TestPlanGroupType::Direct) +} + +fn has_effective_limit(limit: &Option) -> bool { + let Some(limit) = limit else { + return false; + }; + limit.files.is_some() + || limit + .percent + .as_ref() + .and_then(|percent| percent.value()) + .is_some() +} diff --git a/crates/no-mistakes/src/codebase/rules/no_mistakes_config/paths.rs b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/paths.rs new file mode 100644 index 000000000..7abfc7cc7 --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/paths.rs @@ -0,0 +1,82 @@ +use super::finding; +use crate::codebase::rules::RuleFinding; +use crate::config::v2::NoMistakesConfig; +use anyhow::Result; +use std::collections::BTreeSet; + +mod collect; + +pub(super) use collect::frameworks; + +#[derive(Clone, Copy)] +pub(super) enum Kind { + File, + Directory, + Glob, +} + +pub(super) struct Ref { + pub(super) field: String, + pub(super) kind: Kind, + pub(super) value: String, +} + +pub(super) fn lint( + config: &NoMistakesConfig, + tracked: &BTreeSet, + config_file: &str, +) -> Result> { + let mut refs = Vec::new(); + collect::collect(config, &mut refs); + Ok(refs + .into_iter() + .filter_map(|item| missing(item, tracked, config_file)) + .collect()) +} + +fn missing(item: Ref, tracked: &BTreeSet, config_file: &str) -> Option { + let present = match item.kind { + Kind::Glob => glob_matches(&item.value, tracked), + Kind::File => tracked.contains(item.value.trim_start_matches("./")), + Kind::Directory => directory_present(&item.value, tracked), + }; + (!present).then(|| { + finding( + config_file, + format!( + "{}: missing {} `{}`", + item.field, + kind_label(item.kind), + item.value + ), + ) + }) +} + +fn kind_label(kind: Kind) -> &'static str { + match kind { + Kind::File => "file", + Kind::Directory => "directory", + Kind::Glob => "path", + } +} + +fn directory_present(value: &str, tracked: &BTreeSet) -> bool { + let prefix = value.trim_start_matches("./").trim_end_matches('/'); + if prefix.is_empty() || prefix == "." { + return true; + } + tracked.contains(prefix) + || tracked + .iter() + .any(|rel| rel == prefix || rel.starts_with(&format!("{prefix}/"))) +} + +fn glob_matches(pattern: &str, tracked: &BTreeSet) -> bool { + crate::codebase::rules::path_filter::GlobMatcher::new( + &[pattern.to_string()], + "no-mistakes-config path", + ) + .map(|matcher| tracked.iter().any(|rel| matcher.is_match(rel))) + .unwrap_or(false) +} diff --git a/crates/no-mistakes/src/codebase/rules/no_mistakes_config/paths/collect.rs b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/paths/collect.rs new file mode 100644 index 000000000..4a1a891bf --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/paths/collect.rs @@ -0,0 +1,148 @@ +use super::{Kind, Ref}; +use crate::config::v2::schema::{StringOrList, TestPlanFrameworkConfig}; +use crate::config::v2::NoMistakesConfig; + +pub(super) fn collect(config: &NoMistakesConfig, refs: &mut Vec) { + push_opt( + refs, + "frontendRoot", + Kind::Directory, + config.frontend_root.as_deref(), + ); + for (name, project) in &config.projects { + push_opt( + refs, + &format!("projects.{name}.root"), + Kind::Directory, + project.root.as_deref(), + ); + } + collect_tests(config, refs); + collect_test_plan(config, refs); +} + +fn collect_tests(config: &NoMistakesConfig, refs: &mut Vec) { + let playwright = &config.tests.playwright; + push_list( + refs, + "tests.playwright.configs", + Kind::File, + &playwright.configs, + ); + for (index, root) in playwright.selector_roots.iter().enumerate() { + push( + refs, + format!("tests.playwright.selectorRoots[{index}]"), + Kind::Directory, + root, + ); + } + push_opt( + refs, + "tests.playwright.frontendRoot", + Kind::Directory, + playwright.frontend_root.as_deref(), + ); + for (index, helper) in playwright.navigation_helpers.iter().enumerate() { + push( + refs, + format!("tests.playwright.navigationHelpers[{index}]"), + Kind::File, + helper, + ); + } + push_list( + refs, + "tests.vitest.configs", + Kind::File, + &config.tests.vitest.configs, + ); + push_list( + refs, + "tests.jest.configs", + Kind::File, + &config.tests.jest.configs, + ); + push_list( + refs, + "tests.storybook.configs", + Kind::File, + &config.tests.storybook.configs, + ); + for (index, package) in config.tests.swift.packages.iter().enumerate() { + push( + refs, + format!("tests.swift.packages[{index}]"), + Kind::Directory, + package, + ); + } +} + +fn collect_test_plan(config: &NoMistakesConfig, refs: &mut Vec) { + for (framework, plan) in frameworks(config) { + for (index, trigger) in plan.full_suite_triggers.triggers.iter().enumerate() { + for (path_index, path) in trigger.paths.iter().enumerate() { + let kind = if path.contains('*') { + Kind::Glob + } else { + Kind::File + }; + push( + refs, + format!( + "testPlan.{framework}.fullSuiteTriggers.triggers[{index}].paths[{path_index}]" + ), + kind, + path, + ); + } + } + } +} + +pub(crate) fn frameworks( + config: &NoMistakesConfig, +) -> [(&'static str, &TestPlanFrameworkConfig); 14] { + [ + ("dotnet", &config.test_plan.dotnet), + ("playwright", &config.test_plan.playwright), + ("vitest", &config.test_plan.vitest), + ("swift", &config.test_plan.swift), + ("python", &config.test_plan.python), + ("go", &config.test_plan.go), + ("cargo", &config.test_plan.cargo), + ("rails", &config.test_plan.rails), + ("php", &config.test_plan.php), + ("java", &config.test_plan.java), + ("kotlin", &config.test_plan.kotlin), + ("elixir", &config.test_plan.elixir), + ("dart", &config.test_plan.dart), + ("jest", &config.test_plan.jest), + ] +} + +fn push_list(refs: &mut Vec, field: &str, kind: Kind, values: &Option) { + let Some(values) = values else { + return; + }; + for (index, value) in values.values().iter().enumerate() { + push(refs, format!("{field}[{index}]"), kind, value); + } +} + +fn push_opt(refs: &mut Vec, field: &str, kind: Kind, value: Option<&str>) { + if let Some(value) = value { + push(refs, field.to_string(), kind, value); + } +} + +fn push(refs: &mut Vec, field: String, kind: Kind, value: &str) { + if !value.is_empty() { + refs.push(Ref { + field, + kind, + value: value.to_string(), + }); + } +} diff --git a/crates/no-mistakes/src/codebase/rules/no_mistakes_config/tests.rs b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/tests.rs new file mode 100644 index 000000000..445253979 --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/no_mistakes_config/tests.rs @@ -0,0 +1,285 @@ +use super::*; +use crate::config::v2::{ + schema::{RuleDef, RuleScope}, + NoMistakesConfig, +}; +use std::path::{Path, PathBuf}; + +fn fixture(name: &str) -> PathBuf { + crate::codebase::ts_resolver::normalize_path( + &PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../../test-cases/rules/no-mistakes-config/fixture") + .join(name), + ) +} + +fn enable(mut config: NoMistakesConfig) -> NoMistakesConfig { + config.rules.insert( + 0, + RuleDef { + rule: RULE_ID.to_string(), + scope: Some(RuleScope::Repository), + ..Default::default() + }, + ); + config +} + +fn load(root: &Path) -> NoMistakesConfig { + let yaml = std::fs::read_to_string(root.join(".no-mistakes.yml")).unwrap(); + enable(serde_yaml::from_str(&yaml).unwrap()) +} + +fn files(root: &Path) -> Vec { + let mut files = vec![root.join(".no-mistakes.yml")]; + for name in [ + "web/src/index.ts", + "vitest.config.ts", + "playwright/tests/a.spec.mts", + ] { + let path = root.join(name); + if path.exists() { + files.push(path); + } + } + files +} + +fn run(name: &str) -> Vec { + let root = fixture(name); + check_with_files(&root, &load(&root), &files(&root)).unwrap() +} + +#[test] +fn missing_project_root_is_a_finding() { + let findings = run("missing-path"); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("projects.web.root") + && finding.message.contains("missing")), + "{findings:?}" + ); +} + +#[test] +fn empty_exclude_glob_is_a_finding() { + let findings = run("empty-glob"); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("matches no tracked files")), + "{findings:?}" + ); +} + +#[test] +fn env_level_limit_with_direct_group_is_a_finding() { + let findings = run("env-limit"); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("#9440")), + "{findings:?}" + ); +} + +#[test] +fn env_all_with_limit_is_not_a_direct_group_conflict() { + let root = fixture("pass"); + let config = enable( + serde_yaml::from_str( + "testPlan:\n vitest:\n environments:\n prePush:\n all: true\n limit:\n files: 10\n", + ) + .unwrap(), + ); + let findings = check_with_files(&root, &config, &files(&root)).unwrap(); + assert!( + findings + .iter() + .all(|finding| !finding.message.contains("#9440")), + "{findings:?}" + ); +} + +#[test] +fn omitted_groups_still_flag_env_level_limit() { + let root = fixture("pass"); + let config = enable( + serde_yaml::from_str( + "testPlan:\n vitest:\n environments:\n prePush:\n limit:\n files: 10\n", + ) + .unwrap(), + ); + let findings = check_with_files(&root, &config, &files(&root)).unwrap(); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("#9440")), + "{findings:?}" + ); +} + +#[test] +fn valid_config_is_silent() { + assert!(run("pass").is_empty(), "{:?}", run("pass")); +} + +#[test] +fn disabled_rule_is_silent() { + let root = fixture("missing-path"); + let config: NoMistakesConfig = + serde_yaml::from_str(&std::fs::read_to_string(root.join(".no-mistakes.yml")).unwrap()) + .unwrap(); + assert!(check_with_files(&root, &config, &files(&root)) + .unwrap() + .is_empty()); +} + +#[test] +fn repo_root_dot_is_a_present_directory() { + let root = fixture("pass"); + let config = enable(serde_yaml::from_str("projects:\n app:\n root: .\n").unwrap()); + assert!( + check_with_files(&root, &config, &files(&root)) + .unwrap() + .iter() + .all(|finding| !finding.message.contains("projects.app.root")), + "{:?}", + check_with_files(&root, &config, &files(&root)).unwrap() + ); +} + +#[test] +fn percent_limit_and_runner_paths_are_linted() { + let root = fixture("pass"); + let config = enable( + serde_yaml::from_str( + r#" +frontendRoot: web +projects: + web: + root: web + include: ["web/src/**"] + exclude: ["no-such-dir/**"] +tests: + playwright: + configs: [vitest.config.ts, ""] + selectorRoots: [web] + frontendRoot: web + navigationHelpers: [vitest.config.ts] + vitest: + configs: vitest.config.ts + jest: + configs: vitest.config.ts + storybook: + configs: vitest.config.ts + swift: + packages: [web] +testPlan: + vitest: + fullSuiteTriggers: + - name: sources + paths: + - web/src/** + - vitest.config.ts + environments: + prePush: + include: ["web/src/**"] + exclude: [""] + limit: + percent: 40 +"#, + ) + .unwrap(), + ); + let findings = check_with_files(&root, &config, &files(&root)).unwrap(); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("no-such-dir") + && finding.message.contains("matches no tracked files")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("#9440")), + "{findings:?}" + ); +} + +#[test] +fn missing_file_and_glob_paths_use_kind_labels() { + let root = fixture("pass"); + let config = enable( + serde_yaml::from_str( + r#" +testPlan: + vitest: + fullSuiteTriggers: + - name: missing + paths: + - no-such-file.ts + - no-such-glob/** +"#, + ) + .unwrap(), + ); + let findings = check_with_files(&root, &config, &files(&root)).unwrap(); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("missing file `no-such-file.ts`")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("missing path `no-such-glob/**`")), + "{findings:?}" + ); +} + +#[test] +fn invalid_include_and_exclude_globs_error() { + let root = fixture("pass"); + let yaml_cases = [ + "testPlan:\n vitest:\n environments:\n prePush:\n include:\n - \"{\"\n", + "testPlan:\n vitest:\n environments:\n prePush:\n exclude:\n - \"{\"\n", + "projects:\n web:\n root: web\n include:\n - \"{\"\n", + "projects:\n web:\n root: web\n exclude:\n - \"{\"\n", + ]; + for yaml in yaml_cases { + let config = enable(serde_yaml::from_str(yaml).unwrap()); + assert!( + check_with_files(&root, &config, &files(&root)).is_err(), + "{yaml}" + ); + } + let mut config = enable(serde_yaml::from_str("projects:\n web:\n root: web\n").unwrap()); + config.rules[0].include = vec!["{".to_string()]; + assert!(check_with_files(&root, &config, &files(&root)).is_err()); + config.rules[0].include.clear(); + config.rules[0].exclude = vec!["{".to_string()]; + assert!(check_with_files(&root, &config, &files(&root)).is_err()); +} + +#[test] +fn unconfigured_rule_reports_nothing() { + let root = fixture("missing-path"); + let findings = check_with_files(&root, &NoMistakesConfig::default(), &files(&root)).unwrap(); + assert!(findings.is_empty(), "{findings:?}"); +} + +#[test] +fn config_rel_falls_back_without_a_discovered_manifest() { + let root = fixture("missing-path"); + let findings = check_with_files(&root, &load(&root), &[]).unwrap(); + assert!( + findings + .iter() + .any(|finding| finding.file == ".no-mistakes.yml"), + "{findings:?}" + ); +} diff --git a/crates/no-mistakes/src/codebase/rules/workflow_topology_policy.rs b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy.rs new file mode 100644 index 000000000..daf23b8fe --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy.rs @@ -0,0 +1,94 @@ +use super::RuleFinding; +use crate::config::v2::NoMistakesConfig; +use anyhow::Result; +use serde::Deserialize; +use std::collections::BTreeMap; +use std::path::{Path, PathBuf}; + +mod evaluate; +mod evaluate_graph; +mod evaluate_steps; + +pub const RULE_ID: &str = "workflow-topology-policy"; + +#[derive(Deserialize, Default)] +#[serde(default, rename_all = "camelCase")] +pub(crate) struct Options { + pub(crate) job_inventory: BTreeMap>, + pub(crate) unlocked_workflow_reasons: BTreeMap, + pub(crate) required_jobs: Vec, + pub(crate) forbidden_jobs: Vec, + pub(crate) required_direct_edges: Vec<[String; 2]>, + pub(crate) forbidden_direct_edges: Vec<[String; 2]>, + pub(crate) required_transitive_edges: Vec<[String; 2]>, + pub(crate) forbidden_transitive_edges: Vec<[String; 2]>, + pub(crate) required_artifact_edges: Vec, + pub(crate) exact_fan_ins: BTreeMap>, + pub(crate) exact_caller_jobs: BTreeMap>, + pub(crate) step_orders: Vec, +} + +#[derive(Deserialize, Default, Clone)] +#[serde(default, rename_all = "camelCase")] +pub(crate) struct ArtifactEdgeRule { + pub(crate) from: String, + pub(crate) to: String, + pub(crate) name: String, + #[serde(rename = "match")] + pub(crate) match_kind: Option, +} + +#[derive(Deserialize, Default, Clone)] +#[serde(default, rename_all = "camelCase")] +pub(crate) struct StepOrderRule { + pub(crate) job_id: String, + pub(crate) steps: Vec, +} + +#[derive(Deserialize, Default, Clone)] +#[serde(default, rename_all = "camelCase")] +pub(crate) struct StepSelector { + pub(crate) id: Option, + pub(crate) uses: Option, + pub(crate) name: Option, +} + +pub(crate) fn check_with_files( + root: &Path, + config: &NoMistakesConfig, + all_files: &[PathBuf], +) -> Result> { + let sources = super::source_store_for_files(all_files); + check_with_files_and_sources(root, config, all_files, &sources) +} + +pub(crate) fn check_with_files_and_sources( + root: &Path, + config: &NoMistakesConfig, + _all_files: &[PathBuf], + _sources: &crate::codebase::ts_source::SourceStore, +) -> Result> { + let mut findings = Vec::new(); + for rule in config.rule_applications(RULE_ID) { + let opts: Options = rule.rule_options(); + let topology = + crate::codebase::workflow_topology::load_workflow_topology(root, &config.ci, &[]); + findings.extend(evaluate::lint(&topology, &opts)); + } + super::sort_findings(&mut findings); + Ok(findings) +} + +pub(super) fn finding(message: String) -> RuleFinding { + RuleFinding { + rule: RULE_ID.to_string(), + file: ".github/workflows".to_string(), + line: 1, + message, + import: None, + target: None, + } +} + +#[cfg(test)] +mod tests; diff --git a/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate.rs b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate.rs new file mode 100644 index 000000000..878fbfb2c --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate.rs @@ -0,0 +1,162 @@ +use super::{finding, Options}; +use crate::codebase::rules::RuleFinding; +use crate::codebase::workflow_topology::model::WorkflowTopology; +use std::collections::{BTreeMap, BTreeSet}; + +pub(super) struct Index<'a> { + pub(super) jobs: + BTreeMap<&'a str, &'a crate::codebase::workflow_topology::model::WorkflowJobNode>, + pub(super) workflows: + BTreeMap<&'a str, &'a crate::codebase::workflow_topology::model::WorkflowNode>, + downstream: BTreeMap<&'a str, BTreeSet<&'a str>>, + upstream: BTreeMap<&'a str, BTreeSet<&'a str>>, + caller_jobs: BTreeMap<&'a str, BTreeSet<&'a str>>, +} + +impl<'a> Index<'a> { + pub(super) fn new(topology: &'a WorkflowTopology) -> Self { + let jobs: BTreeMap<&str, _> = topology + .jobs + .iter() + .map(|job| (job.id.as_str(), job)) + .collect(); + let workflows: BTreeMap<&str, _> = topology + .workflows + .iter() + .map(|workflow| (workflow.path.as_str(), workflow)) + .collect(); + let mut downstream: BTreeMap<&str, BTreeSet<&str>> = jobs + .keys() + .copied() + .map(|id| (id, BTreeSet::new())) + .collect(); + let mut upstream = downstream.clone(); + let mut caller_jobs: BTreeMap<&str, BTreeSet<&str>> = workflows + .keys() + .copied() + .map(|path| (path, BTreeSet::new())) + .collect(); + for edge in &topology.edges { + match edge { + crate::codebase::workflow_topology::model::WorkflowTopologyEdge::Needs(needs) => { + if let Some(set) = downstream.get_mut(needs.from.as_str()) { + set.insert(needs.to.as_str()); + } + if let Some(set) = upstream.get_mut(needs.to.as_str()) { + set.insert(needs.from.as_str()); + } + } + crate::codebase::workflow_topology::model::WorkflowTopologyEdge::Calls(call) + if call.local => + { + if let Some(to) = call.to.as_deref() { + if let Some(set) = caller_jobs.get_mut(to) { + set.insert(call.from.as_str()); + } + } + } + _ => {} + } + } + Self { + jobs, + workflows, + downstream, + upstream, + caller_jobs, + } + } + + pub(super) fn direct_downstream(&self, job: &str) -> Vec<&str> { + sorted(self.downstream.get(job)) + } + + pub(super) fn transitive_downstream(&self, job: &str) -> Vec<&str> { + let mut visited = BTreeSet::new(); + let mut pending: Vec<&str> = self.direct_downstream(job); + while let Some(current) = pending.pop() { + if current == job || !visited.insert(current) { + continue; + } + pending.extend(self.direct_downstream(current)); + } + visited.into_iter().collect() + } + + pub(super) fn direct_upstream(&self, job: &str) -> Vec<&str> { + sorted(self.upstream.get(job)) + } + + pub(super) fn direct_caller_jobs(&self, workflow: &str) -> Vec<&str> { + sorted(self.caller_jobs.get(workflow)) + } +} + +fn sorted<'a>(set: Option<&BTreeSet<&'a str>>) -> Vec<&'a str> { + set.map(|values| values.iter().copied().collect()) + .unwrap_or_default() +} + +pub(super) fn lint(topology: &WorkflowTopology, opts: &Options) -> Vec { + let index = Index::new(topology); + let mut findings = Vec::new(); + if !opts.job_inventory.is_empty() { + findings.extend(inventory(topology, opts)); + } + findings.extend(job_presence(&index, opts)); + findings.extend(super::evaluate_graph::lint(topology, &index, opts)); + findings.extend(super::evaluate_steps::lint(topology, &index, opts)); + findings +} + +fn inventory(topology: &WorkflowTopology, opts: &Options) -> Vec { + let mut findings = Vec::new(); + let mut actual = BTreeMap::new(); + for workflow in &topology.workflows { + let keys: Vec = workflow + .job_ids + .iter() + .filter_map(|id| id.rsplit_once('#').map(|(_, key)| key.to_string())) + .collect(); + actual.insert(workflow.path.clone(), keys); + } + for (path, keys) in &actual { + match opts.job_inventory.get(path) { + None => findings.push(finding(format!("workflow inventory missing: {path}"))), + Some(expected) => { + let mut expected = expected.clone(); + expected.sort(); + let mut got = keys.clone(); + got.sort(); + if expected != got { + findings.push(finding(format!( + "job inventory mismatch: {path}: expected {}, got {}", + expected.join(", "), + got.join(", ") + ))); + } + } + } + } + for path in opts.job_inventory.keys() { + if !actual.contains_key(path) { + findings.push(finding(format!("workflow inventory stale: {path}"))); + } + } + findings +} + +fn job_presence(index: &Index<'_>, opts: &Options) -> Vec { + let mut findings = Vec::new(); + for id in &opts.required_jobs { + if !index.jobs.contains_key(id.as_str()) { + findings.push(finding(format!("required job missing: {id}"))); + } + } + for id in &opts.forbidden_jobs { + if index.jobs.contains_key(id.as_str()) { + findings.push(finding(format!("forbidden job present: {id}"))); + } + } + findings +} diff --git a/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate_graph.rs b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate_graph.rs new file mode 100644 index 000000000..f0d57168a --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate_graph.rs @@ -0,0 +1,156 @@ +use super::evaluate::Index; +use super::{finding, Options}; +use crate::codebase::rules::RuleFinding; +use crate::codebase::workflow_topology::model::WorkflowTopology; + +pub(super) fn lint( + topology: &WorkflowTopology, + index: &Index<'_>, + opts: &Options, +) -> Vec { + let mut findings = Vec::new(); + findings.extend(edges(index, &opts.required_direct_edges, true, true)); + findings.extend(edges(index, &opts.forbidden_direct_edges, true, false)); + findings.extend(edges(index, &opts.required_transitive_edges, false, true)); + findings.extend(edges(index, &opts.forbidden_transitive_edges, false, false)); + findings.extend(fan_ins(index, opts)); + findings.extend(callers(index, opts)); + findings.extend(unlocked(topology, opts)); + findings +} + +fn edges( + index: &Index<'_>, + rules: &[[String; 2]], + direct: bool, + required: bool, +) -> Vec { + let kind = if direct { "direct" } else { "transitive" }; + rules + .iter() + .filter_map(|[from, to]| { + if !index.jobs.contains_key(from.as_str()) || !index.jobs.contains_key(to.as_str()) { + return required + .then(|| finding(format!("required {kind} edge missing: {from} -> {to}"))); + } + let downstream = if direct { + index.direct_downstream(from) + } else { + index.transitive_downstream(from) + }; + let present = downstream.contains(&to.as_str()); + (present != required).then(|| { + finding(format!( + "{} {kind} edge {}: {from} -> {to}", + if required { "required" } else { "forbidden" }, + if required { "missing" } else { "present" } + )) + }) + }) + .collect() +} + +fn fan_ins(index: &Index<'_>, opts: &Options) -> Vec { + opts.exact_fan_ins + .iter() + .map(|(job_id, expected)| { + if !index.jobs.contains_key(job_id.as_str()) { + return finding(format!("exact fan-in target missing: {job_id}")); + } + let actual = index.direct_upstream(job_id); + let mut expected: Vec<&str> = expected.iter().map(String::as_str).collect(); + expected.sort_unstable(); + if actual == expected { + return finding(String::new()); + } + finding(format!( + "exact fan-in mismatch: {job_id}: expected {}, got {}", + expected.join(", "), + actual.join(", ") + )) + }) + .filter(|finding| !finding.message.is_empty()) + .collect() +} + +fn callers(index: &Index<'_>, opts: &Options) -> Vec { + if opts.exact_caller_jobs.is_empty() { + return Vec::new(); + } + let callable: Vec<&str> = index + .workflows + .values() + .filter(|workflow| workflow.callable) + .map(|workflow| workflow.path.as_str()) + .collect(); + let mut findings = Vec::new(); + for path in &callable { + if !opts.exact_caller_jobs.contains_key(*path) { + findings.push(finding(format!("caller allowlist missing: {path}"))); + } + } + for path in opts.exact_caller_jobs.keys() { + if !callable.contains(&path.as_str()) { + findings.push(finding(format!("caller allowlist stale: {path}"))); + } + } + for (path, expected) in &opts.exact_caller_jobs { + if !index + .workflows + .get(path.as_str()) + .is_some_and(|workflow| workflow.callable) + { + continue; + } + let actual = index.direct_caller_jobs(path); + let mut expected: Vec<&str> = expected.iter().map(String::as_str).collect(); + expected.sort_unstable(); + if actual != expected { + findings.push(finding(format!( + "caller allowlist mismatch: {path}: expected {}, got {}", + expected.join(", "), + actual.join(", ") + ))); + } + } + findings +} + +fn unlocked(topology: &WorkflowTopology, opts: &Options) -> Vec { + if opts.unlocked_workflow_reasons.is_empty() { + return Vec::new(); + } + let mut findings = Vec::new(); + let jobs: std::collections::HashMap<&str, _> = topology + .jobs + .iter() + .map(|job| (job.id.as_str(), job)) + .collect(); + for workflow in &topology.workflows { + let reason = opts.unlocked_workflow_reasons.get(&workflow.path); + let has_unlocked_job = workflow.concurrency.is_none() + && workflow.job_ids.iter().any(|id| { + jobs.get(id.as_str()) + .is_some_and(|job| job.concurrency.is_none()) + }); + if has_unlocked_job && reason.is_none() { + findings.push(finding(format!("lock intent missing: {}", workflow.path))); + } + if !has_unlocked_job && reason.is_some() { + findings.push(finding(format!("unlocked reason stale: {}", workflow.path))); + } + if reason.is_some_and(|value| value.trim().is_empty()) { + findings.push(finding(format!("unlocked reason empty: {}", workflow.path))); + } + } + for path in opts.unlocked_workflow_reasons.keys() { + if !topology + .workflows + .iter() + .any(|workflow| workflow.path == *path) + { + findings.push(finding(format!("unlocked reason stale: {path}"))); + } + } + findings +} diff --git a/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate_steps.rs b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate_steps.rs new file mode 100644 index 000000000..8b3010909 --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/evaluate_steps.rs @@ -0,0 +1,114 @@ +use super::evaluate::Index; +use super::{finding, Options, StepSelector}; +use crate::codebase::rules::RuleFinding; +use crate::codebase::workflow_topology::model::{ + WorkflowStep, WorkflowTopology, WorkflowTopologyEdge, +}; + +pub(super) fn lint( + topology: &WorkflowTopology, + index: &Index<'_>, + opts: &Options, +) -> Vec { + let mut findings = artifact_edges(topology, index, opts); + findings.extend(step_orders(index, opts)); + findings +} + +fn artifact_edges( + topology: &WorkflowTopology, + index: &Index<'_>, + opts: &Options, +) -> Vec { + opts.required_artifact_edges + .iter() + .map(|rule| { + let label = match &rule.match_kind { + Some(kind) => format!("{} -> {}: {} [{kind}]", rule.from, rule.to, rule.name), + None => format!("{} -> {}: {}", rule.from, rule.to, rule.name), + }; + if !index.jobs.contains_key(rule.from.as_str()) + || !index.jobs.contains_key(rule.to.as_str()) + { + return finding(format!("required artifact edge missing: {label}")); + } + let present = topology.edges.iter().any(|edge| match edge { + WorkflowTopologyEdge::Artifact(artifact) => { + artifact.from == rule.from + && artifact.to == rule.to + && artifact.name == rule.name + && rule + .match_kind + .as_deref() + .is_none_or(|kind| artifact.match_kind.as_str() == kind) + } + _ => false, + }); + if present { + finding(String::new()) + } else { + finding(format!("required artifact edge missing: {label}")) + } + }) + .filter(|finding| !finding.message.is_empty()) + .collect() +} + +fn step_orders(index: &Index<'_>, opts: &Options) -> Vec { + opts.step_orders + .iter() + .filter_map(|rule| { + let Some(job) = index.jobs.get(rule.job_id.as_str()) else { + return Some(finding(format!("step-order job missing: {}", rule.job_id))); + }; + let mut prior = -1i32; + for selector in &rule.steps { + let Some(step) = job + .steps + .iter() + .find(|candidate| matches_selector(selector, candidate)) + else { + let label = selector + .id + .as_deref() + .or(selector.uses.as_deref()) + .or(selector.name.as_deref()) + .unwrap_or(""); + return Some(finding(format!( + "required ordered step missing: {}: {label}", + rule.job_id + ))); + }; + if (step.index as i32) <= prior { + let label = selector + .id + .as_deref() + .or(selector.uses.as_deref()) + .or(selector.name.as_deref()) + .unwrap_or(""); + return Some(finding(format!( + "required step order invalid: {}: {label}", + rule.job_id + ))); + } + prior = step.index as i32; + } + None + }) + .collect() +} + +fn matches_selector(selector: &StepSelector, step: &WorkflowStep) -> bool { + selector + .id + .as_deref() + .is_none_or(|id| step.id.as_deref() == Some(id)) + && selector + .uses + .as_deref() + .is_none_or(|uses| step.uses.as_deref() == Some(uses)) + && selector + .name + .as_deref() + .is_none_or(|name| step.name.as_deref() == Some(name)) +} diff --git a/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/tests.rs b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/tests.rs new file mode 100644 index 000000000..6dc665506 --- /dev/null +++ b/crates/no-mistakes/src/codebase/rules/workflow_topology_policy/tests.rs @@ -0,0 +1,269 @@ +use super::*; +use crate::config::v2::{ + schema::{RuleDef, RuleScope}, + NoMistakesConfig, +}; +use std::path::{Path, PathBuf}; + +fn fixture(name: &str) -> PathBuf { + crate::codebase::ts_resolver::normalize_path( + &PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("../../test-cases/rules/workflow-topology-policy/fixture") + .join(name), + ) +} + +fn config(yaml: &str) -> NoMistakesConfig { + NoMistakesConfig { + rules: vec![RuleDef { + rule: RULE_ID.to_string(), + scope: Some(RuleScope::Repository), + options: serde_yaml::from_str(yaml).unwrap(), + ..Default::default() + }], + ..Default::default() + } +} + +fn run(root: &Path, yaml: &str) -> Vec { + check_with_files(root, &config(yaml), &[]).unwrap() +} + +#[test] +fn required_direct_edge_is_enforced() { + let findings = run( + &fixture("needs-basic"), + r#" +requiredDirectEdges: + - [".github/workflows/pipeline.yml#build", ".github/workflows/pipeline.yml#missing"] +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("required direct edge missing")), + "{findings:?}" + ); +} + +#[test] +fn matching_inventory_and_edges_pass() { + let findings = run( + &fixture("needs-basic"), + r#" +jobInventory: + .github/workflows/pipeline.yml: [build, test, deploy] +requiredDirectEdges: + - [".github/workflows/pipeline.yml#build", ".github/workflows/pipeline.yml#test"] +"#, + ); + assert!(findings.is_empty(), "{findings:?}"); +} + +#[test] +fn step_order_reports_missing_steps() { + let findings = run( + &fixture("needs-basic"), + r#" +stepOrders: + - jobId: ".github/workflows/pipeline.yml#build" + steps: + - uses: actions/checkout@v4 +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("required ordered step missing")), + "{findings:?}" + ); +} + +#[test] +fn required_job_missing_and_forbidden_job_present() { + let findings = run( + &fixture("needs-basic"), + r#" +requiredJobs: [".github/workflows/pipeline.yml#ghost"] +forbiddenJobs: [".github/workflows/pipeline.yml#build"] +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("required job missing")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("forbidden job present")), + "{findings:?}" + ); +} + +#[test] +fn forbidden_direct_and_required_transitive_edges() { + let findings = run( + &fixture("needs-basic"), + r#" +forbiddenDirectEdges: + - [".github/workflows/pipeline.yml#build", ".github/workflows/pipeline.yml#test"] +requiredTransitiveEdges: + - [".github/workflows/pipeline.yml#build", ".github/workflows/pipeline.yml#ghost"] +forbiddenTransitiveEdges: + - [".github/workflows/pipeline.yml#build", ".github/workflows/pipeline.yml#deploy"] +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("forbidden direct edge present")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("required transitive edge missing")), + "{findings:?}" + ); + assert!( + findings.iter().any(|finding| finding + .message + .contains("forbidden transitive edge present")), + "{findings:?}" + ); +} + +#[test] +fn inventory_mismatch_stale_and_missing() { + let findings = run( + &fixture("needs-basic"), + r#" +jobInventory: + .github/workflows/pipeline.yml: [build] + .github/workflows/missing.yml: [job] +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("job inventory mismatch")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("workflow inventory stale")), + "{findings:?}" + ); +} + +#[test] +fn exact_fan_in_mismatch_and_missing_target() { + let findings = run( + &fixture("needs-basic"), + r#" +exactFanIns: + ".github/workflows/pipeline.yml#deploy": [".github/workflows/pipeline.yml#build"] + ".github/workflows/pipeline.yml#ghost": [] +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("exact fan-in mismatch")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("exact fan-in target missing")), + "{findings:?}" + ); +} + +#[test] +fn exact_fan_in_matches() { + let findings = run( + &fixture("needs-basic"), + r#" +exactFanIns: + ".github/workflows/pipeline.yml#test": [".github/workflows/pipeline.yml#build"] +"#, + ); + assert!(findings.is_empty(), "{findings:?}"); +} + +#[test] +fn caller_allowlist_stale_for_non_callable() { + let findings = run( + &fixture("needs-basic"), + r#" +exactCallerJobs: + .github/workflows/missing.yml: [] +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("caller allowlist stale")), + "{findings:?}" + ); +} + +#[test] +fn lock_intent_missing_and_stale_reason() { + let findings = run( + &fixture("needs-basic"), + r#" +unlockedWorkflowReasons: + .github/workflows/missing.yml: " " +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("lock intent missing")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("unlocked reason stale")), + "{findings:?}" + ); +} + +#[test] +fn artifact_edge_and_step_order_job_missing() { + let findings = run( + &fixture("needs-basic"), + r#" +requiredArtifactEdges: + - from: ".github/workflows/pipeline.yml#build" + to: ".github/workflows/pipeline.yml#test" + name: dist + match: prefix + - from: ".github/workflows/pipeline.yml#ghost" + to: ".github/workflows/pipeline.yml#test" + name: dist +stepOrders: + - jobId: ".github/workflows/pipeline.yml#ghost" + steps: + - name: build +"#, + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("required artifact edge missing")), + "{findings:?}" + ); + assert!( + findings + .iter() + .any(|finding| finding.message.contains("step-order job missing")), + "{findings:?}" + ); +} diff --git a/crates/no-mistakes/tests/docs_coverage.rs b/crates/no-mistakes/tests/docs_coverage.rs index 063b5c72e..ba5574255 100644 --- a/crates/no-mistakes/tests/docs_coverage.rs +++ b/crates/no-mistakes/tests/docs_coverage.rs @@ -250,6 +250,7 @@ fn no_mistakes_rules_have_docs() { rules::NEXTJS_REDIRECT_DESTINATIONS, rules::NO_EMPTY_OR_COMMENTS_ONLY_FILES, rules::NO_GIT_IDENTITY_MUTATION, + rules::NO_MISTAKES_CONFIG, rules::NO_RAW_EPHEMERAL_PORT, rules::PACKAGE_JSON_REGISTRY_ONLY, rules::POSTGRES_CONSTRAINT_VALIDATE, @@ -288,6 +289,7 @@ fn no_mistakes_rules_have_docs() { rules::TSCONFIG_GATE_COVERAGE, unique_exports::RULE_ID, rules::VITEST_TEST_CORRESPONDENCE, + rules::WORKFLOW_TOPOLOGY_POLICY, ]; for rule_id in rule_ids { let file = format!("{rule_id}.md"); diff --git a/docs/rules/README.md b/docs/rules/README.md index 2887ef4e1..28d6cccc3 100644 --- a/docs/rules/README.md +++ b/docs/rules/README.md @@ -49,6 +49,7 @@ rules: | [`nextjs-redirect-destinations`](nextjs-redirect-destinations.md) | Require Next.js redirect/rewrite destinations to match App Router pages. | | [`no-empty-or-comments-only-files`](no-empty-or-comments-only-files.md) | Ban empty/comment-only files. | | [`no-git-identity-mutation`](no-git-identity-mutation.md) | Ban scripts that mutate git identity. | +| [`no-mistakes-config`](no-mistakes-config.md) | Lint `.no-mistakes.yml` paths, empty globs, and env-level `limit` with `direct`. | | [`no-raw-ephemeral-port`](no-raw-ephemeral-port.md) | Ban raw ephemeral port 0 binds and Node `listen(0)` calls. | | [`package-json-registry-only`](package-json-registry-only.md) | Require package registries to match configured policy. | | [`package-json-workspace-coverage`](package-json-workspace-coverage.md) | Require package directories to be covered by workspace config. | @@ -93,6 +94,7 @@ rules: | [`vitest-ci-path-coverage`](vitest-ci-path-coverage.md) | Require Vitest inputs to be covered by CI path filters. | | [`vitest-project-mapping`](vitest-project-mapping.md) | Require Vitest tests to map to exactly one project. | | [`vitest-test-correspondence`](vitest-test-correspondence.md) | Enforce source/test correspondence for Vitest. | +| [`workflow-topology-policy`](workflow-topology-policy.md) | Declarative GitHub Actions topology inventory, edges, and step order. | | [`workspace-package-cycles`](workspace-package-cycles.md) | Prevent dependency cycles between workspace packages. | ## Suppression diff --git a/docs/rules/no-mistakes-config.md b/docs/rules/no-mistakes-config.md new file mode 100644 index 000000000..349a3d2a5 --- /dev/null +++ b/docs/rules/no-mistakes-config.md @@ -0,0 +1,50 @@ +# `no-mistakes-config` + +Lints the loaded `.no-mistakes.yml` against tracked files: schema path +fields must exist, ignore/exclude globs must match something, and an +environment-level `limit` must not share a budget with a `direct` group +(issue #9440). + +```yaml +rules: + - rule: no-mistakes-config + scope: repository +``` + +Counterexample: `projects.web.root` points at a directory that is not in +the tree, an `exclude` glob matches nothing, or `prePush` sets `limit` +while also listing a `direct` group. + +```yaml +projects: + web: + root: apps/missing +testPlan: + vitest: + environments: + prePush: + limit: + files: 10 + exclude: ["gone/**"] + groups: + - type: direct +``` + +Fix: point path fields at tracked files or directories, delete empty +globs, and move the budget onto non-`direct` groups so changed tests are +not dropped. + +```yaml +projects: + web: + root: web +testPlan: + vitest: + environments: + prePush: + groups: + - type: direct + - type: dependencies + limit: + files: 10 +``` diff --git a/docs/rules/workflow-topology-policy.md b/docs/rules/workflow-topology-policy.md new file mode 100644 index 000000000..b07f5b53d --- /dev/null +++ b/docs/rules/workflow-topology-policy.md @@ -0,0 +1,60 @@ +# `workflow-topology-policy` + +Declarative GitHub Actions topology assertions over the graph produced by +`ciTopology()` / `createWorkflowTopologyIndex()`. Configure inventory, +required and forbidden jobs and `needs` edges, artifact edges, exact +fan-in, reusable-workflow callers, step order, and unlocked-workflow +reasons. + +```yaml +rules: + - rule: workflow-topology-policy + scope: repository + options: + jobInventory: + .github/workflows/ci.yml: [lint, test] + requiredDirectEdges: + - [".github/workflows/ci.yml#lint", ".github/workflows/ci.yml#test"] + stepOrders: + - jobId: ".github/workflows/ci.yml#lint" + steps: + - uses: actions/checkout@v4 + unlockedWorkflowReasons: + .github/workflows/ci.yml: "single-job lint has no overlapping work" +``` + +`jobInventory`, `exactCallerJobs`, and `unlockedWorkflowReasons` are +checked only when those maps are non-empty so a rule can assert a single +edge without listing every workflow. + +Counterexample: `test` does not `needs: lint`, or a required step is +missing. + +```yaml +jobs: + lint: + runs-on: ubuntu-slim + steps: + - run: pnpm lint + test: + runs-on: ubuntu-slim + steps: + - run: pnpm test +``` + +Fix: add the `needs` edge and ordered steps the policy names, or update +the YAML options to match the intended graph. + +```yaml +jobs: + lint: + runs-on: ubuntu-slim + steps: + - uses: actions/checkout@v4 + - run: pnpm lint + test: + needs: lint + runs-on: ubuntu-slim + steps: + - run: pnpm test +``` diff --git a/packages/no-mistakes/package.json b/packages/no-mistakes/package.json index 0257ca26d..00173f0c5 100644 --- a/packages/no-mistakes/package.json +++ b/packages/no-mistakes/package.json @@ -18,6 +18,7 @@ "index.mjs", "planning.js", "workflow-topology-index.js", + "workflow-topology-index-helpers.js", "scripts/install.js", "scripts/install/", "README.md", diff --git a/packages/no-mistakes/scripts/workflow-topology-index.test.js b/packages/no-mistakes/scripts/workflow-topology-index.test.js index 6394454d8..d31ece50b 100644 --- a/packages/no-mistakes/scripts/workflow-topology-index.test.js +++ b/packages/no-mistakes/scripts/workflow-topology-index.test.js @@ -142,3 +142,11 @@ test("deep-freezes enriched job and step metadata", () => { assert.deepEqual(groupOnlyJob.runsOn, { group: "restricted" }); assert.equal(Object.isFrozen(groupOnlyJob.runsOn), true); }); + +test("directCallerJobIdsForUses and stepOrderIndexes query steps", () => { + const index = createWorkflowTopologyIndex(fixtureTopology("needs-basic")); + const build = ".github/workflows/pipeline.yml#build"; + assert.deepEqual(index.directCallerJobIdsForUses("actions/checkout@v4"), []); + assert.deepEqual(index.stepOrderIndexes(build, [{ name: "missing" }]), [-1]); + assert.throws(() => index.stepOrderIndexes("nope", []), /unknown workflow job: nope/); +}); diff --git a/packages/no-mistakes/workflow-topology-index-helpers.js b/packages/no-mistakes/workflow-topology-index-helpers.js new file mode 100644 index 000000000..d1186b55e --- /dev/null +++ b/packages/no-mistakes/workflow-topology-index-helpers.js @@ -0,0 +1,28 @@ +"use strict"; + +function stepMatches(selector, step) { + return ( + (selector.id === undefined || selector.id === step.id) && + (selector.uses === undefined || selector.uses === step.uses) && + (selector.name === undefined || selector.name === step.name) + ); +} + +function directCallerJobIdsForUses(jobsById, uses) { + const ids = []; + for (const job of jobsById.values()) { + if (job.steps.some((step) => step.uses === uses)) ids.push(job.id); + } + return Object.freeze(ids.sort()); +} + +function stepOrderIndexes(job, selectors) { + return Object.freeze( + selectors.map((selector) => { + const step = job.steps.find((candidate) => stepMatches(selector, candidate)); + return step == null ? -1 : step.index; + }), + ); +} + +module.exports = { directCallerJobIdsForUses, stepOrderIndexes }; diff --git a/packages/no-mistakes/workflow-topology-index-types.d.ts b/packages/no-mistakes/workflow-topology-index-types.d.ts index e0e6abc8b..81fb28a45 100644 --- a/packages/no-mistakes/workflow-topology-index-types.d.ts +++ b/packages/no-mistakes/workflow-topology-index-types.d.ts @@ -24,6 +24,13 @@ export interface WorkflowTopologyIndex { directDownstreamJobIds(jobId: string): readonly string[]; transitiveDownstreamJobIds(jobId: string): readonly string[]; directCallerJobIds(workflowPath: string): readonly string[]; + /** Job ids whose steps `uses:` this action or workflow path. */ + directCallerJobIdsForUses(uses: string): readonly string[]; + /** Step indexes for `selectors` in `jobId`, or `-1` when a selector is missing. */ + stepOrderIndexes( + jobId: string, + selectors: ReadonlyArray<{ id?: string; uses?: string; name?: string }>, + ): readonly number[]; directCallerWorkflowPaths(workflowPath: string): readonly string[]; transitiveCallerWorkflowPaths(workflowPath: string): readonly string[]; directCalleeWorkflowPaths(workflowPath: string): readonly string[]; diff --git a/packages/no-mistakes/workflow-topology-index.js b/packages/no-mistakes/workflow-topology-index.js index 347da8f77..0071002f3 100644 --- a/packages/no-mistakes/workflow-topology-index.js +++ b/packages/no-mistakes/workflow-topology-index.js @@ -1,5 +1,10 @@ "use strict"; +const { + directCallerJobIdsForUses, + stepOrderIndexes, +} = require("./workflow-topology-index-helpers"); + // A pure-JS query index rebuilt from the `ciTopology()` JSON, ported from // the original engine's `topology-index.mts` + `frozen-topology.mts`. This // stays JS-only by design: it returns closures over frozen `Map`s, which @@ -214,6 +219,11 @@ function createWorkflowTopologyIndex(topology) { transitiveWorkflowRunSubscriberPaths: workflowQuery(workflowRunSubscribers, true), artifactProducersForConsumerJob: artifactEdgeQuery(artifactProducers), artifactConsumersForProducerJob: artifactEdgeQuery(artifactConsumers), + directCallerJobIdsForUses: (uses) => directCallerJobIdsForUses(jobsById, uses), + stepOrderIndexes: (jobId, selectors) => { + assertKnown(jobsById, jobId, "workflow job"); + return stepOrderIndexes(jobsById.get(jobId), selectors); + }, }); } diff --git a/test-cases/rules/no-mistakes-config/fixture/empty-glob/.no-mistakes.yml b/test-cases/rules/no-mistakes-config/fixture/empty-glob/.no-mistakes.yml new file mode 100644 index 000000000..6e30b1e7b --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/empty-glob/.no-mistakes.yml @@ -0,0 +1,7 @@ +testPlan: + vitest: + environments: + prePush: + exclude: ["does-not-exist/**/*.ts"] + groups: + - type: dependencies diff --git a/test-cases/rules/no-mistakes-config/fixture/empty-glob/web/src/index.ts b/test-cases/rules/no-mistakes-config/fixture/empty-glob/web/src/index.ts new file mode 100644 index 000000000..d45aa497b --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/empty-glob/web/src/index.ts @@ -0,0 +1 @@ +export const ok = true diff --git a/test-cases/rules/no-mistakes-config/fixture/env-limit/.no-mistakes.yml b/test-cases/rules/no-mistakes-config/fixture/env-limit/.no-mistakes.yml new file mode 100644 index 000000000..d0fab50d2 --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/env-limit/.no-mistakes.yml @@ -0,0 +1,11 @@ +testPlan: + vitest: + environments: + prePush: + limit: + files: 10 + groups: + - type: direct + - type: dependencies + limit: + files: 10 diff --git a/test-cases/rules/no-mistakes-config/fixture/missing-path/.no-mistakes.yml b/test-cases/rules/no-mistakes-config/fixture/missing-path/.no-mistakes.yml new file mode 100644 index 000000000..e1e4ffaec --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/missing-path/.no-mistakes.yml @@ -0,0 +1,3 @@ +projects: + web: + root: missing-app diff --git a/test-cases/rules/no-mistakes-config/fixture/pass/.no-mistakes.yml b/test-cases/rules/no-mistakes-config/fixture/pass/.no-mistakes.yml new file mode 100644 index 000000000..ebcef8815 --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/pass/.no-mistakes.yml @@ -0,0 +1,7 @@ +projects: + web: + root: web + +tests: + vitest: + configs: vitest.config.ts diff --git a/test-cases/rules/no-mistakes-config/fixture/pass/vitest.config.ts b/test-cases/rules/no-mistakes-config/fixture/pass/vitest.config.ts new file mode 100644 index 000000000..830a07746 --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/pass/vitest.config.ts @@ -0,0 +1 @@ +export default { test: { include: ['web/**/*.test.ts'] } } diff --git a/test-cases/rules/no-mistakes-config/fixture/pass/web/src/index.ts b/test-cases/rules/no-mistakes-config/fixture/pass/web/src/index.ts new file mode 100644 index 000000000..d45aa497b --- /dev/null +++ b/test-cases/rules/no-mistakes-config/fixture/pass/web/src/index.ts @@ -0,0 +1 @@ +export const ok = true diff --git a/test-cases/rules/workflow-topology-policy/fixture/needs-basic/.github/workflows/pipeline.yml b/test-cases/rules/workflow-topology-policy/fixture/needs-basic/.github/workflows/pipeline.yml new file mode 100644 index 000000000..f96469f0c --- /dev/null +++ b/test-cases/rules/workflow-topology-policy/fixture/needs-basic/.github/workflows/pipeline.yml @@ -0,0 +1,18 @@ +name: Needs Basic +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo build + test: + needs: build + runs-on: ubuntu-latest + if: needs.build.result == 'success' + steps: + - run: echo test + deploy: + needs: test + runs-on: ubuntu-latest + steps: + - run: echo deploy diff --git a/tests/js/npm-packages.test.js b/tests/js/npm-packages.test.js index cf1bdf4bf..ce89ab03a 100644 --- a/tests/js/npm-packages.test.js +++ b/tests/js/npm-packages.test.js @@ -54,7 +54,7 @@ test("native npm packages expose direct executable bin targets", () => { test("every local require() from a published entry point is covered by package.json's files list", () => { const packageDir = join(root, "packages", "no-mistakes"); const manifest = JSON.parse(readFileSync(join(packageDir, "package.json"), "utf8")); - const entryPoints = ["index.js", "planning.js"]; + const entryPoints = ["index.js", "planning.js", "workflow-topology-index.js"]; const requirePattern = /require\("\.\/([\w./-]+)"\)/g; const isCovered = (relativePath) =>