diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 1dd2725c1..cd5b28bbf 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -728,7 +728,9 @@ jobs: # One command per step: pwsh carries on past a failing native command and # the step reports only the last command's exit code, so a multi-line - # block would let an early self-test fail unseen. + # block would let an early self-test fail unseen. Enforced for every pwsh + # step by `no_pwsh_step_lets_a_failing_native_command_pass_unseen` in + # xtask/src/workflow_contract.rs. - name: Acceptance harness self-test (ComfyUI) if: needs.changes.outputs.heavy == 'true' shell: pwsh diff --git a/xtask/src/workflow_contract.rs b/xtask/src/workflow_contract.rs index 921a60dd6..57981c420 100644 --- a/xtask/src/workflow_contract.rs +++ b/xtask/src/workflow_contract.rs @@ -999,23 +999,7 @@ trigger-a-workflow#triggering-a-workflow-from-a-workflow" continue; } let step = step_block(&lines, i); - // `name:` is either the item's own first key (`- name: …`) or one - // of its sibling keys, two columns in from the dash. - let name = step - .lines() - .find_map(|l| { - let key_line = if indent_of(l) == item_indent { - l.trim_start().strip_prefix("- ")? - } else if indent_of(l) == item_indent + 2 { - l.trim_start() - } else { - return None; - }; - key_line.strip_prefix("name:") - }) - .map(|value| strip_quotes(strip_comment(value).trim())) - .unwrap_or_default(); - steps.push((name, step)); + steps.push((step_name(&step), step)); } Some(steps) } @@ -1402,6 +1386,37 @@ trigger-a-workflow#triggering-a-workflow-from-a-workflow" ); } + /// The `name:` of `step`, a block as `step_block` returns it (its first + /// line is the step's `- ` item line); empty when the step has none. + fn step_name(step: &str) -> String { + step_key(step, "name") + } + + /// The one-line value of `step`'s own `{key}:`, unquoted and without a + /// trailing comment; empty when the step has no such key. + /// + /// The key is either the item's own first key (`- name: …`) or one of + /// its sibling keys, two columns in from the dash. Keys any deeper — a + /// `with:` input called `name`, a `name:` line inside the step's script — + /// are not the step's. + fn step_key(step: &str, key: &str) -> String { + let marker = format!("{key}:"); + let item_indent = step.lines().next().map_or(0, indent_of); + step.lines() + .find_map(|l| { + let key_line = if indent_of(l) == item_indent { + l.trim_start().strip_prefix("- ")? + } else if indent_of(l) == item_indent + 2 { + l.trim_start() + } else { + return None; + }; + key_line.strip_prefix(&marker) + }) + .map(|value| strip_quotes(strip_comment(value).trim())) + .unwrap_or_default() + } + /// Every `GPU preflight` step block in `text`, in file order. fn gpu_preflight_steps(text: &str) -> Vec { let lines: Vec<&str> = text.lines().collect(); @@ -4864,4 +4879,423 @@ permissions: ); } } + + /// Every `shell: pwsh` step in `text` as `(name, step block)`. + /// + /// The step is found from its `shell: pwsh` line by walking up to the + /// nearest `- ` item shallower than it, so the key order inside the step + /// does not matter. + fn pwsh_steps(text: &str) -> Vec<(String, String)> { + let lines: Vec<&str> = text.lines().collect(); + let mut steps = Vec::new(); + for (at, line) in lines.iter().enumerate() { + if strip_comment(line).trim() != "shell: pwsh" { + continue; + } + let shell_indent = indent_of(line); + let Some(start) = (0..at).rev().find(|&i| { + indent_of(lines[i]) < shell_indent && lines[i].trim_start().starts_with("- ") + }) else { + continue; + }; + let step = step_block(&lines, start); + steps.push((step_name(&step), step)); + } + steps + } + + /// PowerShell words that open a statement but are not a program to run. + const PWSH_KEYWORDS: [&str; 15] = [ + "if", "elseif", "else", "foreach", "for", "while", "do", "switch", "function", "try", + "catch", "finally", "throw", "exit", "return", + ]; + + /// Whether a trimmed, comment-free pwsh statement runs an external + /// program: `python …`, `cargo …`, `gh …`, `.\tool.exe`, `& $exe …`. + /// + /// Deliberately rough, and erring towards "native": assignments and + /// variables (`$x = …`), keywords, braces, string literals piped + /// somewhere (`"k=v" | Out-File …`) and cmdlets (`Verb-Noun`, which start + /// upper-case) are not; a lower-case first word that is not a keyword is. + fn is_native_command_line(line: &str) -> bool { + let line = line.trim(); + if line.starts_with('&') { + return true; + } + let first = line + .split(|c: char| c.is_whitespace() || c == '(' || c == '{') + .next() + .unwrap_or_default(); + let Some(lead) = first.chars().next() else { + return false; + }; + (lead.is_ascii_lowercase() || lead == '.') + && !PWSH_KEYWORDS.contains(&first.to_ascii_lowercase().as_str()) + } + + /// The statements of a pwsh `run: |` body, in order. + /// + /// A line is split at its top-level `;`s (see + /// [`split_top_level_statements`]), so `Write-Output 'ok'; sccache + /// --start-server` is two statements. A backtick continuation is joined + /// onto the line it continues. A here-string (`@'`/`@"` … `'@`/`"@`) + /// stands in as `@'…'@` for the statement it opens, and whatever follows + /// the closing marker on its line is analysed as the rest of that + /// statement: `'@ | Out-File x; cargo build` still yields `cargo build`. + /// Comments and blank lines are dropped. + fn pwsh_statements(body: &[&str]) -> Vec { + let mut statements = Vec::new(); + // The logical line built so far: a line plus any continuation and + // here-string it carries on into. + let mut pending = String::new(); + let mut here_string_close: Option<&str> = None; + for raw in body { + let line = raw.trim(); + if let Some(close) = here_string_close { + // Only a line that starts with the opener's own closing + // marker ends a here-string; anything else is its text. + let Some(rest) = line.strip_prefix(close) else { + continue; + }; + here_string_close = None; + pending.push('…'); + pending.push_str(close); + pending.push_str(rest); + } else { + if pending.is_empty() && (line.is_empty() || line.starts_with('#')) { + continue; + } + if !pending.is_empty() { + pending.push(' '); + } + pending.push_str(line); + } + if let Some(continued) = pending.strip_suffix('`') { + pending = continued.trim_end().to_owned(); + continue; + } + if pending.ends_with("@'") { + here_string_close = Some("'@"); + continue; + } + if pending.ends_with("@\"") { + here_string_close = Some("\"@"); + continue; + } + statements.extend(split_top_level_statements(&pending)); + pending.clear(); + } + statements.extend(split_top_level_statements(&pending)); + statements + } + + /// One logical pwsh line split at each `;` that ends a statement: those + /// outside quotes, outside `(…)`/`{…}` (script blocks, hashtables, + /// subexpressions — `@{ a = 1; b = 2 }` is one statement), not escaped + /// with a backtick, and before any `#` comment, which is dropped. Each + /// statement comes back trimmed; empty ones are skipped. + fn split_top_level_statements(line: &str) -> Vec { + let mut statements = Vec::new(); + let mut current = String::new(); + let mut depth = 0usize; + let mut quote: Option = None; + // A `#` opens a comment only where a token can start. + let mut at_token_start = true; + let mut chars = line.chars(); + while let Some(c) = chars.next() { + match (quote, c) { + // Inside '…' nothing is special; `''` is two adjacent strings + // as far as splitting is concerned, which is the same thing. + (Some('\''), '\'') | (Some('"'), '"') => quote = None, + (Some('"') | None, '`') => { + current.push(c); + if let Some(escaped) = chars.next() { + current.push(escaped); + } + at_token_start = false; + continue; + } + (Some(_), _) => {} + (None, '\'' | '"') => quote = Some(c), + (None, '#') if at_token_start => break, + (None, '(' | '{') => depth += 1, + (None, ')' | '}') => depth = depth.saturating_sub(1), + (None, ';') if depth == 0 => { + statements.push(std::mem::take(&mut current)); + at_token_start = true; + continue; + } + (None, _) => {} + } + current.push(c); + at_token_start = quote.is_none() && (c.is_whitespace() || "({;|".contains(c)); + } + statements.push(current); + statements + .into_iter() + .map(|statement| statement.trim().to_owned()) + .filter(|statement| !statement.is_empty()) + .collect() + } + + /// The native-command statements in a pwsh body whose exit status + /// nothing reads: not the body's last statement, and not followed by a + /// statement that inspects `$LASTEXITCODE`. + fn unchecked_native_commands(body: &[&str]) -> Vec { + let statements = pwsh_statements(body); + statements + .iter() + .enumerate() + .filter(|(i, line)| { + is_native_command_line(line) + && statements + .get(i + 1) + .is_some_and(|next| !next.contains("$LASTEXITCODE")) + }) + .map(|(_, line)| line.clone()) + .collect() + } + + /// [`unchecked_native_commands`] over a pwsh step's script: its `run: |` + /// block, or its one-line `run:` value, which can chain statements with + /// `;` just the same. + fn unchecked_native_commands_in_step(step: &str) -> Vec { + match run_block(step) { + Some(body) => unchecked_native_commands(&body), + None => unchecked_native_commands(&[step_key(step, "run").as_str()]), + } + } + + /// pwsh steps allowed to run a native command whose status is unread, + /// keyed by `(workflow, step name)`, each with the reason it is safe. + const PWSH_UNCHECKED_NATIVE_ALLOWED: [(&str, &str, &str); 3] = [ + ( + "ci.yml", + "Probe sccache cache backend", + "the probe decides for itself and ends in `exit 0`; the one unread \ + status is a best-effort `sccache --stop-server` cleanup", + ), + ( + "nightly.yml", + "Package Windows bundle", + "a failed `cargo xtask package` leaves no zip or signature, so the \ + `Copy-Item`/`throw` on its outputs stops the step under the runner's \ + `$ErrorActionPreference = 'stop'`", + ), + ( + "release.yml", + "Package Windows bundle", + "a failed `cargo xtask package` leaves no zip or signature, so the \ + `Copy-Item`/`throw` on its outputs stops the step under the runner's \ + `$ErrorActionPreference = 'stop'`", + ), + ]; + + /// GitHub runs a `shell: pwsh` body with `$ErrorActionPreference = 'stop'` + /// and then `exit $LASTEXITCODE`. A failing *cmdlet* stops the step, but a + /// failing *native command* does not: the script carries on, and only the + /// last native command's status decides the step. So in + /// + /// ```text + /// python a.py --self-test # exits 1 + /// python b.py --self-test # exits 0 + /// ``` + /// + /// the step is green. Every native command that is not the body's last + /// statement must therefore have its `$LASTEXITCODE` read on the next + /// line, or be split into its own step (#633), or be allow-listed above + /// with the reason its failure still surfaces. + #[test] + fn no_pwsh_step_lets_a_failing_native_command_pass_unseen() { + let dir = repo_root().join(".github/workflows"); + let mut workflows: Vec = std::fs::read_dir(&dir) + .unwrap_or_else(|e| panic!("reading {}: {e}", dir.display())) + .map(|entry| entry.expect("workflow dir entry").path()) + .filter(|path| { + path.extension().is_some_and(|ext| { + ext.eq_ignore_ascii_case("yml") || ext.eq_ignore_ascii_case("yaml") + }) + }) + .map(|path| { + path.file_name() + .expect("a read_dir entry has a file name") + .to_string_lossy() + .into_owned() + }) + .collect(); + workflows.sort(); + + let mut offenders = Vec::new(); + let mut allowed_seen = Vec::new(); + for workflow in &workflows { + let text = read_workflow(workflow); + for (name, step) in pwsh_steps(&text) { + let unchecked = unchecked_native_commands_in_step(&step); + if unchecked.is_empty() { + continue; + } + if PWSH_UNCHECKED_NATIVE_ALLOWED + .iter() + .any(|(w, n, _)| w == workflow && *n == name) + { + allowed_seen.push((workflow.clone(), name)); + continue; + } + offenders.push(format!("{workflow} step {name:?}: {unchecked:?}")); + } + } + assert!( + offenders.is_empty(), + "these `shell: pwsh` steps run a native command whose failure the step \ + would not report, because pwsh carries on and the step takes only the last \ + command's exit code — split each into its own step, or check \ + `$LASTEXITCODE` right after it:\n {}", + offenders.join("\n ") + ); + // An allow-list entry that no longer matches anything exempts nothing + // and would quietly cover the next step to take that name. + for (workflow, name, _) in PWSH_UNCHECKED_NATIVE_ALLOWED { + assert!( + allowed_seen.iter().any(|(w, n)| w == workflow && n == name), + "{workflow} step {name:?} is allow-listed in \ + PWSH_UNCHECKED_NATIVE_ALLOWED but no longer has an unchecked native \ + command; drop the entry" + ); + } + } + + /// The masking shape #633 removed is still caught, and the shapes that + /// read the status are not. + #[test] + fn unchecked_native_commands_finds_the_masking_shape() { + let merged = [ + "python .\\scripts\\comfyui_therock_gpu_test.py --self-test", + "python .\\scripts\\local_assistant_therock_gpu_test.py --self-test", + "python .\\scripts\\vllm_therock_gpu_test.py --self-test", + ]; + assert_eq!(unchecked_native_commands(&merged), merged[..2].to_vec()); + + let checked = [ + "$targetDir = if ($env:X) { $env:X } else { \"target\" }", + "# a comment", + "sccache --start-server", + "if ($LASTEXITCODE -ne 0) {", + " Write-Output \"::warning::no cache\"", + "}", + "@'", + "cargo build # inside a here-string, not a command", + "'@ | Out-File -FilePath $env:GITHUB_STEP_SUMMARY", + "cargo xtask e2e", + "if ($LASTEXITCODE -ne 0) { exit $LASTEXITCODE }", + "python .\\scripts\\release_readiness.py `", + " --dist dist", + ]; + assert!( + unchecked_native_commands(&checked).is_empty(), + "{:?}", + unchecked_native_commands(&checked) + ); + } + + /// A `;` ends a statement, so a native command chained before another + /// statement on one line is checked like one on a line of its own — but + /// only a top-level `;`: not one in a string, a hashtable or script + /// block, a comment, or escaped with a backtick. + #[test] + fn unchecked_native_commands_splits_statements_at_top_level_semicolons() { + let chained = [ + "Write-Output 'ok'; sccache --start-server", + "cargo xtask e2e", + ]; + assert_eq!( + unchecked_native_commands(&chained), + ["sccache --start-server"] + ); + assert_eq!( + unchecked_native_commands(&["cargo build; cargo test"]), + ["cargo build"] + ); + assert!( + unchecked_native_commands(&["cargo build; if ($LASTEXITCODE -ne 0) { exit 1 }"]) + .is_empty() + ); + + // Each of these is ONE statement; splitting it would leave a + // lower-case fragment (`cargo`, `b`, `then`) that reads as a native + // command and, followed by `cargo xtask e2e`, as an unchecked one. + for line in [ + "Write-Output \"done; cargo is next\"", + "Write-Output 'done; cargo is next'", + "Write-Output \"it`\"s done; cargo is next\"", + "$h = @{ a = 1; b = 2 }", + "$v = $(Get-Date; Get-Location)", + "Write-Output done # then; cargo", + "Write-Output done`; then", + ] { + let body = [line, "cargo xtask e2e"]; + assert!( + unchecked_native_commands(&body).is_empty(), + "{line:?} split into {:?}", + pwsh_statements(&body) + ); + } + + // A one-line `run:` chains statements just the same. + let step = " - name: Lint\n shell: pwsh\n \ + run: cargo fmt --check; cargo clippy # both\n"; + assert_eq!( + unchecked_native_commands_in_step(step), + ["cargo fmt --check"] + ); + } + + /// Only the closing marker of a here-string is skipped: the rest of its + /// line is the tail of the statement the here-string opened, and any + /// statement chained after it is checked. + #[test] + fn unchecked_native_commands_reads_on_past_a_here_string_close() { + let body = [ + "@'", + "## Summary", + "'@ | Out-File -FilePath $env:GITHUB_STEP_SUMMARY; sccache --start-server", + "cargo xtask e2e", + ]; + assert_eq!( + pwsh_statements(&body), + [ + "@'…'@ | Out-File -FilePath $env:GITHUB_STEP_SUMMARY", + "sccache --start-server", + "cargo xtask e2e", + ] + ); + assert_eq!(unchecked_native_commands(&body), ["sccache --start-server"]); + + // A here-string inside the statement that reads the status leaves + // that statement whole: `if ($LASTEXITCODE …) { throw @"…"@ }`. + let checked = [ + "cargo build", + "if ($LASTEXITCODE -ne 0) { throw @\"", + "build failed", + "\"@ }", + "cargo xtask e2e", + ]; + assert!( + unchecked_native_commands(&checked).is_empty(), + "{:?}", + pwsh_statements(&checked) + ); + + // `"@` does not close a `@'` here-string: its text runs on to `'@`. + let mismatched = [ + "@'", + "\"@; cargo inside the text", + "'@ | Out-File x", + "cargo xtask e2e", + ]; + assert!( + unchecked_native_commands(&mismatched).is_empty(), + "{:?}", + pwsh_statements(&mismatched) + ); + } }