ci(test): lint compiled bash bodies with shellcheck#496
Merged
jamesadevine merged 2 commits intomainfrom May 10, 2026
Merged
Conversation
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>
Contributor
🔍 Rust PR ReviewSummary: Solid approach with real bugs fixed — one documentation inconsistency worth fixing before merge. Findings🐛 Bugs / Logic Issues
✅ What Looks Good
|
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>
Contributor
🔍 Rust PR ReviewSummary: Looks good overall — solid engineering with real bugs fixed and a well-designed enforcement mechanism. One documentation inaccuracy and a handful of minor observations. Findings🐛 Bugs / Logic Issues
|
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.
Replaces #492 (the heavy-handed
set -eo pipefailsweep) with a shellcheck-based integration test plus targeted bash fixes for the silent-failure patterns the lint actually surfaces. Closes the dissatisfaction expressed there: noise without enforcement that drifted on the next bash step.Why this is different from #492
set -eo pipefaillines spread across templates and Rust generatorscargo test --test bash_lint_testsblocks regressions; CI setsENFORCE_BASH_LINT=12>/dev/null || truecleanup; in two spots actually masked failures more than the original (grep ... | tail -1 || true)cdwithout||, masked-return assignments, tilde-in-quotes, unquoted variablesReal bugs surfaced and fixed
These were latent and would have stayed latent under the #492 approach:
Engine::Copilot::log_dir()returned"~/.copilot/logs". Tilde does not expand inside the double-quoted[ -d "..." ]test, so the directory check always failed and the2>/dev/null || truemasked it. Fixed to$HOME/.copilot/logs.Ensure .npmrc existsandEnsure nuget.config existsemit invalid YAML. Both generators used Rust\<newline>line continuations informat!, which strip leading whitespace. The emitted body was flush-left against- bash: |, breaking YAML. Fixed by switching to raw string literals. (The fixtureruntime-coverage-agent.mdwould not have parsed before this fix.)set -o pipefail.grep "ado-aw-linux-x64" checksums.txt | sha256sum -c -silently passes when grep returns no match becausesha256sum -c -returns 0 on empty input. The unverified binary would install successfully. Same pattern in the trigger-filters script download. AWF download was already protected (PR fix(compile): fail pipeline step on AWF download errors #439); ado-aw compiler download was missed.curl ... | shwithout pipefail — silent on curl failure.Plus minor hygiene:
cd "$DOWNLOAD_DIR" || exit 1,exit "$AGENT_EXIT_CODE", quoted{{ working_directory }}substitutions,${RESULT_LINE##*THREAT_DETECTION_RESULT:}instead ofecho | sed.What's NOT changed
set -eo pipefailinstances on AWF download / docker pull (PR fix(compile): fail pipeline step on AWF download errors #439) and on thetee-piped agent + threat-analysis runs are preserved. Those were correct.BashStepRust helper, no template injection mechanism — out of scope.set -eo pipefailsweep; targeted only where masked-pipeline exit codes are real.How the lint works
runtime-coverage-agent.mdthat exercises Lean, Node-with-feed-url, .NET-with-feed-url, and cache-memory).*.lock.yml, walks the YAML and collects everybash:body that is the value of a sequence-element mapping (so an arbitrarybashkey inside anenv:block is not treated as a step).shellcheck --shell=bash --format=json --exclude=SC1090,SC1091 -.REQUIRED_STEP_DISPLAY_NAMESwas produced — catches fixture/generator drift.Excludes are limited to
SC1090/SC1091(dynamic source paths) with a documented justification. Project-specific suppressions go inline as# shellcheck disable=SCxxxxcomments — used in exactly one place for the wait-N-timesfor i in $(seq …)loop pattern, whereiis intentionally unused.Skip vs. enforce
shellcheckis not on PATH. Devs without it installed are unblocked..github/workflows/rust-tests.ymlinstalls shellcheck and setsENFORCE_BASH_LINT=1so a missing shellcheck becomes a hard failure rather than a silent skip. This was a blocking concern from the design review — without it, a runner image change could have silently disabled the lint.Validation
cargo clippy --testsclean for the new test.ENFORCE_BASH_LINT=1runs the lint without skip.Documentation
AGENTS.mdTesting section: brief mention of the lint and the install command.docs/extending.md: new "Bash steps in pipeline templates" section explaining the lint, the exclude rationale, the inline-disable pattern for intentional cases, and explicit guidance not to sprinkleset -eo pipefailto silence the lint.