diff --git a/actlint-core/src/lib.rs b/actlint-core/src/lib.rs index 02f5c12..649e266 100644 --- a/actlint-core/src/lib.rs +++ b/actlint-core/src/lib.rs @@ -3,7 +3,7 @@ pub mod context; pub mod detect; pub mod diagnostic; pub mod expr; -pub mod fuzz_regression; +pub(crate) mod fuzz_regression; pub mod linter; pub mod parse; pub mod rules; diff --git a/actlint-core/src/linter.rs b/actlint-core/src/linter.rs index 489ec5d..3485f3d 100644 --- a/actlint-core/src/linter.rs +++ b/actlint-core/src/linter.rs @@ -10,6 +10,9 @@ use crate::{ /// Configuration for the linter (passed from CLI). #[derive(Debug, Default)] pub struct LintConfig { + /// Plain substring patterns. A file is skipped when its path contains any + /// of these strings. These are **not** glob patterns — `node_modules/**` + /// will not work; use `node_modules` instead. pub ignore_patterns: Vec, } @@ -35,8 +38,14 @@ pub fn lint(paths: &[PathBuf], config: &LintConfig) -> Vec { } } - // Sort by file path, then byte offset - diagnostics.sort_by(|a, b| a.span.offset().cmp(&b.span.offset())); + // Sort by file path first, then by byte offset within the file. + // Sorting by offset alone would interleave diagnostics from different + // files whose individual offsets happen to collide. + diagnostics.sort_by(|a, b| { + a.file_path() + .cmp(b.file_path()) + .then_with(|| a.span.offset().cmp(&b.span.offset())) + }); diagnostics } @@ -56,9 +65,9 @@ fn discover_files(dir: &Path, ignore_patterns: &[String], files: &mut Vec 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 @@ -93,9 +92,6 @@ fn check_circular_needs(workflow: &Workflow, source: &SourceFile, diagnostics: & break; // Report first cycle found } } - - // Silence unused variable warning in a clippy-clean way - let _ = job_count; } fn has_cycle<'a>( @@ -126,13 +122,17 @@ fn has_cycle<'a>( false } +/// A job ID must start with a letter or underscore, and contain only +/// letters, digits, underscores, or hyphens. A leading hyphen is NOT +/// valid per the GitHub Actions workflow syntax spec. 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 == '-') + // First char: letter or underscore only (not hyphen) + first.is_some_and(|c| c.is_alphabetic() || c == '_') && chars.all(|c| c.is_alphanumeric() || c == '_' || c == '-') } @@ -141,9 +141,12 @@ mod tests { use std::path::PathBuf; use super::*; - use crate::ast::{ - action::SpannedString, - workflow::{Job, Workflow}, + use crate::{ + ast::{ + action::SpannedString, + workflow::{Job, Workflow}, + }, + diagnostic::Severity, }; fn source() -> SourceFile { @@ -173,7 +176,8 @@ mod tests { 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")); + // Must fire as an Error, not a Warning + assert!(diags.iter().any(|d| d.rule_name == "workflow/no-jobs" && d.severity == Severity::Error)); } #[test] @@ -281,4 +285,35 @@ mod tests { let diags = check_jobs(&wf, &source()); assert!(diags.iter().any(|d| d.rule_name == "workflow/invalid-job-id")); } + + #[test] + fn leading_hyphen_job_id_invalid() { + // GitHub Actions requires job IDs to start with a letter or underscore, + // not a hyphen. `-build` must be rejected. + let wf = Workflow { + name: None, + on: vec![], + env: vec![], + permissions: None, + jobs: vec![job("-build", vec![])], + span: span(), + }; + let diags = check_jobs(&wf, &source()); + assert!(diags.iter().any(|d| d.rule_name == "workflow/invalid-job-id")); + } + + #[test] + fn hyphen_in_middle_of_job_id_valid() { + // Hyphens are allowed after the first character + let wf = Workflow { + name: None, + on: vec![], + env: vec![], + permissions: None, + jobs: vec![job("build-and-test", 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/tests/integration/lint_invalid.rs b/actlint-core/tests/integration/lint_invalid.rs index e0944af..96f79cc 100644 --- a/actlint-core/tests/integration/lint_invalid.rs +++ b/actlint-core/tests/integration/lint_invalid.rs @@ -19,6 +19,14 @@ fn has_rule(diagnostics: &[actlint_core::diagnostic::LintDiagnostic], rule: &str diagnostics.iter().any(|d| d.rule_name == rule) } +fn has_rule_with_severity( + diagnostics: &[actlint_core::diagnostic::LintDiagnostic], + rule: &str, + severity: actlint_core::diagnostic::Severity, +) -> bool { + diagnostics.iter().any(|d| d.rule_name == rule && d.severity == severity) +} + // --- Action schema rules --- #[test] @@ -106,7 +114,12 @@ fn js_missing_file_detected() { #[test] fn no_jobs_detected() { let diags = lint_fixture("invalid/bad-workflow"); - assert!(has_rule(&diags, "workflow/no-jobs")); + // workflow/no-jobs must fire as an Error (not a Warning) + assert!(has_rule_with_severity( + &diags, + "workflow/no-jobs", + actlint_core::diagnostic::Severity::Error, + )); } #[test] @@ -142,16 +155,40 @@ fn duplicate_step_id_detected() { } // --- Aggregate check --- +// Enumerates the full set of expected rules across all invalid fixtures. +// This catches rule regressions that the per-fixture tests above might miss +// if a shared fixture changes behaviour. #[test] -fn invalid_directory_produces_findings() { +fn invalid_directory_has_expected_rules() { let paths = vec![fixtures_path("invalid")]; let config = LintConfig::default(); let diagnostics = lint(&paths, &config); - // We should have at least 15 diagnostics across all invalid fixtures - assert!( - diagnostics.len() >= 15, - "expected at least 15 diagnostics for invalid fixtures, got: {}", - diagnostics.len() - ); + let rules: std::collections::HashSet<&str> = + diagnostics.iter().map(|d| d.rule_name.as_str()).collect(); + + let expected_rules = [ + "action/name-required", + "action/runs-required", + "action/composite-steps-required", + "action/js-main-required", + "action/docker-image-required", + "action/invalid-input-type", + "action/choice-requires-options", + "action/boolean-default-invalid", + "action/invalid-branding-icon", + "action/invalid-branding-color", + "action/output-invalid-step-ref", + "action/file-not-found", + "workflow/no-jobs", + "workflow/invalid-needs-ref", + "workflow/step-no-action", + "workflow/step-uses-and-run", + "workflow/invalid-uses", + "workflow/duplicate-step-id", + ]; + + for rule in expected_rules { + assert!(rules.contains(rule), "expected rule '{rule}' to fire across invalid fixtures, but it did not"); + } }