Skip to content

Commit 55e546f

Browse files
jamesadevineCopilot
jamesadevine
and
Copilot
authored
ci(test): lint compiled bash bodies with shellcheck (#496)
* 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>
1 parent c19c132 commit 55e546f

13 files changed

Lines changed: 589 additions & 57 deletions

File tree

‎.github/workflows/rust-tests.yml‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,8 +22,16 @@ jobs:
2222

2323
- uses: Swatinem/rust-cache@v2
2424

25+
- name: Install shellcheck
26+
# ubuntu-latest already ships shellcheck, but install explicitly to
27+
# guarantee it on every refresh of the runner image and to surface
28+
# a clear failure when the bash-lint integration test is enforced.
29+
run: sudo apt-get update && sudo apt-get install -y shellcheck
30+
2531
- name: Build
2632
run: cargo build --verbose
2733

2834
- name: Run tests
35+
env:
36+
ENFORCE_BASH_LINT: "1"
2937
run: cargo test --verbose

‎AGENTS.md‎

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -273,6 +273,25 @@ cargo test
273273
cargo clippy
274274
```
275275

276+
### Bash step lint
277+
278+
The `tests/bash_lint_tests.rs` integration test compiles a representative set
279+
of fixtures and runs `shellcheck` against every literal `bash:` body in the
280+
generated YAML. It catches silent-failure patterns that ADO's "fail on last
281+
command" default would let through (e.g. `cd "$X"` without `|| exit`, tilde
282+
inside double quotes, masked-return assignments).
283+
284+
The test is skipped if `shellcheck` is not on PATH. Install locally with
285+
`brew install shellcheck` (macOS) or `apt-get install -y shellcheck` (Debian
286+
/ Ubuntu); CI installs it in `.github/workflows/rust-tests.yml` and sets
287+
`ENFORCE_BASH_LINT=1` so a missing shellcheck becomes a hard failure rather
288+
than a silent skip.
289+
290+
When adding a new bash step, run `cargo test --test bash_lint_tests` and fix
291+
anything it flags. If a finding is genuinely intentional, add a
292+
`# shellcheck disable=SCxxxx` comment immediately above the offending line in
293+
the bash body — shellcheck honours the directive and it's inert at runtime.
294+
276295
## Common Tasks
277296

278297
### Compile a markdown pipeline

‎docs/extending.md‎

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -90,3 +90,45 @@ To add a new filter type:
9090
`validate_pr_filters()` or `validate_pipeline_filters()`
9191
5. **Write tests** — lowering test, validation test, and codegen test in
9292
`filter_ir.rs`
93+
94+
## Bash steps in pipeline templates
95+
96+
Pipeline templates and Rust step generators emit dozens of multi-line `bash:`
97+
steps. ADO bash steps fail only on the *last* command's exit status by
98+
default, so a chain like `mkdir … && curl … && cd … && cmd` can silently
99+
swallow earlier failures.
100+
101+
Rather than spread `set -eo pipefail` boilerplate across every step, the
102+
project enforces hygiene via `tests/bash_lint_tests.rs`, which compiles a set
103+
of fixtures and runs `shellcheck` against every literal `bash:` body in the
104+
generated YAML. The lint catches:
105+
106+
- **SC2164** — `cd $X` without `|| exit` (the canonical silent-failure)
107+
- **SC2155** — `local var=$(cmd)` masking the inner exit code
108+
- **SC2086 / SC2046** — unquoted variables / command substitutions
109+
- **SC2154** — variables referenced but never assigned
110+
- **SC2088** — tilde inside double quotes (does not expand at all)
111+
112+
When you add or modify a bash step:
113+
114+
1. Run `cargo test --test bash_lint_tests` (locally requires `shellcheck` on
115+
PATH; install with `brew install shellcheck` or
116+
`apt-get install -y shellcheck`). CI sets `ENFORCE_BASH_LINT=1` so a
117+
missing shellcheck becomes a hard failure rather than a silent skip.
118+
2. Fix any finding by adjusting the bash. Common fixes: `cd "$X" || exit 1`,
119+
`exit "$CODE"`, `"$HOME/.foo"` instead of `"~/.foo"`, quoting variable
120+
expansions.
121+
3. If a finding is genuinely intentional, add a
122+
`# shellcheck disable=SCxxxx` comment immediately above the line in the
123+
bash body. Such directives are bash comments and have no runtime effect.
124+
125+
Do **not** sprinkle `set -eo pipefail` into every step to silence the lint —
126+
that approach was tried (PR #492) and was rejected because it adds noise,
127+
drifts as new steps are added, and doesn't address the actual silent-failure
128+
patterns that the lint surfaces. Use targeted `set -eo pipefail` only when a
129+
step has a real fail-fast requirement that the lint cannot express (the
130+
current uses are on AWF/MCPG download and the `tee`-piped agent run).
131+
132+
The exclude list (`SC1090`, `SC1091`, `SC2034`, `SC2016`) is documented in
133+
`tests/bash_lint_tests.rs`. Each entry has a justification — do not extend
134+
without one.

‎src/compile/extensions/trigger_filters.rs‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,6 +101,7 @@ impl CompilerExtension for TriggerFiltersExtension {
101101
let mut steps = Vec::new();
102102
steps.push(format!(
103103
r#"- bash: |
104+
set -eo pipefail
104105
mkdir -p /tmp/ado-aw-scripts
105106
curl -fsSL "{RELEASE_BASE_URL}/v{version}/checksums.txt" -o /tmp/ado-aw-scripts/checksums.txt
106107
curl -fsSL "{RELEASE_BASE_URL}/v{version}/scripts.zip" -o /tmp/ado-aw-scripts/scripts.zip

‎src/data/1es-base.yml‎

Lines changed: 22 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ extends:
5959
{{ engine_install_steps }}
6060

6161
- bash: |
62+
set -eo pipefail
6263
COMPILER_VERSION="{{ compiler_version }}"
6364
DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler"
6465
DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64"
@@ -70,7 +71,7 @@ extends:
7071
curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL"
7172
7273
echo "Verifying checksum..."
73-
cd "$DOWNLOAD_DIR"
74+
cd "$DOWNLOAD_DIR" || exit 1
7475
grep "ado-aw-linux-x64" checksums.txt | sha256sum -c -
7576
mv ado-aw-linux-x64 ado-aw
7677
chmod +x ado-aw
@@ -156,7 +157,7 @@ extends:
156157
curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL"
157158
158159
echo "Verifying checksum..."
159-
cd "$DOWNLOAD_DIR"
160+
cd "$DOWNLOAD_DIR" || exit 1
160161
grep "awf-linux-x64" checksums.txt | sha256sum -c -
161162
mv awf-linux-x64 awf
162163
chmod +x awf
@@ -204,6 +205,7 @@ extends:
204205
205206
# Wait for server to be ready
206207
READY=false
208+
# shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop
207209
for i in $(seq 1 30); do
208210
if curl -sf "http://localhost:$SAFE_OUTPUTS_PORT/health" > /dev/null 2>&1; then
209211
echo "SafeOutputs HTTP server is ready"
@@ -221,14 +223,15 @@ extends:
221223
# Start MCP Gateway (MCPG) on host
222224
- bash: |
223225
# Substitute runtime values into MCPG config
224-
MCPG_CONFIG=$(cat /tmp/awf-tools/staging/mcpg-config.json \
225-
| sed "s|\${SAFE_OUTPUTS_PORT}|$(SAFE_OUTPUTS_PORT)|g" \
226-
| sed "s|\${SAFE_OUTPUTS_API_KEY}|$(SAFE_OUTPUTS_API_KEY)|g" \
227-
| sed "s|\${MCP_GATEWAY_API_KEY}|$(MCP_GATEWAY_API_KEY)|g")
226+
MCPG_CONFIG=$(sed \
227+
-e "s|\${SAFE_OUTPUTS_PORT}|$(SAFE_OUTPUTS_PORT)|g" \
228+
-e "s|\${SAFE_OUTPUTS_API_KEY}|$(SAFE_OUTPUTS_API_KEY)|g" \
229+
-e "s|\${MCP_GATEWAY_API_KEY}|$(MCP_GATEWAY_API_KEY)|g" \
230+
/tmp/awf-tools/staging/mcpg-config.json)
228231
229232
# Log the template config (before API key substitution) for debugging.
230233
echo "Starting MCPG with config template:"
231-
cat /tmp/awf-tools/staging/mcpg-config.json | python3 -m json.tool
234+
python3 -m json.tool < /tmp/awf-tools/staging/mcpg-config.json
232235
233236
# Remove any leftover container or stale output from a previous interrupted run
234237
# (--rm only cleans up on clean exit; OOM/SIGKILL may leave it behind)
@@ -267,6 +270,7 @@ extends:
267270
268271
# Wait for MCPG to be ready
269272
READY=false
273+
# shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop
270274
for i in $(seq 1 30); do
271275
if curl -sf "http://localhost:{{ mcpg_port }}/health" > /dev/null 2>&1; then
272276
echo "MCPG is ready"
@@ -284,6 +288,7 @@ extends:
284288
# Health check passing doesn't guarantee stdout is flushed, so poll.
285289
echo "Waiting for gateway output file..."
286290
GATEWAY_READY=false
291+
# shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop
287292
for i in $(seq 1 15); do
288293
if [ -s "$GATEWAY_OUTPUT" ] && jq -e '.mcpServers' "$GATEWAY_OUTPUT" > /dev/null 2>&1; then
289294
echo "Gateway output is ready"
@@ -361,7 +366,7 @@ extends:
361366
"$(Pipeline.Workspace)/awf/awf" logs summary --source "$(Agent.TempDirectory)/staging/logs/firewall" 2>/dev/null || true
362367
fi
363368
364-
exit $AGENT_EXIT_CODE
369+
exit "$AGENT_EXIT_CODE"
365370
displayName: "Run copilot (AWF network isolated)"
366371
workingDirectory: {{ working_directory }}
367372
env:
@@ -433,6 +438,7 @@ extends:
433438
{{ engine_install_steps }}
434439

435440
- bash: |
441+
set -eo pipefail
436442
COMPILER_VERSION="{{ compiler_version }}"
437443
DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler"
438444
DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64"
@@ -444,7 +450,7 @@ extends:
444450
curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL"
445451
446452
echo "Verifying checksum..."
447-
cd "$DOWNLOAD_DIR"
453+
cd "$DOWNLOAD_DIR" || exit 1
448454
grep "ado-aw-linux-x64" checksums.txt | sha256sum -c -
449455
mv ado-aw-linux-x64 ado-aw
450456
chmod +x ado-aw
@@ -469,7 +475,7 @@ extends:
469475
curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL"
470476
471477
echo "Verifying checksum..."
472-
cd "$DOWNLOAD_DIR"
478+
cd "$DOWNLOAD_DIR" || exit 1
473479
grep "awf-linux-x64" checksums.txt | sha256sum -c -
474480
mv awf-linux-x64 awf
475481
chmod +x awf
@@ -487,8 +493,8 @@ extends:
487493
displayName: "Pre-pull AWF container images (v{{ firewall_version }})"
488494
489495
- bash: |
490-
mkdir -p {{ working_directory }}/safe_outputs
491-
cp -a "$(Pipeline.Workspace)/agent_outputs_$(Build.BuildId)/." {{ working_directory }}/safe_outputs
496+
mkdir -p "{{ working_directory }}/safe_outputs"
497+
cp -a "$(Pipeline.Workspace)/agent_outputs_$(Build.BuildId)/." "{{ working_directory }}/safe_outputs"
492498
displayName: "Prepare safe outputs for analysis"
493499
494500
- bash: |
@@ -526,7 +532,7 @@ extends:
526532
| tee "$THREAT_OUTPUT_FILE" \
527533
&& AGENT_EXIT_CODE=0 || AGENT_EXIT_CODE=$?
528534
529-
exit $AGENT_EXIT_CODE
535+
exit "$AGENT_EXIT_CODE"
530536
displayName: "Run threat analysis (AWF network isolated)"
531537
workingDirectory: {{ working_directory }}
532538
env:
@@ -550,7 +556,7 @@ extends:
550556
RESULT_LINE=$(grep "THREAT_DETECTION_RESULT:" "$(Agent.TempDirectory)/threat-analysis-output.txt" | tail -1)
551557
if [ -n "$RESULT_LINE" ]; then
552558
# Extract JSON after the prefix
553-
JSON_CONTENT=$(echo "$RESULT_LINE" | sed 's/.*THREAT_DETECTION_RESULT://')
559+
JSON_CONTENT="${RESULT_LINE##*THREAT_DETECTION_RESULT:}"
554560
echo "$JSON_CONTENT" > "$(Agent.TempDirectory)/analyzed_outputs/threat-analysis.json"
555561
echo "Extracted threat analysis JSON:"
556562
cat "$(Agent.TempDirectory)/analyzed_outputs/threat-analysis.json"
@@ -635,6 +641,7 @@ extends:
635641
artifact: analyzed_outputs_$(Build.BuildId)
636642

637643
- bash: |
644+
set -eo pipefail
638645
COMPILER_VERSION="{{ compiler_version }}"
639646
DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler"
640647
DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64"
@@ -646,7 +653,7 @@ extends:
646653
curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL"
647654
648655
echo "Verifying checksum..."
649-
cd "$DOWNLOAD_DIR"
656+
cd "$DOWNLOAD_DIR" || exit 1
650657
grep "ado-aw-linux-x64" checksums.txt | sha256sum -c -
651658
mv ado-aw-linux-x64 ado-aw
652659
chmod +x ado-aw

0 commit comments

Comments
 (0)