refactor(compile): structure generated ado-proxy and az wrapper scripts

[jamesadevine]

Summary

Closes #1833.

start_ado_proxy_step was a ~200-line body built by format!, and render_az_wrapper a ~120-line interpolated standalone executable. Neither was reviewable as shell — and reviewing them as shell is the only way to know they are correct.

The unreadability came from the escaping, not from bash: \n\ continuations to fake multi-line source, doubled braces to survive format! itself (a Docker Go-template read {{{{.State.Status}}}}), and an escaped quote for every quoted word. So bash stays; the escaping goes.

The approach

A new src/compile/shell/ module. A script is a raw-string const co-located with its producer, registered by shell_script!, with substitution restricted to a typed, shell-quoted prelude:

shell_script! {
    /// Greppable: search `mkfifo` and you land on the producer.
    START_ADO_PROXY {
        interpreter: Bash,
        bindings: [PROXY_CONTAINER, AGENT_TEMP],
        externals: [],
        fragments: [resolve_org],
        body: r#"
set -euo pipefail
# ado-aw:fragment resolve_org
PROXY_DIR=$(mktemp -d "$AGENT_TEMP/ado-proxy.XXXXXX")
docker inspect -f 'state={{.State.Status}}' "$PROXY_CONTAINER"
"#,
    }
}

• One injection position. A value can only be the right-hand side of a prelude assignment, so it can never alter the structure of the script. Typed binders (text, number, boolean, words, ado_macro, ado_path, document) each validate their own shape.
• Declared surface. Every variable a body reads is a binding (compiler-supplied) or an external (runtime-supplied). Enforced at render and by a registry-wide test.
• Credentials cannot be bindings — the prelude is committed to the repo. They stay on EnvValue::secret, which ADO masks.
• Single-hop editing. The shell stays in the file that produces it: grep the shell, land on the producer.

Issue #1833 specifics

start_ado_proxy_step is decomposed into eight registered phases (work directory, policy, material minting, material assembly, container start, handover, destruction, readiness) spliced into an outline at # ado-aw:fragment markers.

It remains one atomic Bash task. The phases are Rust-level and shell-level structure, not additional ADO steps, so the bearer, CA private key and leaf keys still never touch a runner path, argv, environment or container layer. All 15 credential-custody tests pass unchanged.

The container's nested sh -c entrypoint — previously an opaque string shellcheck could not see into — is now its own registered Sh script and is linted for the first time.

Lint coverage was the real gap

Coverage used to depend on fixture reachability: a generator no fixture exercised was linted by nothing. That was exactly the ado-proxy lifecycle and the az wrapper — several hundred lines of unlinted shell.

There are now two levels:

Level	What it proves
src/compile/shell/lint.rs — every registered script in isolation	the shell is correct
tests/bash_lint_tests.rs — bodies in compiled YAML	the shell is emitted

A composed script is linted with its phases spliced in, so cross-phase variable flow — the one new risk decomposition introduces — is checked rather than assumed.

ado-aw export-bash-scripts --output <dir> materialises all 87 scripts as reviewable .sh files with provenance headers.

Bugs found and fixed

Three of these were the tooling distorting the source rather than the source being wrong:

• referenced_vars demanded a declaration for awk's $NF inside single quotes. An agent had renamed it to $n to comply; the original expression is restored and the checker now skips single-quoted spans.
• PATH was stub-assigned, tripping SC2123 — a bug the real script does not have.
• The export header pushed shebangs off line 1 (SC1128), so exported sh scripts were unrunnable. Caught only by shellchecking the exported files independently of the harness that generates them.
• -v /tmp/ado-aw-lib:... duplicated the AZ_WRAPPER_DIR constant in the proxy container mount; now bound.

Scope note

Also migrated: the az wrapper, every extensions/ generator, the four runtimes, cache_memory, common.rs, filter_ir.rs, engine.rs, and the supply-chain payload staging helpers (which carried explicit "SAFETY: unescaped interpolation" comments — now enforced rather than requested). bash() and dedent() are deleted.

safe_outputs/create_pull_request.rs is deliberately not migrated. It has 38 \n\ continuations and no shell at all — they are Markdown PR-description text, and its 13 git calls are argv-based. Wrapping them would have added a shell that is not there today.

tests/generated_shell_guard.rs prevents regression, and a test proves the guard distinguishes an inline body from a format! display name — a guard that only ever passes is indistinguishable from one that does nothing.

Test plan

All run locally against shellcheck 0.10.0 with ENFORCE_BASH_LINT=1:

