diff --git a/.github/workflows/rust-tests.yml b/.github/workflows/rust-tests.yml index ff13a1da..37a0c22d 100644 --- a/.github/workflows/rust-tests.yml +++ b/.github/workflows/rust-tests.yml @@ -22,8 +22,16 @@ jobs: - uses: Swatinem/rust-cache@v2 + - name: Install shellcheck + # ubuntu-latest already ships shellcheck, but install explicitly to + # guarantee it on every refresh of the runner image and to surface + # a clear failure when the bash-lint integration test is enforced. + run: sudo apt-get update && sudo apt-get install -y shellcheck + - name: Build run: cargo build --verbose - name: Run tests + env: + ENFORCE_BASH_LINT: "1" run: cargo test --verbose diff --git a/AGENTS.md b/AGENTS.md index 10f3beaa..4deb9bb1 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -273,6 +273,25 @@ cargo test cargo clippy ``` +### Bash step lint + +The `tests/bash_lint_tests.rs` integration test compiles a representative set +of fixtures and runs `shellcheck` against every literal `bash:` body in the +generated YAML. It catches silent-failure patterns that ADO's "fail on last +command" default would let through (e.g. `cd "$X"` without `|| exit`, tilde +inside double quotes, masked-return assignments). + +The test is skipped if `shellcheck` is not on PATH. Install locally with +`brew install shellcheck` (macOS) or `apt-get install -y shellcheck` (Debian +/ Ubuntu); CI installs it in `.github/workflows/rust-tests.yml` and sets +`ENFORCE_BASH_LINT=1` so a missing shellcheck becomes a hard failure rather +than a silent skip. + +When adding a new bash step, run `cargo test --test bash_lint_tests` and fix +anything it flags. If a finding is genuinely intentional, add a +`# shellcheck disable=SCxxxx` comment immediately above the offending line in +the bash body — shellcheck honours the directive and it's inert at runtime. + ## Common Tasks ### Compile a markdown pipeline diff --git a/docs/extending.md b/docs/extending.md index 636b7a7a..2f680762 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -90,3 +90,45 @@ To add a new filter type: `validate_pr_filters()` or `validate_pipeline_filters()` 5. **Write tests** — lowering test, validation test, and codegen test in `filter_ir.rs` + +## Bash steps in pipeline templates + +Pipeline templates and Rust step generators emit dozens of multi-line `bash:` +steps. ADO bash steps fail only on the *last* command's exit status by +default, so a chain like `mkdir … && curl … && cd … && cmd` can silently +swallow earlier failures. + +Rather than spread `set -eo pipefail` boilerplate across every step, the +project enforces hygiene via `tests/bash_lint_tests.rs`, which compiles a set +of fixtures and runs `shellcheck` against every literal `bash:` body in the +generated YAML. The lint catches: + +- **SC2164** — `cd $X` without `|| exit` (the canonical silent-failure) +- **SC2155** — `local var=$(cmd)` masking the inner exit code +- **SC2086 / SC2046** — unquoted variables / command substitutions +- **SC2154** — variables referenced but never assigned +- **SC2088** — tilde inside double quotes (does not expand at all) + +When you add or modify a bash step: + +1. Run `cargo test --test bash_lint_tests` (locally requires `shellcheck` on + PATH; install with `brew install shellcheck` or + `apt-get install -y shellcheck`). CI sets `ENFORCE_BASH_LINT=1` so a + missing shellcheck becomes a hard failure rather than a silent skip. +2. Fix any finding by adjusting the bash. Common fixes: `cd "$X" || exit 1`, + `exit "$CODE"`, `"$HOME/.foo"` instead of `"~/.foo"`, quoting variable + expansions. +3. If a finding is genuinely intentional, add a + `# shellcheck disable=SCxxxx` comment immediately above the line in the + bash body. Such directives are bash comments and have no runtime effect. + +Do **not** sprinkle `set -eo pipefail` into every step to silence the lint — +that approach was tried (PR #492) and was rejected because it adds noise, +drifts as new steps are added, and doesn't address the actual silent-failure +patterns that the lint surfaces. Use targeted `set -eo pipefail` only when a +step has a real fail-fast requirement that the lint cannot express (the +current uses are on AWF/MCPG download and the `tee`-piped agent run). + +The exclude list (`SC1090`, `SC1091`, `SC2034`, `SC2016`) is documented in +`tests/bash_lint_tests.rs`. Each entry has a justification — do not extend +without one. diff --git a/src/compile/extensions/trigger_filters.rs b/src/compile/extensions/trigger_filters.rs index a0d84a6d..1b0be9cc 100644 --- a/src/compile/extensions/trigger_filters.rs +++ b/src/compile/extensions/trigger_filters.rs @@ -101,6 +101,7 @@ impl CompilerExtension for TriggerFiltersExtension { let mut steps = Vec::new(); steps.push(format!( r#"- bash: | + set -eo pipefail mkdir -p /tmp/ado-aw-scripts curl -fsSL "{RELEASE_BASE_URL}/v{version}/checksums.txt" -o /tmp/ado-aw-scripts/checksums.txt curl -fsSL "{RELEASE_BASE_URL}/v{version}/scripts.zip" -o /tmp/ado-aw-scripts/scripts.zip diff --git a/src/data/1es-base.yml b/src/data/1es-base.yml index 920da5eb..d482a8fe 100644 --- a/src/data/1es-base.yml +++ b/src/data/1es-base.yml @@ -59,6 +59,7 @@ extends: {{ engine_install_steps }} - bash: | + set -eo pipefail COMPILER_VERSION="{{ compiler_version }}" DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler" DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64" @@ -70,7 +71,7 @@ extends: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "ado-aw-linux-x64" checksums.txt | sha256sum -c - mv ado-aw-linux-x64 ado-aw chmod +x ado-aw @@ -156,7 +157,7 @@ extends: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "awf-linux-x64" checksums.txt | sha256sum -c - mv awf-linux-x64 awf chmod +x awf @@ -204,6 +205,7 @@ extends: # Wait for server to be ready READY=false + # shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop for i in $(seq 1 30); do if curl -sf "http://localhost:$SAFE_OUTPUTS_PORT/health" > /dev/null 2>&1; then echo "SafeOutputs HTTP server is ready" @@ -221,14 +223,15 @@ extends: # Start MCP Gateway (MCPG) on host - bash: | # Substitute runtime values into MCPG config - MCPG_CONFIG=$(cat /tmp/awf-tools/staging/mcpg-config.json \ - | sed "s|\${SAFE_OUTPUTS_PORT}|$(SAFE_OUTPUTS_PORT)|g" \ - | sed "s|\${SAFE_OUTPUTS_API_KEY}|$(SAFE_OUTPUTS_API_KEY)|g" \ - | sed "s|\${MCP_GATEWAY_API_KEY}|$(MCP_GATEWAY_API_KEY)|g") + MCPG_CONFIG=$(sed \ + -e "s|\${SAFE_OUTPUTS_PORT}|$(SAFE_OUTPUTS_PORT)|g" \ + -e "s|\${SAFE_OUTPUTS_API_KEY}|$(SAFE_OUTPUTS_API_KEY)|g" \ + -e "s|\${MCP_GATEWAY_API_KEY}|$(MCP_GATEWAY_API_KEY)|g" \ + /tmp/awf-tools/staging/mcpg-config.json) # Log the template config (before API key substitution) for debugging. echo "Starting MCPG with config template:" - cat /tmp/awf-tools/staging/mcpg-config.json | python3 -m json.tool + python3 -m json.tool < /tmp/awf-tools/staging/mcpg-config.json # Remove any leftover container or stale output from a previous interrupted run # (--rm only cleans up on clean exit; OOM/SIGKILL may leave it behind) @@ -267,6 +270,7 @@ extends: # Wait for MCPG to be ready READY=false + # shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop for i in $(seq 1 30); do if curl -sf "http://localhost:{{ mcpg_port }}/health" > /dev/null 2>&1; then echo "MCPG is ready" @@ -284,6 +288,7 @@ extends: # Health check passing doesn't guarantee stdout is flushed, so poll. echo "Waiting for gateway output file..." GATEWAY_READY=false + # shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop for i in $(seq 1 15); do if [ -s "$GATEWAY_OUTPUT" ] && jq -e '.mcpServers' "$GATEWAY_OUTPUT" > /dev/null 2>&1; then echo "Gateway output is ready" @@ -361,7 +366,7 @@ extends: "$(Pipeline.Workspace)/awf/awf" logs summary --source "$(Agent.TempDirectory)/staging/logs/firewall" 2>/dev/null || true fi - exit $AGENT_EXIT_CODE + exit "$AGENT_EXIT_CODE" displayName: "Run copilot (AWF network isolated)" workingDirectory: {{ working_directory }} env: @@ -433,6 +438,7 @@ extends: {{ engine_install_steps }} - bash: | + set -eo pipefail COMPILER_VERSION="{{ compiler_version }}" DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler" DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64" @@ -444,7 +450,7 @@ extends: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "ado-aw-linux-x64" checksums.txt | sha256sum -c - mv ado-aw-linux-x64 ado-aw chmod +x ado-aw @@ -469,7 +475,7 @@ extends: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "awf-linux-x64" checksums.txt | sha256sum -c - mv awf-linux-x64 awf chmod +x awf @@ -487,8 +493,8 @@ extends: displayName: "Pre-pull AWF container images (v{{ firewall_version }})" - bash: | - mkdir -p {{ working_directory }}/safe_outputs - cp -a "$(Pipeline.Workspace)/agent_outputs_$(Build.BuildId)/." {{ working_directory }}/safe_outputs + mkdir -p "{{ working_directory }}/safe_outputs" + cp -a "$(Pipeline.Workspace)/agent_outputs_$(Build.BuildId)/." "{{ working_directory }}/safe_outputs" displayName: "Prepare safe outputs for analysis" - bash: | @@ -526,7 +532,7 @@ extends: | tee "$THREAT_OUTPUT_FILE" \ && AGENT_EXIT_CODE=0 || AGENT_EXIT_CODE=$? - exit $AGENT_EXIT_CODE + exit "$AGENT_EXIT_CODE" displayName: "Run threat analysis (AWF network isolated)" workingDirectory: {{ working_directory }} env: @@ -550,7 +556,7 @@ extends: RESULT_LINE=$(grep "THREAT_DETECTION_RESULT:" "$(Agent.TempDirectory)/threat-analysis-output.txt" | tail -1) if [ -n "$RESULT_LINE" ]; then # Extract JSON after the prefix - JSON_CONTENT=$(echo "$RESULT_LINE" | sed 's/.*THREAT_DETECTION_RESULT://') + JSON_CONTENT="${RESULT_LINE##*THREAT_DETECTION_RESULT:}" echo "$JSON_CONTENT" > "$(Agent.TempDirectory)/analyzed_outputs/threat-analysis.json" echo "Extracted threat analysis JSON:" cat "$(Agent.TempDirectory)/analyzed_outputs/threat-analysis.json" @@ -635,6 +641,7 @@ extends: artifact: analyzed_outputs_$(Build.BuildId) - bash: | + set -eo pipefail COMPILER_VERSION="{{ compiler_version }}" DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler" DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64" @@ -646,7 +653,7 @@ extends: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "ado-aw-linux-x64" checksums.txt | sha256sum -c - mv ado-aw-linux-x64 ado-aw chmod +x ado-aw diff --git a/src/data/base.yml b/src/data/base.yml index d5ca1500..bbaa6863 100644 --- a/src/data/base.yml +++ b/src/data/base.yml @@ -30,6 +30,7 @@ jobs: {{ engine_install_steps }} - bash: | + set -eo pipefail COMPILER_VERSION="{{ compiler_version }}" DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler" DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64" @@ -41,7 +42,7 @@ jobs: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "ado-aw-linux-x64" checksums.txt | sha256sum -c - mv ado-aw-linux-x64 ado-aw chmod +x ado-aw @@ -127,7 +128,7 @@ jobs: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "awf-linux-x64" checksums.txt | sha256sum -c - mv awf-linux-x64 awf chmod +x awf @@ -175,6 +176,7 @@ jobs: # Wait for server to be ready READY=false + # shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop for i in $(seq 1 30); do if curl -sf "http://localhost:$SAFE_OUTPUTS_PORT/health" > /dev/null 2>&1; then echo "SafeOutputs HTTP server is ready" @@ -192,14 +194,15 @@ jobs: # Start MCP Gateway (MCPG) on host - bash: | # Substitute runtime values into MCPG config - MCPG_CONFIG=$(cat /tmp/awf-tools/staging/mcpg-config.json \ - | sed "s|\${SAFE_OUTPUTS_PORT}|$(SAFE_OUTPUTS_PORT)|g" \ - | sed "s|\${SAFE_OUTPUTS_API_KEY}|$(SAFE_OUTPUTS_API_KEY)|g" \ - | sed "s|\${MCP_GATEWAY_API_KEY}|$(MCP_GATEWAY_API_KEY)|g") + MCPG_CONFIG=$(sed \ + -e "s|\${SAFE_OUTPUTS_PORT}|$(SAFE_OUTPUTS_PORT)|g" \ + -e "s|\${SAFE_OUTPUTS_API_KEY}|$(SAFE_OUTPUTS_API_KEY)|g" \ + -e "s|\${MCP_GATEWAY_API_KEY}|$(MCP_GATEWAY_API_KEY)|g" \ + /tmp/awf-tools/staging/mcpg-config.json) # Log the template config (before API key substitution) for debugging. echo "Starting MCPG with config template:" - cat /tmp/awf-tools/staging/mcpg-config.json | python3 -m json.tool + python3 -m json.tool < /tmp/awf-tools/staging/mcpg-config.json # Remove any leftover container or stale output from a previous interrupted run # (--rm only cleans up on clean exit; OOM/SIGKILL may leave it behind) @@ -238,6 +241,7 @@ jobs: # Wait for MCPG to be ready READY=false + # shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop for i in $(seq 1 30); do if curl -sf "http://localhost:{{ mcpg_port }}/health" > /dev/null 2>&1; then echo "MCPG is ready" @@ -255,6 +259,7 @@ jobs: # Health check passing doesn't guarantee stdout is flushed, so poll. echo "Waiting for gateway output file..." GATEWAY_READY=false + # shellcheck disable=SC2034 # i is intentionally unused; wait-N-times loop for i in $(seq 1 15); do if [ -s "$GATEWAY_OUTPUT" ] && jq -e '.mcpServers' "$GATEWAY_OUTPUT" > /dev/null 2>&1; then echo "Gateway output is ready" @@ -332,7 +337,7 @@ jobs: "$(Pipeline.Workspace)/awf/awf" logs summary --source "$(Agent.TempDirectory)/staging/logs/firewall" 2>/dev/null || true fi - exit $AGENT_EXIT_CODE + exit "$AGENT_EXIT_CODE" displayName: "Run copilot (AWF network isolated)" workingDirectory: {{ working_directory }} env: @@ -402,6 +407,7 @@ jobs: {{ engine_install_steps }} - bash: | + set -eo pipefail COMPILER_VERSION="{{ compiler_version }}" DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler" DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64" @@ -413,7 +419,7 @@ jobs: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "ado-aw-linux-x64" checksums.txt | sha256sum -c - mv ado-aw-linux-x64 ado-aw chmod +x ado-aw @@ -438,7 +444,7 @@ jobs: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "awf-linux-x64" checksums.txt | sha256sum -c - mv awf-linux-x64 awf chmod +x awf @@ -456,8 +462,8 @@ jobs: displayName: "Pre-pull AWF container images (v{{ firewall_version }})" - bash: | - mkdir -p {{ working_directory }}/safe_outputs - cp -a "$(Pipeline.Workspace)/agent_outputs_$(Build.BuildId)/." {{ working_directory }}/safe_outputs + mkdir -p "{{ working_directory }}/safe_outputs" + cp -a "$(Pipeline.Workspace)/agent_outputs_$(Build.BuildId)/." "{{ working_directory }}/safe_outputs" displayName: "Prepare safe outputs for analysis" - bash: | @@ -495,7 +501,7 @@ jobs: | tee "$THREAT_OUTPUT_FILE" \ && AGENT_EXIT_CODE=0 || AGENT_EXIT_CODE=$? - exit $AGENT_EXIT_CODE + exit "$AGENT_EXIT_CODE" displayName: "Run threat analysis (AWF network isolated)" workingDirectory: {{ working_directory }} env: @@ -519,7 +525,7 @@ jobs: RESULT_LINE=$(grep "THREAT_DETECTION_RESULT:" "$(Agent.TempDirectory)/threat-analysis-output.txt" | tail -1) if [ -n "$RESULT_LINE" ]; then # Extract JSON after the prefix - JSON_CONTENT=$(echo "$RESULT_LINE" | sed 's/.*THREAT_DETECTION_RESULT://') + JSON_CONTENT="${RESULT_LINE##*THREAT_DETECTION_RESULT:}" echo "$JSON_CONTENT" > "$(Agent.TempDirectory)/analyzed_outputs/threat-analysis.json" echo "Extracted threat analysis JSON:" cat "$(Agent.TempDirectory)/analyzed_outputs/threat-analysis.json" @@ -603,6 +609,7 @@ jobs: artifact: analyzed_outputs_$(Build.BuildId) - bash: | + set -eo pipefail COMPILER_VERSION="{{ compiler_version }}" DOWNLOAD_DIR="$(Pipeline.Workspace)/agentic-pipeline-compiler" DOWNLOAD_URL="https://github.com/githubnext/ado-aw/releases/download/v${COMPILER_VERSION}/ado-aw-linux-x64" @@ -614,7 +621,7 @@ jobs: curl -fsSL -o "$DOWNLOAD_DIR/checksums.txt" "$CHECKSUM_URL" echo "Verifying checksum..." - cd "$DOWNLOAD_DIR" + cd "$DOWNLOAD_DIR" || exit 1 grep "ado-aw-linux-x64" checksums.txt | sha256sum -c - mv ado-aw-linux-x64 ado-aw chmod +x ado-aw diff --git a/src/engine.rs b/src/engine.rs index 0d8e182c..182d74b4 100644 --- a/src/engine.rs +++ b/src/engine.rs @@ -105,7 +105,11 @@ impl Engine { /// Used by log collection steps to copy engine logs to pipeline artifacts. pub fn log_dir(&self) -> &str { match self { - Engine::Copilot => "~/.copilot/logs", + // `$HOME` (not `~`) so that the bash `[ -d "..." ]` test below + // actually expands. Tilde does not expand inside double quotes, + // so the previous value caused the directory check to always + // fail and Copilot logs were silently never collected. + Engine::Copilot => "$HOME/.copilot/logs", } } diff --git a/src/runtimes/dotnet/mod.rs b/src/runtimes/dotnet/mod.rs index d91e705d..eb885cb2 100644 --- a/src/runtimes/dotnet/mod.rs +++ b/src/runtimes/dotnet/mod.rs @@ -207,23 +207,23 @@ pub fn generate_ensure_nuget_config(config: &DotnetRuntimeConfig) -> String { let feed_url = config.feed_url().unwrap_or("https://api.nuget.org/v3/index.json"); format!( - "\ -- bash: |\n\ - if [ ! -f nuget.config ] && [ ! -f NuGet.config ] && [ ! -f NuGet.Config ]; then\n\ - cat > nuget.config <<'EOF'\n\ - \n\ - \n\ - \n\ - \n\ - \n\ - \n\ - \n\ - EOF\n\ - echo 'Created nuget.config with source={feed_url}'\n\ - else\n\ - echo 'nuget.config already exists, skipping creation'\n\ - fi\n\ - displayName: 'Ensure nuget.config exists'" + r#"- bash: | + set -eo pipefail + if [ ! -f nuget.config ] && [ ! -f NuGet.config ] && [ ! -f NuGet.Config ]; then + cat > nuget.config <<'EOF' + + + + + + + + EOF + echo 'Created nuget.config with source={feed_url}' + else + echo 'nuget.config already exists, skipping creation' + fi + displayName: 'Ensure nuget.config exists'"# ) } diff --git a/src/runtimes/lean/mod.rs b/src/runtimes/lean/mod.rs index fb070195..37728446 100644 --- a/src/runtimes/lean/mod.rs +++ b/src/runtimes/lean/mod.rs @@ -91,6 +91,7 @@ pub fn generate_lean_install(config: &LeanRuntimeConfig) -> String { let toolchain = config.toolchain().unwrap_or("stable"); let script = format!( "\ +set -eo pipefail curl https://elan.lean-lang.org/elan-init.sh -sSf | sh -s -- -y --default-toolchain {toolchain} echo \"##vso[task.prependpath]$HOME/.elan/bin\" export PATH=\"$HOME/.elan/bin:$PATH\" diff --git a/src/runtimes/node/mod.rs b/src/runtimes/node/mod.rs index 8933b81d..6ace5407 100644 --- a/src/runtimes/node/mod.rs +++ b/src/runtimes/node/mod.rs @@ -155,15 +155,15 @@ pub fn generate_ensure_npmrc(config: &NodeRuntimeConfig) -> String { .unwrap_or("https://registry.npmjs.org/"); format!( - "\ -- bash: |\n\ - if [ ! -f .npmrc ]; then\n\ - echo 'registry={registry}' > .npmrc\n\ - echo 'Created .npmrc with registry={registry}'\n\ - else\n\ - echo '.npmrc already exists, skipping creation'\n\ - fi\n\ - displayName: 'Ensure .npmrc exists'" + r#"- bash: | + set -eo pipefail + if [ ! -f .npmrc ]; then + echo 'registry={registry}' > .npmrc + echo 'Created .npmrc with registry={registry}' + else + echo '.npmrc already exists, skipping creation' + fi + displayName: 'Ensure .npmrc exists'"# ) } diff --git a/tests/bash_lint_tests.rs b/tests/bash_lint_tests.rs new file mode 100644 index 00000000..35969390 --- /dev/null +++ b/tests/bash_lint_tests.rs @@ -0,0 +1,397 @@ +//! Integration test that lints the bash bodies of compiled pipeline YAML +//! using `shellcheck`. +//! +//! ## Why this test exists +//! +//! Pipeline templates contain dozens of multi-line `bash:` steps. ADO bash +//! steps fail only on the *last* command's exit code by default, which makes +//! it easy for an earlier command to fail silently and the step to still +//! report green. Rather than spread `set -eo pipefail` boilerplate across +//! every step, we lint each bash body with shellcheck. Real silent-failure +//! patterns surface here: +//! +//! * **SC2164** — `cd $X` without `|| exit` (the canonical silent-failure) +//! * **SC2155** — `local var=$(cmd)` masking the inner exit code +//! * **SC2086 / SC2046** — unquoted variables / command substitutions +//! * **SC2154** — variables referenced but never assigned +//! * **SC2088** — tilde inside double quotes (does not expand) +//! +//! ## How it works +//! +//! 1. Compiles a representative set of fixtures with `ado-aw compile`. +//! 2. For each generated `*.lock.yml`, walks the YAML and collects every +//! `bash:` body that is the value of a step entry (i.e., a mapping that +//! is itself an element of a sequence). This avoids false positives from +//! arbitrary `bash` keys nested inside `env:` blocks or comments. +//! 3. Pipes each body to `shellcheck --shell=bash --format=json -`. +//! 4. Aggregates findings; the test fails with a structured report listing +//! every finding by fixture / step / line / code / message. +//! +//! ## Skip vs. enforce +//! +//! By default, if `shellcheck` is not installed locally the test prints a +//! notice and returns early. CI runners are expected to set the +//! `ENFORCE_BASH_LINT` environment variable so a missing shellcheck becomes +//! a hard failure rather than a silent skip. To install shellcheck locally: +//! +//! * macOS: `brew install shellcheck` +//! * Debian / Ubuntu: `apt-get install -y shellcheck` + +use std::collections::BTreeMap; +use std::io::Write; +use std::path::{Path, PathBuf}; +use std::process::{Command, Stdio}; + +use serde_yaml::Value; +use tempfile::TempDir; + +/// Shellcheck rule codes that are intentionally suppressed for ADO bash steps. +/// +/// Each entry has a justification — do not extend this list without one. +/// The list is deliberately short: project-specific suppressions belong as +/// per-line `# shellcheck disable=SCxxxx` comments inside the bash body, not +/// as a global override. +/// +/// * **SC1090, SC1091** — `source` paths that include ADO macros +/// (e.g. `$(Pipeline.Workspace)`) are dynamic and cannot be resolved by +/// shellcheck. +const SHELLCHECK_EXCLUDE: &str = "SC1090,SC1091"; + +/// Fixtures exercised by the lint. Chosen to collectively cover every bash-step +/// generator in the codebase: standalone + 1ES templates, every runtime that +/// emits bash steps (Lean, Node with feed-url, .NET with feed-url), and every +/// first-class tool that emits bash (cache-memory). Add a fixture here only +/// when a new generator is introduced that none of the existing fixtures +/// exercises. +/// +/// Note: `runtime-coverage-agent.md` and `runtime-coverage-1es-agent.md` are +/// the same agent compiled to different targets so we exercise code-generated +/// runtime/tool bash on both `standalone` and `1es`. Today their bash bodies +/// are byte-identical, but a future target-specific divergence in a generator +/// would only be caught with both fixtures in the harness. +const FIXTURES: &[&str] = &[ + "minimal-agent.md", + "complete-agent.md", + "1es-test-agent.md", + "azure-devops-mcp-agent.md", + "pipeline-trigger-agent.md", + "pipeline-filter-agent.md", + "runtime-coverage-agent.md", + "runtime-coverage-1es-agent.md", +]; + +/// Step display names that the lint expects to find at least once across all +/// fixtures. If any of these is missing it means the corresponding generator +/// is not being exercised — almost always because a fixture was deleted or +/// the generator's output changed without updating the coverage list. +const REQUIRED_STEP_DISPLAY_NAMES: &[&str] = &[ + // Static templates (standalone + 1ES) + "Prepare MCPG config", + "Prepare tooling", + "Prepare agent prompt", + "Run copilot (AWF network isolated)", + "Run threat analysis (AWF network isolated)", + "Evaluate threat analysis", + "Execute safe outputs (Stage 3)", + // Rust generators + "Install Lean 4 (elan)", // src/runtimes/lean/mod.rs + "Append Lean 4 prompt", // src/runtimes/lean/extension.rs + "Ensure .npmrc exists", // src/runtimes/node/mod.rs + "Ensure nuget.config exists", // src/runtimes/dotnet/mod.rs + "Restore previous agent memory", // src/tools/cache_memory/extension.rs + "Initialize empty agent memory (clearMemory=true)", + "Generate GITHUB_PATH file", // src/compile/common.rs (AWF path step) +]; + +fn ado_aw_binary() -> PathBuf { + PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")) +} + +fn fixtures_dir() -> PathBuf { + PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("tests/fixtures") +} + +/// Probe for the shellcheck binary. Returns `None` if it is not on PATH or +/// fails to report a version. +fn shellcheck_version() -> Option { + let output = Command::new("shellcheck").arg("--version").output().ok()?; + if !output.status.success() { + return None; + } + let text = String::from_utf8_lossy(&output.stdout); + text.lines() + .find(|l| l.starts_with("version:")) + .map(|l| l.trim().to_string()) +} + +/// A fresh `TempDir` plus a `.git/` marker so `ado-aw compile` can resolve +/// repo-relative paths. RAII cleans up on drop, even on panic. +fn fresh_workspace() -> TempDir { + let dir = tempfile::Builder::new() + .prefix("ado-aw-bash-lint-") + .tempdir() + .expect("create temp dir"); + std::fs::create_dir(dir.path().join(".git")).expect("create .git dir"); + dir +} + +/// Compile a fixture by copying it into `workspace` and invoking +/// `ado-aw compile`. Returns the path to the generated `.lock.yml` and the +/// target (`"standalone"` or `"1es"`) reported in compiler stdout. +fn compile_fixture(workspace: &Path, fixture: &str) -> (PathBuf, String) { + let src = fixtures_dir().join(fixture); + let dest = workspace.join(fixture); + std::fs::copy(&src, &dest) + .unwrap_or_else(|e| panic!("copy fixture {fixture}: {e}")); + + let output = Command::new(ado_aw_binary()) + .args(["compile", dest.to_str().unwrap()]) + .current_dir(workspace) + .output() + .unwrap_or_else(|e| panic!("spawn ado-aw compile: {e}")); + + assert!( + output.status.success(), + "ado-aw compile failed for {fixture}\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + + // `ado-aw compile` prints `Generated pipeline: ` to stdout; + // parse the target so the test can assert coverage of every known target. + let stdout = String::from_utf8_lossy(&output.stdout); + let target = if stdout.contains("Generated 1ES pipeline:") { + "1es" + } else if stdout.contains("Generated standalone pipeline:") { + "standalone" + } else { + panic!( + "could not determine compile target for {fixture} from stdout:\n{stdout}" + ) + }; + + let lock = dest.with_extension("lock.yml"); + assert!(lock.exists(), "expected lock file {}", lock.display()); + (lock, target.to_string()) +} + +/// A single bash body extracted from a compiled pipeline. +struct BashBody { + display_name: String, + body: String, +} + +/// Walk a parsed YAML document and collect every step that has a literal +/// block-scalar `bash:` body. Only mappings reached via a sequence element +/// are considered candidate steps — this avoids treating an arbitrary +/// `bash:` key inside, e.g., an `env:` block as a step. +fn extract_bash_bodies(yml_path: &Path) -> Vec { + let content = std::fs::read_to_string(yml_path) + .unwrap_or_else(|e| panic!("read {}: {e}", yml_path.display())); + let doc: Value = serde_yaml::from_str(&content) + .unwrap_or_else(|e| panic!("parse YAML {}: {e}", yml_path.display())); + + let mut out = Vec::new(); + collect(&doc, /* in_sequence_element = */ false, &mut out); + out +} + +fn collect(node: &Value, in_sequence_element: bool, out: &mut Vec) { + match node { + Value::Mapping(map) => { + // Only treat this mapping as a step candidate if we reached it + // by descending into a sequence (i.e., it's `[bash: |, …]`). + if in_sequence_element + && let Some(Value::String(body)) = map.get(Value::String("bash".into())) + { + let display_name = map + .get(Value::String("displayName".into())) + .and_then(Value::as_str) + .unwrap_or("") + .to_string(); + out.push(BashBody { + display_name, + body: body.clone(), + }); + } + for (_, v) in map { + collect(v, /* in_sequence_element = */ false, out); + } + } + Value::Sequence(seq) => { + for v in seq { + collect(v, /* in_sequence_element = */ true, out); + } + } + _ => {} + } +} + +/// Run shellcheck on a bash body. Returns the parsed JSON findings. +fn run_shellcheck(body: &str) -> serde_json::Value { + let mut child = Command::new("shellcheck") + .args([ + "--shell=bash", + "--format=json", + &format!("--exclude={SHELLCHECK_EXCLUDE}"), + "-", + ]) + .stdin(Stdio::piped()) + .stdout(Stdio::piped()) + .stderr(Stdio::piped()) + .spawn() + .expect("spawn shellcheck"); + + child + .stdin + .as_mut() + .expect("shellcheck stdin") + .write_all(body.as_bytes()) + .expect("write to shellcheck stdin"); + drop(child.stdin.take()); + + let output = child.wait_with_output().expect("wait for shellcheck"); + + // shellcheck exits 0 when clean and 1 when findings exist; both produce + // valid JSON on stdout. Higher exit codes (e.g. parse error) are real + // failures and should surface in the test output. + let exit = output.status.code().unwrap_or(-1); + if exit > 1 { + panic!( + "shellcheck failed (exit {exit}):\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); + } + + let stdout = String::from_utf8_lossy(&output.stdout); + let trimmed = stdout.trim(); + if trimmed.is_empty() { + return serde_json::Value::Array(Vec::new()); + } + + serde_json::from_str(trimmed) + .unwrap_or_else(|e| panic!("parse shellcheck JSON: {e}\nraw:\n{stdout}")) +} + +/// Format a single shellcheck finding for the test failure report. +fn format_finding(fixture: &str, display_name: &str, finding: &serde_json::Value) -> String { + let code = finding["code"].as_i64().unwrap_or(0); + let level = finding["level"].as_str().unwrap_or("?"); + let line = finding["line"].as_i64().unwrap_or(0); + let message = finding["message"].as_str().unwrap_or("?"); + format!(" [{level}] SC{code} {fixture} :: {display_name:?} (body L{line}): {message}") +} + +#[test] +fn compiled_bash_bodies_pass_shellcheck() { + let enforce = std::env::var_os("ENFORCE_BASH_LINT").is_some(); + + let Some(version) = shellcheck_version() else { + if enforce { + panic!( + "ENFORCE_BASH_LINT is set but `shellcheck` is not on PATH. \ + Install it in CI (e.g. `apt-get install -y shellcheck`) \ + or unset ENFORCE_BASH_LINT for local development." + ); + } + eprintln!( + "skipping bash lint test: `shellcheck` not found on PATH. \ + Install via your OS package manager (e.g. `brew install shellcheck`, \ + `apt-get install -y shellcheck`). \ + Set ENFORCE_BASH_LINT=1 to make this a hard failure (CI does)." + ); + return; + }; + eprintln!("using {version}"); + + let workspace = fresh_workspace(); + let mut report: BTreeMap> = BTreeMap::new(); + let mut all_display_names: Vec = Vec::new(); + let mut targets_seen: std::collections::BTreeSet = + std::collections::BTreeSet::new(); + + for fixture in FIXTURES { + let (lock, target) = compile_fixture(workspace.path(), fixture); + targets_seen.insert(target); + for body in extract_bash_bodies(&lock) { + all_display_names.push(body.display_name.clone()); + let findings = run_shellcheck(&body.body); + if let Some(arr) = findings.as_array() { + for finding in arr { + report + .entry((*fixture).to_string()) + .or_default() + .push(format_finding(fixture, &body.display_name, finding)); + } + } + } + } + + // Target coverage — assert that every known compile target is exercised by + // at least one fixture, so we shellcheck the bash output of every template + // (`src/data/base.yml` and `src/data/1es-base.yml`) and every code-generated + // step on both targets. + const REQUIRED_TARGETS: &[&str] = &["standalone", "1es"]; + let missing_targets: Vec<&str> = REQUIRED_TARGETS + .iter() + .copied() + .filter(|t| !targets_seen.contains(*t)) + .collect(); + assert!( + missing_targets.is_empty(), + "no fixture compiles to the following target(s): {:?}\n\ + Each compile target has its own template under src/data/ whose bash \ + bodies need shellchecking. Add a fixture with `target: ` to \ + tests/fixtures/ and list it in FIXTURES.", + missing_targets + ); + + // Coverage check — every required generator must appear in the harvested + // step list, otherwise a fixture has stopped exercising its generator. + let missing: Vec<&str> = REQUIRED_STEP_DISPLAY_NAMES + .iter() + .copied() + .filter(|name| !all_display_names.iter().any(|d| d == name)) + .collect(); + assert!( + missing.is_empty(), + "the following step display names were not produced by any fixture, \ + meaning their generator is not being linted:\n {}\n\ + Either add a fixture exercising the generator, or update \ + REQUIRED_STEP_DISPLAY_NAMES if the generator was removed.", + missing.join("\n ") + ); + + if !report.is_empty() { + let mut msg = String::from( + "shellcheck flagged silent-failure patterns in compiled bash bodies. \ + Each finding represents a real or stylistic concern; fix the \ + offending bash, or if intentional add `# shellcheck disable=SCxxxx` \ + inline in the bash body (the directive is a comment and does not \ + affect runtime behaviour).\n", + ); + for (fixture, lines) in &report { + msg.push_str(&format!("\n--- {fixture} ---\n")); + for line in lines { + msg.push_str(line); + msg.push('\n'); + } + } + panic!("{msg}"); + } +} + +/// Sanity check: every listed fixture exists. Catches typos in `FIXTURES` +/// without paying the cost of compiling. +#[test] +fn every_listed_fixture_exists() { + for fixture in FIXTURES { + let path = fixtures_dir().join(fixture); + assert!( + path.exists(), + "fixture listed in FIXTURES but not on disk: {}", + path.display() + ); + } +} diff --git a/tests/fixtures/runtime-coverage-1es-agent.md b/tests/fixtures/runtime-coverage-1es-agent.md new file mode 100644 index 00000000..49b90e38 --- /dev/null +++ b/tests/fixtures/runtime-coverage-1es-agent.md @@ -0,0 +1,24 @@ +--- +name: Runtime Coverage 1ES Agent +description: 1ES variant of runtime-coverage-agent.md so the bash-lint test exercises code-generated runtime/tool bash bodies on the 1ES target as well as standalone +target: 1es +on: + schedule: daily +runtimes: + lean: true + node: + version: "22.x" + feed-url: "https://pkgs.dev.azure.com/example/example/_packaging/example/npm/registry/" + dotnet: + version: "8.0.x" + feed-url: "https://pkgs.dev.azure.com/example/example/_packaging/example/nuget/v3/index.json" +tools: + cache-memory: true +--- + +## Runtime Coverage 1ES Agent + +1ES variant of `runtime-coverage-agent.md`. Same runtimes and tools, compiled to +the 1ES target so the bash-lint integration test analyses code-generated bash +bodies on both targets. Today the runtime/tool extension generators emit +target-agnostic bash, but this fixture guards against future divergence. diff --git a/tests/fixtures/runtime-coverage-agent.md b/tests/fixtures/runtime-coverage-agent.md new file mode 100644 index 00000000..1a727deb --- /dev/null +++ b/tests/fixtures/runtime-coverage-agent.md @@ -0,0 +1,22 @@ +--- +name: Runtime Coverage Agent +description: Lint-only fixture exercising Lean, Node, .NET runtimes and the cache-memory tool +on: + schedule: daily +runtimes: + lean: true + node: + version: "22.x" + feed-url: "https://pkgs.dev.azure.com/example/example/_packaging/example/npm/registry/" + dotnet: + version: "8.0.x" + feed-url: "https://pkgs.dev.azure.com/example/example/_packaging/example/nuget/v3/index.json" +tools: + cache-memory: true +--- + +## Runtime Coverage Agent + +This agent enables every runtime that produces a code-generated bash step, +plus the `cache-memory` tool. Its sole job is to compile cleanly so the +bash-step lint can analyse those generated bodies.