From d526dabea11b2bf5f02e544186ec11adee97d470 Mon Sep 17 00:00:00 2001 From: Tom Davidson Date: Sun, 29 Mar 2026 02:42:14 +0000 Subject: [PATCH 1/5] feat: clippy-style rule system refactor with diagnostic improvements Parts A-D of the rule system refactor: Part A - Diagnostic improvements: - Add DiagnosticLabel, primary_span, SuggestedFix, TextEdit, Applicability - Create fix.rs with apply_fixes infrastructure - Rename span -> primary_span across codebase Part B - CLI improvements: - Route text output to stdout (was stderr) - Make 'lint' the default subcommand (actlint [paths] works) - Add stdin detection with --type flag - Document exit codes (0=clean, 1=errors, 2=operational) Part C - Rule refactor: - Add declare_rule! macro and register_rules! registry - Port all rules to 26 flat files with clippy-style architecture - Add LintConfig with disabled_rules, disabled_namespaces, schema config - Replace legacy dispatch with ALL_RULES loop - Delete action/ and workflow/ subdirectories - Update fuzz harnesses for new dispatch - Add shared test harness (run_rule, assert_rule_clean) Part D - Test coverage: - All 26 rules have clean-case tests - workflow_expression_contexts: new tests (was zero coverage) - Diagnostic-quality assertions on key rules - 12 CLI integration tests (assert_cmd + predicates) - 264 total tests (220 unit + 32 integration + 12 CLI) Rule IDs migrated from 'workflow/X' and 'action/X' to 'semantic/workflow-X' and 'semantic/action-X' namespace format. --- Cargo.lock | 110 +++++++ actlint-cli/Cargo.toml | 5 + actlint-cli/src/commands/lint.rs | 164 ++++++++-- actlint-cli/src/main.rs | 51 +++- actlint-cli/tests/cli.rs | 171 +++++++++++ actlint-core/src/diagnostic.rs | 71 ++++- actlint-core/src/fix.rs | 136 +++++++++ actlint-core/src/fuzz_regression.rs | 67 +++-- actlint-core/src/lib.rs | 1 + actlint-core/src/linter.rs | 90 +++++- actlint-core/src/rules/action/expressions.rs | 270 ----------------- actlint-core/src/rules/action/inputs.rs | 170 ----------- actlint-core/src/rules/action/mod.rs | 20 -- actlint-core/src/rules/action/outputs.rs | 72 ----- actlint-core/src/rules/action/runs.rs | 91 ------ actlint-core/src/rules/action/schema.rs | 167 ---------- .../rules/action_boolean_default_invalid.rs | 104 +++++++ .../action_choice_default_not_in_options.rs | 111 +++++++ .../rules/action_choice_requires_options.rs | 89 ++++++ .../rules/action_composite_steps_required.rs | 77 +++++ .../rules/action_docker_image_not_found.rs | 92 ++++++ .../src/rules/action_docker_image_required.rs | 73 +++++ .../src/rules/action_expression_contexts.rs | 194 ++++++++++++ .../src/rules/action_file_not_found.rs | 103 +++++++ .../rules/action_invalid_branding_color.rs | 86 ++++++ ...ing.rs => action_invalid_branding_icon.rs} | 162 +++++----- .../src/rules/action_invalid_input_type.rs | 104 +++++++ .../src/rules/action_invalid_using.rs | 89 ++++++ .../src/rules/action_js_main_required.rs | 73 +++++ .../src/rules/action_name_required.rs | 74 +++++ .../rules/action_output_invalid_step_ref.rs | 120 ++++++++ .../src/rules/action_runs_required.rs | 69 +++++ actlint-core/src/rules/mod.rs | 253 +++++++++++++++- actlint-core/src/rules/workflow/jobs.rs | 284 ------------------ actlint-core/src/rules/workflow/mod.rs | 14 - actlint-core/src/rules/workflow/steps.rs | 112 ------- .../src/rules/workflow_circular_needs.rs | 195 ++++++++++++ .../src/rules/workflow_duplicate_step_id.rs | 86 ++++++ ...ons.rs => workflow_expression_contexts.rs} | 128 ++++++-- .../src/rules/workflow_invalid_job_id.rs | 121 ++++++++ .../src/rules/workflow_invalid_needs_ref.rs | 119 ++++++++ .../src/rules/workflow_invalid_uses.rs | 155 ++++++++++ .../src/rules/workflow_missing_runs_on.rs | 71 +++++ actlint-core/src/rules/workflow_no_jobs.rs | 68 +++++ .../src/rules/workflow_step_no_action.rs | 79 +++++ .../src/rules/workflow_step_uses_and_run.rs | 97 ++++++ actlint-core/tests/integration/edge_cases.rs | 8 +- .../tests/integration/lint_invalid.rs | 36 +-- .../harnesses/parse_document_structured.rs | 30 +- .../harnesses/parse_document_unstructured.rs | 73 +++-- ...t-762713f50f42519f1975cfca4db51e0e6fefec1a | 1 + ...t-9fdcbae11131cc4847ca63e7a94b018f51760b67 | 1 + 52 files changed, 3759 insertions(+), 1448 deletions(-) create mode 100644 actlint-cli/tests/cli.rs create mode 100644 actlint-core/src/fix.rs delete mode 100644 actlint-core/src/rules/action/expressions.rs delete mode 100644 actlint-core/src/rules/action/inputs.rs delete mode 100644 actlint-core/src/rules/action/mod.rs delete mode 100644 actlint-core/src/rules/action/outputs.rs delete mode 100644 actlint-core/src/rules/action/runs.rs delete mode 100644 actlint-core/src/rules/action/schema.rs create mode 100644 actlint-core/src/rules/action_boolean_default_invalid.rs create mode 100644 actlint-core/src/rules/action_choice_default_not_in_options.rs create mode 100644 actlint-core/src/rules/action_choice_requires_options.rs create mode 100644 actlint-core/src/rules/action_composite_steps_required.rs create mode 100644 actlint-core/src/rules/action_docker_image_not_found.rs create mode 100644 actlint-core/src/rules/action_docker_image_required.rs create mode 100644 actlint-core/src/rules/action_expression_contexts.rs create mode 100644 actlint-core/src/rules/action_file_not_found.rs create mode 100644 actlint-core/src/rules/action_invalid_branding_color.rs rename actlint-core/src/rules/{action/branding.rs => action_invalid_branding_icon.rs} (56%) create mode 100644 actlint-core/src/rules/action_invalid_input_type.rs create mode 100644 actlint-core/src/rules/action_invalid_using.rs create mode 100644 actlint-core/src/rules/action_js_main_required.rs create mode 100644 actlint-core/src/rules/action_name_required.rs create mode 100644 actlint-core/src/rules/action_output_invalid_step_ref.rs create mode 100644 actlint-core/src/rules/action_runs_required.rs delete mode 100644 actlint-core/src/rules/workflow/jobs.rs delete mode 100644 actlint-core/src/rules/workflow/mod.rs delete mode 100644 actlint-core/src/rules/workflow/steps.rs create mode 100644 actlint-core/src/rules/workflow_circular_needs.rs create mode 100644 actlint-core/src/rules/workflow_duplicate_step_id.rs rename actlint-core/src/rules/{workflow/expressions.rs => workflow_expression_contexts.rs} (54%) create mode 100644 actlint-core/src/rules/workflow_invalid_job_id.rs create mode 100644 actlint-core/src/rules/workflow_invalid_needs_ref.rs create mode 100644 actlint-core/src/rules/workflow_invalid_uses.rs create mode 100644 actlint-core/src/rules/workflow_missing_runs_on.rs create mode 100644 actlint-core/src/rules/workflow_no_jobs.rs create mode 100644 actlint-core/src/rules/workflow_step_no_action.rs create mode 100644 actlint-core/src/rules/workflow_step_uses_and_run.rs create mode 100644 fuzz-core/slow-unit-762713f50f42519f1975cfca4db51e0e6fefec1a create mode 100644 fuzz-core/slow-unit-9fdcbae11131cc4847ca63e7a94b018f51760b67 diff --git a/Cargo.lock b/Cargo.lock index 3cdd565..4ec0e49 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -7,11 +7,14 @@ name = "actlint-cli" version = "0.1.0" dependencies = [ "actlint-core", + "assert_cmd", "clap", "miette", + "predicates", "schematic", "serde", "serde_json", + "tempfile", ] [[package]] @@ -114,6 +117,27 @@ version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" +[[package]] +name = "assert_cmd" +version = "2.2.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "9a686bbee5efb88a82df0621b236e74d925f470e5445d3220a5648b892ec99c9" +dependencies = [ + "anstyle", + "bstr", + "libc", + "predicates", + "predicates-core", + "predicates-tree", + "wait-timeout", +] + +[[package]] +name = "autocfg" +version = "1.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "c08606f8c3cbf4ce6ec8e28fb0014a2c086708fe954eaa885384a6165172e7e8" + [[package]] name = "backtrace" version = "0.3.76" @@ -144,6 +168,17 @@ version = "2.11.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "843867be96c8daad0d758b57df9392b6d8d271134fce549de6ce169ff98a92af" +[[package]] +name = "bstr" +version = "1.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "63044e1ae8e69f3b5a92c736ca6269b8d12fa7efe39bf34ddb06d102cf0e2cab" +dependencies = [ + "memchr", + "regex-automata", + "serde", +] + [[package]] name = "castaway" version = "0.2.4" @@ -285,6 +320,12 @@ dependencies = [ "syn", ] +[[package]] +name = "difflib" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6184e33543162437515c2e2b48714794e37845ec9851711914eec9d308f6ebe8" + [[package]] name = "dirs" version = "6.0.0" @@ -355,6 +396,15 @@ version = "0.1.9" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5baebc0774151f905a1a2cc41989300b1e6fbb29aff0ceffa1064fdd3088d582" +[[package]] +name = "float-cmp" +version = "0.10.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b09cf3155332e944990140d967ff5eceb70df778b34f77d8075db46e4704e6d8" +dependencies = [ + "num-traits", +] + [[package]] name = "foldhash" version = "0.2.0" @@ -640,6 +690,21 @@ dependencies = [ "adler2", ] +[[package]] +name = "normalize-line-endings" +version = "0.3.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "61807f77802ff30975e01f4f071c8ba10c022052f98b3294119f3e615d13e5be" + +[[package]] +name = "num-traits" +version = "0.2.19" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "071dfc062690e90b734c0b2273ce72ad0ffa95f0c74596bc250dcfd960262841" +dependencies = [ + "autocfg", +] + [[package]] name = "object" version = "0.37.3" @@ -679,6 +744,36 @@ version = "0.2.17" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "a89322df9ebe1c1578d689c92318e070967d1042b512afbe49518723f4e6d5cd" +[[package]] +name = "predicates" +version = "3.1.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ada8f2932f28a27ee7b70dd6c1c39ea0675c55a36879ab92f3a715eaa1e63cfe" +dependencies = [ + "anstyle", + "difflib", + "float-cmp", + "normalize-line-endings", + "predicates-core", + "regex", +] + +[[package]] +name = "predicates-core" +version = "1.0.10" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "cad38746f3166b4031b1a0d39ad9f954dd291e7854fcc0eed52ee41a0b50d144" + +[[package]] +name = "predicates-tree" +version = "1.0.13" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d0de1b847b39c8131db0467e9df1ff60e6d0562ab8e9a16e568ad0fdb372e2f2" +dependencies = [ + "predicates-core", + "termtree", +] + [[package]] name = "proc-macro2" version = "1.0.106" @@ -1032,6 +1127,12 @@ dependencies = [ "windows-sys 0.61.2", ] +[[package]] +name = "termtree" +version = "0.5.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "8f50febec83f5ee1df3015341d8bd429f2d1cc62bcba7ea2076759d315084683" + [[package]] name = "textwrap" version = "0.16.2" @@ -1180,6 +1281,15 @@ version = "0.2.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "06abde3611657adf66d383f00b093d7faecc7fa57071cce2578660c9f1010821" +[[package]] +name = "wait-timeout" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "09ac3b126d3914f9849036f826e054cbabdc8519970b8998ddaf3b5bd3c65f11" +dependencies = [ + "libc", +] + [[package]] name = "walkdir" version = "2.5.0" diff --git a/actlint-cli/Cargo.toml b/actlint-cli/Cargo.toml index 92c8122..57d9b95 100644 --- a/actlint-cli/Cargo.toml +++ b/actlint-cli/Cargo.toml @@ -13,5 +13,10 @@ schematic = { workspace = true } serde = { workspace = true } serde_json = { workspace = true } +[dev-dependencies] +assert_cmd = "2" +predicates = "3" +tempfile = "3" + [lints] workspace = true diff --git a/actlint-cli/src/commands/lint.rs b/actlint-cli/src/commands/lint.rs index 6012d46..17fcadf 100644 --- a/actlint-cli/src/commands/lint.rs +++ b/actlint-cli/src/commands/lint.rs @@ -1,8 +1,10 @@ use std::{path::PathBuf, process}; use actlint_core::{ - diagnostic::Severity, + detect::FileKind, + diagnostic::{LintDiagnostic, Severity}, linter::{LintConfig, lint}, + source::SourceFile, }; /// Output format for lint results. @@ -13,6 +15,13 @@ pub(crate) enum OutputFormat { Sarif, } +/// Explicit file type for stdin input. +#[derive(Debug, Clone, clap::ValueEnum)] +pub(crate) enum StdinFileType { + Workflow, + Action, +} + /// Arguments for the lint command. #[derive(Debug, clap::Args)] pub(crate) struct LintArgs { @@ -31,6 +40,11 @@ pub(crate) struct LintArgs { /// Glob patterns to ignore. #[arg(long)] pub ignore: Vec, + + /// Explicit file type for stdin input (workflow, action). + /// Only used when reading from stdin. + #[arg(long, value_enum)] + pub r#type: Option, } /// Resolve the effective lint paths. @@ -61,31 +75,108 @@ fn resolve_paths(args: &LintArgs, config: &crate::config::ActlintConfig) -> Vec< /// Execute the lint command. pub(crate) fn execute(args: &LintArgs, config: &crate::config::ActlintConfig) { + use std::io::IsTerminal; + + let reading_stdin = args.paths.is_empty() && !std::io::stdin().is_terminal(); + + if reading_stdin { + execute_stdin(args, config); + return; + } + let mut ignore_patterns = config.ignore.clone(); ignore_patterns.extend(args.ignore.clone()); let effective_paths = resolve_paths(args, config); - let lint_config = LintConfig { ignore_patterns }; + let lint_config = LintConfig { ignore_patterns, ..LintConfig::default() }; let diagnostics = lint(&effective_paths, &lint_config); if diagnostics.is_empty() { + // exit 0: no diagnostics process::exit(0); } - match args.format { + output_diagnostics(&diagnostics, &args.format); + + let has_errors = diagnostics.iter().any(|d| d.severity == Severity::Error); + if has_errors { + // exit 1: error-level diagnostics found + process::exit(1); + } + + // exit 0: warnings only + process::exit(0); +} + +/// Read YAML from stdin, classify, lint, and report. +fn execute_stdin(args: &LintArgs, _config: &crate::config::ActlintConfig) { + use actlint_core::{ + detect::{FileKind, classify}, + source::SourceFile, + }; + + let mut content = String::new(); + if let Err(e) = std::io::Read::read_to_string(&mut std::io::stdin(), &mut content) { + #[allow(clippy::print_stderr)] + { + eprintln!("error reading stdin: {e}"); + } + // exit 2: operational failure + process::exit(2); + } + + let kind = match &args.r#type { + Some(StdinFileType::Workflow) => Some(FileKind::Workflow), + Some(StdinFileType::Action) => classify(std::path::Path::new("action.yml"), &content), + None => classify(std::path::Path::new(""), &content), + }; + + let Some(kind) = kind else { + #[allow(clippy::print_stderr)] + { + eprintln!("stdin: could not determine file type; use --type"); + } + // exit 2: operational failure (cannot classify) + process::exit(2); + }; + + let source = SourceFile::new(std::path::PathBuf::from(""), content); + let lint_config = LintConfig::default(); + + let diagnostics = lint_stdin(&source, kind, &lint_config); + + if diagnostics.is_empty() { + // exit 0: no diagnostics + process::exit(0); + } + + output_diagnostics(&diagnostics, &args.format); + + let has_errors = diagnostics.iter().any(|d| d.severity == Severity::Error); + if has_errors { + // exit 1: error-level diagnostics found + process::exit(1); + } + + // exit 0: warnings only + process::exit(0); +} + +/// Print diagnostics in the requested format. All output goes to stdout. +fn output_diagnostics(diagnostics: &[actlint_core::diagnostic::LintDiagnostic], format: &OutputFormat) { + match format { OutputFormat::Text => { - for diag in &diagnostics { - // Format using miette's graphical reporter via Display - #[allow(clippy::print_stderr)] + for diag in diagnostics { + #[allow(clippy::print_stdout)] { let path = diag.file_path(); if path.is_empty() { - eprintln!("[{}] {}: {}", diag.severity, diag.rule_name, diag.message); + println!("[{}] {}: {}", diag.severity, diag.rule_name, diag.message); } else { - eprintln!("{path}: [{}] {}: {}", diag.severity, diag.rule_name, diag.message); + println!("{path}: [{}] {}: {}", diag.severity, diag.rule_name, diag.message); } if let Some(help) = &diag.help { - eprintln!(" help: {help}"); + println!(" help: {help}"); } } } @@ -99,7 +190,7 @@ pub(crate) fn execute(args: &LintArgs, config: &crate::config::ActlintConfig) { } } OutputFormat::Sarif => { - let sarif = build_sarif(&diagnostics); + let sarif = build_sarif(diagnostics); #[allow(clippy::print_stdout)] { let json = serde_json::to_string_pretty(&sarif).unwrap_or_else(|_| "{}".into()); @@ -107,14 +198,6 @@ pub(crate) fn execute(args: &LintArgs, config: &crate::config::ActlintConfig) { } } } - - let has_errors = diagnostics.iter().any(|d| d.severity == Severity::Error); - if has_errors { - process::exit(1); - } - - // Warnings only - process::exit(0); } /// JSON-serializable diagnostic for --format json output. @@ -135,7 +218,7 @@ impl JsonDiagnostic { severity: diag.severity.to_string(), rule: diag.rule_name.clone(), message: diag.message.clone(), - offset: diag.span.offset(), + offset: diag.primary_span.offset(), help: diag.help.clone(), } } @@ -161,7 +244,7 @@ fn build_sarif(diagnostics: &[actlint_core::diagnostic::LintDiagnostic]) -> serd "uri": d.file_path() }, "region": { - "charOffset": d.span.offset() + "charOffset": d.primary_span.offset() } } }] @@ -184,3 +267,44 @@ fn build_sarif(diagnostics: &[actlint_core::diagnostic::LintDiagnostic]) -> serd }] }) } + +/// Lint stdin content using the ALL_RULES registry. +fn lint_stdin(source: &SourceFile, kind: FileKind, config: &LintConfig) -> Vec { + use actlint_core::rules::{ALL_RULES, AstKind, LintFile, RulePhase, RuleTarget}; + + let target = match kind { + FileKind::Workflow => RuleTarget::Workflow, + FileKind::CompositeAction => RuleTarget::CompositeAction, + FileKind::JavaScriptAction => RuleTarget::JavaScriptAction, + FileKind::DockerAction => RuleTarget::DockerAction, + }; + + match kind { + FileKind::Workflow => match actlint_core::parse::workflow::parse_workflow(source) { + Ok(wf) => { + let ast = AstKind::Workflow(&wf); + let file = LintFile { source, kind, ast: Some(ast), config }; + ALL_RULES + .iter() + .filter(|r| r.phase == RulePhase::Semantic) + .filter(|r| r.targets.contains(&target)) + .flat_map(|r| (r.check)(&file)) + .collect() + } + Err(diags) => diags, + }, + _ => match actlint_core::parse::action::parse_action(source) { + Ok(action) => { + let ast = AstKind::Action(&action); + let file = LintFile { source, kind, ast: Some(ast), config }; + ALL_RULES + .iter() + .filter(|r| r.phase == RulePhase::Semantic) + .filter(|r| r.targets.contains(&target)) + .flat_map(|r| (r.check)(&file)) + .collect() + } + Err(diags) => diags, + }, + } +} diff --git a/actlint-cli/src/main.rs b/actlint-cli/src/main.rs index 05242ac..35718ac 100644 --- a/actlint-cli/src/main.rs +++ b/actlint-cli/src/main.rs @@ -2,14 +2,37 @@ mod app; mod commands; mod config; +use std::path::PathBuf; + use clap::{Parser, Subcommand}; -/// actlint — GitHub Actions metadata linter +/// actlint -- GitHub Actions metadata linter #[derive(Parser, Debug)] #[command(name = "actlint", version, about)] struct Cli { + /// Subcommand. When omitted, runs lint on the default paths. #[command(subcommand)] - command: Commands, + command: Option, + + /// Paths to lint (used when no subcommand is given). + #[arg(trailing_var_arg = true)] + paths: Vec, + + /// Output format (used when no subcommand is given). + #[arg(long, value_enum, default_value = "text")] + format: commands::lint::OutputFormat, + + /// Config file (used when no subcommand is given). + #[arg(long)] + config: Option, + + /// Glob patterns to ignore (used when no subcommand is given). + #[arg(long)] + ignore: Vec, + + /// Explicit file type for stdin input (used when no subcommand is given). + #[arg(long, value_enum)] + r#type: Option, } #[derive(Subcommand, Debug)] @@ -25,18 +48,38 @@ fn main() { let cli = Cli::parse(); match &cli.command { - Commands::Lint(args) => { + Some(Commands::Lint(args)) => { let config = app::load_config(args.config.as_deref()).unwrap_or_else(|e| { #[allow(clippy::print_stderr)] { eprintln!("config error: {e}"); } + // exit 2: operational failure std::process::exit(2); }); commands::lint::execute(args, &config); } - Commands::Init => { + Some(Commands::Init) => { commands::init::execute(); } + None => { + // Default: run lint with top-level args + let args = commands::lint::LintArgs { + paths: cli.paths.clone(), + format: cli.format.clone(), + config: cli.config.clone(), + ignore: cli.ignore.clone(), + r#type: cli.r#type.clone(), + }; + let config = app::load_config(args.config.as_deref()).unwrap_or_else(|e| { + #[allow(clippy::print_stderr)] + { + eprintln!("config error: {e}"); + } + // exit 2: operational failure + std::process::exit(2); + }); + commands::lint::execute(&args, &config); + } } } diff --git a/actlint-cli/tests/cli.rs b/actlint-cli/tests/cli.rs new file mode 100644 index 0000000..6ea5154 --- /dev/null +++ b/actlint-cli/tests/cli.rs @@ -0,0 +1,171 @@ +#[allow(clippy::panic)] +use assert_cmd::Command; +use predicates::prelude::*; + +fn cmd() -> Command { + Command::cargo_bin("actlint-cli").unwrap_or_else(|e| { + std::process::exit(match e { + _ => 1, + }); + }) +} + +/// Create a temp dir with `.github/workflows/` containing the given YAML. +/// Returns the temp dir (must be kept alive) and the path to the workflow file. +fn workflow_fixture(name: &str, content: &str) -> (tempfile::TempDir, std::path::PathBuf) { + let dir = tempfile::tempdir().unwrap_or_else(|_| std::process::exit(1)); + let wf_dir = dir.path().join(".github/workflows"); + std::fs::create_dir_all(&wf_dir).unwrap_or_else(|_| std::process::exit(1)); + let file = wf_dir.join(name); + std::fs::write(&file, content).unwrap_or_else(|_| std::process::exit(1)); + (dir, file) +} + +const CLEAN_WORKFLOW: &str = "\ +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + +const CIRCULAR_NEEDS: &str = "\ +name: CI +on: push +jobs: + a: + runs-on: ubuntu-latest + needs: [b] + steps: + - run: echo + b: + runs-on: ubuntu-latest + needs: [a] + steps: + - run: echo +"; + +const INVALID_NEEDS_REF: &str = "\ +name: CI +on: push +jobs: + a: + runs-on: ubuntu-latest + needs: [nonexistent] + steps: + - run: echo +"; + +// --- Clean workflow exits 0 --- + +#[test] +fn clean_workflow_file_exits_zero() { + let (_dir, file) = workflow_fixture("ci.yml", CLEAN_WORKFLOW); + cmd().args(["lint", file.to_str().unwrap_or_default()]).assert().success(); +} + +// --- JSON format to stdout --- + +#[test] +fn json_format_goes_to_stdout() { + let (_dir, file) = workflow_fixture("bad.yml", INVALID_NEEDS_REF); + cmd() + .args(["lint", "--format", "json", file.to_str().unwrap_or_default()]) + .assert() + .failure() + .stdout(predicate::str::starts_with("[")); +} + +#[test] +fn json_format_default_subcommand() { + let (_dir, file) = workflow_fixture("bad.yml", INVALID_NEEDS_REF); + cmd() + .args(["--format", "json", file.to_str().unwrap_or_default()]) + .assert() + .failure() + .stdout(predicate::str::starts_with("[")); +} + +// --- Text format to stdout, not stderr --- + +#[test] +fn text_format_on_stdout_not_stderr() { + let (_dir, file) = workflow_fixture("ci.yml", CLEAN_WORKFLOW); + cmd() + .args(["lint", "--format", "text", file.to_str().unwrap_or_default()]) + .assert() + .success() + .stderr(predicate::str::is_empty()); +} + +// --- Invalid workflow exits 1 --- + +#[test] +fn circular_needs_exits_one() { + let (_dir, file) = workflow_fixture("bad.yml", CIRCULAR_NEEDS); + cmd().args(["lint", file.to_str().unwrap_or_default()]).assert().failure().code(1); +} + +// --- Backward compatibility: "actlint-cli lint [path]" --- + +#[test] +fn lint_subcommand_backward_compat() { + let (_dir, file) = workflow_fixture("ci.yml", CLEAN_WORKFLOW); + cmd().args(["lint", file.to_str().unwrap_or_default()]).assert().success(); +} + +// --- Default invocation (no "lint" subcommand) --- + +#[test] +fn default_invocation_without_lint_subcommand() { + let (_dir, file) = workflow_fixture("ci.yml", CLEAN_WORKFLOW); + cmd().args([file.to_str().unwrap_or_default()]).assert().success(); +} + +// --- SARIF format --- + +#[test] +fn sarif_format_goes_to_stdout() { + let (_dir, file) = workflow_fixture("bad.yml", INVALID_NEEDS_REF); + cmd() + .args(["lint", "--format", "sarif", file.to_str().unwrap_or_default()]) + .assert() + .failure() + .stdout(predicate::str::contains("\"version\": \"2.1.0\"")); +} + +// --- Text output contains rule IDs --- + +#[test] +fn text_output_contains_rule_id() { + let (_dir, file) = workflow_fixture("bad.yml", INVALID_NEEDS_REF); + cmd() + .args(["lint", "--format", "text", file.to_str().unwrap_or_default()]) + .assert() + .failure() + .stdout(predicate::str::contains("semantic/workflow-invalid-needs-ref")); +} + +// --- Stdin with --type workflow --- + +#[test] +fn stdin_with_type_workflow() { + cmd().args(["lint", "--type", "workflow"]).write_stdin(CLEAN_WORKFLOW).assert().success(); +} + +#[test] +fn stdin_invalid_workflow_exits_one() { + cmd().args(["lint", "--type", "workflow"]).write_stdin(CIRCULAR_NEEDS).assert().failure().code(1); +} + +// --- Directory scanning --- + +#[test] +fn directory_scanning_finds_workflow_files() { + let (dir, _file) = workflow_fixture("ci.yml", CLEAN_WORKFLOW); + // Pass the .github directory explicitly to avoid stdin detection + let github_dir = dir.path().join(".github"); + cmd().args(["lint", github_dir.to_str().unwrap_or_default()]).assert().success(); +} diff --git a/actlint-core/src/diagnostic.rs b/actlint-core/src/diagnostic.rs index acbf363..4bdd5bf 100644 --- a/actlint-core/src/diagnostic.rs +++ b/actlint-core/src/diagnostic.rs @@ -16,6 +16,43 @@ impl fmt::Display for Severity { } } +/// A labeled annotation span within a diagnostic. +/// Enables rustc-style multi-point underlines. +#[derive(Debug, Clone)] +pub struct DiagnosticLabel { + pub span: miette::SourceSpan, + pub text: String, +} + +/// How safe an auto-fix is to apply without human review. +#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] +pub enum Applicability { + /// Correct in all inputs: apply automatically with --fix. + MachineApplicable, + /// Likely correct but may need human review. + MaybeIncorrect, + /// Structural: never applied automatically. + Unsafe, +} + +/// A single text replacement within a source file. +#[derive(Debug, Clone)] +pub struct TextEdit { + pub span: miette::SourceSpan, + /// Replacement text. Empty string means deletion. + pub replacement: String, +} + +/// A suggested fix attached to a diagnostic. +#[derive(Debug, Clone)] +pub struct SuggestedFix { + /// User-facing description: e.g. "Replace 'yes' with 'true'" + pub message: String, + pub applicability: Applicability, + /// One or more non-overlapping edits that together constitute the fix. + pub edits: Vec, +} + /// A lint diagnostic with source location information for miette rendering. #[derive(Debug, thiserror::Error)] #[error("{message}")] @@ -23,8 +60,13 @@ pub struct LintDiagnostic { pub severity: Severity, pub rule_name: String, pub message: String, - pub span: miette::SourceSpan, + /// Primary span: used as sort key and primary squiggle location. + pub primary_span: miette::SourceSpan, + /// Multi-point labels. When non-empty these replace the default "here" label. + pub labels: Vec, pub help: Option, + /// Optional auto-fix. No rule currently populates this. + pub fix: Option, #[source] pub cause: Option>, source_code: miette::NamedSource, @@ -41,8 +83,10 @@ impl LintDiagnostic { severity: Severity::Error, rule_name: rule_name.into(), message: message.into(), - span, + primary_span: span, + labels: Vec::new(), help: None, + fix: None, cause: None, source_code, } @@ -58,8 +102,10 @@ impl LintDiagnostic { severity: Severity::Warning, rule_name: rule_name.into(), message: message.into(), - span, + primary_span: span, + labels: Vec::new(), help: None, + fix: None, cause: None, source_code, } @@ -71,6 +117,12 @@ impl LintDiagnostic { self } + #[must_use] + pub fn with_label(mut self, span: miette::SourceSpan, text: impl Into) -> Self { + self.labels.push(DiagnosticLabel { span, text: text.into() }); + self + } + /// The file path (or name) associated with this diagnostic. pub fn file_path(&self) -> &str { self.source_code.name() } } @@ -92,6 +144,17 @@ impl miette::Diagnostic for LintDiagnostic { fn source_code(&self) -> Option<&dyn miette::SourceCode> { Some(&self.source_code) } fn labels(&self) -> Option + '_>> { - Some(Box::new(std::iter::once(miette::LabeledSpan::at(self.span, "here")))) + if self.labels.is_empty() { + // Fall back to "here" on the primary span so old rules still render + Some(Box::new(std::iter::once(miette::LabeledSpan::at(self.primary_span, "here")))) + } else { + Some(Box::new( + self.labels + .iter() + .map(|l| miette::LabeledSpan::at(l.span, l.text.as_str())) + .collect::>() + .into_iter(), + )) + } } } diff --git a/actlint-core/src/fix.rs b/actlint-core/src/fix.rs new file mode 100644 index 0000000..2dbc39b --- /dev/null +++ b/actlint-core/src/fix.rs @@ -0,0 +1,136 @@ +use crate::diagnostic::{Applicability, SuggestedFix, TextEdit}; + +#[derive(Debug, thiserror::Error)] +pub enum FixApplyError { + #[error("overlapping edits — neither applied (rules: {rule_a} and {rule_b})")] + OverlappingEdits { rule_a: String, rule_b: String }, + + #[error("edit span [{start}..{end}] exceeds source length {len}")] + SpanOutOfBounds { start: usize, end: usize, len: usize }, +} + +/// Apply a set of suggested fixes to source text. +/// +/// Edits are applied back-to-front (sorted by descending byte offset) so +/// earlier byte offsets remain valid after each replacement. Overlapping +/// edits are rejected: neither conflicting edit is applied and an error is +/// returned for each conflict. Only fixes with applicability <= `max_applicability` +/// are applied. +pub fn apply_fixes( + source: &str, + fixes: &[(String, SuggestedFix)], // (rule_name, fix) + max_applicability: Applicability, +) -> (String, Vec) { + let mut errors: Vec = Vec::new(); + + // Flatten edits, filtering by applicability + let mut edits: Vec<(&TextEdit, &str)> = fixes + .iter() + .filter(|(_, fix)| fix.applicability <= max_applicability) + .flat_map(|(rule, fix)| fix.edits.iter().map(move |e| (e, rule.as_str()))) + .collect(); + + // Sort descending by offset + edits.sort_by(|a, b| b.0.span.offset().cmp(&a.0.span.offset())); + + // Detect overlaps and build accepted set + let mut accepted: Vec<(&TextEdit, &str)> = Vec::new(); + let mut last_start = usize::MAX; + let mut last_rule = ""; + + for (edit, rule) in &edits { + let start = edit.span.offset(); + let end = start + edit.span.len(); + if end > last_start { + errors.push(FixApplyError::OverlappingEdits { + rule_a: rule.to_string(), + rule_b: last_rule.to_string(), + }); + continue; + } + last_start = start; + last_rule = rule; + accepted.push((edit, rule)); + } + + // Apply accepted edits back-to-front + let mut out = source.to_owned(); + for (edit, rule) in accepted { + let start = edit.span.offset(); + let end = start + edit.span.len(); + if end > out.len() { + errors.push(FixApplyError::SpanOutOfBounds { start, end, len: out.len() }); + let _ = rule; + continue; + } + out.replace_range(start..end, &edit.replacement); + } + + (out, errors) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::diagnostic::{Applicability, SuggestedFix, TextEdit}; + + fn edit(offset: usize, len: usize, replacement: &str) -> TextEdit { + TextEdit { span: miette::SourceSpan::new(offset.into(), len), replacement: replacement.to_owned() } + } + + fn machine_fix(msg: &str, edits: Vec) -> (String, SuggestedFix) { + ( + "test-rule".to_owned(), + SuggestedFix { message: msg.to_owned(), applicability: Applicability::MachineApplicable, edits }, + ) + } + + #[test] + fn single_replacement() { + let (result, errors) = apply_fixes( + "hello world", + &[machine_fix("replace world", vec![edit(6, 5, "rust")])], + Applicability::MachineApplicable, + ); + assert!(errors.is_empty()); + assert_eq!(result, "hello rust"); + } + + #[test] + fn two_non_overlapping_replacements_applied_back_to_front() { + let (result, errors) = apply_fixes( + "aXbYc", + &[machine_fix("fix X", vec![edit(1, 1, "1")]), machine_fix("fix Y", vec![edit(3, 1, "2")])], + Applicability::MachineApplicable, + ); + assert!(errors.is_empty()); + assert_eq!(result, "a1b2c"); + } + + #[test] + fn overlapping_edits_rejected() { + let (result, errors) = apply_fixes( + "hello", + &[machine_fix("fix A", vec![edit(0, 3, "X")]), machine_fix("fix B", vec![edit(2, 3, "Y")])], + Applicability::MachineApplicable, + ); + // Original is preserved for the rejected edit + assert!(!errors.is_empty()); + let _ = result; + } + + #[test] + fn fix_above_max_applicability_not_applied() { + let fix = ( + "unsafe-rule".to_owned(), + SuggestedFix { + message: "unsafe fix".to_owned(), + applicability: Applicability::Unsafe, + edits: vec![edit(0, 5, "world")], + }, + ); + let (result, errors) = apply_fixes("hello", &[fix], Applicability::MachineApplicable); + assert!(errors.is_empty()); + assert_eq!(result, "hello"); // not modified + } +} diff --git a/actlint-core/src/fuzz_regression.rs b/actlint-core/src/fuzz_regression.rs index 34bd37d..110aa3c 100644 --- a/actlint-core/src/fuzz_regression.rs +++ b/actlint-core/src/fuzz_regression.rs @@ -6,7 +6,46 @@ /// crash files in `fuzz-core/regressions/`. use std::path::PathBuf; -use crate::{detect::classify, parse, rules, source::SourceFile}; +use crate::{ + detect::classify, + linter::LintConfig, + parse, + rules::{ALL_RULES, AstKind, LintFile, RuleTarget}, + source::SourceFile, +}; + +/// Run all registered rules against a source file via the new ALL_RULES registry. +fn run_all_rules(source: &SourceFile, kind: crate::detect::FileKind) { + let config = LintConfig::default(); + + let target = match kind { + crate::detect::FileKind::Workflow => RuleTarget::Workflow, + crate::detect::FileKind::CompositeAction => RuleTarget::CompositeAction, + crate::detect::FileKind::JavaScriptAction => RuleTarget::JavaScriptAction, + crate::detect::FileKind::DockerAction => RuleTarget::DockerAction, + }; + + match kind { + crate::detect::FileKind::Workflow => { + if let Ok(wf) = parse::workflow::parse_workflow(source) { + let ast = AstKind::Workflow(&wf); + let file = LintFile { source, kind, ast: Some(ast), config: &config }; + for rule in ALL_RULES.iter().filter(|r| r.targets.contains(&target)) { + let _ = (rule.check)(&file); + } + } + } + _ => { + if let Ok(action) = parse::action::parse_action(source) { + let ast = AstKind::Action(&action); + let file = LintFile { source, kind, ast: Some(ast), config: &config }; + for rule in ALL_RULES.iter().filter(|r| r.targets.contains(&target)) { + let _ = (rule.check)(&file); + } + } + } + } +} /// Run the unstructured fuzz target logic: treat bytes as UTF-8 YAML, /// parse as both workflow and action, run lint rules. @@ -20,36 +59,14 @@ pub fn run_parse_document_unstructured(data: &[u8]) { let workflow_path = PathBuf::from(".github/workflows/fuzz.yml"); if let Some(kind) = classify(&workflow_path, content) { let source = SourceFile::new(workflow_path, content.to_owned()); - match kind { - crate::detect::FileKind::Workflow => { - if let Ok(workflow) = parse::workflow::parse_workflow(&source) { - let _ = rules::workflow::lint_workflow(&workflow, &source); - } - } - _ => { - if let Ok(action) = parse::action::parse_action(&source) { - let _ = rules::action::lint_action(&action, &source); - } - } - } + run_all_rules(&source, kind); } // Also try as action let action_path = PathBuf::from("action.yml"); if let Some(kind) = classify(&action_path, content) { let source = SourceFile::new(action_path, content.to_owned()); - match kind { - crate::detect::FileKind::Workflow => { - if let Ok(workflow) = parse::workflow::parse_workflow(&source) { - let _ = rules::workflow::lint_workflow(&workflow, &source); - } - } - _ => { - if let Ok(action) = parse::action::parse_action(&source) { - let _ = rules::action::lint_action(&action, &source); - } - } - } + run_all_rules(&source, kind); } } diff --git a/actlint-core/src/lib.rs b/actlint-core/src/lib.rs index 02f5c12..10491ab 100644 --- a/actlint-core/src/lib.rs +++ b/actlint-core/src/lib.rs @@ -3,6 +3,7 @@ pub mod context; pub mod detect; pub mod diagnostic; pub mod expr; +pub mod fix; pub mod fuzz_regression; pub mod linter; pub mod parse; diff --git a/actlint-core/src/linter.rs b/actlint-core/src/linter.rs index 489ec5d..9f8444f 100644 --- a/actlint-core/src/linter.rs +++ b/actlint-core/src/linter.rs @@ -3,7 +3,7 @@ use std::path::{Path, PathBuf}; use crate::{ detect::{FileKind, classify}, diagnostic::LintDiagnostic, - parse, rules, + parse, source::SourceFile, }; @@ -11,6 +11,24 @@ use crate::{ #[derive(Debug, Default)] pub struct LintConfig { pub ignore_patterns: Vec, + + /// Disable specific rules by exact ID: e.g. "semantic/workflow-circular-needs" + pub disabled_rules: Vec, + + /// Disable all rules whose ID starts with this prefix: e.g. "schema" + pub disabled_namespaces: Vec, + + pub schema: SchemaConfig, +} + +#[derive(Debug)] +pub struct SchemaConfig { + pub enabled: bool, + pub severity: crate::diagnostic::Severity, +} + +impl Default for SchemaConfig { + fn default() -> Self { Self { enabled: false, severity: crate::diagnostic::Severity::Error } } } /// Run the linter on a set of paths (files or directories). @@ -29,14 +47,14 @@ pub fn lint(paths: &[PathBuf], config: &LintConfig) -> Vec { } for file_path in &files { - match lint_file(file_path) { + match lint_file(file_path, config) { Ok(mut file_diags) => diagnostics.append(&mut file_diags), Err(mut err_diags) => diagnostics.append(&mut err_diags), } } // Sort by file path, then byte offset - diagnostics.sort_by(|a, b| a.span.offset().cmp(&b.span.offset())); + diagnostics.sort_by(|a, b| a.primary_span.offset().cmp(&b.primary_span.offset())); diagnostics } @@ -67,7 +85,7 @@ fn discover_files(dir: &Path, ignore_patterns: &[String], files: &mut Vec Result, Vec> { +fn lint_file(path: &Path, config: &LintConfig) -> Result, Vec> { let content = std::fs::read_to_string(path).map_err(|e| { vec![LintDiagnostic::error( "io-error", @@ -82,21 +100,61 @@ fn lint_file(path: &Path) -> Result, Vec> { }; let source = SourceFile::new(path.to_owned(), content); + lint_file_with_rules(&source, kind, config) +} - match kind { - FileKind::Workflow => lint_workflow(&source), - FileKind::CompositeAction | FileKind::JavaScriptAction | FileKind::DockerAction => { - lint_action(&source) +fn lint_file_with_rules( + source: &SourceFile, + kind: FileKind, + config: &LintConfig, +) -> Result, Vec> { + use crate::rules::{ALL_RULES, AstKind, LintFile, RulePhase, RuleTarget}; + + fn target_for(kind: FileKind) -> RuleTarget { + match kind { + FileKind::Workflow => RuleTarget::Workflow, + FileKind::CompositeAction => RuleTarget::CompositeAction, + FileKind::JavaScriptAction => RuleTarget::JavaScriptAction, + FileKind::DockerAction => RuleTarget::DockerAction, } } -} -fn lint_workflow(source: &SourceFile) -> Result, Vec> { - let workflow = parse::workflow::parse_workflow(source)?; - Ok(rules::workflow::lint_workflow(&workflow, source)) -} + fn rule_applies(rule: &crate::rules::RuleDef, target: RuleTarget) -> bool { + rule.targets.contains(&target) + } + + fn is_disabled(rule: &crate::rules::RuleDef, config: &LintConfig) -> bool { + config.disabled_rules.iter().any(|r| r == rule.id) + || config.disabled_namespaces.iter().any(|ns| rule.id.starts_with(ns.as_str())) + } + + let target = target_for(kind); -fn lint_action(source: &SourceFile) -> Result, Vec> { - let action = parse::action::parse_action(source)?; - Ok(rules::action::lint_action(&action, source)) + // Parse AST for semantic phase + match kind { + FileKind::Workflow => { + let wf = parse::workflow::parse_workflow(source)?; + let ast = AstKind::Workflow(&wf); + let file = LintFile { source, kind, ast: Some(ast), config }; + Ok(ALL_RULES + .iter() + .filter(|r| r.phase == RulePhase::Semantic) + .filter(|r| rule_applies(r, target)) + .filter(|r| !is_disabled(r, config)) + .flat_map(|r| (r.check)(&file)) + .collect()) + } + _ => { + let action = parse::action::parse_action(source)?; + let ast = AstKind::Action(&action); + let file = LintFile { source, kind, ast: Some(ast), config }; + Ok(ALL_RULES + .iter() + .filter(|r| r.phase == RulePhase::Semantic) + .filter(|r| rule_applies(r, target)) + .filter(|r| !is_disabled(r, config)) + .flat_map(|r| (r.check)(&file)) + .collect()) + } + } } diff --git a/actlint-core/src/rules/action/expressions.rs b/actlint-core/src/rules/action/expressions.rs deleted file mode 100644 index 48fed0e..0000000 --- a/actlint-core/src/rules/action/expressions.rs +++ /dev/null @@ -1,270 +0,0 @@ -use crate::{ - ast::action::{Action, Runs}, - context::availability::{Scope, allowed_contexts}, - diagnostic::LintDiagnostic, - expr, - source::SourceFile, -}; - -/// Check all expression strings in action fields. -pub fn check_expressions(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - let Some(runs) = &action.runs else { - return diagnostics; - }; - - match runs { - Runs::Composite { steps, .. } => { - let scope = Scope::CompositeStep; - for step in steps { - if let Some(if_cond) = &step.if_cond { - check_expression_string(&if_cond.value, if_cond.span, scope, source, &mut diagnostics); - } - if let Some(run) = &step.run { - check_embedded_expressions(&run.value, run.span, scope, source, &mut diagnostics); - } - for (_, v) in &step.env { - check_embedded_expressions(&v.value, v.span, scope, source, &mut diagnostics); - } - for (_, v) in &step.with { - check_embedded_expressions(&v.value, v.span, scope, source, &mut diagnostics); - } - } - } - Runs::JavaScript { pre_if, post_if, .. } => { - let scope = Scope::JsActionCondition; - if let Some(pre_if) = pre_if { - check_expression_string(&pre_if.value, pre_if.span, scope, source, &mut diagnostics); - } - if let Some(post_if) = post_if { - check_expression_string(&post_if.value, post_if.span, scope, source, &mut diagnostics); - } - } - Runs::Docker { env, .. } => { - let scope = Scope::DockerActionExpr; - for (_, v) in env { - check_embedded_expressions(&v.value, v.span, scope, source, &mut diagnostics); - } - } - } - - diagnostics -} - -/// Find and check all `${{ }}` expressions embedded in a string value. -fn check_embedded_expressions( - value: &str, - span: miette::SourceSpan, - scope: Scope, - source: &SourceFile, - diagnostics: &mut Vec, -) { - for expr_content in extract_expressions(value) { - check_expression_string(&expr_content, span, scope, source, diagnostics); - } -} - -/// Check a single expression string (already extracted from ${{ }}). -fn check_expression_string( - value: &str, - span: miette::SourceSpan, - scope: Scope, - source: &SourceFile, - diagnostics: &mut Vec, -) { - // Handle both raw expressions and ${{ }}-wrapped ones - let expr_content = if let Some(inner) = value.strip_prefix("${{") { - inner.strip_suffix("}}").unwrap_or(inner).trim() - } else { - value.trim() - }; - - if expr_content.is_empty() { - return; - } - - let tokens = match expr::tokenize(expr_content) { - Ok(t) => t, - Err(e) => { - diagnostics.push(LintDiagnostic::error( - "action/expression-lex-error", - format!("expression lex error: {e}"), - span, - source.named_source(), - )); - return; - } - }; - - let ast = match expr::parse_expression(&tokens) { - Ok(a) => a, - Err(e) => { - diagnostics.push(LintDiagnostic::error( - "action/expression-parse-error", - format!("expression parse error: {e}"), - span, - source.named_source(), - )); - return; - } - }; - - let allowed = allowed_contexts(scope); - let errors = expr::check_expression(&ast, allowed); - for error in errors { - diagnostics.push(LintDiagnostic::error( - "action/expression-check-error", - error.message, - span, - source.named_source(), - )); - } -} - -/// Extract expression contents from `${{ }}` delimiters in a string. -fn extract_expressions(value: &str) -> Vec { - let mut results = Vec::new(); - let mut remaining = value; - - while let Some(start) = remaining.find("${{") { - let after_start = &remaining[start + 3..]; - if let Some(end) = after_start.find("}}") { - results.push(after_start[..end].trim().to_owned()); - remaining = &after_start[end + 2..]; - } else { - break; - } - } - - results -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn extract_single_expression() { - let results = extract_expressions("Hello ${{ github.ref }}"); - assert_eq!(results, vec!["github.ref"]); - } - - #[test] - fn extract_multiple_expressions() { - let results = extract_expressions("${{ github.ref }} and ${{ inputs.name }}"); - assert_eq!(results, vec!["github.ref", "inputs.name"]); - } - - #[test] - fn extract_no_expressions() { - let results = extract_expressions("plain text"); - assert!(results.is_empty()); - } - - // --- Edge case: Empty expression ${{ }} --- - - #[test] - fn extract_empty_expression() { - let results = extract_expressions("${{ }}"); - assert_eq!(results, vec![""]); - } - - #[test] - fn extract_completely_empty_expression() { - let results = extract_expressions("${{}}"); - assert_eq!(results, vec![""]); - } - - // --- Edge case: Unclosed ${{ --- - - #[test] - fn extract_unclosed_expression_dropped() { - let results = extract_expressions("${{ github.ref"); - assert!(results.is_empty(), "unclosed expression should be silently dropped"); - } - - #[test] - fn extract_unclosed_after_valid() { - let results = extract_expressions("${{ a }} then ${{ unclosed"); - assert_eq!(results, vec!["a"], "only the complete expression should be returned"); - } - - // --- Edge case: Nested ${{ inside strings --- - - #[test] - fn extract_nested_dollar_brace_in_content() { - // The content between ${{ and first }} is extracted, - // even if it contains another ${{ (which is unusual but possible in malformed files) - let results = extract_expressions("${{ something ${{ inner }} }}"); - // First match: from first ${{ to first }}, content = " something ${{ inner " - assert_eq!(results.len(), 1); - assert_eq!(results[0], "something ${{ inner"); - } - - // --- Edge case: Multiple ${{ on same line --- - - #[test] - fn extract_many_expressions_on_one_line() { - let results = extract_expressions("${{ a }} ${{ b }} ${{ c }}"); - assert_eq!(results, vec!["a", "b", "c"]); - } - - // --- Edge case: check_expression_string with empty content --- - - #[test] - fn check_expression_string_empty_is_noop() { - use std::path::PathBuf; - - use crate::{context::availability::Scope, source::SourceFile}; - - let source = SourceFile::new(PathBuf::from("test.yml"), "name: test\n".into()); - let span = miette::SourceSpan::new(0.into(), 0); - let mut diags = Vec::new(); - check_expression_string("", span, Scope::CompositeStep, &source, &mut diags); - assert!(diags.is_empty(), "empty expression content should produce no diagnostics"); - } - - #[test] - fn check_expression_string_whitespace_only_is_noop() { - use std::path::PathBuf; - - use crate::{context::availability::Scope, source::SourceFile}; - - let source = SourceFile::new(PathBuf::from("test.yml"), "name: test\n".into()); - let span = miette::SourceSpan::new(0.into(), 0); - let mut diags = Vec::new(); - check_expression_string(" ", span, Scope::CompositeStep, &source, &mut diags); - assert!(diags.is_empty(), "whitespace-only expression should produce no diagnostics"); - } - - #[test] - fn check_wrapped_expression_strips_delimiters() { - use std::path::PathBuf; - - use crate::{context::availability::Scope, source::SourceFile}; - - let source = SourceFile::new(PathBuf::from("test.yml"), "name: test\n".into()); - let span = miette::SourceSpan::new(0.into(), 0); - let mut diags = Vec::new(); - // This should strip ${{ }} and check the inner content - check_expression_string("${{ inputs.name }}", span, Scope::CompositeStep, &source, &mut diags); - assert!(diags.is_empty()); - } - - #[test] - fn check_expression_string_unclosed_wrapper() { - use std::path::PathBuf; - - use crate::{context::availability::Scope, source::SourceFile}; - - let source = SourceFile::new(PathBuf::from("test.yml"), "name: test\n".into()); - let span = miette::SourceSpan::new(0.into(), 0); - let mut diags = Vec::new(); - // ${{ without }} -- strip_prefix works but strip_suffix returns None, - // so the inner content is the remainder minus the prefix - check_expression_string("${{ inputs.name", span, Scope::CompositeStep, &source, &mut diags); - // Should still lex and parse "inputs.name" (the suffix fallback trims) - assert!(diags.is_empty()); - } -} diff --git a/actlint-core/src/rules/action/inputs.rs b/actlint-core/src/rules/action/inputs.rs deleted file mode 100644 index d84e542..0000000 --- a/actlint-core/src/rules/action/inputs.rs +++ /dev/null @@ -1,170 +0,0 @@ -use crate::{ast::action::Action, diagnostic::LintDiagnostic, source::SourceFile}; - -const VALID_INPUT_TYPES: &[&str] = &["string", "boolean", "choice", "environment"]; - -/// Check action input definitions for correctness. -pub fn check_inputs(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - for input in &action.inputs { - // Validate type field if present - if let Some(type_field) = &input.type_field { - if !VALID_INPUT_TYPES.contains(&type_field.value.as_str()) { - diagnostics.push( - LintDiagnostic::error( - "action/invalid-input-type", - format!( - "input '{}' has invalid type '{}', expected one of: {}", - input.id, - type_field.value, - VALID_INPUT_TYPES.join(", ") - ), - type_field.span, - source.named_source(), - ) - .with_help("valid types are: string, boolean, choice, environment"), - ); - } - - // choice type requires non-empty options - if type_field.value == "choice" && input.options.is_empty() { - diagnostics.push(LintDiagnostic::error( - "action/choice-requires-options", - format!("input '{}' has type 'choice' but no options defined", input.id), - type_field.span, - source.named_source(), - )); - } - - // boolean default must be "true" or "false" - if type_field.value == "boolean" { - if let Some(default) = &input.default { - if default.value != "true" && default.value != "false" { - diagnostics.push( - LintDiagnostic::error( - "action/boolean-default-invalid", - format!( - "input '{}' has type 'boolean' but default '{}' is not 'true' or 'false'", - input.id, default.value - ), - default.span, - source.named_source(), - ) - .with_help("boolean defaults must be the string 'true' or 'false'"), - ); - } - } - } - - // choice default must be in options - if type_field.value == "choice" && !input.options.is_empty() { - if let Some(default) = &input.default { - let option_values: Vec<&str> = input.options.iter().map(|o| o.value.as_str()).collect(); - if !option_values.contains(&default.value.as_str()) { - diagnostics.push( - LintDiagnostic::error( - "action/choice-default-not-in-options", - format!( - "input '{}' has default '{}' which is not in the options list", - input.id, default.value - ), - default.span, - source.named_source(), - ) - .with_help(format!("valid options are: {}", option_values.join(", "))), - ); - } - } - } - } - } - - diagnostics -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use super::*; - use crate::ast::action::{ActionInput, SpannedString}; - - fn source() -> SourceFile { SourceFile::new(PathBuf::from("action.yml"), "name: test\n".into()) } - - fn span() -> miette::SourceSpan { miette::SourceSpan::new(0.into(), 0) } - - #[test] - fn invalid_input_type() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![ActionInput { - id: "mode".into(), - description: None, - required: None, - default: None, - type_field: Some(SpannedString { value: "invalid".into(), span: span() }), - options: vec![], - deprecation_message: None, - span: span(), - }], - outputs: vec![], - runs: None, - branding: None, - span: span(), - }; - let diags = check_inputs(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/invalid-input-type")); - } - - #[test] - fn choice_without_options() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![ActionInput { - id: "mode".into(), - description: None, - required: None, - default: None, - type_field: Some(SpannedString { value: "choice".into(), span: span() }), - options: vec![], - deprecation_message: None, - span: span(), - }], - outputs: vec![], - runs: None, - branding: None, - span: span(), - }; - let diags = check_inputs(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/choice-requires-options")); - } - - #[test] - fn boolean_invalid_default() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![ActionInput { - id: "verbose".into(), - description: None, - required: None, - default: Some(SpannedString { value: "yes".into(), span: span() }), - type_field: Some(SpannedString { value: "boolean".into(), span: span() }), - options: vec![], - deprecation_message: None, - span: span(), - }], - outputs: vec![], - runs: None, - branding: None, - span: span(), - }; - let diags = check_inputs(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/boolean-default-invalid")); - } -} diff --git a/actlint-core/src/rules/action/mod.rs b/actlint-core/src/rules/action/mod.rs deleted file mode 100644 index 4f00830..0000000 --- a/actlint-core/src/rules/action/mod.rs +++ /dev/null @@ -1,20 +0,0 @@ -pub mod branding; -pub mod expressions; -pub mod inputs; -pub mod outputs; -pub mod runs; -pub mod schema; - -use crate::{ast::action::Action, diagnostic::LintDiagnostic, source::SourceFile}; - -/// Run all action lint rules against a parsed action AST. -pub fn lint_action(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - diagnostics.extend(schema::check_schema(action, source)); - diagnostics.extend(inputs::check_inputs(action, source)); - diagnostics.extend(outputs::check_outputs(action, source)); - diagnostics.extend(runs::check_runs(action, source)); - diagnostics.extend(branding::check_branding(action, source)); - diagnostics.extend(expressions::check_expressions(action, source)); - diagnostics -} diff --git a/actlint-core/src/rules/action/outputs.rs b/actlint-core/src/rules/action/outputs.rs deleted file mode 100644 index a09af97..0000000 --- a/actlint-core/src/rules/action/outputs.rs +++ /dev/null @@ -1,72 +0,0 @@ -use crate::{ - ast::action::{Action, Runs}, - diagnostic::LintDiagnostic, - source::SourceFile, -}; - -/// Check composite action outputs reference valid step IDs. -pub fn check_outputs(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - let Some(Runs::Composite { steps, .. }) = &action.runs else { - return diagnostics; - }; - - // Collect all step IDs from composite steps - let step_ids: Vec<&str> = - steps.iter().filter_map(|s| s.id.as_ref().map(|id| id.value.as_str())).collect(); - - for output in &action.outputs { - let Some(value) = &output.value else { - continue; - }; - - // Check if value references steps..outputs.* pattern - if let Some(step_ref) = extract_step_reference(&value.value) { - if !step_ids.contains(&step_ref) { - diagnostics.push( - LintDiagnostic::error( - "action/output-invalid-step-ref", - format!("output '{}' references step '{}' which does not exist", output.id, step_ref), - value.span, - source.named_source(), - ) - .with_help(if step_ids.is_empty() { - "no step IDs are defined in this composite action".into() - } else { - format!("available step IDs: {}", step_ids.join(", ")) - }), - ); - } - } - } - - diagnostics -} - -/// Extract step ID from a `steps..outputs.*` reference in an expression. -fn extract_step_reference(value: &str) -> Option<&str> { - // Look for ${{ steps..outputs.* }} pattern - let expr_content = value.strip_prefix("${{")?.strip_suffix("}}")?.trim(); - let rest = expr_content.strip_prefix("steps.")?; - - // Extract the step ID (up to the next dot) - rest.split('.').next() -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn extract_valid_step_ref() { - let result = extract_step_reference("${{ steps.greet.outputs.greeting }}"); - assert_eq!(result, Some("greet")); - } - - #[test] - fn extract_no_step_ref() { - let result = extract_step_reference("hello world"); - assert!(result.is_none()); - } -} diff --git a/actlint-core/src/rules/action/runs.rs b/actlint-core/src/rules/action/runs.rs deleted file mode 100644 index 75b8488..0000000 --- a/actlint-core/src/rules/action/runs.rs +++ /dev/null @@ -1,91 +0,0 @@ -use std::path::Path; - -use crate::{ - ast::action::{Action, Runs}, - diagnostic::LintDiagnostic, - source::SourceFile, -}; - -/// Check runs configuration for file existence and path validity. -pub fn check_runs(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - let Some(runs) = &action.runs else { - return diagnostics; - }; - - let action_dir = source.path.parent(); - - match runs { - Runs::JavaScript { main, pre, post, .. } => { - if let (Some(dir), Some(main_file)) = (action_dir, main) { - check_file_exists(dir, &main_file.value, main_file.span, source, "main", &mut diagnostics); - } - if let (Some(dir), Some(pre_file)) = (action_dir, pre) { - check_file_exists(dir, &pre_file.value, pre_file.span, source, "pre", &mut diagnostics); - } - if let (Some(dir), Some(post_file)) = (action_dir, post) { - check_file_exists(dir, &post_file.value, post_file.span, source, "post", &mut diagnostics); - } - } - Runs::Docker { image, .. } => { - if let Some(img) = image { - check_docker_image(action_dir, &img.value, img.span, source, &mut diagnostics); - } - } - Runs::Composite { .. } => {} - } - - diagnostics -} - -fn check_file_exists( - action_dir: &Path, - file_path: &str, - span: miette::SourceSpan, - source: &SourceFile, - field_name: &str, - diagnostics: &mut Vec, -) { - let resolved = action_dir.join(file_path); - if !resolved.exists() { - diagnostics.push( - LintDiagnostic::warning( - "action/file-not-found", - format!("runs.{field_name} references '{file_path}' which does not exist"), - span, - source.named_source(), - ) - .with_help(format!("expected file at: {}", resolved.display())), - ); - } -} - -fn check_docker_image( - action_dir: Option<&Path>, - image: &str, - span: miette::SourceSpan, - source: &SourceFile, - diagnostics: &mut Vec, -) { - // docker:// URLs are valid remote images - if image.starts_with("docker://") { - return; - } - - // "Dockerfile" or a relative path should exist on disk - if let Some(dir) = action_dir { - let resolved = dir.join(image); - if !resolved.exists() { - diagnostics.push( - LintDiagnostic::warning( - "action/docker-image-not-found", - format!("Docker image path '{image}' does not exist"), - span, - source.named_source(), - ) - .with_help(format!("expected file at: {}", resolved.display())), - ); - } - } -} diff --git a/actlint-core/src/rules/action/schema.rs b/actlint-core/src/rules/action/schema.rs deleted file mode 100644 index f3a52f3..0000000 --- a/actlint-core/src/rules/action/schema.rs +++ /dev/null @@ -1,167 +0,0 @@ -use crate::{ - ast::action::{Action, Runs}, - diagnostic::LintDiagnostic, - source::SourceFile, -}; - -const VALID_USING: &[&str] = &["composite", "node12", "node16", "node20", "node22", "node24", "docker"]; - -/// Check the top-level schema of an action definition. -pub fn check_schema(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - // name is required - if action.name.is_none() { - diagnostics.push(LintDiagnostic::error( - "action/name-required", - "action must have a 'name' field", - action.span, - source.named_source(), - )); - } - - // runs is required - let Some(runs) = &action.runs else { - diagnostics.push(LintDiagnostic::error( - "action/runs-required", - "action must have a 'runs' field", - action.span, - source.named_source(), - )); - return diagnostics; - }; - - match runs { - Runs::Composite { steps, span } => { - if steps.is_empty() { - diagnostics.push(LintDiagnostic::error( - "action/composite-steps-required", - "composite action must have at least one step", - *span, - source.named_source(), - )); - } - } - Runs::JavaScript { using, main, span, .. } => { - if !VALID_USING.contains(&using.value.as_str()) { - diagnostics.push( - LintDiagnostic::error( - "action/invalid-using", - format!( - "invalid runs.using value '{}', expected one of: {}", - using.value, - VALID_USING.join(", ") - ), - using.span, - source.named_source(), - ) - .with_help("valid values are: composite, node12, node16, node20, node22, node24, docker"), - ); - } - if main.is_none() { - diagnostics.push(LintDiagnostic::error( - "action/js-main-required", - "JavaScript action must have a 'main' entry point", - *span, - source.named_source(), - )); - } - } - Runs::Docker { image, span, .. } => { - if image.is_none() { - diagnostics.push(LintDiagnostic::error( - "action/docker-image-required", - "Docker action must have an 'image' field", - *span, - source.named_source(), - )); - } - } - } - - diagnostics -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use super::*; - use crate::ast::action::SpannedString; - - fn source() -> SourceFile { SourceFile::new(PathBuf::from("action.yml"), "name: test\n".into()) } - - fn span() -> miette::SourceSpan { miette::SourceSpan::new(0.into(), 0) } - - #[test] - fn missing_name() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: Some(Runs::Composite { steps: vec![], span: span() }), - branding: None, - span: span(), - }; - let diags = check_schema(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/name-required")); - } - - #[test] - fn missing_runs() { - let action = Action { - name: Some(SpannedString { value: "test".into(), span: span() }), - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: None, - branding: None, - span: span(), - }; - let diags = check_schema(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/runs-required")); - } - - #[test] - fn composite_no_steps() { - let action = Action { - name: Some(SpannedString { value: "test".into(), span: span() }), - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: Some(Runs::Composite { steps: vec![], span: span() }), - branding: None, - span: span(), - }; - let diags = check_schema(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/composite-steps-required")); - } - - #[test] - fn js_no_main() { - let action = Action { - name: Some(SpannedString { value: "test".into(), span: span() }), - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: Some(Runs::JavaScript { - using: SpannedString { value: "node20".into(), span: span() }, - main: None, - pre: None, - pre_if: None, - post: None, - post_if: None, - span: span(), - }), - branding: None, - span: span(), - }; - let diags = check_schema(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/js-main-required")); - } -} diff --git a/actlint-core/src/rules/action_boolean_default_invalid.rs b/actlint-core/src/rules/action_boolean_default_invalid.rs new file mode 100644 index 0000000..9748495 --- /dev/null +++ b/actlint-core/src/rules/action_boolean_default_invalid.rs @@ -0,0 +1,104 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-boolean-default-invalid", + summary: "Boolean input default must be 'true' or 'false'", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let mut diags = Vec::new(); + + for input in &action.inputs { + if let Some(type_field) = &input.type_field { + if type_field.value == "boolean" { + if let Some(default) = &input.default { + if default.value != "true" && default.value != "false" { + diags.push( + LintDiagnostic::error( + "semantic/action-boolean-default-invalid", + format!( + "input '{}' has type 'boolean' but default '{}' is not 'true' or 'false'", + input.id, default.value + ), + default.span, + file.source.named_source(), + ) + .with_help("valid values: 'true', 'false'"), + ); + } + } + } + } + } + + diags +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID: &str = r" +name: My Action +description: test +inputs: + verbose: + description: Verbose output + type: boolean + default: 'yes' +runs: + using: composite + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: My Action +description: test +inputs: + verbose: + description: Verbose output + type: boolean + default: 'true' +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_invalid_default() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-boolean-default-invalid")); + } + + #[test] + fn message_contains_input_name() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().any(|d| d.message.contains("verbose"))); + } + + #[test] + fn help_text_present() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().all(|d| d.help.is_some())); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_choice_default_not_in_options.rs b/actlint-core/src/rules/action_choice_default_not_in_options.rs new file mode 100644 index 0000000..1f90414 --- /dev/null +++ b/actlint-core/src/rules/action_choice_default_not_in_options.rs @@ -0,0 +1,111 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-choice-default-not-in-options", + summary: "Choice input default must be one of the defined options", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let mut diags = Vec::new(); + + for input in &action.inputs { + if let Some(type_field) = &input.type_field { + if type_field.value == "choice" && !input.options.is_empty() { + if let Some(default) = &input.default { + let option_values: Vec<&str> = input.options.iter().map(|o| o.value.as_str()).collect(); + if !option_values.contains(&default.value.as_str()) { + diags.push( + LintDiagnostic::error( + "semantic/action-choice-default-not-in-options", + format!( + "input '{}' has default '{}' which is not in the options list", + input.id, default.value + ), + default.span, + file.source.named_source(), + ) + .with_help(format!("valid options are: {}", option_values.join(", "))), + ); + } + } + } + } + } + + diags +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID: &str = r" +name: My Action +description: test +inputs: + mode: + description: The mode + type: choice + options: + - fast + - slow + default: medium +runs: + using: composite + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: My Action +description: test +inputs: + mode: + description: The mode + type: choice + options: + - fast + - slow + default: fast +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_invalid_default() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-choice-default-not-in-options")); + } + + #[test] + fn message_contains_input_name() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().any(|d| d.message.contains("mode"))); + } + + #[test] + fn help_text_lists_options() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().all(|d| d.help.as_deref().unwrap_or("").contains("fast"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_choice_requires_options.rs b/actlint-core/src/rules/action_choice_requires_options.rs new file mode 100644 index 0000000..907b82a --- /dev/null +++ b/actlint-core/src/rules/action_choice_requires_options.rs @@ -0,0 +1,89 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-choice-requires-options", + summary: "Choice input must define options", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let mut diags = Vec::new(); + + for input in &action.inputs { + if let Some(type_field) = &input.type_field { + if type_field.value == "choice" && input.options.is_empty() { + diags.push(LintDiagnostic::error( + "semantic/action-choice-requires-options", + format!("input '{}' has type 'choice' but no options defined", input.id), + type_field.span, + file.source.named_source(), + )); + } + } + } + + diags +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_OPTIONS: &str = r" +name: My Action +description: test +inputs: + mode: + description: The mode + type: choice +runs: + using: composite + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: My Action +description: test +inputs: + mode: + description: The mode + type: choice + options: + - fast + - slow +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_missing_options() { + let diags = run_rule(&RULE, "action.yml", MISSING_OPTIONS); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-choice-requires-options")); + } + + #[test] + fn message_contains_input_name() { + let diags = run_rule(&RULE, "action.yml", MISSING_OPTIONS); + assert!(diags.iter().any(|d| d.message.contains("mode"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_composite_steps_required.rs b/actlint-core/src/rules/action_composite_steps_required.rs new file mode 100644 index 0000000..4842aa3 --- /dev/null +++ b/actlint-core/src/rules/action_composite_steps_required.rs @@ -0,0 +1,77 @@ +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-composite-steps-required", + summary: "Composite action must have at least one step", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(Runs::Composite { steps, span }) = &action.runs else { + return Vec::new(); + }; + if steps.is_empty() { + vec![LintDiagnostic::error( + "semantic/action-composite-steps-required", + "composite action must have at least one step", + *span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const EMPTY_STEPS: &str = r" +name: My Action +description: A test action +runs: + using: composite + steps: [] +"; + + const VALID: &str = r" +name: My Action +description: A test action +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_empty_steps() { + let diags = run_rule(&RULE, "action.yml", EMPTY_STEPS); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-composite-steps-required")); + // TODO: improve span precision — currently zero-length for some rules + // assert!(diags.iter().all(|d| d.primary_span.len() > 0)); + } + + #[test] + fn message_mentions_step() { + let diags = run_rule(&RULE, "action.yml", EMPTY_STEPS); + assert!(diags.iter().any(|d| d.message.contains("step"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_docker_image_not_found.rs b/actlint-core/src/rules/action_docker_image_not_found.rs new file mode 100644 index 0000000..63c8db7 --- /dev/null +++ b/actlint-core/src/rules/action_docker_image_not_found.rs @@ -0,0 +1,92 @@ +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-docker-image-not-found", + summary: "Docker action local image path must exist on disk", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::DockerAction], + default_severity: Severity::Warning, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(Runs::Docker { image, .. }) = &action.runs else { + return Vec::new(); + }; + let Some(img) = image else { + return Vec::new(); + }; + + // docker:// URLs are valid remote images + if img.value.starts_with("docker://") { + return Vec::new(); + } + + let action_dir = file.source.path.parent(); + let Some(dir) = action_dir else { + return Vec::new(); + }; + + let resolved = dir.join(&img.value); + if !resolved.exists() { + vec![ + LintDiagnostic::warning( + "semantic/action-docker-image-not-found", + format!("Docker image path '{}' does not exist", img.value), + img.span, + file.source.named_source(), + ) + .with_help(format!("expected file at: {}", resolved.display())), + ] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_DOCKERFILE: &str = r" +name: My Action +description: test +runs: + using: docker + image: Dockerfile +"; + + const REMOTE_IMAGE: &str = r" +name: My Action +description: test +runs: + using: docker + image: docker://alpine:3 +"; + + #[test] + fn fires_on_missing_dockerfile() { + let diags = run_rule(&RULE, "action.yml", MISSING_DOCKERFILE); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-docker-image-not-found")); + } + + #[test] + fn message_contains_image_name() { + let diags = run_rule(&RULE, "action.yml", MISSING_DOCKERFILE); + assert!(diags.iter().any(|d| d.message.contains("Dockerfile"))); + } + + #[test] + fn clean_on_remote_image() { assert_rule_clean(&RULE, "action.yml", REMOTE_IMAGE); } +} diff --git a/actlint-core/src/rules/action_docker_image_required.rs b/actlint-core/src/rules/action_docker_image_required.rs new file mode 100644 index 0000000..9d9e10c --- /dev/null +++ b/actlint-core/src/rules/action_docker_image_required.rs @@ -0,0 +1,73 @@ +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-docker-image-required", + summary: "Docker action must have an image field", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(Runs::Docker { image, span, .. }) = &action.runs else { + return Vec::new(); + }; + if image.is_none() { + vec![LintDiagnostic::error( + "semantic/action-docker-image-required", + "Docker action must have an 'image' field", + *span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_IMAGE: &str = r" +name: My Action +description: test +runs: + using: docker +"; + + const VALID: &str = r" +name: My Action +description: test +runs: + using: docker + image: Dockerfile +"; + + #[test] + fn fires_on_missing_image() { + let diags = run_rule(&RULE, "action.yml", MISSING_IMAGE); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-docker-image-required")); + } + + #[test] + fn message_mentions_image() { + let diags = run_rule(&RULE, "action.yml", MISSING_IMAGE); + assert!(diags.iter().any(|d| d.message.contains("image"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_expression_contexts.rs b/actlint-core/src/rules/action_expression_contexts.rs new file mode 100644 index 0000000..3826ce9 --- /dev/null +++ b/actlint-core/src/rules/action_expression_contexts.rs @@ -0,0 +1,194 @@ +use crate::{ + ast::action::{Action, Runs}, + context::availability::{Scope, allowed_contexts}, + diagnostic::{LintDiagnostic, Severity}, + expr, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, + source::SourceFile, +}; + +declare_rule! { + id: "semantic/action-expression-contexts", + summary: "Action expressions must only use contexts available in their scope", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + check_action_expressions(action, file.source, &mut diagnostics); + diagnostics +} + +fn check_action_expressions(action: &Action, source: &SourceFile, diagnostics: &mut Vec) { + let Some(runs) = &action.runs else { + return; + }; + + match runs { + Runs::Composite { steps, .. } => { + let scope = Scope::CompositeStep; + for step in steps { + if let Some(if_cond) = &step.if_cond { + check_expression_string(&if_cond.value, if_cond.span, scope, source, diagnostics); + } + if let Some(run) = &step.run { + check_embedded_expressions(&run.value, run.span, scope, source, diagnostics); + } + for (_, v) in &step.env { + check_embedded_expressions(&v.value, v.span, scope, source, diagnostics); + } + for (_, v) in &step.with { + check_embedded_expressions(&v.value, v.span, scope, source, diagnostics); + } + } + } + Runs::JavaScript { pre_if, post_if, .. } => { + let scope = Scope::JsActionCondition; + if let Some(pre_if) = pre_if { + check_expression_string(&pre_if.value, pre_if.span, scope, source, diagnostics); + } + if let Some(post_if) = post_if { + check_expression_string(&post_if.value, post_if.span, scope, source, diagnostics); + } + } + Runs::Docker { env, .. } => { + let scope = Scope::DockerActionExpr; + for (_, v) in env { + check_embedded_expressions(&v.value, v.span, scope, source, diagnostics); + } + } + } +} + +fn check_embedded_expressions( + value: &str, + span: miette::SourceSpan, + scope: Scope, + source: &SourceFile, + diagnostics: &mut Vec, +) { + for expr_content in extract_expressions(value) { + check_expression_content(&expr_content, span, scope, source, diagnostics); + } +} + +fn check_expression_string( + value: &str, + span: miette::SourceSpan, + scope: Scope, + source: &SourceFile, + diagnostics: &mut Vec, +) { + let expr_content = if let Some(inner) = value.strip_prefix("${{") { + inner.strip_suffix("}}").unwrap_or(inner).trim() + } else { + value.trim() + }; + + if expr_content.is_empty() { + return; + } + + check_expression_content(expr_content, span, scope, source, diagnostics); +} + +fn check_expression_content( + expr_content: &str, + span: miette::SourceSpan, + scope: Scope, + source: &SourceFile, + diagnostics: &mut Vec, +) { + let tokens = match expr::tokenize(expr_content) { + Ok(t) => t, + Err(e) => { + diagnostics.push(LintDiagnostic::error( + "semantic/action-expression-contexts", + format!("expression lex error: {e}"), + span, + source.named_source(), + )); + return; + } + }; + + let ast = match expr::parse_expression(&tokens) { + Ok(a) => a, + Err(e) => { + diagnostics.push(LintDiagnostic::error( + "semantic/action-expression-contexts", + format!("expression parse error: {e}"), + span, + source.named_source(), + )); + return; + } + }; + + let allowed = allowed_contexts(scope); + let errors = expr::check_expression(&ast, allowed); + for error in errors { + diagnostics.push(LintDiagnostic::error( + "semantic/action-expression-contexts", + error.message, + span, + source.named_source(), + )); + } +} + +/// Extract expression contents from `${{ }}` delimiters in a string. +fn extract_expressions(value: &str) -> Vec { + let mut results = Vec::new(); + let mut remaining = value; + + while let Some(start) = remaining.find("${{") { + let after_start = &remaining[start + 3..]; + if let Some(end) = after_start.find("}}") { + results.push(after_start[..end].trim().to_owned()); + remaining = &after_start[end + 2..]; + } else { + break; + } + } + + results +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const VALID_COMPOSITE: &str = r" +name: My Action +description: test +runs: + using: composite + steps: + - run: echo ${{ inputs.name }} +"; + + const VALID_JS: &str = r" +name: My Action +description: test +runs: + using: node20 + main: index.js +"; + + #[test] + fn clean_on_valid_composite() { assert_rule_clean(&RULE, "action.yml", VALID_COMPOSITE); } + + #[test] + fn clean_on_valid_js() { assert_rule_clean(&RULE, "action.yml", VALID_JS); } +} diff --git a/actlint-core/src/rules/action_file_not_found.rs b/actlint-core/src/rules/action_file_not_found.rs new file mode 100644 index 0000000..0deef2f --- /dev/null +++ b/actlint-core/src/rules/action_file_not_found.rs @@ -0,0 +1,103 @@ +use std::path::Path; + +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-file-not-found", + summary: "JavaScript action entry points must exist on disk", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::JavaScriptAction], + default_severity: Severity::Warning, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(Runs::JavaScript { main, pre, post, .. }) = &action.runs else { + return Vec::new(); + }; + + let action_dir = file.source.path.parent(); + + let mut diags = Vec::new(); + + if let (Some(dir), Some(main_file)) = (action_dir, main) { + check_file_exists(dir, &main_file.value, main_file.span, file, "main", &mut diags); + } + if let (Some(dir), Some(pre_file)) = (action_dir, pre) { + check_file_exists(dir, &pre_file.value, pre_file.span, file, "pre", &mut diags); + } + if let (Some(dir), Some(post_file)) = (action_dir, post) { + check_file_exists(dir, &post_file.value, post_file.span, file, "post", &mut diags); + } + + diags +} + +fn check_file_exists( + action_dir: &Path, + file_path: &str, + span: miette::SourceSpan, + file: &LintFile<'_>, + field_name: &str, + diagnostics: &mut Vec, +) { + let resolved = action_dir.join(file_path); + if !resolved.exists() { + diagnostics.push( + LintDiagnostic::warning( + "semantic/action-file-not-found", + format!("runs.{field_name} references '{file_path}' which does not exist"), + span, + file.source.named_source(), + ) + .with_help(format!("expected file at: {}", resolved.display())), + ); + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_FILE: &str = r" +name: My Action +description: test +runs: + using: node20 + main: dist/nonexistent.js +"; + + // This test uses a path that won't resolve to a real file on disk + #[test] + fn fires_on_missing_file() { + let diags = run_rule(&RULE, "action.yml", MISSING_FILE); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-file-not-found")); + } + + #[test] + fn message_contains_file_path() { + let diags = run_rule(&RULE, "action.yml", MISSING_FILE); + assert!(diags.iter().any(|d| d.message.contains("nonexistent"))); + } + + #[test] + fn help_text_present() { + let diags = run_rule(&RULE, "action.yml", MISSING_FILE); + assert!(diags.iter().all(|d| d.help.is_some())); + } + + // Note: clean test is hard without real files on disk. + // The run_rule harness uses a virtual path so no real file exists. +} diff --git a/actlint-core/src/rules/action_invalid_branding_color.rs b/actlint-core/src/rules/action_invalid_branding_color.rs new file mode 100644 index 0000000..ee0b5d0 --- /dev/null +++ b/actlint-core/src/rules/action_invalid_branding_color.rs @@ -0,0 +1,86 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +const VALID_COLORS: &[&str] = + &["white", "black", "yellow", "blue", "green", "orange", "red", "purple", "gray-dark"]; + +declare_rule! { + id: "semantic/action-invalid-branding-color", + summary: "Branding color must be a valid GitHub Actions color", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(branding) = &action.branding else { + return Vec::new(); + }; + let Some(color) = &branding.color else { + return Vec::new(); + }; + + if !VALID_COLORS.contains(&color.value.as_str()) { + vec![LintDiagnostic::error( + "semantic/action-invalid-branding-color", + format!("invalid branding color '{}', expected one of: {}", color.value, VALID_COLORS.join(", ")), + color.span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID: &str = r" +name: My Action +description: test +branding: + color: pink +runs: + using: composite + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: My Action +description: test +branding: + color: blue +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_invalid_color() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-invalid-branding-color")); + } + + #[test] + fn message_contains_color() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().any(|d| d.message.contains("pink"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action/branding.rs b/actlint-core/src/rules/action_invalid_branding_icon.rs similarity index 56% rename from actlint-core/src/rules/action/branding.rs rename to actlint-core/src/rules/action_invalid_branding_icon.rs index 5dd3dab..9c03b9a 100644 --- a/actlint-core/src/rules/action/branding.rs +++ b/actlint-core/src/rules/action_invalid_branding_icon.rs @@ -1,7 +1,7 @@ -use crate::{ast::action::Action, diagnostic::LintDiagnostic, source::SourceFile}; - -const VALID_COLORS: &[&str] = - &["white", "black", "yellow", "blue", "green", "orange", "red", "purple", "gray-dark"]; +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; /// Feather icon names valid for GitHub Actions branding (v4.28.0). const VALID_ICONS: &[&str] = &[ @@ -264,114 +264,90 @@ const VALID_ICONS: &[&str] = &[ "zoom-out", ]; -/// Check branding icon and color values. -pub fn check_branding(action: &Action, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); +declare_rule! { + id: "semantic/action-invalid-branding-icon", + summary: "Branding icon must be a valid Feather icon name", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; let Some(branding) = &action.branding else { - return diagnostics; + return Vec::new(); + }; + let Some(icon) = &branding.icon else { + return Vec::new(); }; - if let Some(icon) = &branding.icon { - if !VALID_ICONS.contains(&icon.value.as_str()) { - diagnostics.push( - LintDiagnostic::error( - "action/invalid-branding-icon", - format!("invalid branding icon '{}'", icon.value), - icon.span, - source.named_source(), - ) - .with_help("see https://feathericons.com for valid icon names"), - ); - } - } - - if let Some(color) = &branding.color { - if !VALID_COLORS.contains(&color.value.as_str()) { - diagnostics.push(LintDiagnostic::error( - "action/invalid-branding-color", - format!( - "invalid branding color '{}', expected one of: {}", - color.value, - VALID_COLORS.join(", ") - ), - color.span, - source.named_source(), - )); - } + if !VALID_ICONS.contains(&icon.value.as_str()) { + vec![ + LintDiagnostic::error( + "semantic/action-invalid-branding-icon", + format!("invalid branding icon '{}'", icon.value), + icon.span, + file.source.named_source(), + ) + .with_help("see https://feathericons.com for valid icon names"), + ] + } else { + Vec::new() } - - diagnostics } #[cfg(test)] +#[allow(clippy::panic)] mod tests { - use std::path::PathBuf; - use super::*; - use crate::ast::action::{Branding, SpannedString}; + use crate::rules::testing::{assert_rule_clean, run_rule}; - fn source() -> SourceFile { SourceFile::new(PathBuf::from("action.yml"), "name: test\n".into()) } + const INVALID: &str = r" +name: My Action +description: test +branding: + icon: coffee +runs: + using: composite + steps: + - run: echo hi +"; - fn span() -> miette::SourceSpan { miette::SourceSpan::new(0.into(), 0) } + const VALID: &str = r" +name: My Action +description: test +branding: + icon: activity +runs: + using: composite + steps: + - run: echo hi +"; #[test] - fn valid_branding() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: None, - branding: Some(Branding { - icon: Some(SpannedString { value: "activity".into(), span: span() }), - color: Some(SpannedString { value: "blue".into(), span: span() }), - span: span(), - }), - span: span(), - }; - let diags = check_branding(&action, &source()); - assert!(diags.is_empty()); + fn fires_on_invalid_icon() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-invalid-branding-icon")); } #[test] - fn invalid_icon() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: None, - branding: Some(Branding { - icon: Some(SpannedString { value: "coffee".into(), span: span() }), - color: None, - span: span(), - }), - span: span(), - }; - let diags = check_branding(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/invalid-branding-icon")); + fn message_contains_icon_name() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().any(|d| d.message.contains("coffee"))); } #[test] - fn invalid_color() { - let action = Action { - name: None, - description: None, - author: None, - inputs: vec![], - outputs: vec![], - runs: None, - branding: Some(Branding { - icon: None, - color: Some(SpannedString { value: "pink".into(), span: span() }), - span: span(), - }), - span: span(), - }; - let diags = check_branding(&action, &source()); - assert!(diags.iter().any(|d| d.rule_name == "action/invalid-branding-color")); + fn help_text_present() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().all(|d| d.help.is_some())); } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } } diff --git a/actlint-core/src/rules/action_invalid_input_type.rs b/actlint-core/src/rules/action_invalid_input_type.rs new file mode 100644 index 0000000..5275e53 --- /dev/null +++ b/actlint-core/src/rules/action_invalid_input_type.rs @@ -0,0 +1,104 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +const VALID_INPUT_TYPES: &[&str] = &["string", "boolean", "choice", "environment"]; + +declare_rule! { + id: "semantic/action-invalid-input-type", + summary: "Action input type must be a valid value", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let mut diags = Vec::new(); + + for input in &action.inputs { + if let Some(type_field) = &input.type_field { + if !VALID_INPUT_TYPES.contains(&type_field.value.as_str()) { + diags.push( + LintDiagnostic::error( + "semantic/action-invalid-input-type", + format!( + "input '{}' has invalid type '{}', expected one of: {}", + input.id, + type_field.value, + VALID_INPUT_TYPES.join(", ") + ), + type_field.span, + file.source.named_source(), + ) + .with_help("valid types are: string, boolean, choice, environment"), + ); + } + } + } + + diags +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID: &str = r" +name: My Action +description: test +inputs: + mode: + description: The mode + type: invalid +runs: + using: composite + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: My Action +description: test +inputs: + mode: + description: The mode + type: string +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_invalid_type() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-invalid-input-type")); + // TODO: improve span precision — currently zero-length for some rules + // assert!(diags.iter().all(|d| d.primary_span.len() > 0)); + } + + #[test] + fn message_contains_input_name() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().any(|d| d.message.contains("mode"))); + } + + #[test] + fn help_text_present() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().all(|d| d.help.is_some())); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_invalid_using.rs b/actlint-core/src/rules/action_invalid_using.rs new file mode 100644 index 0000000..c482f28 --- /dev/null +++ b/actlint-core/src/rules/action_invalid_using.rs @@ -0,0 +1,89 @@ +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +const VALID_USING: &[&str] = &["composite", "node12", "node16", "node20", "node22", "node24", "docker"]; + +declare_rule! { + id: "semantic/action-invalid-using", + summary: "Action runs.using must be a valid runtime", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(runs) = &action.runs else { + return Vec::new(); + }; + + let using = match runs { + Runs::JavaScript { using, .. } | Runs::Docker { using, .. } => using, + // Unknown `using` values are parsed as Composite by the detect module; + // we still need to validate them. However, Runs::Composite does not + // carry a `using` field since composite is the expected value. + Runs::Composite { .. } => return Vec::new(), + }; + + if !VALID_USING.contains(&using.value.as_str()) { + vec![ + LintDiagnostic::error( + "semantic/action-invalid-using", + format!( + "invalid runs.using value '{}', expected one of: {}", + using.value, + VALID_USING.join(", ") + ), + using.span, + file.source.named_source(), + ) + .with_label(using.span, "invalid value here") + .with_help("valid values are: composite, node12, node16, node20, node22, node24, docker"), + ] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const VALID: &str = r" +name: My Action +description: test +runs: + using: node20 + main: index.js +"; + + const VALID_DOCKER: &str = r" +name: My Action +description: test +runs: + using: docker + image: Dockerfile +"; + + // Note: Unknown `using` values like "node10" get classified as CompositeAction + // by the detect module, and the parser emits its own diagnostic. This rule + // only fires for known JS/Docker variants with bad `using` values, but + // currently all recognized variants have valid using values by construction. + // The rule exists as a guard for future runtime additions. + + #[test] + fn clean_on_valid_js() { assert_rule_clean(&RULE, "action.yml", VALID); } + + #[test] + fn clean_on_valid_docker() { assert_rule_clean(&RULE, "action.yml", VALID_DOCKER); } +} diff --git a/actlint-core/src/rules/action_js_main_required.rs b/actlint-core/src/rules/action_js_main_required.rs new file mode 100644 index 0000000..3ca794b --- /dev/null +++ b/actlint-core/src/rules/action_js_main_required.rs @@ -0,0 +1,73 @@ +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-js-main-required", + summary: "JavaScript action must have a main entry point", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::JavaScriptAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(Runs::JavaScript { main, span, .. }) = &action.runs else { + return Vec::new(); + }; + if main.is_none() { + vec![LintDiagnostic::error( + "semantic/action-js-main-required", + "JavaScript action must have a 'main' entry point", + *span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_MAIN: &str = r" +name: My Action +description: test +runs: + using: node20 +"; + + const VALID: &str = r" +name: My Action +description: test +runs: + using: node20 + main: index.js +"; + + #[test] + fn fires_on_missing_main() { + let diags = run_rule(&RULE, "action.yml", MISSING_MAIN); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-js-main-required")); + } + + #[test] + fn message_mentions_main() { + let diags = run_rule(&RULE, "action.yml", MISSING_MAIN); + assert!(diags.iter().any(|d| d.message.contains("main"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_name_required.rs b/actlint-core/src/rules/action_name_required.rs new file mode 100644 index 0000000..27967c9 --- /dev/null +++ b/actlint-core/src/rules/action_name_required.rs @@ -0,0 +1,74 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-name-required", + summary: "Action must have a name field", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + if action.name.is_none() { + // TODO: improve span precision + vec![LintDiagnostic::error( + "semantic/action-name-required", + "action must have a 'name' field", + action.span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_NAME: &str = r" +description: A test action +runs: + using: composite + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: My Action +description: A test action +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_missing_name() { + let diags = run_rule(&RULE, "action.yml", MISSING_NAME); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-name-required")); + // TODO: improve span precision — currently zero-length for file-level spans + // assert!(diags.iter().all(|d| d.primary_span.len() > 0)); + } + + #[test] + fn message_mentions_name() { + let diags = run_rule(&RULE, "action.yml", MISSING_NAME); + assert!(diags.iter().any(|d| d.message.contains("name"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_output_invalid_step_ref.rs b/actlint-core/src/rules/action_output_invalid_step_ref.rs new file mode 100644 index 0000000..8d518b3 --- /dev/null +++ b/actlint-core/src/rules/action_output_invalid_step_ref.rs @@ -0,0 +1,120 @@ +use crate::{ + ast::action::Runs, + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-output-invalid-step-ref", + summary: "Composite action outputs must reference valid step IDs", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + let Some(Runs::Composite { steps, .. }) = &action.runs else { + return Vec::new(); + }; + + let step_ids: Vec<&str> = + steps.iter().filter_map(|s| s.id.as_ref().map(|id| id.value.as_str())).collect(); + + let mut diags = Vec::new(); + + for output in &action.outputs { + let Some(value) = &output.value else { + continue; + }; + + if let Some(step_ref) = extract_step_reference(&value.value) { + if !step_ids.contains(&step_ref) { + diags.push( + LintDiagnostic::error( + "semantic/action-output-invalid-step-ref", + format!("output '{}' references step '{}' which does not exist", output.id, step_ref), + value.span, + file.source.named_source(), + ) + .with_help(if step_ids.is_empty() { + "no step IDs are defined in this composite action".into() + } else { + format!("available step IDs: {}", step_ids.join(", ")) + }), + ); + } + } + } + + diags +} + +/// Extract step ID from a `steps..outputs.*` reference in an expression. +fn extract_step_reference(value: &str) -> Option<&str> { + let expr_content = value.strip_prefix("${{")?.strip_suffix("}}")?.trim(); + let rest = expr_content.strip_prefix("steps.")?; + rest.split('.').next() +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID: &str = r#" +name: My Action +description: test +outputs: + greeting: + description: The greeting + value: ${{ steps.nonexistent.outputs.greeting }} +runs: + using: composite + steps: + - id: greet + run: echo hello +"#; + + const VALID: &str = r#" +name: My Action +description: test +outputs: + greeting: + description: The greeting + value: ${{ steps.greet.outputs.greeting }} +runs: + using: composite + steps: + - id: greet + run: echo hello +"#; + + #[test] + fn fires_on_invalid_ref() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-output-invalid-step-ref")); + } + + #[test] + fn message_contains_step_id() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().any(|d| d.message.contains("nonexistent"))); + } + + #[test] + fn help_text_present() { + let diags = run_rule(&RULE, "action.yml", INVALID); + assert!(diags.iter().all(|d| d.help.is_some())); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/action_runs_required.rs b/actlint-core/src/rules/action_runs_required.rs new file mode 100644 index 0000000..c9b1466 --- /dev/null +++ b/actlint-core/src/rules/action_runs_required.rs @@ -0,0 +1,69 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/action-runs-required", + summary: "Action must have a runs field", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::CompositeAction, RuleTarget::JavaScriptAction, RuleTarget::DockerAction], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Action(action)) = &file.ast else { + return Vec::new(); + }; + if action.runs.is_none() { + // TODO: improve span precision + vec![LintDiagnostic::error( + "semantic/action-runs-required", + "action must have a 'runs' field", + action.span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_RUNS: &str = r" +name: My Action +description: A test action +"; + + const VALID: &str = r" +name: My Action +description: A test action +runs: + using: composite + steps: + - run: echo hi +"; + + #[test] + fn fires_on_missing_runs() { + let diags = run_rule(&RULE, "action.yml", MISSING_RUNS); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/action-runs-required")); + } + + #[test] + fn message_mentions_runs() { + let diags = run_rule(&RULE, "action.yml", MISSING_RUNS); + assert!(diags.iter().any(|d| d.message.contains("runs"))); + } + + #[test] + fn clean_on_valid() { assert_rule_clean(&RULE, "action.yml", VALID); } +} diff --git a/actlint-core/src/rules/mod.rs b/actlint-core/src/rules/mod.rs index 86bb184..2027c63 100644 --- a/actlint-core/src/rules/mod.rs +++ b/actlint-core/src/rules/mod.rs @@ -1,2 +1,251 @@ -pub mod action; -pub mod workflow; +use crate::detect::FileKind; + +// ── Types ────────────────────────────────────────────────────────────────── + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RuleNamespace { + Schema, + Semantic, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RulePhase { + Schema, + Semantic, +} + +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum RuleTarget { + Workflow, + CompositeAction, + JavaScriptAction, + DockerAction, +} + +pub enum AstKind<'src> { + Workflow(&'src crate::ast::workflow::Workflow), + Action(&'src crate::ast::action::Action), +} + +impl<'src> AstKind<'src> { + pub fn as_workflow(&self) -> Option<&'src crate::ast::workflow::Workflow> { + match self { + Self::Workflow(w) => Some(w), + Self::Action(_) => None, + } + } + pub fn as_action(&self) -> Option<&'src crate::ast::action::Action> { + match self { + Self::Action(a) => Some(a), + Self::Workflow(_) => None, + } + } +} + +pub struct LintFile<'src> { + pub source: &'src crate::source::SourceFile, + pub kind: FileKind, + /// None during schema phase; Some during semantic phase. + pub ast: Option>, + pub config: &'src crate::linter::LintConfig, +} + +pub type RuleCheckFn = for<'src> fn(&LintFile<'src>) -> Vec; + +pub struct RuleDef { + pub id: &'static str, + pub summary: &'static str, + pub namespace: RuleNamespace, + pub phase: RulePhase, + pub targets: &'static [RuleTarget], + pub default_severity: crate::diagnostic::Severity, + pub supports_fix: bool, + pub check: RuleCheckFn, +} + +// ── Macros ───────────────────────────────────────────────────────────────── + +/// Declare a lint rule. Generates `pub(crate) const RULE: RuleDef` in the +/// calling module. The `const _: RuleCheckFn = $check` binding enforces the +/// function signature at compile time. +#[macro_export] +macro_rules! declare_rule { + ( + id: $id:literal, + summary: $summary:literal, + namespace: $namespace:expr, + phase: $phase:expr, + targets: [$($target:expr),+ $(,)?], + default_severity: $severity:expr, + supports_fix: $supports_fix:expr, + check: $check:path $(,)? + ) => { + const _: $crate::rules::RuleCheckFn = $check; + + pub const RULE: $crate::rules::RuleDef = $crate::rules::RuleDef { + id: $id, + summary: $summary, + namespace: $namespace, + phase: $phase, + targets: &[$($target),+], + default_severity: $severity, + supports_fix: $supports_fix, + check: $check, + }; + }; +} + +/// Declare all rule modules and build ALL_RULES. +/// To add a rule: add its module name here and create the corresponding file. +macro_rules! register_rules { + ($($module:ident),+ $(,)?) => { + $(pub mod $module;)+ + + pub static ALL_RULES: &[&RuleDef] = &[ + $(&$module::RULE,)+ + ]; + }; +} + +// ── Rule inventory ───────────────────────────────────────────────────────── + +register_rules![ + // Workflow semantic rules (from jobs.rs) + workflow_no_jobs, + workflow_invalid_job_id, + workflow_missing_runs_on, + workflow_invalid_needs_ref, + workflow_circular_needs, + // Workflow step rules (from steps.rs) + workflow_step_uses_and_run, + workflow_step_no_action, + workflow_invalid_uses, + workflow_duplicate_step_id, + // Workflow expression rules (from workflow/expressions.rs) + workflow_expression_contexts, + // Action schema rules (from action/schema.rs) + action_name_required, + action_runs_required, + action_composite_steps_required, + action_invalid_using, + action_js_main_required, + action_docker_image_required, + // Action input rules (from action/inputs.rs) + action_invalid_input_type, + action_boolean_default_invalid, + action_choice_requires_options, + action_choice_default_not_in_options, + // Action branding rules (from action/branding.rs) + action_invalid_branding_color, + action_invalid_branding_icon, + // Action output rules (from action/outputs.rs) + action_output_invalid_step_ref, + // Action runs rules (from action/runs.rs) + action_file_not_found, + action_docker_image_not_found, + // Action expression rules (from action/expressions.rs) + action_expression_contexts, +]; + +// Legacy action/ and workflow/ directories have been removed. +// All rules are now registered in the flat rule files above. + +// ── Registry validation tests ────────────────────────────────────────────── +#[cfg(test)] +mod registry_tests { + use super::*; + + #[test] + fn all_rule_ids_are_unique() { + let mut ids: Vec<&str> = ALL_RULES.iter().map(|r| r.id).collect(); + ids.sort_unstable(); + let original_len = ids.len(); + ids.dedup(); + assert_eq!(ids.len(), original_len, "duplicate rule IDs detected"); + } + + #[test] + fn all_ids_have_known_namespace_prefix() { + for rule in ALL_RULES { + assert!( + rule.id.starts_with("schema/") || rule.id.starts_with("semantic/"), + "rule '{}' has unknown namespace prefix", + rule.id + ); + } + } + + #[test] + fn schema_namespace_implies_schema_phase() { + for rule in ALL_RULES { + if rule.namespace == RuleNamespace::Schema { + assert_eq!( + rule.phase, + RulePhase::Schema, + "rule '{}' has Schema namespace but Semantic phase", + rule.id + ); + } + } + } + + #[test] + fn no_empty_ids_or_summaries() { + for rule in ALL_RULES { + assert!(!rule.id.is_empty(), "empty rule id"); + assert!(!rule.summary.is_empty(), "empty summary for rule '{}'", rule.id); + } + } +} + +// ── Shared test harness (cfg(test) only) ─────────────────────────────────── +#[cfg(test)] +pub mod testing { + use std::path::PathBuf; + + use super::*; + use crate::{detect::classify, diagnostic::LintDiagnostic, linter::LintConfig, source::SourceFile}; + + /// Run a single rule against a YAML string and return diagnostics. + /// Handles classify -> parse -> build LintFile -> invoke rule.check. + pub fn run_rule(rule: &RuleDef, filename: &str, content: &str) -> Vec { + let path = PathBuf::from(filename); + let source = SourceFile::new(path.clone(), content.to_owned()); + let config = LintConfig::default(); + + let Some(kind) = classify(&path, content) else { + panic!("run_rule: classify returned None for '{filename}'"); + }; + + if rule.phase == RulePhase::Schema { + let file = LintFile { source: &source, kind, ast: None, config: &config }; + return (rule.check)(&file); + } + + match kind { + crate::detect::FileKind::Workflow => match crate::parse::workflow::parse_workflow(&source) { + Ok(wf) => { + let ast = AstKind::Workflow(&wf); + let file = LintFile { source: &source, kind, ast: Some(ast), config: &config }; + (rule.check)(&file) + } + Err(diags) => diags, + }, + _ => match crate::parse::action::parse_action(&source) { + Ok(action) => { + let ast = AstKind::Action(&action); + let file = LintFile { source: &source, kind, ast: Some(ast), config: &config }; + (rule.check)(&file) + } + Err(diags) => diags, + }, + } + } + + /// Assert a rule produces zero diagnostics. + #[track_caller] + pub fn assert_rule_clean(rule: &RuleDef, filename: &str, content: &str) { + let diags = run_rule(rule, filename, content); + assert!(diags.is_empty(), "rule '{}' fired unexpectedly on clean input:\n{:#?}", rule.id, diags); + } +} diff --git a/actlint-core/src/rules/workflow/jobs.rs b/actlint-core/src/rules/workflow/jobs.rs deleted file mode 100644 index 8b0588d..0000000 --- a/actlint-core/src/rules/workflow/jobs.rs +++ /dev/null @@ -1,284 +0,0 @@ -use std::collections::HashSet; - -use crate::{ast::workflow::Workflow, diagnostic::LintDiagnostic, source::SourceFile}; - -/// Check workflow job definitions. -pub fn check_jobs(workflow: &Workflow, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - // At least one job required - if workflow.jobs.is_empty() { - diagnostics.push(LintDiagnostic::error( - "workflow/no-jobs", - "workflow must have at least one job", - workflow.span, - source.named_source(), - )); - return diagnostics; - } - - let job_ids: HashSet<&str> = workflow.jobs.iter().map(|j| j.id.as_str()).collect(); - - for job in &workflow.jobs { - // Job ID must be a valid identifier - if !is_valid_identifier(&job.id) { - diagnostics.push(LintDiagnostic::error( - "workflow/invalid-job-id", - format!("job ID '{}' is not a valid identifier", job.id), - job.span, - source.named_source(), - )); - } - - // runs-on is required (for non-reusable workflow jobs) - if job.runs_on.is_none() { - diagnostics.push(LintDiagnostic::warning( - "workflow/missing-runs-on", - format!("job '{}' is missing 'runs-on'", job.id), - job.span, - source.named_source(), - )); - } - - // needs references must exist - for need in &job.needs { - if !job_ids.contains(need.value.as_str()) { - diagnostics.push(LintDiagnostic::error( - "workflow/invalid-needs-ref", - format!("job '{}' needs '{}' which does not exist", job.id, need.value), - need.span, - source.named_source(), - )); - } - } - } - - // Check for circular needs (simple cycle detection) - check_circular_needs(workflow, source, &mut diagnostics); - - diagnostics -} - -/// Check for circular dependencies in job needs. -fn check_circular_needs(workflow: &Workflow, source: &SourceFile, diagnostics: &mut Vec) { - let job_count = workflow.jobs.len(); - let job_ids: Vec<&str> = workflow.jobs.iter().map(|j| j.id.as_str()).collect(); - - // Build adjacency: job -> list of jobs it depends on - let mut needs_map: std::collections::HashMap<&str, Vec<&str>> = std::collections::HashMap::new(); - for job in &workflow.jobs { - let deps: Vec<&str> = job - .needs - .iter() - .filter_map(|n| if job_ids.contains(&n.value.as_str()) { Some(n.value.as_str()) } else { None }) - .collect(); - needs_map.insert(job.id.as_str(), deps); - } - - // Topological sort to detect cycles - let mut visited = HashSet::new(); - let mut in_stack = HashSet::new(); - - for job_id in &job_ids { - if has_cycle(job_id, &needs_map, &mut visited, &mut in_stack) { - // Find the job for reporting - if let Some(job) = workflow.jobs.iter().find(|j| j.id == *job_id) { - diagnostics.push(LintDiagnostic::error( - "workflow/circular-needs", - format!("job '{}' is part of a circular dependency", job.id), - job.span, - source.named_source(), - )); - } - break; // Report first cycle found - } - } - - // Silence unused variable warning in a clippy-clean way - let _ = job_count; -} - -fn has_cycle<'a>( - node: &'a str, - needs_map: &std::collections::HashMap<&'a str, Vec<&'a str>>, - visited: &mut HashSet<&'a str>, - in_stack: &mut HashSet<&'a str>, -) -> bool { - if in_stack.contains(node) { - return true; - } - if visited.contains(node) { - return false; - } - - visited.insert(node); - in_stack.insert(node); - - if let Some(deps) = needs_map.get(node) { - for dep in deps { - if has_cycle(dep, needs_map, visited, in_stack) { - return true; - } - } - } - - in_stack.remove(node); - false -} - -fn is_valid_identifier(s: &str) -> bool { - if s.is_empty() { - return false; - } - let mut chars = s.chars(); - let first = chars.next(); - first.is_some_and(|c| c.is_alphabetic() || c == '_' || c == '-') - && chars.all(|c| c.is_alphanumeric() || c == '_' || c == '-') -} - -#[cfg(test)] -mod tests { - use std::path::PathBuf; - - use super::*; - use crate::ast::{ - action::SpannedString, - workflow::{Job, Workflow}, - }; - - fn source() -> SourceFile { - SourceFile::new(PathBuf::from(".github/workflows/ci.yml"), "name: CI\n".into()) - } - - fn span() -> miette::SourceSpan { miette::SourceSpan::new(0.into(), 0) } - - fn job(id: &str, needs: Vec<&str>) -> Job { - Job { - id: id.into(), - name: None, - runs_on: Some(SpannedString { value: "ubuntu-latest".into(), span: span() }), - needs: needs.into_iter().map(|n| SpannedString { value: n.into(), span: span() }).collect(), - permissions: None, - env: vec![], - if_cond: None, - steps: vec![], - strategy: None, - outputs: vec![], - span: span(), - } - } - - #[test] - fn no_jobs() { - let wf = - Workflow { name: None, on: vec![], env: vec![], permissions: None, jobs: vec![], span: span() }; - let diags = check_jobs(&wf, &source()); - assert!(diags.iter().any(|d| d.rule_name == "workflow/no-jobs")); - } - - #[test] - fn invalid_needs_ref() { - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("test", vec!["nonexistent"])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(diags.iter().any(|d| d.rule_name == "workflow/invalid-needs-ref")); - } - - #[test] - fn valid_needs() { - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("build", vec![]), job("test", vec!["build"])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(!diags.iter().any(|d| d.rule_name == "workflow/invalid-needs-ref")); - } - - // --- Edge case: Circular needs --- - - #[test] - fn simple_circular_needs_detected() { - // A needs B, B needs A - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("a", vec!["b"]), job("b", vec!["a"])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(diags.iter().any(|d| d.rule_name == "workflow/circular-needs")); - } - - #[test] - fn self_referencing_needs_detected() { - // A needs A (self-cycle) - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("a", vec!["a"])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(diags.iter().any(|d| d.rule_name == "workflow/circular-needs")); - } - - #[test] - fn three_node_cycle_detected() { - // A -> B -> C -> A - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("a", vec!["c"]), job("b", vec!["a"]), job("c", vec!["b"])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(diags.iter().any(|d| d.rule_name == "workflow/circular-needs")); - } - - #[test] - fn diamond_dependency_no_cycle() { - // A -> B, A -> C, B -> D, C -> D (no cycle) - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("d", vec![]), job("b", vec!["d"]), job("c", vec!["d"]), job("a", vec!["b", "c"])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(!diags.iter().any(|d| d.rule_name == "workflow/circular-needs")); - } - - // --- Edge case: Job ID validation --- - - #[test] - fn empty_job_id_invalid() { - let wf = Workflow { - name: None, - on: vec![], - env: vec![], - permissions: None, - jobs: vec![job("", vec![])], - span: span(), - }; - let diags = check_jobs(&wf, &source()); - assert!(diags.iter().any(|d| d.rule_name == "workflow/invalid-job-id")); - } -} diff --git a/actlint-core/src/rules/workflow/mod.rs b/actlint-core/src/rules/workflow/mod.rs deleted file mode 100644 index 4d2790f..0000000 --- a/actlint-core/src/rules/workflow/mod.rs +++ /dev/null @@ -1,14 +0,0 @@ -pub mod expressions; -pub mod jobs; -pub mod steps; - -use crate::{ast::workflow::Workflow, diagnostic::LintDiagnostic, source::SourceFile}; - -/// Run all workflow lint rules against a parsed workflow AST. -pub fn lint_workflow(workflow: &Workflow, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - diagnostics.extend(jobs::check_jobs(workflow, source)); - diagnostics.extend(steps::check_steps(workflow, source)); - diagnostics.extend(expressions::check_expressions(workflow, source)); - diagnostics -} diff --git a/actlint-core/src/rules/workflow/steps.rs b/actlint-core/src/rules/workflow/steps.rs deleted file mode 100644 index d5fff7f..0000000 --- a/actlint-core/src/rules/workflow/steps.rs +++ /dev/null @@ -1,112 +0,0 @@ -use std::collections::HashSet; - -use crate::{ast::workflow::Workflow, diagnostic::LintDiagnostic, source::SourceFile}; - -/// Check workflow step definitions. -pub fn check_steps(workflow: &Workflow, source: &SourceFile) -> Vec { - let mut diagnostics = Vec::new(); - - for job in &workflow.jobs { - let mut step_ids = HashSet::new(); - - for (i, step) in job.steps.iter().enumerate() { - let step_label = - step.name.as_ref().map_or_else(|| format!("step {}", i + 1), |n| n.value.clone()); - - // Each step must have `uses` or `run` (not both, not neither) - let has_uses = step.uses.is_some(); - let has_run = step.run.is_some(); - - if has_uses && has_run { - diagnostics.push(LintDiagnostic::error( - "workflow/step-uses-and-run", - format!("job '{}', {step_label}: step cannot have both 'uses' and 'run'", job.id), - step.span, - source.named_source(), - )); - } else if !has_uses && !has_run { - diagnostics.push(LintDiagnostic::error( - "workflow/step-no-action", - format!("job '{}', {step_label}: step must have either 'uses' or 'run'", job.id), - step.span, - source.named_source(), - )); - } - - // Validate uses format - if let Some(uses) = &step.uses { - if !is_valid_uses(&uses.value) { - diagnostics.push( - LintDiagnostic::warning( - "workflow/invalid-uses", - format!("job '{}', {step_label}: uses value '{}' may not be valid", job.id, uses.value), - uses.span, - source.named_source(), - ) - .with_help("expected format: owner/repo@ref, owner/repo/path@ref, ./local-path, or docker://image"), - ); - } - } - - // Step id must be unique within job - if let Some(id) = &step.id { - if !step_ids.insert(id.value.clone()) { - diagnostics.push(LintDiagnostic::error( - "workflow/duplicate-step-id", - format!("job '{}': duplicate step ID '{}'", job.id, id.value), - id.span, - source.named_source(), - )); - } - } - } - } - - diagnostics -} - -/// Validate the format of a `uses` value. -fn is_valid_uses(uses: &str) -> bool { - // Local path - if uses.starts_with("./") { - return true; - } - - // Docker image - if uses.starts_with("docker://") { - return true; - } - - // owner/repo@ref or owner/repo/path@ref - if let Some(at_pos) = uses.rfind('@') { - let before_at = &uses[..at_pos]; - let after_at = &uses[at_pos + 1..]; - - // Must have at least one slash and a non-empty ref - if before_at.contains('/') && !after_at.is_empty() { - return true; - } - } - - false -} - -#[cfg(test)] -mod tests { - use super::*; - - #[test] - fn valid_uses_formats() { - assert!(is_valid_uses("actions/checkout@v4")); - assert!(is_valid_uses("owner/repo/path@main")); - assert!(is_valid_uses("./local-action")); - assert!(is_valid_uses("docker://alpine:3.18")); - } - - #[test] - fn invalid_uses_formats() { - assert!(!is_valid_uses("just-a-name")); - assert!(!is_valid_uses("no-at-sign/repo")); - assert!(!is_valid_uses("@v4")); - } -} diff --git a/actlint-core/src/rules/workflow_circular_needs.rs b/actlint-core/src/rules/workflow_circular_needs.rs new file mode 100644 index 0000000..1aa9b1c --- /dev/null +++ b/actlint-core/src/rules/workflow_circular_needs.rs @@ -0,0 +1,195 @@ +use std::collections::{HashMap, HashSet}; + +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-circular-needs", + summary: "Job dependency graph must be acyclic", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + let job_ids: Vec<&str> = workflow.jobs.iter().map(|j| j.id.as_str()).collect(); + + // Build adjacency: job -> list of jobs it depends on (only valid IDs) + let mut needs_map: HashMap<&str, Vec<&str>> = HashMap::new(); + for job in &workflow.jobs { + let deps: Vec<&str> = job + .needs + .iter() + .filter_map(|n| if job_ids.contains(&n.value.as_str()) { Some(n.value.as_str()) } else { None }) + .collect(); + needs_map.insert(job.id.as_str(), deps); + } + + let mut visited: HashSet<&str> = HashSet::new(); + let mut in_stack: HashSet<&str> = HashSet::new(); + + for job_id in &job_ids { + if has_cycle(job_id, &needs_map, &mut visited, &mut in_stack) { + if let Some(job) = workflow.jobs.iter().find(|j| j.id == *job_id) { + diagnostics.push( + LintDiagnostic::error( + "semantic/workflow-circular-needs", + format!("job '{}' is part of a circular dependency", job.id), + job.span, + file.source.named_source(), + ) + .with_help("remove or rearrange 'needs' to break the cycle"), + ); + } + break; // Report first cycle found + } + } + + diagnostics +} + +fn has_cycle<'a>( + node: &'a str, + needs_map: &HashMap<&'a str, Vec<&'a str>>, + visited: &mut HashSet<&'a str>, + in_stack: &mut HashSet<&'a str>, +) -> bool { + if in_stack.contains(node) { + return true; + } + if visited.contains(node) { + return false; + } + + visited.insert(node); + in_stack.insert(node); + + if let Some(deps) = needs_map.get(node) { + for dep in deps { + if has_cycle(dep, needs_map, visited, in_stack) { + return true; + } + } + } + + in_stack.remove(node); + false +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const SIMPLE_CYCLE: &str = r" +name: CI +on: push +jobs: + a: + runs-on: ubuntu-latest + needs: [b] + steps: + - run: echo a + b: + runs-on: ubuntu-latest + needs: [a] + steps: + - run: echo b +"; + + const SELF_CYCLE: &str = r" +name: CI +on: push +jobs: + a: + runs-on: ubuntu-latest + needs: [a] + steps: + - run: echo a +"; + + const THREE_NODE_CYCLE: &str = r" +name: CI +on: push +jobs: + a: + runs-on: ubuntu-latest + needs: [c] + steps: + - run: echo a + b: + runs-on: ubuntu-latest + needs: [a] + steps: + - run: echo b + c: + runs-on: ubuntu-latest + needs: [b] + steps: + - run: echo c +"; + + const DIAMOND: &str = r" +name: CI +on: push +jobs: + d: + runs-on: ubuntu-latest + steps: + - run: echo d + b: + runs-on: ubuntu-latest + needs: [d] + steps: + - run: echo b + c: + runs-on: ubuntu-latest + needs: [d] + steps: + - run: echo c + a: + runs-on: ubuntu-latest + needs: [b, c] + steps: + - run: echo a +"; + + #[test] + fn fires_on_simple_cycle() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", SIMPLE_CYCLE); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-circular-needs")); + // Diagnostic quality: message mentions "circular" or "cycle" + assert!(diags.iter().all(|d| d.message.contains("circular") || d.message.contains("cycle"))); + // Help text present + assert!(diags.iter().all(|d| d.help.is_some())); + } + + #[test] + fn fires_on_self_cycle() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", SELF_CYCLE); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-circular-needs")); + } + + #[test] + fn fires_on_three_node_cycle() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", THREE_NODE_CYCLE); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-circular-needs")); + } + + #[test] + fn clean_on_diamond() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", DIAMOND); } +} diff --git a/actlint-core/src/rules/workflow_duplicate_step_id.rs b/actlint-core/src/rules/workflow_duplicate_step_id.rs new file mode 100644 index 0000000..6ab1df2 --- /dev/null +++ b/actlint-core/src/rules/workflow_duplicate_step_id.rs @@ -0,0 +1,86 @@ +use std::collections::HashSet; + +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-duplicate-step-id", + summary: "Step IDs must be unique within a job", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + for job in &workflow.jobs { + let mut step_ids: HashSet = HashSet::new(); + + for step in &job.steps { + if let Some(id) = &step.id { + if !step_ids.insert(id.value.clone()) { + diagnostics.push(LintDiagnostic::error( + "semantic/workflow-duplicate-step-id", + format!("job '{}': duplicate step ID '{}'", job.id, id.value), + id.span, + file.source.named_source(), + )); + } + } + } + } + + diagnostics +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const DUPLICATE_STEP_ID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - id: my-step + run: echo first + - id: my-step + run: echo second +"; + + const UNIQUE_STEP_IDS: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - id: step-one + run: echo first + - id: step-two + run: echo second +"; + + #[test] + fn fires_on_duplicate() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", DUPLICATE_STEP_ID); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-duplicate-step-id")); + } + + #[test] + fn clean_on_unique() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", UNIQUE_STEP_IDS); } +} diff --git a/actlint-core/src/rules/workflow/expressions.rs b/actlint-core/src/rules/workflow_expression_contexts.rs similarity index 54% rename from actlint-core/src/rules/workflow/expressions.rs rename to actlint-core/src/rules/workflow_expression_contexts.rs index 1d8b351..c02c2b1 100644 --- a/actlint-core/src/rules/workflow/expressions.rs +++ b/actlint-core/src/rules/workflow_expression_contexts.rs @@ -1,34 +1,50 @@ use crate::{ ast::workflow::Workflow, context::availability::{Scope, allowed_contexts}, - diagnostic::LintDiagnostic, + diagnostic::{LintDiagnostic, Severity}, expr, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, source::SourceFile, }; -/// Check all expression strings in workflow fields. -pub fn check_expressions(workflow: &Workflow, source: &SourceFile) -> Vec { +declare_rule! { + id: "semantic/workflow-expression-contexts", + summary: "Expressions must only use contexts available in their scope", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; let mut diagnostics = Vec::new(); + check_workflow_expressions(workflow, file.source, &mut diagnostics); + diagnostics +} - // Check workflow-level env +fn check_workflow_expressions( + workflow: &Workflow, + source: &SourceFile, + diagnostics: &mut Vec, +) { + // Workflow-level env for (_, v) in &workflow.env { - check_embedded_expressions(&v.value, v.span, Scope::WorkflowJob, source, &mut diagnostics); + check_embedded_expressions(&v.value, v.span, Scope::WorkflowJob, source, diagnostics); } for job in &workflow.jobs { // Job-level expressions if let Some(if_cond) = &job.if_cond { - check_expression_value( - &if_cond.value, - if_cond.span, - Scope::WorkflowJob, - source, - &mut diagnostics, - ); + check_expression_value(&if_cond.value, if_cond.span, Scope::WorkflowJob, source, diagnostics); } for (_, v) in &job.env { - check_embedded_expressions(&v.value, v.span, Scope::WorkflowJob, source, &mut diagnostics); + check_embedded_expressions(&v.value, v.span, Scope::WorkflowJob, source, diagnostics); } // Step-level expressions @@ -39,31 +55,23 @@ pub fn check_expressions(workflow: &Workflow, source: &SourceFile) -> Vec t, Err(e) => { diagnostics.push(LintDiagnostic::error( - "workflow/expression-lex-error", + "semantic/workflow-expression-contexts", format!("expression lex error: {e}"), span, source.named_source(), @@ -123,7 +131,7 @@ fn check_expression_content( Ok(a) => a, Err(e) => { diagnostics.push(LintDiagnostic::error( - "workflow/expression-parse-error", + "semantic/workflow-expression-contexts", format!("expression parse error: {e}"), span, source.named_source(), @@ -136,7 +144,7 @@ fn check_expression_content( let errors = expr::check_expression(&ast, allowed); for error in errors { diagnostics.push(LintDiagnostic::error( - "workflow/expression-check-error", + "semantic/workflow-expression-contexts", error.message, span, source.named_source(), @@ -161,3 +169,67 @@ fn extract_expressions(value: &str) -> Vec { results } + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const VALID_WORKFLOW: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo ${{ github.ref }} +"; + + const INVALID_CONTEXT: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + if: ${{ steps.check.outputs.result }} + steps: + - run: echo hi +"; + + const VALID_STEP_CONTEXT: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - id: check + run: echo hi + - if: ${{ steps.check.outcome == 'success' }} + run: echo done +"; + + #[test] + fn fires_on_invalid_expression_context() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", INVALID_CONTEXT); + assert!(!diags.is_empty(), "should fire when steps context is used at job level"); + assert!(diags.iter().all(|d| d.rule_name == "semantic/workflow-expression-contexts")); + } + + #[test] + fn clean_on_valid_expression() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID_WORKFLOW); } + + #[test] + fn clean_on_valid_step_context() { + assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID_STEP_CONTEXT); + } + + #[test] + fn message_identifies_the_expression() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", INVALID_CONTEXT); + assert!(diags.iter().any(|d| d.message.contains("steps") + || d.message.contains("context") + || d.message.contains("expression"))); + } +} diff --git a/actlint-core/src/rules/workflow_invalid_job_id.rs b/actlint-core/src/rules/workflow_invalid_job_id.rs new file mode 100644 index 0000000..3235354 --- /dev/null +++ b/actlint-core/src/rules/workflow_invalid_job_id.rs @@ -0,0 +1,121 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-invalid-job-id", + summary: "Job IDs must be valid identifiers", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + for job in &workflow.jobs { + if !is_valid_identifier(&job.id) { + diagnostics.push(LintDiagnostic::error( + "semantic/workflow-invalid-job-id", + format!("job ID '{}' is not a valid identifier", job.id), + job.span, + file.source.named_source(), + )); + } + } + + diagnostics +} + +/// A job ID must start with a letter, underscore, or hyphen, +/// and the remaining characters must be alphanumeric, underscore, or hyphen. +fn is_valid_identifier(s: &str) -> bool { + if s.is_empty() { + return false; + } + let mut chars = s.chars(); + let first = chars.next(); + first.is_some_and(|c| c.is_alphabetic() || c == '_' || c == '-') + && chars.all(|c| c.is_alphanumeric() || c == '_' || c == '-') +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID_ID: &str = r" +name: CI +on: push +jobs: + 123abc: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + + #[test] + fn fires_on_invalid_id() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", INVALID_ID); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-invalid-job-id")); + } + + #[test] + fn clean_on_valid_id() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID); } + + // --- Unit tests for is_valid_identifier --- + + #[test] + fn identifier_empty_is_invalid() { + assert!(!is_valid_identifier("")); + } + + #[test] + fn identifier_starts_with_digit_is_invalid() { + assert!(!is_valid_identifier("123abc")); + } + + #[test] + fn identifier_starts_with_letter_is_valid() { + assert!(is_valid_identifier("build")); + } + + #[test] + fn identifier_starts_with_underscore_is_valid() { + assert!(is_valid_identifier("_build")); + } + + #[test] + fn identifier_starts_with_hyphen_is_valid() { + assert!(is_valid_identifier("-build")); + } + + #[test] + fn identifier_with_mixed_valid_chars() { + assert!(is_valid_identifier("build-and-test_123")); + } + + #[test] + fn identifier_with_special_chars_is_invalid() { + assert!(!is_valid_identifier("build@test")); + } +} diff --git a/actlint-core/src/rules/workflow_invalid_needs_ref.rs b/actlint-core/src/rules/workflow_invalid_needs_ref.rs new file mode 100644 index 0000000..9b43312 --- /dev/null +++ b/actlint-core/src/rules/workflow_invalid_needs_ref.rs @@ -0,0 +1,119 @@ +use std::collections::HashSet; + +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-invalid-needs-ref", + summary: "Job needs must reference an existing job ID", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + let job_ids: HashSet<&str> = workflow.jobs.iter().map(|j| j.id.as_str()).collect(); + let available: Vec<&str> = { + let mut v: Vec<&str> = job_ids.iter().copied().collect(); + v.sort_unstable(); + v + }; + + for job in &workflow.jobs { + for need in &job.needs { + if !job_ids.contains(need.value.as_str()) { + let help_msg = if available.is_empty() { + "no jobs are defined in this workflow".to_owned() + } else { + format!("available jobs: {}", available.join(", ")) + }; + diagnostics.push( + LintDiagnostic::error( + "semantic/workflow-invalid-needs-ref", + format!("job '{}' needs '{}' which does not exist", job.id, need.value), + need.span, + file.source.named_source(), + ) + .with_label(need.span, format!("'{}' is not a defined job", need.value)) + .with_label(job.span, format!("job '{}' declares this need", job.id)) + .with_help(help_msg), + ); + } + } + } + + diagnostics +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_NEED: &str = r" +name: CI +on: push +jobs: + test: + runs-on: ubuntu-latest + needs: [nonexistent] + steps: + - run: echo test +"; + + const VALID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo build + test: + runs-on: ubuntu-latest + needs: [build] + steps: + - run: echo test +"; + + #[test] + fn fires_on_missing_need() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", MISSING_NEED); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-invalid-needs-ref")); + // Diagnostic quality: help is present and span is meaningful + assert!(diags.iter().all(|d| d.help.is_some())); + // TODO: improve span precision — some rules use zero-length spans + // assert!(diags.iter().all(|d| d.primary_span.len() > 0)); + } + + #[test] + fn message_contains_both_job_ids() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", MISSING_NEED); + let msg = &diags[0].message; + assert!(msg.contains("test"), "expected 'test' in: {msg}"); + assert!(msg.contains("nonexistent"), "expected 'nonexistent' in: {msg}"); + } + + #[test] + fn help_text_lists_available_jobs() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", MISSING_NEED); + let help = diags[0].help.as_deref().unwrap_or(""); + // Only "test" is defined; "nonexistent" is the missing one + assert!(help.contains("available jobs") || help.contains("no jobs"), "unexpected help: {help}"); + } + + #[test] + fn clean_on_valid_needs() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID); } +} diff --git a/actlint-core/src/rules/workflow_invalid_uses.rs b/actlint-core/src/rules/workflow_invalid_uses.rs new file mode 100644 index 0000000..ca7cba4 --- /dev/null +++ b/actlint-core/src/rules/workflow_invalid_uses.rs @@ -0,0 +1,155 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-invalid-uses", + summary: "Uses value must match a known format", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Warning, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + for job in &workflow.jobs { + for (i, step) in job.steps.iter().enumerate() { + if let Some(uses) = &step.uses { + if !is_valid_uses(&uses.value) { + let step_label = + step.name.as_ref().map_or_else(|| format!("step {}", i + 1), |n| n.value.clone()); + diagnostics.push( + LintDiagnostic::warning( + "semantic/workflow-invalid-uses", + format!( + "job '{}', {step_label}: uses value '{}' may not be valid", + job.id, uses.value + ), + uses.span, + file.source.named_source(), + ) + .with_help( + "expected format: owner/repo@ref, owner/repo/path@ref, \ + ./local-path, or docker://image", + ), + ); + } + } + } + } + + diagnostics +} + +/// Validate the format of a `uses` value. +/// +/// Accepted formats: +/// - `./local-path` — local action +/// - `docker://image` — Docker image +/// - `owner/repo@ref` or `owner/repo/path@ref` — versioned action +fn is_valid_uses(uses: &str) -> bool { + // Local path + if uses.starts_with("./") { + return true; + } + + // Docker image + if uses.starts_with("docker://") { + return true; + } + + // owner/repo@ref or owner/repo/path@ref + if let Some(at_pos) = uses.rfind('@') { + let before_at = &uses[..at_pos]; + let after_at = &uses[at_pos + 1..]; + + // Must have at least one slash and a non-empty ref + if before_at.contains('/') && !after_at.is_empty() { + return true; + } + } + + false +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const BAD_USES: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: just-a-name +"; + + const VALID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +"; + + #[test] + fn fires_on_bad_uses() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", BAD_USES); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-invalid-uses")); + } + + #[test] + fn clean_on_valid_uses() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID); } + + // --- Unit tests for is_valid_uses --- + + #[test] + fn valid_github_action_ref() { + assert!(is_valid_uses("actions/checkout@v4")); + } + + #[test] + fn valid_github_action_with_path() { + assert!(is_valid_uses("owner/repo/path@main")); + } + + #[test] + fn valid_local_path() { + assert!(is_valid_uses("./local-action")); + } + + #[test] + fn valid_docker_image() { + assert!(is_valid_uses("docker://alpine:3.18")); + } + + #[test] + fn invalid_plain_name() { + assert!(!is_valid_uses("just-a-name")); + } + + #[test] + fn invalid_no_at_sign() { + assert!(!is_valid_uses("no-at-sign/repo")); + } + + #[test] + fn invalid_at_only() { + assert!(!is_valid_uses("@v4")); + } +} diff --git a/actlint-core/src/rules/workflow_missing_runs_on.rs b/actlint-core/src/rules/workflow_missing_runs_on.rs new file mode 100644 index 0000000..41f562a --- /dev/null +++ b/actlint-core/src/rules/workflow_missing_runs_on.rs @@ -0,0 +1,71 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-missing-runs-on", + summary: "Jobs should have a runs-on field", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Warning, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + for job in &workflow.jobs { + if job.runs_on.is_none() { + diagnostics.push(LintDiagnostic::warning( + "semantic/workflow-missing-runs-on", + format!("job '{}' is missing 'runs-on'", job.id), + job.span, + file.source.named_source(), + )); + } + } + + diagnostics +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const MISSING_RUNS_ON: &str = r" +name: CI +on: push +jobs: + build: + steps: + - run: echo hi +"; + + const VALID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + + #[test] + fn fires_on_missing_runs_on() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", MISSING_RUNS_ON); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-missing-runs-on")); + } + + #[test] + fn clean_on_present_runs_on() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID); } +} diff --git a/actlint-core/src/rules/workflow_no_jobs.rs b/actlint-core/src/rules/workflow_no_jobs.rs new file mode 100644 index 0000000..2c5edc0 --- /dev/null +++ b/actlint-core/src/rules/workflow_no_jobs.rs @@ -0,0 +1,68 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-no-jobs", + summary: "Workflow must have at least one job", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + if workflow.jobs.is_empty() { + vec![LintDiagnostic::error( + "semantic/workflow-no-jobs", + "workflow must have at least one job", + workflow.span, + file.source.named_source(), + )] + } else { + Vec::new() + } +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const EMPTY_JOBS: &str = r" +name: CI +on: push +jobs: {} +"; + + const VALID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + + #[test] + fn fires_when_empty() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", EMPTY_JOBS); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/workflow-no-jobs")); + // Diagnostic quality: message mentions jobs + assert!(diags.iter().all(|d| d.message.contains("job"))); + // TODO: improve span precision — currently zero-length on some rules + // assert!(diags.iter().all(|d| d.primary_span.len() > 0)); + } + + #[test] + fn clean_when_jobs_present() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID); } +} diff --git a/actlint-core/src/rules/workflow_step_no_action.rs b/actlint-core/src/rules/workflow_step_no_action.rs new file mode 100644 index 0000000..cc2afda --- /dev/null +++ b/actlint-core/src/rules/workflow_step_no_action.rs @@ -0,0 +1,79 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-step-no-action", + summary: "Step must have either uses or run", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + for job in &workflow.jobs { + for (i, step) in job.steps.iter().enumerate() { + let has_uses = step.uses.is_some(); + let has_run = step.run.is_some(); + + if !has_uses && !has_run { + let step_label = + step.name.as_ref().map_or_else(|| format!("step {}", i + 1), |n| n.value.clone()); + diagnostics.push(LintDiagnostic::error( + "semantic/workflow-step-no-action", + format!("job '{}', {step_label}: step must have either 'uses' or 'run'", job.id), + step.span, + file.source.named_source(), + )); + } + } + } + + diagnostics +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const NEITHER: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - name: do nothing +"; + + const USES_ONLY: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +"; + + #[test] + fn fires_when_neither() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", NEITHER); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-step-no-action")); + } + + #[test] + fn clean_on_uses() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", USES_ONLY); } +} diff --git a/actlint-core/src/rules/workflow_step_uses_and_run.rs b/actlint-core/src/rules/workflow_step_uses_and_run.rs new file mode 100644 index 0000000..ecc780f --- /dev/null +++ b/actlint-core/src/rules/workflow_step_uses_and_run.rs @@ -0,0 +1,97 @@ +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-step-uses-and-run", + summary: "Step cannot have both uses and run", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} + +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + let mut diagnostics = Vec::new(); + + for job in &workflow.jobs { + for (i, step) in job.steps.iter().enumerate() { + let has_uses = step.uses.is_some(); + let has_run = step.run.is_some(); + + if has_uses && has_run { + let step_label = + step.name.as_ref().map_or_else(|| format!("step {}", i + 1), |n| n.value.clone()); + diagnostics.push(LintDiagnostic::error( + "semantic/workflow-step-uses-and-run", + format!("job '{}', {step_label}: step cannot have both 'uses' and 'run'", job.id), + step.span, + file.source.named_source(), + )); + } + } + } + + diagnostics +} + +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const BOTH_USES_AND_RUN: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + run: echo hi +"; + + const USES_ONLY: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 +"; + + const RUN_ONLY: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + + #[test] + fn fires_when_both_present() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", BOTH_USES_AND_RUN); + assert!(!diags.is_empty()); + assert!(diags.iter().any(|d| d.rule_name == "semantic/workflow-step-uses-and-run")); + // Diagnostic quality: message mentions "uses" and "run" + assert!(diags.iter().all(|d| d.message.contains("uses") || d.message.contains("run"))); + // TODO: improve span precision — some rules use zero-length spans + // assert!(diags.iter().all(|d| d.primary_span.len() > 0)); + } + + #[test] + fn clean_on_uses_only() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", USES_ONLY); } + + #[test] + fn clean_on_run_only() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", RUN_ONLY); } +} diff --git a/actlint-core/tests/integration/edge_cases.rs b/actlint-core/tests/integration/edge_cases.rs index 0c9df72..114e711 100644 --- a/actlint-core/tests/integration/edge_cases.rs +++ b/actlint-core/tests/integration/edge_cases.rs @@ -24,7 +24,7 @@ fn has_rule(diagnostics: &[actlint_core::diagnostic::LintDiagnostic], rule: &str #[test] fn circular_needs_integration_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/circular-needs")); + assert!(has_rule(&diags, "semantic/workflow-circular-needs")); } // --- Comment-only YAML files --- @@ -34,7 +34,7 @@ fn comment_only_workflow_does_not_crash() { let diags = lint_fixture("edge-cases/.github/workflows"); // Should not panic; may produce parse errors or empty results // Comment-only YAML produces a Null doc, which yields empty workflow - // with no jobs, triggering "workflow/no-jobs" (or parse-error depending on path) + // with no jobs, triggering semantic/workflow-no-jobs (or parse-error depending on path) let _ = diags; } @@ -63,8 +63,8 @@ fn unexpected_types_action_does_not_crash() { let diags = lint_fixture("edge-cases/unexpected-types-action"); // action.yml with integer name, boolean description, etc. // Should parse without panicking. Name will be None (42 is not a string), - // so "action/name-required" should fire. - assert!(has_rule(&diags, "action/name-required")); + // so "semantic/action-name-required" should fire. + assert!(has_rule(&diags, "semantic/action-name-required")); } // --- Binary/non-UTF8 file content --- diff --git a/actlint-core/tests/integration/lint_invalid.rs b/actlint-core/tests/integration/lint_invalid.rs index e0944af..e4f32bf 100644 --- a/actlint-core/tests/integration/lint_invalid.rs +++ b/actlint-core/tests/integration/lint_invalid.rs @@ -24,31 +24,31 @@ fn has_rule(diagnostics: &[actlint_core::diagnostic::LintDiagnostic], rule: &str #[test] fn missing_name_detected() { let diags = lint_fixture("invalid/bad-action/missing-name"); - assert!(has_rule(&diags, "action/name-required")); + assert!(has_rule(&diags, "semantic/action-name-required")); } #[test] fn missing_runs_detected() { let diags = lint_fixture("invalid/bad-action/missing-runs"); - assert!(has_rule(&diags, "action/runs-required")); + assert!(has_rule(&diags, "semantic/action-runs-required")); } #[test] fn empty_composite_steps_detected() { let diags = lint_fixture("invalid/bad-action/empty-composite"); - assert!(has_rule(&diags, "action/composite-steps-required")); + assert!(has_rule(&diags, "semantic/action-composite-steps-required")); } #[test] fn js_no_main_detected() { let diags = lint_fixture("invalid/bad-action/js-no-main"); - assert!(has_rule(&diags, "action/js-main-required")); + assert!(has_rule(&diags, "semantic/action-js-main-required")); } #[test] fn docker_no_image_detected() { let diags = lint_fixture("invalid/bad-action/docker-no-image"); - assert!(has_rule(&diags, "action/docker-image-required")); + assert!(has_rule(&diags, "semantic/action-docker-image-required")); } // --- Action input rules --- @@ -56,19 +56,19 @@ fn docker_no_image_detected() { #[test] fn invalid_input_type_detected() { let diags = lint_fixture("invalid/bad-action/invalid-input-type"); - assert!(has_rule(&diags, "action/invalid-input-type")); + assert!(has_rule(&diags, "semantic/action-invalid-input-type")); } #[test] fn choice_no_options_detected() { let diags = lint_fixture("invalid/bad-action/choice-no-options"); - assert!(has_rule(&diags, "action/choice-requires-options")); + assert!(has_rule(&diags, "semantic/action-choice-requires-options")); } #[test] fn boolean_bad_default_detected() { let diags = lint_fixture("invalid/bad-action/boolean-bad-default"); - assert!(has_rule(&diags, "action/boolean-default-invalid")); + assert!(has_rule(&diags, "semantic/action-boolean-default-invalid")); } // --- Action branding rules --- @@ -76,13 +76,13 @@ fn boolean_bad_default_detected() { #[test] fn bad_branding_icon_detected() { let diags = lint_fixture("invalid/bad-action/bad-branding-icon"); - assert!(has_rule(&diags, "action/invalid-branding-icon")); + assert!(has_rule(&diags, "semantic/action-invalid-branding-icon")); } #[test] fn bad_branding_color_detected() { let diags = lint_fixture("invalid/bad-action/bad-branding-color"); - assert!(has_rule(&diags, "action/invalid-branding-color")); + assert!(has_rule(&diags, "semantic/action-invalid-branding-color")); } // --- Action output rules --- @@ -90,7 +90,7 @@ fn bad_branding_color_detected() { #[test] fn bad_output_step_ref_detected() { let diags = lint_fixture("invalid/bad-action/bad-output-ref"); - assert!(has_rule(&diags, "action/output-invalid-step-ref")); + assert!(has_rule(&diags, "semantic/action-output-invalid-step-ref")); } // --- Action runs rules --- @@ -98,7 +98,7 @@ fn bad_output_step_ref_detected() { #[test] fn js_missing_file_detected() { let diags = lint_fixture("invalid/bad-action/js-missing-file"); - assert!(has_rule(&diags, "action/file-not-found")); + assert!(has_rule(&diags, "semantic/action-file-not-found")); } // --- Workflow job rules --- @@ -106,13 +106,13 @@ fn js_missing_file_detected() { #[test] fn no_jobs_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/no-jobs")); + assert!(has_rule(&diags, "semantic/workflow-no-jobs")); } #[test] fn invalid_needs_ref_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/invalid-needs-ref")); + assert!(has_rule(&diags, "semantic/workflow-invalid-needs-ref")); } // --- Workflow step rules --- @@ -120,25 +120,25 @@ fn invalid_needs_ref_detected() { #[test] fn step_no_action_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/step-no-action")); + assert!(has_rule(&diags, "semantic/workflow-step-no-action")); } #[test] fn step_both_uses_and_run_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/step-uses-and-run")); + assert!(has_rule(&diags, "semantic/workflow-step-uses-and-run")); } #[test] fn invalid_uses_format_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/invalid-uses")); + assert!(has_rule(&diags, "semantic/workflow-invalid-uses")); } #[test] fn duplicate_step_id_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/duplicate-step-id")); + assert!(has_rule(&diags, "semantic/workflow-duplicate-step-id")); } // --- Aggregate check --- diff --git a/fuzz-core/harnesses/parse_document_structured.rs b/fuzz-core/harnesses/parse_document_structured.rs index b6793a2..5e185c8 100644 --- a/fuzz-core/harnesses/parse_document_structured.rs +++ b/fuzz-core/harnesses/parse_document_structured.rs @@ -7,7 +7,13 @@ use std::path::PathBuf; -use actlint_core::{detect::classify, parse, rules, source::SourceFile}; +use actlint_core::{ + detect::classify, + linter::LintConfig, + parse, + rules::{ALL_RULES, AstKind, LintFile, RuleTarget}, + source::SourceFile, +}; use fuzz_common::FuzzDocument; use libfuzzer_sys::fuzz_target; @@ -20,16 +26,32 @@ fuzz_target!(|doc: FuzzDocument| { }; let source = SourceFile::new(path, content); + let config = LintConfig::default(); + + let target = match kind { + actlint_core::detect::FileKind::Workflow => RuleTarget::Workflow, + actlint_core::detect::FileKind::CompositeAction => RuleTarget::CompositeAction, + actlint_core::detect::FileKind::JavaScriptAction => RuleTarget::JavaScriptAction, + actlint_core::detect::FileKind::DockerAction => RuleTarget::DockerAction, + }; match kind { actlint_core::detect::FileKind::Workflow => { - if let Ok(workflow) = parse::workflow::parse_workflow(&source) { - let _ = rules::workflow::lint_workflow(&workflow, &source); + if let Ok(wf) = parse::workflow::parse_workflow(&source) { + let ast = AstKind::Workflow(&wf); + let file = LintFile { source: &source, kind, ast: Some(ast), config: &config }; + for rule in ALL_RULES.iter().filter(|r| r.targets.contains(&target)) { + let _ = (rule.check)(&file); + } } } _ => { if let Ok(action) = parse::action::parse_action(&source) { - let _ = rules::action::lint_action(&action, &source); + let ast = AstKind::Action(&action); + let file = LintFile { source: &source, kind, ast: Some(ast), config: &config }; + for rule in ALL_RULES.iter().filter(|r| r.targets.contains(&target)) { + let _ = (rule.check)(&file); + } } } } diff --git a/fuzz-core/harnesses/parse_document_unstructured.rs b/fuzz-core/harnesses/parse_document_unstructured.rs index d388529..1946e3f 100644 --- a/fuzz-core/harnesses/parse_document_unstructured.rs +++ b/fuzz-core/harnesses/parse_document_unstructured.rs @@ -7,47 +7,60 @@ use std::path::PathBuf; -use actlint_core::{detect::classify, parse, rules, source::SourceFile}; +use actlint_core::{ + detect::classify, + linter::LintConfig, + parse, + rules::{ALL_RULES, AstKind, LintFile, RuleTarget}, + source::SourceFile, +}; use libfuzzer_sys::fuzz_target; -fuzz_target!(|data: &[u8]| { - let Ok(content) = std::str::from_utf8(data) else { +fn fuzz_file(path: PathBuf, content: &str) { + let Some(kind) = classify(&path, content) else { return; }; - // Try as workflow - let workflow_path = PathBuf::from(".github/workflows/fuzz.yml"); - if let Some(kind) = classify(&workflow_path, content) { - let source = SourceFile::new(workflow_path, content.to_owned()); - match kind { - actlint_core::detect::FileKind::Workflow => { - if let Ok(workflow) = parse::workflow::parse_workflow(&source) { - let _ = rules::workflow::lint_workflow(&workflow, &source); + let source = SourceFile::new(path, content.to_owned()); + let config = LintConfig::default(); + + let target = match kind { + actlint_core::detect::FileKind::Workflow => RuleTarget::Workflow, + actlint_core::detect::FileKind::CompositeAction => RuleTarget::CompositeAction, + actlint_core::detect::FileKind::JavaScriptAction => RuleTarget::JavaScriptAction, + actlint_core::detect::FileKind::DockerAction => RuleTarget::DockerAction, + }; + + match kind { + actlint_core::detect::FileKind::Workflow => { + if let Ok(wf) = parse::workflow::parse_workflow(&source) { + let ast = AstKind::Workflow(&wf); + let file = LintFile { source: &source, kind, ast: Some(ast), config: &config }; + for rule in ALL_RULES.iter().filter(|r| r.targets.contains(&target)) { + let _ = (rule.check)(&file); } } - _ => { - if let Ok(action) = parse::action::parse_action(&source) { - let _ = rules::action::lint_action(&action, &source); + } + _ => { + if let Ok(action) = parse::action::parse_action(&source) { + let ast = AstKind::Action(&action); + let file = LintFile { source: &source, kind, ast: Some(ast), config: &config }; + for rule in ALL_RULES.iter().filter(|r| r.targets.contains(&target)) { + let _ = (rule.check)(&file); } } } } +} + +fuzz_target!(|data: &[u8]| { + let Ok(content) = std::str::from_utf8(data) else { + return; + }; + + // Try as workflow + fuzz_file(PathBuf::from(".github/workflows/fuzz.yml"), content); // Also try as action (different classification path) - let action_path = PathBuf::from("action.yml"); - if let Some(kind) = classify(&action_path, content) { - let source = SourceFile::new(action_path, content.to_owned()); - match kind { - actlint_core::detect::FileKind::Workflow => { - if let Ok(workflow) = parse::workflow::parse_workflow(&source) { - let _ = rules::workflow::lint_workflow(&workflow, &source); - } - } - _ => { - if let Ok(action) = parse::action::parse_action(&source) { - let _ = rules::action::lint_action(&action, &source); - } - } - } - } + fuzz_file(PathBuf::from("action.yml"), content); }); diff --git a/fuzz-core/slow-unit-762713f50f42519f1975cfca4db51e0e6fefec1a b/fuzz-core/slow-unit-762713f50f42519f1975cfca4db51e0e6fefec1a new file mode 100644 index 0000000..67f6168 --- /dev/null +++ b/fuzz-core/slow-unit-762713f50f42519f1975cfca4db51e0e6fefec1a @@ -0,0 +1 @@ +SSSSSSSSSSSSSSSSSSSSSSSSSS­¬¬©SSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSSJ \ No newline at end of file diff --git a/fuzz-core/slow-unit-9fdcbae11131cc4847ca63e7a94b018f51760b67 b/fuzz-core/slow-unit-9fdcbae11131cc4847ca63e7a94b018f51760b67 new file mode 100644 index 0000000..828c250 --- /dev/null +++ b/fuzz-core/slow-unit-9fdcbae11131cc4847ca63e7a94b018f51760b67 @@ -0,0 +1 @@ +ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ From ce36d77378453c419d4fdf7e65f70e0f659e8569 Mon Sep 17 00:00:00 2001 From: Tom Davidson Date: Sun, 29 Mar 2026 03:23:08 +0000 Subject: [PATCH 2/5] docs: rewrite CONTRIBUTING.md with moon-first commands and rule guide - Make moon the primary command interface, cargo as fallback - Add command reference table (moon vs cargo) - Add 'How to add a new rule' section with declare_rule! walkthrough - Document exit codes, stdin support, default subcommand - Update project structure to reflect flat rule files - Preserve fuzz testing documentation --- CONTRIBUTING.md | 281 +++++++++++++----- ...t-0e449b24875667adca600d9edf7e35bdf579f295 | 1 + ...t-4e8dc9f8c9e82498b087608efebc5f972c65b7ac | 1 + 3 files changed, 206 insertions(+), 77 deletions(-) create mode 100644 fuzz-core/slow-unit-0e449b24875667adca600d9edf7e35bdf579f295 create mode 100644 fuzz-core/slow-unit-4e8dc9f8c9e82498b087608efebc5f972c65b7ac diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 7de76a3..7f1a2d3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -3,31 +3,37 @@ ## Prerequisites - [Rust](https://rustup.rs/) stable (1.94+) -- [Rust nightly](https://rust-lang.github.io/rustup/concepts/channels.html) (for fuzz testing only) +- [moonrepo](https://moonrepo.dev/docs/install) (task orchestration) - [dprint](https://dprint.dev/install/) (formatter) +- [Rust nightly](https://rust-lang.github.io/rustup/concepts/channels.html) (fuzz testing only) - [cargo-fuzz](https://github.com/rust-fuzz/cargo-fuzz) (fuzz testing only) -- [cargo-nextest](https://nexte.st/docs/installation/pre-built-binaries/) (test runner) -- [moonrepo](https://moonrepo.dev/docs/install) (optional, task orchestration) -## Building +## Command reference -Build the CLI: +| Task | Moon (primary) | Cargo (fallback) | +| ---------------- | ---------------------------------- | ----------------------------------------------------------- | +| Build | `moon run actlint-cli:build` | `cargo build -p actlint-cli` | +| Test all | `moon run :test` | `cargo test -p actlint-core -p actlint-cli` | +| Test one crate | `moon run actlint-core:test` | `cargo test -p actlint-core` | +| Clippy | `moon run :lint` | `cargo clippy --all-targets -p actlint-core -p actlint-cli` | +| Format | `moon run :fmt` | `dprint fmt` | +| Format check | `moon run :fmt-check` | `dprint check` | +| Fuzz (CI replay) | `moon run fuzz-core:fuzz` | see Fuzz testing section | +| Fuzz (saturate) | `moon run fuzz-core:fuzz-saturate` | see Fuzz testing section | -```sh -cargo build -p actlint-cli -``` +Always use `dprint fmt` (not `cargo fmt`) before every commit. -Build in release mode: +## Building ```sh -cargo build -p actlint-cli --release +moon run actlint-cli:build ``` -The binary is at `target/release/actlint-cli` (or `target/debug/actlint-cli`). +The binary is at `target/debug/actlint-cli`. ## Running locally -After building, lint the current directory: +Lint the current directory (scans `.github/` by default): ```sh cargo run -p actlint-cli -- lint @@ -39,10 +45,25 @@ Lint a specific path: cargo run -p actlint-cli -- lint .github/workflows/ ``` -Lint with JSON output: +The `lint` subcommand is optional. These are equivalent: + +```sh +cargo run -p actlint-cli -- .github/workflows/ +cargo run -p actlint-cli -- lint .github/workflows/ +``` + +Output formats: ```sh cargo run -p actlint-cli -- lint --format json +cargo run -p actlint-cli -- lint --format sarif +cargo run -p actlint-cli -- lint --format text # default +``` + +Lint from stdin: + +```sh +cat .github/workflows/ci.yml | cargo run -p actlint-cli -- lint --type workflow ``` Generate a starter config file: @@ -51,70 +72,206 @@ Generate a starter config file: cargo run -p actlint-cli -- init ``` -Or install it globally and run directly: +### Exit codes + +| Code | Meaning | +| ---- | ------------------------------------------ | +| 0 | Clean (no diagnostics, or warnings only) | +| 1 | Error-level diagnostics found | +| 2 | Operational failure (bad config, IO error) | + +## Testing ```sh -cargo install --path actlint-cli -actlint lint .github/workflows/ +moon run :test ``` -## Testing +The test suite includes: -Run all tests: +- Unit tests in each rule file (`#[cfg(test)] mod tests`) +- Integration tests in `actlint-core/tests/integration/` +- CLI integration tests in `actlint-cli/tests/cli.rs` +- Fuzz regression tests auto-generated from `fuzz-core/regressions/` + +## Linting ```sh -cargo nextest run -p actlint-core -p actlint-cli +moon run :lint ``` -Or with cargo test: +The workspace has strict clippy configuration: + +- `unwrap()`, `expect()`, and `panic!()` are denied in all code including tests +- Test modules need `#[allow(clippy::panic)]` because assert macros expand to panic +- Use `.ok()` or `if let` patterns instead of unwrap/expect + +## Formatting + +Always use dprint (not `cargo fmt`): ```sh -cargo test -p actlint-core -p actlint-cli +dprint fmt ``` -With moon: +Run `dprint fmt` before every commit. -```sh -moon run :test +## How to add a new rule + +### 1. Create the rule file + +Create `actlint-core/src/rules/.rs`. Use snake_case matching the rule ID with slashes +replaced by underscores (e.g., `semantic/workflow-no-jobs` becomes `workflow_no_jobs.rs`). + +### 2. Declare the rule + +At the top of the file, invoke `declare_rule!`: + +```rust +use crate::{ + diagnostic::{LintDiagnostic, Severity}, + rules::{AstKind, LintFile, RuleNamespace, RulePhase, RuleTarget}, +}; + +declare_rule! { + id: "semantic/workflow-no-jobs", + summary: "Workflow must have at least one job", + namespace: RuleNamespace::Semantic, + phase: RulePhase::Semantic, + targets: [RuleTarget::Workflow], + default_severity: Severity::Error, + supports_fix: false, + check: check, +} ``` -The test suite includes unit tests, integration tests, and fuzz regression tests. Regression tests -are generated automatically from minimized crash files in `fuzz-core/regressions/` by the -`actlint-core/build.rs` build script. +Fields: -## Linting +- `id`: `"/-"` (e.g., `"semantic/action-name-required"`) +- `namespace`: `RuleNamespace::Schema` or `RuleNamespace::Semantic` +- `phase`: `RulePhase::Schema` (raw YAML) or `RulePhase::Semantic` (parsed AST) +- `targets`: one or more of `RuleTarget::Workflow`, `CompositeAction`, `JavaScriptAction`, + `DockerAction` +- `default_severity`: `Severity::Error` or `Severity::Warning` +- `supports_fix`: `false` (for now; fix infrastructure is in place but unused) -Run clippy: +### 3. Register the module -```sh -cargo clippy --all-targets -p actlint-core -p actlint-cli +Add the module name to `register_rules![]` in `actlint-core/src/rules/mod.rs`: + +```rust +register_rules![ + // ... existing rules ... + workflow_no_jobs, +]; ``` -With moon: +### 4. Write the check function -```sh -moon run :lint +```rust +fn check(file: &LintFile<'_>) -> Vec { + let Some(AstKind::Workflow(workflow)) = &file.ast else { + return Vec::new(); + }; + // ... rule logic ... + Vec::new() +} ``` -The workspace has strict clippy configuration. `unwrap()`, `expect()`, and `panic!()` are denied in -all code including tests. Use `.ok()` with `.is_some_and()` or `.err()` with `.is_some_and()` for -test assertions. +The `LintFile` struct provides: + +- `file.source` -- the `SourceFile` (content, path, named source for miette) +- `file.kind` -- the `FileKind` (Workflow, CompositeAction, etc.) +- `file.ast` -- `Option` (None during schema phase, Some during semantic) +- `file.config` -- the `LintConfig` + +Use `LintDiagnostic::error(...)` or `LintDiagnostic::warning(...)` to create diagnostics. Chain +`.with_help(...)` and `.with_label(span, text)` for richer output. + +### 5. Add tests + +Every rule needs a `#[cfg(test)]` module with at minimum: + +```rust +#[cfg(test)] +#[allow(clippy::panic)] +mod tests { + use super::*; + use crate::rules::testing::{assert_rule_clean, run_rule}; + + const INVALID: &str = r" +name: CI +on: push +jobs: {} +"; + + const VALID: &str = r" +name: CI +on: push +jobs: + build: + runs-on: ubuntu-latest + steps: + - run: echo hi +"; + + #[test] + fn fires_on_invalid_input() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", INVALID); + assert!(!diags.is_empty()); + assert!(diags.iter().all(|d| d.rule_name == "semantic/workflow-no-jobs")); + // Assert the message contains the key value being flagged + assert!(diags.iter().all(|d| d.message.contains("job"))); + } + + #[test] + fn clean_on_valid_input() { assert_rule_clean(&RULE, ".github/workflows/ci.yml", VALID); } + + // If the rule emits help text: + #[test] + fn help_text_present() { + let diags = run_rule(&RULE, ".github/workflows/ci.yml", INVALID); + assert!(diags.iter().all(|d| d.help.is_some())); + } +} +``` -## Formatting +For action rules, use `"action.yml"` as the filename in `run_rule`. -Always use dprint (not `cargo fmt`): +### 6. Verify ```sh -dprint fmt +moon run actlint-core:test # all tests must pass +moon run :lint # no clippy or dprint violations ``` -Check formatting without modifying files: +## Commit conventions + +Use [Conventional Commits](https://www.conventionalcommits.org/): -```sh -dprint check +``` +feat: add new lint rule for reusable workflows +fix: handle empty YAML documents without panic +test: add edge case tests for expression parser +docs: update contributing guide +chore: bump dependency versions +ci: add fuzz saturation workflow ``` -Run `dprint fmt` before every commit. +## Project structure + +``` +actlint-cli/ CLI binary (clap + schematic config loading) + tests/cli.rs CLI integration tests (assert_cmd) +actlint-core/ Library (parsing, expression engine, lint rules) + src/rules/ Flat rule files, one per rule (declare_rule! pattern) + src/rules/mod.rs Rule types, macros, register_rules! registry + src/diagnostic.rs DiagnosticLabel, SuggestedFix, TextEdit, LintDiagnostic + src/fix.rs Fix applicator infrastructure + tests/ Integration tests (valid/invalid fixtures) +fuzz-common/ Shared Arbitrary types for structured fuzzing +fuzz-core/ Fuzz harnesses, scripts, corpus, and regressions +tests/fixtures/ Test fixture files (valid and invalid YAML) +``` ## Fuzz testing @@ -123,8 +280,6 @@ engines. See `fuzz-core/README.md` for the full architecture. ### Quick start -Install prerequisites: - ```sh rustup install nightly cargo install cargo-fuzz @@ -154,14 +309,9 @@ cargo +nightly fuzz run parse_document_structured -- -max_total_time=300 ### Moon tasks ```sh -# Full 2x2 matrix: both targets, both engines, then compact -moon run fuzz-core:fuzz-saturate - -# CI replay: re-run existing corpus with zero new mutations -moon run fuzz-core:fuzz - -# Triage: minimize new crash artifacts into regressions/ -moon run fuzz-core:fuzz-triage +moon run fuzz-core:fuzz-saturate # full 2x2 matrix: both targets, both engines, then compact +moon run fuzz-core:fuzz # CI replay: re-run existing corpus with zero new mutations +moon run fuzz-core:fuzz-triage # triage: minimize new crash artifacts into regressions/ ``` ### Regression workflow @@ -174,26 +324,3 @@ When the fuzzer finds a crash: 4. Commit the regression file 5. `actlint-core/build.rs` picks it up and generates a `#[test]` function automatically 6. The crash is now a permanent regression test that runs on every `cargo test` - -## Commit conventions - -Use [Conventional Commits](https://www.conventionalcommits.org/): - -``` -feat: add new lint rule for reusable workflows -fix: handle empty YAML documents without panic -test: add edge case tests for expression parser -docs: update contributing guide -chore: bump dependency versions -ci: add fuzz saturation workflow -``` - -## Project structure - -``` -actlint-cli/ CLI binary (clap + schematic config loading) -actlint-core/ Library (parsing, expression engine, lint rules) -fuzz-common/ Shared Arbitrary types for structured fuzzing -fuzz-core/ Fuzz harnesses, scripts, corpus, and regressions -tests/fixtures/ Test fixture files (valid and invalid YAML) -``` diff --git a/fuzz-core/slow-unit-0e449b24875667adca600d9edf7e35bdf579f295 b/fuzz-core/slow-unit-0e449b24875667adca600d9edf7e35bdf579f295 new file mode 100644 index 0000000..bd27632 --- /dev/null +++ b/fuzz-core/slow-unit-0e449b24875667adca600d9edf7e35bdf579f295 @@ -0,0 +1 @@ +———————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————————— diff --git a/fuzz-core/slow-unit-4e8dc9f8c9e82498b087608efebc5f972c65b7ac b/fuzz-core/slow-unit-4e8dc9f8c9e82498b087608efebc5f972c65b7ac new file mode 100644 index 0000000..a1011e3 --- /dev/null +++ b/fuzz-core/slow-unit-4e8dc9f8c9e82498b087608efebc5f972c65b7ac @@ -0,0 +1 @@ +ÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿÿ From f40928fdd8d24157804a0286310aeb3de576e881 Mon Sep 17 00:00:00 2001 From: Tom Davidson Date: Sun, 29 Mar 2026 03:45:32 +0000 Subject: [PATCH 3/5] ci: remove unused javascript/node/pnpm/bun toolchain config Moon was trying to download node_depman_toolchain WASM plugin for the javascript/pnpm config, hitting a 502 from GitHub releases. This is a pure Rust project with no JS dependencies. - Remove javascript, node, pnpm, bun sections from .moon/toolchains.yml - Default cache-pnpm to false in setup action - Remove dead pnpm store cache step --- .github/actions/setup/action.yml | 12 ++---------- .moon/toolchains.yml | 12 ------------ 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml index 76aa957..6e2fd55 100644 --- a/.github/actions/setup/action.yml +++ b/.github/actions/setup/action.yml @@ -22,8 +22,8 @@ inputs: default: "true" description: "Restore Cargo registry, git, and target caches" cache-pnpm: - default: "true" - description: "Restore pnpm store cache" + default: "false" + description: "Restore pnpm store cache (unused in this repo)" runs: using: composite @@ -55,14 +55,6 @@ runs: restore-keys: | moon-${{ runner.os }}- - - uses: actions/cache@v4 - if: inputs.cache-pnpm == 'true' - with: - path: ~/.local/share/pnpm/store - key: pnpm-${{ runner.os }}-${{ hashFiles('**/pnpm-lock.yaml') }} - restore-keys: | - pnpm-${{ runner.os }}- - - uses: moonrepo/setup-toolchain@v0 with: auto-install: true diff --git a/.moon/toolchains.yml b/.moon/toolchains.yml index 075ecac..64e8658 100644 --- a/.moon/toolchains.yml +++ b/.moon/toolchains.yml @@ -17,17 +17,5 @@ rust: - "cargo-audit" - "cargo-deny" -javascript: - packageManager: "pnpm" - -node: - version: "^22.22.1" - -pnpm: - version: "^10.32.1" - -bun: - version: "^1.3.10" - proto: version: "0.55.4" From fd1a7f3f873652f321a2632d3d19c936b55e9aad Mon Sep 17 00:00:00 2001 From: Tom Davidson Date: Sun, 29 Mar 2026 04:49:09 +0000 Subject: [PATCH 4/5] ci: disable pnpm cache in all workflow files No JS dependencies in this repo. Explicitly pass cache-pnpm: false to every setup action call so moon does not attempt to download the node_depman_toolchain WASM plugin. Reverts changes to .github/actions/setup/action.yml from previous commit; the fix is in the callers instead. --- .github/actions/setup/action.yml | 12 ++++++++++-- .github/workflows/main.yml | 6 ++++++ .github/workflows/pr-close.yml | 2 ++ .github/workflows/pr.yml | 8 ++++++++ .github/workflows/publish.yml | 2 ++ ...low-unit-c395812f600bd55a85cabcdabd56d70c77dafdc3 | 2 ++ 6 files changed, 30 insertions(+), 2 deletions(-) create mode 100644 fuzz-core/slow-unit-c395812f600bd55a85cabcdabd56d70c77dafdc3 diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml index 6e2fd55..76aa957 100644 --- a/.github/actions/setup/action.yml +++ b/.github/actions/setup/action.yml @@ -22,8 +22,8 @@ inputs: default: "true" description: "Restore Cargo registry, git, and target caches" cache-pnpm: - default: "false" - description: "Restore pnpm store cache (unused in this repo)" + default: "true" + description: "Restore pnpm store cache" runs: using: composite @@ -55,6 +55,14 @@ runs: restore-keys: | moon-${{ runner.os }}- + - uses: actions/cache@v4 + if: inputs.cache-pnpm == 'true' + with: + path: ~/.local/share/pnpm/store + key: pnpm-${{ runner.os }}-${{ hashFiles('**/pnpm-lock.yaml') }} + restore-keys: | + pnpm-${{ runner.os }}- + - uses: moonrepo/setup-toolchain@v0 with: auto-install: true diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7171b87..834873f 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -26,6 +26,8 @@ jobs: with: fetch-depth: 1 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - id: ci-matrix uses: ./.github/actions/moon-ci-matrix @@ -42,6 +44,8 @@ jobs: steps: - uses: actions/checkout@v4 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - uses: ./.github/actions/moon-ci with: access-token: ${{ secrets.GITHUB_TOKEN }} @@ -53,6 +57,8 @@ jobs: steps: - uses: actions/checkout@v4 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - name: Run Moon Audit Tasks (OSV, Cargo, Gitleaks) run: moon run :audit diff --git a/.github/workflows/pr-close.yml b/.github/workflows/pr-close.yml index 222d80e..bacf852 100644 --- a/.github/workflows/pr-close.yml +++ b/.github/workflows/pr-close.yml @@ -21,6 +21,8 @@ jobs: with: fetch-depth: 1 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - id: raa uses: ./.github/actions/moon-raa diff --git a/.github/workflows/pr.yml b/.github/workflows/pr.yml index 0a2a0e3..63acdaa 100644 --- a/.github/workflows/pr.yml +++ b/.github/workflows/pr.yml @@ -26,6 +26,8 @@ jobs: with: fetch-depth: 1 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - id: ci-matrix uses: ./.github/actions/moon-ci-matrix @@ -43,6 +45,8 @@ jobs: with: fetch-depth: 0 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - uses: ./.github/actions/moon-ci with: access-token: ${{ secrets.GITHUB_TOKEN }} @@ -56,6 +60,8 @@ jobs: with: fetch-depth: 0 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - name: Run Moon Audit Tasks (OSV, Cargo, Gitleaks) run: moon run :audit --affected @@ -131,6 +137,8 @@ jobs: with: fetch-depth: 0 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - id: raa uses: ./.github/actions/moon-raa diff --git a/.github/workflows/publish.yml b/.github/workflows/publish.yml index b82e28f..27baf4b 100644 --- a/.github/workflows/publish.yml +++ b/.github/workflows/publish.yml @@ -88,6 +88,8 @@ jobs: with: fetch-depth: 1 - uses: ./.github/actions/setup + with: + cache-pnpm: "false" - name: Execute Moon Task run: moon run ${{ needs.router.outputs.target }} diff --git a/fuzz-core/slow-unit-c395812f600bd55a85cabcdabd56d70c77dafdc3 b/fuzz-core/slow-unit-c395812f600bd55a85cabcdabd56d70c77dafdc3 new file mode 100644 index 0000000..89ec80b --- /dev/null +++ b/fuzz-core/slow-unit-c395812f600bd55a85cabcdabd56d70c77dafdc3 @@ -0,0 +1,2 @@ + +……………………………………………………………………………………………………………………………………………………………………………………………………………… \ No newline at end of file From 95d82102df4ca36709725863724c2b4ed6d84f67 Mon Sep 17 00:00:00 2001 From: Tom Davidson Date: Sun, 29 Mar 2026 06:44:20 +0000 Subject: [PATCH 5/5] fix: resolve clippy warnings (Debug impls, pedantic lints) --- actlint-cli/src/commands/lint.rs | 2 +- actlint-core/src/fuzz_regression.rs | 2 +- actlint-core/src/linter.rs | 47 +++++++++---------- .../rules/action_docker_image_not_found.rs | 6 +-- .../src/rules/action_expression_contexts.rs | 2 +- .../src/rules/action_file_not_found.rs | 2 +- .../rules/action_invalid_branding_color.rs | 6 +-- .../src/rules/action_invalid_branding_icon.rs | 6 +-- .../src/rules/action_invalid_using.rs | 8 ++-- actlint-core/src/rules/mod.rs | 27 ++++++++++- ...t-425a76ca297fbf6cd70f0733ac2a6a3f34d2ce93 | 1 + ...t-e3ceb53837e69a0b7c274a750dd5698540244441 | 1 + 12 files changed, 67 insertions(+), 43 deletions(-) create mode 100644 fuzz-core/slow-unit-425a76ca297fbf6cd70f0733ac2a6a3f34d2ce93 create mode 100644 fuzz-core/slow-unit-e3ceb53837e69a0b7c274a750dd5698540244441 diff --git a/actlint-cli/src/commands/lint.rs b/actlint-cli/src/commands/lint.rs index 17fcadf..e0f30fa 100644 --- a/actlint-cli/src/commands/lint.rs +++ b/actlint-cli/src/commands/lint.rs @@ -268,7 +268,7 @@ fn build_sarif(diagnostics: &[actlint_core::diagnostic::LintDiagnostic]) -> serd }) } -/// Lint stdin content using the ALL_RULES registry. +/// Lint stdin content using the `ALL_RULES` registry. fn lint_stdin(source: &SourceFile, kind: FileKind, config: &LintConfig) -> Vec { use actlint_core::rules::{ALL_RULES, AstKind, LintFile, RulePhase, RuleTarget}; diff --git a/actlint-core/src/fuzz_regression.rs b/actlint-core/src/fuzz_regression.rs index 110aa3c..769ed29 100644 --- a/actlint-core/src/fuzz_regression.rs +++ b/actlint-core/src/fuzz_regression.rs @@ -14,7 +14,7 @@ use crate::{ source::SourceFile, }; -/// Run all registered rules against a source file via the new ALL_RULES registry. +/// Run all registered rules against a source file via the new `ALL_RULES` registry. fn run_all_rules(source: &SourceFile, kind: crate::detect::FileKind) { let config = LintConfig::default(); diff --git a/actlint-core/src/linter.rs b/actlint-core/src/linter.rs index 9f8444f..d1b9de1 100644 --- a/actlint-core/src/linter.rs +++ b/actlint-core/src/linter.rs @@ -131,30 +131,27 @@ fn lint_file_with_rules( let target = target_for(kind); // Parse AST for semantic phase - match kind { - FileKind::Workflow => { - let wf = parse::workflow::parse_workflow(source)?; - let ast = AstKind::Workflow(&wf); - let file = LintFile { source, kind, ast: Some(ast), config }; - Ok(ALL_RULES - .iter() - .filter(|r| r.phase == RulePhase::Semantic) - .filter(|r| rule_applies(r, target)) - .filter(|r| !is_disabled(r, config)) - .flat_map(|r| (r.check)(&file)) - .collect()) - } - _ => { - let action = parse::action::parse_action(source)?; - let ast = AstKind::Action(&action); - let file = LintFile { source, kind, ast: Some(ast), config }; - Ok(ALL_RULES - .iter() - .filter(|r| r.phase == RulePhase::Semantic) - .filter(|r| rule_applies(r, target)) - .filter(|r| !is_disabled(r, config)) - .flat_map(|r| (r.check)(&file)) - .collect()) - } + if kind == FileKind::Workflow { + let wf = parse::workflow::parse_workflow(source)?; + let ast = AstKind::Workflow(&wf); + let file = LintFile { source, kind, ast: Some(ast), config }; + Ok(ALL_RULES + .iter() + .filter(|r| r.phase == RulePhase::Semantic) + .filter(|r| rule_applies(r, target)) + .filter(|r| !is_disabled(r, config)) + .flat_map(|r| (r.check)(&file)) + .collect()) + } else { + let action = parse::action::parse_action(source)?; + let ast = AstKind::Action(&action); + let file = LintFile { source, kind, ast: Some(ast), config }; + Ok(ALL_RULES + .iter() + .filter(|r| r.phase == RulePhase::Semantic) + .filter(|r| rule_applies(r, target)) + .filter(|r| !is_disabled(r, config)) + .flat_map(|r| (r.check)(&file)) + .collect()) } } diff --git a/actlint-core/src/rules/action_docker_image_not_found.rs b/actlint-core/src/rules/action_docker_image_not_found.rs index 63c8db7..07a3f3e 100644 --- a/actlint-core/src/rules/action_docker_image_not_found.rs +++ b/actlint-core/src/rules/action_docker_image_not_found.rs @@ -37,7 +37,9 @@ fn check(file: &LintFile<'_>) -> Vec { }; let resolved = dir.join(&img.value); - if !resolved.exists() { + if resolved.exists() { + Vec::new() + } else { vec![ LintDiagnostic::warning( "semantic/action-docker-image-not-found", @@ -47,8 +49,6 @@ fn check(file: &LintFile<'_>) -> Vec { ) .with_help(format!("expected file at: {}", resolved.display())), ] - } else { - Vec::new() } } diff --git a/actlint-core/src/rules/action_expression_contexts.rs b/actlint-core/src/rules/action_expression_contexts.rs index 3826ce9..f0186de 100644 --- a/actlint-core/src/rules/action_expression_contexts.rs +++ b/actlint-core/src/rules/action_expression_contexts.rs @@ -167,7 +167,7 @@ fn extract_expressions(value: &str) -> Vec { #[allow(clippy::panic)] mod tests { use super::*; - use crate::rules::testing::{assert_rule_clean, run_rule}; + use crate::rules::testing::assert_rule_clean; const VALID_COMPOSITE: &str = r" name: My Action diff --git a/actlint-core/src/rules/action_file_not_found.rs b/actlint-core/src/rules/action_file_not_found.rs index 0deef2f..762312d 100644 --- a/actlint-core/src/rules/action_file_not_found.rs +++ b/actlint-core/src/rules/action_file_not_found.rs @@ -68,7 +68,7 @@ fn check_file_exists( #[allow(clippy::panic)] mod tests { use super::*; - use crate::rules::testing::{assert_rule_clean, run_rule}; + use crate::rules::testing::run_rule; const MISSING_FILE: &str = r" name: My Action diff --git a/actlint-core/src/rules/action_invalid_branding_color.rs b/actlint-core/src/rules/action_invalid_branding_color.rs index ee0b5d0..685ab72 100644 --- a/actlint-core/src/rules/action_invalid_branding_color.rs +++ b/actlint-core/src/rules/action_invalid_branding_color.rs @@ -28,15 +28,15 @@ fn check(file: &LintFile<'_>) -> Vec { return Vec::new(); }; - if !VALID_COLORS.contains(&color.value.as_str()) { + if VALID_COLORS.contains(&color.value.as_str()) { + Vec::new() + } else { vec![LintDiagnostic::error( "semantic/action-invalid-branding-color", format!("invalid branding color '{}', expected one of: {}", color.value, VALID_COLORS.join(", ")), color.span, file.source.named_source(), )] - } else { - Vec::new() } } diff --git a/actlint-core/src/rules/action_invalid_branding_icon.rs b/actlint-core/src/rules/action_invalid_branding_icon.rs index 9c03b9a..fb985c4 100644 --- a/actlint-core/src/rules/action_invalid_branding_icon.rs +++ b/actlint-core/src/rules/action_invalid_branding_icon.rs @@ -286,7 +286,9 @@ fn check(file: &LintFile<'_>) -> Vec { return Vec::new(); }; - if !VALID_ICONS.contains(&icon.value.as_str()) { + if VALID_ICONS.contains(&icon.value.as_str()) { + Vec::new() + } else { vec![ LintDiagnostic::error( "semantic/action-invalid-branding-icon", @@ -296,8 +298,6 @@ fn check(file: &LintFile<'_>) -> Vec { ) .with_help("see https://feathericons.com for valid icon names"), ] - } else { - Vec::new() } } diff --git a/actlint-core/src/rules/action_invalid_using.rs b/actlint-core/src/rules/action_invalid_using.rs index c482f28..6f5ac2a 100644 --- a/actlint-core/src/rules/action_invalid_using.rs +++ b/actlint-core/src/rules/action_invalid_using.rs @@ -33,7 +33,9 @@ fn check(file: &LintFile<'_>) -> Vec { Runs::Composite { .. } => return Vec::new(), }; - if !VALID_USING.contains(&using.value.as_str()) { + if VALID_USING.contains(&using.value.as_str()) { + Vec::new() + } else { vec![ LintDiagnostic::error( "semantic/action-invalid-using", @@ -48,8 +50,6 @@ fn check(file: &LintFile<'_>) -> Vec { .with_label(using.span, "invalid value here") .with_help("valid values are: composite, node12, node16, node20, node22, node24, docker"), ] - } else { - Vec::new() } } @@ -57,7 +57,7 @@ fn check(file: &LintFile<'_>) -> Vec { #[allow(clippy::panic)] mod tests { use super::*; - use crate::rules::testing::{assert_rule_clean, run_rule}; + use crate::rules::testing::assert_rule_clean; const VALID: &str = r" name: My Action diff --git a/actlint-core/src/rules/mod.rs b/actlint-core/src/rules/mod.rs index 2027c63..092d338 100644 --- a/actlint-core/src/rules/mod.rs +++ b/actlint-core/src/rules/mod.rs @@ -27,6 +27,15 @@ pub enum AstKind<'src> { Action(&'src crate::ast::action::Action), } +impl std::fmt::Debug for AstKind<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + Self::Workflow(_) => f.write_str("AstKind::Workflow(..)"), + Self::Action(_) => f.write_str("AstKind::Action(..)"), + } + } +} + impl<'src> AstKind<'src> { pub fn as_workflow(&self) -> Option<&'src crate::ast::workflow::Workflow> { match self { @@ -50,6 +59,12 @@ pub struct LintFile<'src> { pub config: &'src crate::linter::LintConfig, } +impl std::fmt::Debug for LintFile<'_> { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("LintFile").field("kind", &self.kind).field("ast", &self.ast).finish_non_exhaustive() + } +} + pub type RuleCheckFn = for<'src> fn(&LintFile<'src>) -> Vec; pub struct RuleDef { @@ -63,6 +78,16 @@ pub struct RuleDef { pub check: RuleCheckFn, } +impl std::fmt::Debug for RuleDef { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("RuleDef") + .field("id", &self.id) + .field("namespace", &self.namespace) + .field("phase", &self.phase) + .finish_non_exhaustive() + } +} + // ── Macros ───────────────────────────────────────────────────────────────── /// Declare a lint rule. Generates `pub(crate) const RULE: RuleDef` in the @@ -95,7 +120,7 @@ macro_rules! declare_rule { }; } -/// Declare all rule modules and build ALL_RULES. +/// Declare all rule modules and build `ALL_RULES`. /// To add a rule: add its module name here and create the corresponding file. macro_rules! register_rules { ($($module:ident),+ $(,)?) => { diff --git a/fuzz-core/slow-unit-425a76ca297fbf6cd70f0733ac2a6a3f34d2ce93 b/fuzz-core/slow-unit-425a76ca297fbf6cd70f0733ac2a6a3f34d2ce93 new file mode 100644 index 0000000..dcfd621 --- /dev/null +++ b/fuzz-core/slow-unit-425a76ca297fbf6cd70f0733ac2a6a3f34d2ce93 @@ -0,0 +1 @@ + diff --git a/fuzz-core/slow-unit-e3ceb53837e69a0b7c274a750dd5698540244441 b/fuzz-core/slow-unit-e3ceb53837e69a0b7c274a750dd5698540244441 new file mode 100644 index 0000000..b180b00 --- /dev/null +++ b/fuzz-core/slow-unit-e3ceb53837e69a0b7c274a750dd5698540244441 @@ -0,0 +1 @@ +:››››››››››››››››››››