fix(compile): fail pipeline step on AWF download errors#439
Merged
jamesadevine merged 1 commit intomainfrom May 7, 2026
Merged
fix(compile): fail pipeline step on AWF download errors#439jamesadevine merged 1 commit intomainfrom
jamesadevine merged 1 commit intomainfrom
Conversation
Add set -eo pipefail to all AWF download and docker pull steps in both base.yml and 1es-base.yml templates. Remove the || echo fallback on ./awf --version that was silently swallowing download failures, causing the step to report green even when the binary was not present. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
🔍 Rust PR ReviewSummary: Looks good — a focused, correct fix for the silent failure bug. Findings✅ What Looks Good
|
jamesadevine
added a commit
that referenced
this pull request
May 10, 2026
* ci(test): lint compiled bash bodies with shellcheck Adds tests/bash_lint_tests.rs, an integration test that compiles a representative set of fixtures and runs shellcheck against every literal bash: body in the generated YAML. The lint catches the actual silent-failure patterns ADO's "fail on last command" default lets through (SC2164 cd-without-||, SC2155 masked-return, SC2086/2046 unquoted variables, SC2154 unset refs, SC2088 tilde-in-quotes). This replaces the previously proposed approach of sprinkling `set -eo pipefail` across every bash step (PR #492). That approach added boilerplate to ~27 sites without enforcement, drifted as new steps were added, and in two spots actually masked errors more than the original code (`grep ... | tail -1 || true`). Real bugs surfaced and fixed by the new lint: * `src/engine.rs` — `Engine::Copilot::log_dir()` returned `~/.copilot/logs`. Tilde does not expand inside the double-quoted `[ -d "..." ]` test that consumes this value, so the directory check always failed and Copilot logs were silently never collected to the pipeline artifact. Replaced with `$HOME/.copilot/logs`. * `src/runtimes/node/mod.rs` and `src/runtimes/dotnet/mod.rs` — the ensure-`.npmrc` and ensure-`nuget.config` step generators used Rust `\<newline>` line continuations in their format strings, which strip leading whitespace. The emitted YAML had body lines flush-left against `- bash: |`, producing invalid YAML. Replaced with raw string literals so indentation is preserved. * Multiple `cd "$DOWNLOAD_DIR"` in `base.yml` / `1es-base.yml` had no `|| exit` guard. Added. * `exit $AGENT_EXIT_CODE` (multiple sites) — quoted. * `mkdir -p {{ working_directory }}/safe_outputs` and the matching `cp -a ...` — quoted the substitution. * `JSON_CONTENT=$(echo "$RESULT_LINE" | sed 's/.*PFX://')` rewritten to `${RESULT_LINE##*PFX:}` (avoids forking sed and removes a shellcheck SC2001 finding). Targeted `set -eo pipefail` additions (only where masked-pipeline exit codes matter): * `base.yml` / `1es-base.yml` ado-aw download steps (3 stages × 2 templates): `grep "ado-aw-linux-x64" checksums.txt | sha256sum -c -` silently passes when grep matches nothing because sha256sum returns 0 on empty stdin. Without pipefail, the unverified binary would install successfully. * `src/compile/extensions/trigger_filters.rs` script-download step: same `grep | sha256sum` pattern. * `src/runtimes/lean/mod.rs` install step: `curl ... | sh` would silently install nothing on curl failure. The two pre-existing `set -eo pipefail` instances on the AWF download + docker pull steps (introduced in PR #439) and on the `tee`-piped agent / threat-analysis runs are preserved — those were correct. Skip vs. enforce: * Locally, the test prints a notice and returns early when shellcheck is missing. * CI installs shellcheck and sets `ENFORCE_BASH_LINT=1` so a missing shellcheck becomes a hard failure rather than a silent skip. A new `tests/fixtures/runtime-coverage-agent.md` exercises the Lean, Node-with-feed-url, and .NET-with-feed-url runtimes plus the cache-memory tool, ensuring every code-generated bash step is reached. The lint enforces a `REQUIRED_STEP_DISPLAY_NAMES` coverage list to catch fixture/generator drift. Documented in AGENTS.md and docs/extending.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix(bash-lint): close 1ES coverage gap and fix shellcheck 0.9 SC2002 Two changes the CI surfaced after PR #496 landed locally: 1. **shellcheck 0.9.0 (Ubuntu's pinned) flags SC2002 ("Useless cat") on `cat file | sed ...` patterns that 0.11.0 does not.** Fixed by rewriting the two offending sites in the MCPG start step: * `MCPG_CONFIG=$(cat … | sed | sed | sed)` → `MCPG_CONFIG=$(sed -e … -e … -e … file)`. Semantically equivalent because the three substitutions are over independent placeholder patterns. * `cat … | python3 -m json.tool` → `python3 -m json.tool < …`. Avoids forking `cat` for nothing and is stable across shellcheck versions. 2. **Add a `runtime-coverage-1es-agent.md` fixture and assert that every known compile target is exercised by at least one fixture.** Previously only `1es-test-agent.md` compiled to the 1ES target, and it had no `runtimes:` or `tools.cache-memory`. The code-generated bash bodies from those extensions (Lean install, `.npmrc`, `nuget.config`, cache-memory restore/init) were being linted only on the standalone target. Today their bodies are byte-identical across targets, but a future target-specific divergence would slip past the lint without a 1ES variant. `compile_fixture()` now parses `Generated <target> pipeline:` from stdout, accumulates targets seen, and the test asserts every entry in `REQUIRED_TARGETS = ["standalone", "1es"]` is covered. Sanity- checked that removing the 1ES fixtures causes the test to fail with `no fixture compiles to the following target(s): ["1es"]`. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jamesadevine
added a commit
that referenced
this pull request
May 10, 2026
…498) * ci(test): lint compiled bash bodies with shellcheck Adds tests/bash_lint_tests.rs, an integration test that compiles a representative set of fixtures and runs shellcheck against every literal bash: body in the generated YAML. The lint catches the actual silent-failure patterns ADO's "fail on last command" default lets through (SC2164 cd-without-||, SC2155 masked-return, SC2086/2046 unquoted variables, SC2154 unset refs, SC2088 tilde-in-quotes). This replaces the previously proposed approach of sprinkling `set -eo pipefail` across every bash step (PR #492). That approach added boilerplate to ~27 sites without enforcement, drifted as new steps were added, and in two spots actually masked errors more than the original code (`grep ... | tail -1 || true`). Real bugs surfaced and fixed by the new lint: * `src/engine.rs` — `Engine::Copilot::log_dir()` returned `~/.copilot/logs`. Tilde does not expand inside the double-quoted `[ -d "..." ]` test that consumes this value, so the directory check always failed and Copilot logs were silently never collected to the pipeline artifact. Replaced with `$HOME/.copilot/logs`. * `src/runtimes/node/mod.rs` and `src/runtimes/dotnet/mod.rs` — the ensure-`.npmrc` and ensure-`nuget.config` step generators used Rust `\<newline>` line continuations in their format strings, which strip leading whitespace. The emitted YAML had body lines flush-left against `- bash: |`, producing invalid YAML. Replaced with raw string literals so indentation is preserved. * Multiple `cd "$DOWNLOAD_DIR"` in `base.yml` / `1es-base.yml` had no `|| exit` guard. Added. * `exit $AGENT_EXIT_CODE` (multiple sites) — quoted. * `mkdir -p {{ working_directory }}/safe_outputs` and the matching `cp -a ...` — quoted the substitution. * `JSON_CONTENT=$(echo "$RESULT_LINE" | sed 's/.*PFX://')` rewritten to `${RESULT_LINE##*PFX:}` (avoids forking sed and removes a shellcheck SC2001 finding). Targeted `set -eo pipefail` additions (only where masked-pipeline exit codes matter): * `base.yml` / `1es-base.yml` ado-aw download steps (3 stages × 2 templates): `grep "ado-aw-linux-x64" checksums.txt | sha256sum -c -` silently passes when grep matches nothing because sha256sum returns 0 on empty stdin. Without pipefail, the unverified binary would install successfully. * `src/compile/extensions/trigger_filters.rs` script-download step: same `grep | sha256sum` pattern. * `src/runtimes/lean/mod.rs` install step: `curl ... | sh` would silently install nothing on curl failure. The two pre-existing `set -eo pipefail` instances on the AWF download + docker pull steps (introduced in PR #439) and on the `tee`-piped agent / threat-analysis runs are preserved — those were correct. Skip vs. enforce: * Locally, the test prints a notice and returns early when shellcheck is missing. * CI installs shellcheck and sets `ENFORCE_BASH_LINT=1` so a missing shellcheck becomes a hard failure rather than a silent skip. A new `tests/fixtures/runtime-coverage-agent.md` exercises the Lean, Node-with-feed-url, and .NET-with-feed-url runtimes plus the cache-memory tool, ensuring every code-generated bash step is reached. The lint enforces a `REQUIRED_STEP_DISPLAY_NAMES` coverage list to catch fixture/generator drift. Documented in AGENTS.md and docs/extending.md. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * ci(workflows): add daily bash-step hygiene auditor agentic workflow Adds `.github/workflows/bash-lint-auditor.md`, a daily agentic workflow that complements the PR-gate lint added in PR #496. The PR gate gives fast feedback on every PR; this workflow runs once a day and lands small, mechanical improvements that the gate can't: * When a finding does slip onto main (e.g. via merge conflict), the auditor fixes it the next morning instead of waiting for the next contributor PR. * Audits stale `# shellcheck disable=` directives — removes ones that no longer fire (i.e. the underlying code has been cleaned up but the suppression was forgotten). * Audits whether the lint's exclude list could be tightened. * Verifies fixture coverage of every bash-step generator and proposes fixture additions when a new generator appears. When the auditor finds something actionable, it opens a focused PR (one concern per PR) with the structured "what was found / how it was fixed / verification" body. When the lint is green and no proactive improvement is feasible, it exits cleanly with `noop`. Configuration notes: * `schedule: daily around 09:00` — fuzzy schedule scattering across the hour, matching the convention of other daily workflows in this repo (e.g. `cyclomatic-complexity-reducer.md`). * `allowed-files` restricts the auditor to bash-generator code paths plus the tests/fixtures it depends on. `protected-files: fallback-to-issue` ensures that if it tries to edit anything else, the change falls back to an issue rather than a PR. * `cache-memory: true` persists state across runs so the auditor doesn't loop on the same suggestion if a maintainer rejects it. * `bash: ["*"]` + `network.allowed: [defaults, rust]` gives the agent what it needs to install shellcheck (via apt with a static- binary fallback) and run cargo against the rust ecosystem. Compiled with `gh aw compile bash-lint-auditor`; the matching `.lock.yml` is included along with new SHAs in `.github/aw/actions-lock.json` (cache, checkout, download-artifact registered for the first time by this workflow's setup steps). Stacked on top of branch `lint-bash-steps` (PR #496) because the auditor relies on `tests/bash_lint_tests.rs` and `tests/fixtures/runtime-coverage-agent.md`, which are introduced there. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
AWF download steps in generated pipelines report green even when the download fails (e.g. HTTP 404). The
|| echo "AWF binary ready"fallback on./awf --versionswallows the failure exit code, causing all downstream steps to fail with a confusing "No such file or directory" error instead of surfacing the real problem at the download step.Fix
set -eo pipefailto all AWF download and docker pull bash steps in bothbase.ymland1es-base.ymltemplates (4 blocks total: agent stage + detection stage × 2 templates).|| echo "AWF binary ready"so./awf --versionfailures propagate correctly.With this change, any download failure (curl 404, checksum mismatch, missing binary) immediately fails the step with a clear error.