• cargo test — 2981 unit + all integration suites green
• cargo clippy --all-targets — zero warnings, zero errors
• cargo test --test bash_lint_tests — shellcheck over every bash body in compiled YAML
• cargo test --bin ado-aw compile::shell — registry-wide shellcheck plus declared-surface, fragment-marker and shell-name guards
• cargo test --test generated_shell_guard — regression guard
• Independent verification: exported all 87 scripts and ran shellcheck on them as standalone files, outside the in-process harness — 0 findings. This is what caught the shebang bug; a harness that generates and then checks its own input can agree with itself while both are wrong.
• Credential custody: the 15 ado_proxy_* tests (bearer/CA-key never under /tmp, stdin-only handover, key destruction after handover, single-task startup) all pass unchanged.

Emitted YAML bytes change (a binding prelude is added and literals become variable references); behaviour and step ordering do not. Rebased onto main, resolving conflicts with #1858 (hidden export-command docs) and #1843 (Azure DevOps MCP version override — its override path is preserved and still asserted).

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).
1 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[jamesadevine]

/review

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

[github-actions]

Warning

Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding.

What happened

The threat detection results could not be parsed.

Review the workflow run logs for details.

🧪 Test quality analysis by Test Quality Sentinel · auto · 67.2 AIC · ⊞ 11.1K
_{Comment /review to run again}

[github-actions]

Reviewed the Rust engineering quality of the src/compile/shell/ refactor and its call-site migrations (agentic_pipeline.rs, az_wrapper.rs, azure_cli.rs, ado_script.rs, exec_context/, filter_ir.rs, runtimes/, generated_shell_guard.rs, and the shell/ module itself).

Verdict: COMMENT — no merge-blocking defects found. The migration is disciplined: Binding construction is well-validated (injection-safety unit tests are thorough), all new unwrap()/expect() calls are confined to test code or documented internal-invariant panics, and the mechanical format! → ShellScript conversions I spot-checked are behaviorally equivalent to what they replace.

Notes

• One inline comment posted: a possible pipe deadlock in shell/lint.rs's check() (writes to child stdin before reading output) — currently low-risk since scripts are small, but worth hardening given a lint-only failure mode of a hung CI job is a bad debugging experience.
• The rust-critic sub-agent I started in Step 1 returned no usable output on two reads; per the review contract I discarded it and relied on my own manual pass.
• Did not review ado-aw-specific concerns (front-matter schemas, generated YAML shape, safe-output/bundle drift) — that's the Compiler Contract Reviewer's remit.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 151.9 AIC · ⌖ 2.17 AIC · ⊞ 11.2K
_{Comment /review to run again}

[jamesadevine]

@copilot merge main

[Copilot]

@copilot merge main

Merged origin/main (4a36dbc) into the branch — no conflicts. cargo test --all-targets is green after the merge. Merge commit: ab9fa2a.

[jamesadevine]

/review

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Test-quality pass over the shell-refactor PR. Most of the ~5,700 lines of new/changed test code are solid: the new src/compile/shell/ module (bindings.rs, mod.rs, registry.rs, lint.rs) is thoroughly unit-tested with meaningful negative cases (should_panic on credentials, command substitutions, unterminated macros, undeclared bindings/fragments), and tests/generated_shell_guard.rs includes a self-test proving its own discriminator (the_guard_catches_an_inline_body_but_not_a_formatted_display_name) — a good practice that's rare to see. The gate_e2e.rs and ado_aw_marker.rs test updates correctly track the underlying binding changes rather than just loosening assertions to compile.

One finding: in src/compile/az_wrapper.rs, advertises_only_what_the_policy_actually_grants and rest_stays_available_whatever_the_capabilities replaced space-padded substring checks (" repos ", !contains(" {absent} ")) with bare .contains("repos") / .contains(absent) against the same space-joined ALLOWED_GROUPS string. This drops word-boundary protection the original test relied on to guard against one group name being a substring of another — see inline comment.

No missing-test gaps or weakened assertions found elsewhere in the diff.

🧪 Test quality analysis by Test Quality Sentinel · auto · 75.5 AIC · ⌖ 2.25 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Security review: no regressions found

This PR refactors ~200-line format!-built shell (ado-proxy startup, az wrapper) into a typed src/compile/shell/ module (ShellScript + Binding). Reviewed for security-relevant behavior change:

