Repository navigation
test(workflows): fail a pwsh step that can mask a native failure - #658
Conversation
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed this with two independent passes (one explicitly blind, since my own driving context had already read the PR description and the #633 thread).
The core logic checks out: all three allow-list entries, the "33 shell: powershell steps" count, and the described mutation checks all match the real workflow files exactly, and the powershell-exclusion / deferred nightly-release fix are reasonable, disclosed scope decisions rather than silent gaps.
One real issue I'd like fixed before merge, left inline below: pwsh_steps's step-name extraction duplicates the existing job_steps logic in this same file line-for-line. Worth pulling into a shared step_name helper per the repo's "reuse existing shared helpers" convention rather than maintaining two copies that can drift.
Two more inline notes on the new detection logic - neither is a live bug (no current workflow line is mis-classified), but both are real false-negative gaps worth knowing about for future edits.
Separately: "E2E tests" is currently failing on this PR (scenario chat-tool-definitions-accepted) - this isn't one of the known-flaky tests (comfyui status/cli_progress ticker/examine framework probe/therock tarball unpack/Strix Halo cancellation), and doesn't obviously trace back to this PR's own changed files (xtask test code + a workflow comment), but it's a live red X and worth checking before merging rather than assuming it's unrelated. "E2E tests (MI300X)" is also still pending.
Requesting changes mainly for the duplication - happy to re-review once that's split out and the E2E failure is sorted out or explained.
| /// 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)> { |
There was a problem hiding this comment.
This step-name extraction (walk up to the nearest shallower - item, then pull the name: key) duplicates job_steps's existing logic in this same file. Could this call into a shared step_name helper instead, so the two don't drift independently? Not a correctness bug, but it's the one hard convention violation I'd want fixed before merge.
There was a problem hiding this comment.
Done in 378e1f83. The lookup now lives in one step_key(step, key) helper, and step_name is step_key(step, "name"); job_steps and pwsh_steps both use it. It reads the item indent from the step's first line, so neither caller passes it. To check nothing moved, I dumped every step name job_steps extracts from every workflow, and every pwsh_steps entry, before and after. The dumps are identical, and all workflow_contract tests pass.
| if line.ends_with("@'") || line.ends_with("@\"") { | ||
| in_here_string = true; | ||
| } | ||
| statements.push(line.to_string()); |
There was a problem hiding this comment.
Latent gap, not a live bug today: statements aren't split on ;, so a semicolon-chained line like Write-Output 'ok'; sccache --start-server is treated as one statement and the trailing native command's exit status would never be checked. No current workflow line hits this, but worth a follow-up if it's cheap - otherwise just flagging it for awareness.
There was a problem hiding this comment.
Fixed in 9ef57a17. Each logical line is now split at top-level ;, but not inside '…', "…", (…), $(…), {…} or @{…}, after a # comment, or when backtick-escaped. Backtick continuations are joined instead of dropped. One-line run: values are checked too, since they can also chain with ;. unchecked_native_commands_splits_statements_at_top_level_semicolons covers your example and the must-not-split cases, and removing any one guard fails it. On the real workflows the unchecked set is unchanged: the same three allow-listed steps. Still out of reach by design: a native command inside a one-line script block such as try { cargo a; cargo b }.
| for raw in body { | ||
| let line = raw.trim(); | ||
| if in_here_string { | ||
| in_here_string = !(line.starts_with("'@") || line.starts_with("\"@")); |
There was a problem hiding this comment.
Latent gap: when a here-string's closing line ('@/"@) has real code appended after it on the same line, that whole line is dropped from analysis rather than just the closing marker. ci.yml's "Add install instructions to job summary" step already has this shape, and it's harmless today only because what gets dropped there is a cmdlet rather than a native command. Worth a narrower check (only the closing marker itself, not the whole line) if you're touching this again.
There was a problem hiding this comment.
Fixed in 9ef57a17. Only the opener's own closing marker ends a here-string, so "@ no longer closes @'. The here-string stands in as @'…'@ in the statement that opened it, and the rest of the closing line is read as that statement's tail. So '@ | Out-File x; cargo build yields cargo build. ci.yml's "Add install instructions to job summary" now parses as @'…'@ | Out-File …, still correctly not a native command. Test: unchecked_native_commands_reads_on_past_a_here_string_close. Making it drop the rest of the line, or letting any marker close, fails it.
|
Pushed
Each fix has a test, and 9 mutations of the new parsing are each caught. Re-running the contract over all workflows flags nothing new; the unchecked set is still the three allow-listed steps. On the red E2E tests: the failing scenario is chat-06 ( |
GitHub runs a `shell: pwsh` body under `$ErrorActionPreference = 'stop'`, which stops on a failing cmdlet but not on a failing native command; the step then reports only the last command's exit code. #633 split the Windows acceptance self-tests into one step each for that reason, but nothing stopped them being merged back. The new workflow-contract test walks every `shell: pwsh` step in every workflow and fails on a native command that is neither the body's last statement nor followed by a `$LASTEXITCODE` check. Three existing steps are allow-listed with the reason their failures still surface (the sccache probe decides for itself and ends in `exit 0`; the nightly and release packaging steps act on `cargo xtask package`'s outputs, which throw when missing), and a stale allow-list entry fails too. Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…h_steps Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
…tring close
The pwsh contract treated each line as one statement, so a native command
chained before another with `;` was never checked, and a here-string's
closing line was dropped whole, hiding anything after the `'@`.
Split each logical line at `;`s outside quotes, `(...)`/`{...}` and
comments; join backtick continuations; consume only the closing marker
of a here-string and analyse the rest of its line; and check one-line
`run:` values too, since they can chain with `;` as well.
Signed-off-by: Roman Inflianskas <Roman.Inflianskas@amd.com>
9ef57a1 to
c56db1a
Compare
|
Rebased onto main
|
juhovainio
left a comment
juhovainio
left a comment
There was a problem hiding this comment.
Reviewed the workflow-contract test change. Verified locally in a separate worktree: cargo test -p xtask workflow_contract (69/69 passing, including the 3 new tests) and cargo clippy -p xtask --all-targets --locked -- -D warnings (clean). The step_key refactor is behavior-preserving across job_steps and pwsh_steps, and the new tests cover the risk areas well (merge-back-into-one-step masking, top-level ; splitting with false-split traps, here-string open/close/mismatch).
A few non-blocking edge cases for future awareness, none of which are triggered by the current workflow files:
unchecked_native_commandsonly checks the statement immediately after a native command for$LASTEXITCODE; a step that inserts anything in between (even aWrite-Output) before the check would false-positive.run: |-/run: >/run: >-block-style keys aren't matched by therun: |detector, so a pwsh step authored that way would silently skip the contract check.- The
$LASTEXITCODEmatch is case-sensitive, andPWSH_KEYWORDSis missing a few keywords (break,continue,begin,process,end,param,trap), which could misclassify a native command.
Leaving the nightly/release.yml $LASTEXITCODE cleanup out of scope seems reasonable given it's a release-path change and the current allow-list reasoning is sound and self-verifying.
CI is green on everything relevant (clippy, lint, builds, coverage, sign-off). The two failing checks (MI300X E2E and its downstream consolidated report) are self-hosted runner pool timeouts, unrelated to this change.
|
Thanks. All three edge cases are real, and I will cover them in one follow-up PR once this lands:
Each fix will come with a test that fails without it. |
|
The merge queue ejected this on |
2 participants
Follows up #633 (juhovainio's note about a contract test).
GitHub runs pwsh bodies with
$ErrorActionPreference = 'stop'. That stops on cmdlet errors but not on native ones, and only the last native command's exit code decides the step. The new workflow-contract test checks everyshell: pwshstep in every workflow: a native command must be the last statement, or be followed by a$LASTEXITCODEcheck.Three existing steps are allow-listed with reasons, and a stale allow-list entry fails the test:
sccache --stop-serveris best-effort, and the step ends inexit 0.cargo xtask packageis only caught indirectly, when the followingCopy-Item/throwfails on the missing outputs. An explicit$LASTEXITCODEcheck there would be the cleaner fix. It's a release-path change, so I left it out of this PR.shell: powershellsteps are not covered yet; there are 33 of them, mostly self-hosted.Mutation checks:
pythonlines.Verified locally:
xtask(249) ande2e-cucumberlib (142), plusfeature_namingx86_64-pc-windows-gnuNo scenario: this is a CI-only change.