fix: code quality and test integrity

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;

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<String>,
 }
 
@@ -35,8 +38,14 @@ pub fn lint(paths: &[PathBuf], config: &LintConfig) -> Vec<LintDiagnostic> {
         }
     }
 
-    // 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<PathBu
             continue;
         }
 
-        // Check ignore patterns
+        // Check ignore patterns (substring match — see LintConfig docs)
         let path_str = path.to_string_lossy();
-        if ignore_patterns.iter().any(|pattern| path_str.contains(pattern)) {
+        if ignore_patterns.iter().any(|pattern| path_str.contains(pattern.as_str())) {
             continue;
         }
 

actlint-core/src/rules/workflow/jobs.rs

@@ -61,7 +61,6 @@ pub fn check_jobs(workflow: &Workflow, source: &SourceFile) -> Vec<LintDiagnosti
 
 /// Check for circular dependencies in job needs.
 fn check_circular_needs(workflow: &Workflow, source: &SourceFile, diagnostics: &mut Vec<LintDiagnostic>) {
-    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"));
+    }
 }

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");
+    }
 }