fix: harden steps. boundary check, document condition semantics, dedup cycles

The steps. word-boundary heuristic in evaluate_job_condition was
checking for alphanumeric characters before "steps." to avoid false
positives on env vars like "env.MY_STEPS_COUNT". It turns out that
underscore is *not* alphanumeric, so "env._STEPS_CHECK" would
incorrectly trigger the step-reference path and return false.

While at it, the always() && failure() compound expression returning
true got a proper comment explaining *why* that's intentional — we
lack step-status context locally, so we'd rather over-run than
silently skip steps. Not ideal, but honest.

The DFS cycle detector in detect_cyclic_needs could report the same
cycle multiple times depending on HashMap iteration order. Normalize
cycles by rotating the node list to start at the lexicographically
smallest node, then deduplicate via a HashSet. Same cycle from
different entry points now gets reported exactly once.

crates/executor/src/engine.rs

@@ -2968,11 +2968,14 @@ fn evaluate_job_condition(
     let has_failure = condition.contains("failure()");
     let has_cancelled = condition.contains("cancelled()");
     // Match "steps." only at word boundaries to avoid false positives on env var
-    // names like "env.MY_STEPS_COUNT". We check for start-of-string or a non-alphanumeric
-    // character immediately before "steps.".
-    let has_steps_ref = condition
-        .match_indices("steps.")
-        .any(|(pos, _)| pos == 0 || !condition.as_bytes()[pos - 1].is_ascii_alphanumeric());
+    // names like "env.MY_STEPS_COUNT" or "env._STEPS_CHECK". We check for
+    // start-of-string or a character that isn't alphanumeric/underscore before "steps.".
+    let has_steps_ref = condition.match_indices("steps.").any(|(pos, _)| {
+        pos == 0 || {
+            let b = condition.as_bytes()[pos - 1];
+            !b.is_ascii_alphanumeric() && b != b'_'
+        }
+    });
     let has_unsupported =
         has_always || has_success || has_failure || has_cancelled || has_steps_ref;
 
@@ -2982,7 +2985,17 @@ fn evaluate_job_condition(
             condition
         ));
 
-        // always() or success() → true (common "run this step" intent)
+        // In GitHub Actions, `always()` means "run this step regardless of job
+        // status" — it is a *scheduling* directive, not a boolean `true` literal.
+        // Similarly, `success()` means "run when all previous steps succeeded".
+        // Since we can't evaluate actual job/step status locally, we treat
+        // `always()` and `success()` as "likely to run" → true, and `failure()`
+        // / `cancelled()` as "unlikely" → false.
+        //
+        // Known limitation: compound expressions like `always() && failure()` will
+        // return true (because `always()` is present) even though a real evaluator
+        // would AND the two. This is acceptable because we lack step-status context
+        // and would rather over-run than silently skip steps.
         if has_always || has_success {
             return true;
         }
@@ -3471,6 +3484,12 @@ mod tests {
             &env,
             &wf
         ));
+        // Underscore-prefixed names should also NOT be treated as step refs
+        assert!(evaluate_job_condition(
+            "env._STEPS_CHECK == 'ok'",
+            &env,
+            &wf
+        ));
         assert!(!evaluate_job_condition(
             "steps.build.outcome == 'success'",
             &env,

crates/validators/src/jobs.rs

@@ -149,6 +149,7 @@ fn detect_cyclic_needs(jobs_map: &serde_yaml::Mapping, result: &mut ValidationRe
     let mut visited = HashSet::new();
     let mut in_stack = HashSet::new();
     let mut rec_stack = Vec::new();
+    let mut reported_cycles: HashSet<Vec<String>> = HashSet::new();
 
     for job_name in graph.keys() {
         if !visited.contains(job_name.as_str()) {
@@ -158,6 +159,7 @@ fn detect_cyclic_needs(jobs_map: &serde_yaml::Mapping, result: &mut ValidationRe
                 &mut visited,
                 &mut in_stack,
                 &mut rec_stack,
+                &mut reported_cycles,
                 result,
             );
         }
@@ -170,6 +172,7 @@ fn dfs_detect_cycle(
     visited: &mut HashSet<String>,
     in_stack: &mut HashSet<String>,
     rec_stack: &mut Vec<String>,
+    reported_cycles: &mut HashSet<Vec<String>>,
     result: &mut ValidationResult,
 ) {
     visited.insert(node.to_string());
@@ -181,19 +184,41 @@ fn dfs_detect_cycle(
             if in_stack.contains(neighbor.as_str()) {
                 // Found a cycle — build the cycle path from the stack
                 if let Some(pos) = rec_stack.iter().position(|x| x == neighbor) {
-                    let cycle = rec_stack[pos..]
+                    // Normalize the cycle: rotate so the lexicographically smallest
+                    // node is first, ensuring the same cycle isn't reported twice
+                    // from different entry points.
+                    let mut cycle_nodes: Vec<String> = rec_stack[pos..].iter().cloned().collect();
+                    if let Some(min_pos) = cycle_nodes
                         .iter()
-                        .chain(std::iter::once(neighbor))
-                        .cloned()
-                        .collect::<Vec<_>>()
-                        .join(" -> ");
-                    result.add_issue(format!(
-                        "Circular dependency detected in 'needs': {}",
-                        cycle
-                    ));
+                        .enumerate()
+                        .min_by_key(|(_, n)| n.clone())
+                        .map(|(i, _)| i)
+                    {
+                        cycle_nodes.rotate_left(min_pos);
+                    }
+                    if reported_cycles.insert(cycle_nodes.clone()) {
+                        let display = cycle_nodes
+                            .iter()
+                            .chain(std::iter::once(&cycle_nodes[0]))
+                            .cloned()
+                            .collect::<Vec<_>>()
+                            .join(" -> ");
+                        result.add_issue(format!(
+                            "Circular dependency detected in 'needs': {}",
+                            display
+                        ));
+                    }
                 }
             } else if !visited.contains(neighbor.as_str()) {
-                dfs_detect_cycle(neighbor, graph, visited, in_stack, rec_stack, result);
+                dfs_detect_cycle(
+                    neighbor,
+                    graph,
+                    visited,
+                    in_stack,
+                    rec_stack,
+                    reported_cycles,
+                    result,
+                );
             }
         }
     }