diff --git a/crates/executor/src/engine.rs b/crates/executor/src/engine.rs index 2ecd420..0d5691d 100644 --- a/crates/executor/src/engine.rs +++ b/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, diff --git a/crates/validators/src/jobs.rs b/crates/validators/src/jobs.rs index 48321e5..69d2934 100644 --- a/crates/validators/src/jobs.rs +++ b/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> = 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, in_stack: &mut HashSet, rec_stack: &mut Vec, + reported_cycles: &mut HashSet>, 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 = rec_stack[pos..].iter().cloned().collect(); + if let Some(min_pos) = cycle_nodes .iter() - .chain(std::iter::once(neighbor)) - .cloned() - .collect::>() - .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::>() + .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, + ); } } }