• Injection surface: Binding constructors (text/words/ado_macro/ado_path/document) validate and single-quote every substituted value, closing off the exact positions format! interpolation used to smuggle command substitution or metacharacters. ado_path explicitly rejects ` and un-validated $(...).
• Credential handling unchanged: SC_READ_TOKEN/SC_WRITE_TOKEN/SYSTEM_ACCESSTOKEN/GITHUB_TOKEN/etc. still flow only through with_env(..., EnvValue::secret(...)), never through the new binding/prelude channel. Binding constructors assert-panic if a value mentions a secret name (assert_not_secret), which is a new defense-in-depth check that did not exist before.
• Fragments (verbatim spliced shell, e.g. resolve_ado_organization_bash, the az wrapper body) are compiler-authored constants, not user/agent-controlled input, so the lack of a secret scan on fragment text is not a regression — same trust level as the format! body it replaces.
• No changes to src/validate.rs, src/sanitize.rs, or src/secure.rs — no validation logic was touched, widened, or removed.
• Token projection points (ado_bundle.rs, token_source_for) are untouched by this diff.
• Values like awf_paths/pipeline template paths remain compiler-internal (not agent/PR-controlled), so routing them through Binding::document/heredoc is a readability improvement, not a new trust boundary.

Overall this is a security-neutral (net-positive: adds assert_not_secret guard) internal refactor with extensive new tests (shellcheck-every-script lint, fragment-marker drift checks). No exploitable injection, token-scope, or validation regressions found in the diff.

🔒 Security review by PR Security Reviewer · auto · 60.4 AIC · ⌖ 4.53 AIC · ⊞ 11.6K
_{Comment /review to run again}

[github-actions]

Compiler Contract Review — no blocking findings

Reviewed this refactor against ado-aw's compiler contracts (front-matter grammar, safe-output tools, typed IR, generated shell, extensions, docs sync). This is a clean, well-scoped, thoroughly self-documenting refactor.

Drift checks — all clear:

• No .github/workflows/*.md changed without its .lock.yml.
• scripts/ado-script/src/** was not touched, so no bundle-drift concern.
• filter_ir.rs changes are mechanical IR-step return-type adjustments, not Fact/gate-schema changes — no codegen drift.
• No tests/safe-outputs/ lock files added.

Contract checks:

• New src/compile/shell/ module (bindings/registry/lint/export/mod) is fully documented in AGENTS.md (tree entry + description), docs/extending.md (new "Generated shell scripts" section), docs/cli.md (export-bash-scripts), and the root README-equivalent testing section — all four required touch points are present.
• The new Binding injection-position design (single RHS-of-assignment substitution, credential-name rejection via SECRET_NAMES, ado_macro/ado_path restricted to dotted-name macros) is a genuine security improvement over the prior format!-built shell, and is backed by targeted should_panic tests for each rejection path.
• tests/generated_shell_guard.rs is a real enforcement mechanism (not just documentation) — it greps for BashStep::new calls with an inline format!/\n\ script body and fails the build.
• No raw String identifier fields were introduced in Params/safe-output types in this diff.

Verification performed:

• cargo test --bin ado-aw compile::shell — 58/58 pass, including every_registered_script_passes_shellcheck.
• cargo test --test generated_shell_guard — 4/4 pass.
• cargo test --test bash_lint_tests — 2/2 pass (compiled-YAML shellcheck).
• cargo test --bin ado-aw (full unit suite) — 2993/2993 pass.
• cargo test --test compiler_tests --test gate_e2e — 222/222 pass.

No merge-blocking issues found.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

• spsprodeus21.vssps.visualstudio.com

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "spsprodeus21.vssps.visualstudio.com"

See Network Configuration for more information.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 100.8 AIC · ⌖ 1.99 AIC · ⊞ 12.7K
_{Comment /review to run again}

[github-actions]

Rust code quality review

No merge-blocking Rust defects found in the changed lines. This PR mechanically migrates hand-built format!/heredoc shell scripts to the new typed ShellScript/Binding registry (src/compile/shell/), and the migration is unusually careful: every conversion I checked preserves exact runtime behavior, keeps the credential-custody contract intact (bearer via env: only, private material streamed via stdin/FIFO, destroyed before polling in start_ado_proxy_step), and is backed by tests that assert on the new prelude/binding shape rather than loosening coverage.

💡 Areas checked

• src/compile/shell/{bindings,mod,registry,lint}.rs (new) — Binding constructors validate metacharacters/whitespace/secrets before shell-quoting; all_scripts() explicitly sorts for determinism instead of relying on inventory link order.
• src/engine.rs, src/compile/az_wrapper.rs, src/compile/common.rs — refactored install/wrapper/org-resolution steps produce equivalent scripts to the prior format! versions.
• src/compile/extensions/ado_script.rs, azure_cli.rs — bundle download/mint/revoke/prepare-pr-base steps correctly route paths and API URLs through bindings, including the ${API_URL:+...} empty-value guard.
• src/compile/agentic_pipeline.rs::start_ado_proxy_step (security-critical) — the FIFO material handover, LEAF_HOSTS word-list binding (validated, shellcheck disable=SC2086 correctly documented for the intentional unquoted word-split), and stdin-only bearer custody are preserved verbatim.

Note: the rust-critic sub-agent I started produced no usable output after two poll cycles (idle with empty response both times); its findings are discarded per the review contract, and this review reflects only my own manual pass.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 175.2 AIC · ⌖ 9.3 AIC · ⊞ 11.4K
_{Comment /review to run again}

[jamesadevine]

/azp run ado-aw candidate compiler smoke

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[jamesadevine]

/azp run ado-aw candidate compiler smoke

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[jamesadevine]

/azp run ado-aw candidate compiler smoke

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).