[rominf]

Follows up juhovainio's three non-blocking notes on #658. The shell: pwsh contract in xtask/src/workflow_contract.rs fails a step whose native command can fail without the step noticing. Each note was a case where it misjudged a step: either it flagged a step whose status was read, or it never looked at a step that should be checked.

A $LASTEXITCODE check later in the step. Until now a check counted only in the statement directly after the native command, so putting a Write-Output or an assignment before the check flagged a command whose status was in fact read. A later check now counts if every statement in between leaves $LASTEXITCODE alone. The rule is deliberately narrow. Text inside single quotes is ignored, since it is literal. Outside it, the statement may not contain ( (a $(…) or (…) runs a command, even inside double quotes), { (a script block, or a hashtable whose values run), & (the call operator, &&) or an expanding @" here-string. It must also be a pipeline that starts with a plain value, or an assignment of one, or with one of a short list of output/file cmdlets (Write-Output, Write-Host, Out-File, Add-Content, …), and every later element must be one of those cmdlets. Assigning $LASTEXITCODE is not a read. Anything else counts as possibly resetting the status, including exit, a function call, a loop's closing } and another native command. Reaching the end of the body also counts as a read. GitHub's pwsh wrapper appends if ((Test-Path -LiteralPath variable:\LASTEXITCODE)) { exit $LASTEXITCODE }, so a last native command followed only by a Write-Output is no longer flagged.

Every run: block form. run_block matched only the literal run: | header. A pwsh step written as run: |-, run: > or run: >- fell through to the one-line reading, whose value is the bare indicator, so the step escaped the contract. Every block-scalar header now matches, with any chomping or indentation indicator and an optional trailing comment. A folded > block is read the way YAML folds it: two lines at the block's indent become one line of script, so they are one command. The folding was compared against PyYAML on the same fixtures. Two more ways a step escaped turned up while doing this, and they are fixed in a separate commit: shell: or run: written as the step's first key (- shell: pwsh, - run: |). run_block is shared, so the other workflow checks that use it read these forms too. No workflow in the repository uses any of them, so those checks behave as before.

Case and keywords. PowerShell variable names are case-insensitive, so $LastExitCode now counts as a check. break, continue, begin, process, end, param and trap join PWSH_KEYWORDS. pwsh's own tokenizer confirms that every entry is a keyword.

Verification:

• Each fix has a test that fails with the fix reverted. Mutation runs on the new rule also caught removing each forbidden character, the scoped-assignment check, chained-assignment re-checking ($a = $b = git …), the end-of-body read, and both the single- and double-quote handling. One mutant survives: allowing a plain value after a pipe. pwsh rejects that at parse time ("Expressions are only allowed as the first element of a pipeline"), so no real step can take that form.
• Every case on both sides of the later-check rule was run through pwsh 7.4. A native command exited 3, then the in-between statements ran, then the status was printed. Every accepted case still printed 3. Every rejected case lost it, except Write-Output 'x' || git …, which is rejected conservatively.
• Over all real workflows the contract finds the same 26 pwsh steps and the same three unchecked commands as before. Those are the three allow-listed steps, so nothing is newly flagged and no allow-list entry goes stale.

Out of scope, and still not seen by the contract: a step that inherits pwsh from defaults: run: shell: pwsh (no workflow sets one); a native command whose output is assigned ($out = cargo metadata), which is not classed as native; and the literal-only run: | matching in multiline_run_blocks and preflight_step_shape, which are separate from this contract.

This is test-only (xtask contract tests) and changes nothing a CLI user can observe, so it needs no Gherkin scenario.

Review follow-up: a statement with an unclosed quote is never treated as leaving $LASTEXITCODE alone; $LASTEXITCODE counts as read only outside single quotes and as the whole name; a folded run: >N block folds at the indent its indicator sets; and the comments and messages that said only run: | is read now name every block-scalar form.

• If this PR fixes a bug, searched tests/e2e-cucumber/expectations.toml for the fixed ticket ID and removed/narrowed any now-stale xfail rows. (No ticket and no xfail rows involved.)
• If this PR adds a new subcommand or subsystem, its domain implementation lives in its own file per docs/architecture.md. (Not applicable.)
• Every new or changed user-facing message was read against the code path that runs after it. (The only changed message is the contract's own failure text. It now says to check $LASTEXITCODE before anything that could run another program or reset it, which is the rule the tests exercise.)

[juhovainio]

Looks good. The rule is narrow and conservative, both sides of it are tested, and the four commits stay on one theme. Nothing here blocks merge; a few non-blocking suggestions:

• Unterminated single quote (main one). without_single_quoted_text drops everything after a ' that never closes. The closing line of a here-string with a pipe, e.g. '@ | git hash-object --stdin, reduces to just ', which is_value_or_assignment accepts. A native command before it then reads as checked even though git resets $LASTEXITCODE. That cuts against the "errs towards might" intent. Returning "might reset" on an open quote, plus a reject case next to the accepted '@ | Add-Content ... one, would close it. The same hole hides (, {, & and later pipe elements on any line with an odd '.
• reads_last_exit_code also matches inside single-quoted text (Write-Output '$LASTEXITCODE') and as a prefix ($LASTEXITCODEX). Pre-existing, but a read now ends the walk as "read", so it is a bit more reachable.
• The doc comment after run_script still says run: | only. run_block_header also matches run: at any indent (nested keys under with:), which is fine for this repo but worth a note.
• Since this is xtask-only, the PR body could say explicitly that no Gherkin scenario applies because the change is internal.

This is based on reading the diff; CI (the Rust test jobs in particular) was still running when I looked.

[rominf]

Thanks for the review. All four points are addressed in 0e13ba7:

• The unterminated single quote: see the inline thread.
• reads_last_exit_code now only counts $LASTEXITCODE outside single quotes and as the whole name, so Write-Output '$LASTEXITCODE' and $LASTEXITCODEX no longer end the walk as a read. Both have reject cases that go red with the change reverted.
• The doc comments after run_script and on pwsh_statements now say every block-scalar form is read; two failure messages and one doc comment from test(xtask): read wrapped step names, and say which run form is checked (ROCMAI-679) #598 that still said only run: | was read are fixed too. run_block_header's doc now says it takes a run: key at any depth, so a run: nested under with: would be read as the step's script; no workflow here has one.
• No Gherkin scenario: this is an internal, test-only change to the xtask contract tests and changes nothing a CLI user can observe.