feat(engine): add runtime Copilot model controls

[Copilot]

Summary

ADO operators could not switch Agent or Detection Copilot models during an outage without editing workflow markdown and recompiling. This adds runtime model controls through Azure DevOps YAML variables, UI variables, and variable groups while preserving explicit front-matter model precedence.

• Runtime precedence
  • engine.model / effective Detection model
  • ADO_AW_MODEL_AGENT_COPILOT or ADO_AW_MODEL_DETECTION_COPILOT
  • ADO_AW_DEFAULT_MODEL_COPILOT
  • existing Copilot CLI default

variables:
  ADO_AW_MODEL_AGENT_COPILOT: gpt-5-mini
  ADO_AW_MODEL_DETECTION_COPILOT: claude-sonnet-4.5

• Bundled Copilot invocation

  • replaces generated Copilot command/model Bash with a strict, versioned copilot-invoker.js protocol
  • constructs real argv arrays, resolves runtime models inside AWF, forwards signals, and preserves Copilot exit codes
  • writes a requested-model result before launch; trusted host code validates and merges it into aw_info.json
  • stages the version-matched ado-script.zip in every Agent and enabled Detection job through existing release/feed/pipeline-artifact supply-chain paths

• Safe runtime plumbing

  • maps Azure DevOps model variables through typed step env entries, avoiding macro expansion inside generated Bash
  • resolves them when the Agent or Detection task starts, so preceding task.setvariable steps are honored
  • validates runtime model identifiers and sets Copilot CLI's native COPILOT_MODEL only in the child environment
  • rejects competing raw engine.args --model and engine.env.COPILOT_MODEL configuration
  • keeps explicit configured models static and authoritative

• Accurate run metadata

  • records the requested Agent and Detection session models in aw_info.json
  • reports the OTel-observed Agent model separately when a custom-agent model declaration supersedes the requested model
  • treats unavailable Agent metadata as non-fatal so isolated runtime probes can still execute

Test plan

• full cargo test
• cargo clippy --all-targets -- -D warnings
• cargo check --all-targets
• cargo test --bin ado-aw compile::shell
• cargo test --test generated_shell_guard
• cargo test --test bash_lint_tests
• npm --prefix scripts/ado-script test -- --pool=forks --fileParallelism=false
• npm --prefix scripts/ado-script run typecheck
• npm --prefix scripts/ado-script run build
• npm --prefix scripts/ado-script run test:smoke
• standalone, 1ES, job, and stage compiler regressions
• real Linux AWF + Copilot + SafeOutputs contract
• PR CI and ado-script E2E

• Fixes [agent-issue]: Mirror gh-aw's runtime model controls in ado-aw #2245

[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]

@copilot taw string preferrred... like the rest of the codebase

[Copilot]

Addressed in d34ee67 by converting the runtime model preamble to a raw string literal.

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

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

Warning

Firewall blocked 1 domain

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

• releaseassets.githubusercontent.com

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

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

Security review: no regressions found

Reviewed the full diff (docs/engine.md, agentic_pipeline.rs, ado_aw_marker.rs, engine.rs) introducing runtime Copilot model pipeline-variable overrides.

• Runtime model values pass a strict allowlist ([A-Za-z0-9._:-]) in both the bash preamble (engine.rs::runtime_model_preamble) and the aw_info.json writer, before reaching --model "$ADO_AW_EFFECTIVE_MODEL" (quoted) or JSON output — no shell/JSON injection path.
• The new variable names (ADO_AW_MODEL_AGENT_COPILOT, ADO_AW_MODEL_DETECTION_COPILOT, ADO_AW_DEFAULT_MODEL_COPILOT) are added to BLOCKED_ENV_KEYS, so untrusted workflow engine.env cannot override them.
• Explicit engine.model still takes precedence over runtime overrides, and no write-capable token or Stage 3 write path is touched.

No exploitable finding. Minor non-security note: the allowlist/precedence logic is duplicated between engine.rs and ado_aw_marker.rs — low risk of the two drifting out of sync over time, not blocking.

🔒 Security review by PR Security Reviewer · auto · 61.6 AIC · ⌖ 5.94 AIC · ⊞ 10.2K
_{Comment /review to run again}

[github-actions]

Test Quality review

The new tests are well-scoped for verifying what strings appear in generated invocations/env/scripts, but they don't verify the runtime shell logic actually behaves correctly — every assertion is a contains() check on generated text, never an execution of the generated bash. Three findings posted inline:

1. copilot_detection_env's explicit-model omission path is untested (only the Agent-side copilot_env omission has a test), leaving an asymmetric gap.
2. The runtime-model precedence/validation shell logic (runtime_model_preamble) is only string-matched, never executed, so a broken fallback order or validation regex would pass all four new invocation tests.
3. The precedence/validation algorithm is independently duplicated between ado_aw_marker.rs's ado_aw_runtime_model (bash) and engine.rs's runtime_model_preamble (bash-generating Rust) with nothing checking they agree.

None of these are merge-blocking on their own — the string-based tests do catch gross omissions — but the actual security-relevant validation logic (rejecting invalid model characters before they reach an embedded shell command) ships with zero behavioral test coverage.

🧪 Test quality analysis by Test Quality Sentinel · auto · 143.7 AIC · ⌖ 6.34 AIC · ⊞ 9.8K
_{Comment /review to run again}

[github-actions]

copilot_detection_env gates the runtime model vars on engine_config.model().is_none() (line 1005), mirroring copilot_env's gating — but only the Agent-side omission is tested (copilot_engine_env_omits_runtime_agent_model_vars_for_explicit_model). There is no copilot_detection_env test asserting the runtime vars are omitted when an explicit Detection model is configured, so a regression that always injects ADO_AW_MODEL_DETECTION_COPILOT/ADO_AW_DEFAULT_MODEL_COPILOT even with an explicit model would ship silently.

💡 Suggested test

#[test]
fn copilot_detection_env_omits_runtime_model_vars_for_explicit_model() {
    let (front_matter, _) = parse_markdown(
        "---\nname: test\ndescription: test\nsafe-outputs:\n  threat-detection:\n    engine:\n      model: detector-model\n---\n",
    )
    .unwrap();
    let env = copilot_detection_env(&front_matter.engine).unwrap();
    assert!(!env.iter().any(|(key, _)| key == ADO_AW_MODEL_DETECTION_COPILOT));
    assert!(!env.iter().any(|(key, _)| key == ADO_AW_DEFAULT_MODEL_COPILOT));
}

[github-actions]

The new runtime-model invocation tests only check that the generated string contains fragments (e.g. invocation.contains("*[!A-Za-z0-9._:-]*)"), invocation.contains("ADO_AW_MODEL_DETECTION_COPILOT")), never that the shell preamble in runtime_model_preamble actually resolves the precedence or rejects invalid characters when executed. This mirrors the implementation rather than testing behaviour — a change that silently breaks the fallback order (specific var, then default var) or the character-validation case guard would pass all four new tests unchanged.

💡 Suggested approach

Extract the generated runtime_model_preamble output to a temp script and execute it via bash -c with different env combinations (unset, specific-var set, default-var set, both set, invalid characters), asserting on $ADO_AW_EFFECTIVE_MODEL / exit status — similar to the pattern already used in tests/compiler_tests.rs::exercise_attached_prompt_with_pinned_copilot_cli for running generated shell.

[github-actions]

ado_aw_runtime_model (bash, EMIT_AW_INFO) reimplements the same precedence/validation algorithm as runtime_model_preamble in src/engine.rs — two independent hand-written copies of the fallback-and-character-validation logic, each only string-matched in tests, with nothing asserting they agree. If one copy's regex/case-guard is tightened or loosened without updating the other, the aw_info.json metadata and the actual --model argument passed to Copilot could silently diverge (e.g. a model rejected by one path but accepted and used by the other).

💡 Suggested approach

Add a small end-to-end test that renders both EMIT_AW_INFO's script and runtime_model_preamble's output for the same ADO_AW_MODEL_AGENT_COPILOT/ADO_AW_DEFAULT_MODEL_COPILOT input combinations (valid, invalid-chars, $(VAR) literal, empty) and checks their outcomes match — or, better, factor the character-validation case pattern into one shared constant/fragment so a change to the allowed character set can't be made in only one place.

[github-actions]

Rust review: COMMENT

Note: the rust-critic sub-agent failed to return output (model access error) on two attempts; findings below are from my own pass only.

Themes

• One pub method (invocation_with_config) is left as dead code behind #[allow(dead_code)] after this PR rewires its only caller to detection_invocation_with_config.
• The runtime-model character validation regex/case is duplicated across three locations (shell EMIT_AW_INFO, shell runtime_model_preamble, Rust validate_model_name), risking silent drift.

Both are maintainability concerns, not merge-blockers — the actual injection-prevention logic is sound (validated before use in all three copies), tests pass (cargo test copilot_), and the documented precedence order matches the implementation.

Warning

Firewall blocked 1 domain

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

• releaseassets.githubusercontent.com

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

network:
  allowed:
    - defaults
    - "releaseassets.githubusercontent.com"

See Network Configuration for more information.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 128.9 AIC · ⌖ 1.96 AIC · ⊞ 10.1K
_{Comment /review to run again}

[github-actions]

invocation_with_config is now dead code: after this PR its only caller was replaced by detection_invocation_with_config, and a repo-wide grep shows zero remaining call sites. It is kept alive solely by #[allow(dead_code)] on a pub method, so the compiler warning that would normally catch this is suppressed rather than fixed.

💡 Suggested fix

Either delete invocation_with_config entirely (it duplicates detection_invocation_with_config with runtime_model_role: None, so a caller could just pass None if this ever becomes needed again), or remove the #[allow(dead_code)] and let the compiler flag it if it is truly meant to be temporarily retained. Leaving pub dead code with a suppressed lint is a maintenance trap: a future refactor can silently rot this path without any signal.

[github-actions]

The runtime-model character-class validation (*[!A-Za-z0-9._:-]* in bash) is now implemented three times: here in the EMIT_AW_INFO shell script, again in runtime_model_preamble (src/engine.rs), and as the Rust-side validate_model_name for explicit engine.model. The three copies must stay byte-for-byte in sync or a future edit to one silently diverges from the others, re-opening the injection surface this validation exists to close.

💡 Suggested fix

Extract the bash validation into a single fragment/binding shared by both EMIT_AW_INFO and runtime_model_preamble, or at minimum add a comment cross-referencing all three call sites so a reviewer touching one is prompted to check the others. A regression test asserting the two shell-emitted validation blocks are textually identical would also catch drift automatically.

[github-actions]

Compiler-contract review focused on drift and typed-contract invariants (not general Rust style, covered by other reviewers).

Checked and clean:

• Front-matter grammar: no new/renamed fields in src/compile/types.rs.
• BLOCKED_ENV_KEYS correctly extended with the three new ADO_AW_MODEL_* runtime variables, and a test (copilot_engine_env_rejects_user_runtime_model_var_override) proves engine.env cannot shadow them.
• EMIT_AW_INFO/RUN_AGENT shell changes stay inside the ShellScript/Binding/shell_script! framework — new externals: entries are declared for the three runtime model vars, and ENFORCE_BASH_LINT=1 cargo test --bin ado-aw compile::shell / --test bash_lint_tests both pass locally with the change in place.
• docs/engine.md documents the new Runtime model controls section, precedence order, and the Detection-engine interaction — matches the code.
• No public PipelineSummary/GraphSummary fields touched; aw_info.json's model/detection_model keys are optional strings already handled by src/audit/model.rs.
• No ado-script bundle sources or codegen inputs (filter_ir.rs, Fact) changed, so the bundle/codegen drift checks don't apply here.

One finding posted inline on src/engine.rs: the renamed detection_invocation_with_config orphaned the old invocation_with_config, which is now dead code hidden behind #[allow(dead_code)] instead of being deleted.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 200.2 AIC · ⌖ 1.92 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

invocation_with_config is now unreadable dead code kept alive with #[allow(dead_code)]

Removing the #[allow(dead_code)] and rebuilding produces warning: methods ... invocation_with_config ... never used — the only prior caller was rewired to detection_invocation_with_config in this same PR (src/compile/agentic_pipeline.rs), so this method has zero remaining call sites anywhere in the tree.

💡 Why this matters / fix

Suppressing the warning instead of deleting the method hides genuinely orphaned public API from cargo build/clippy going forward — the next contributor who edits invocation_with_config gets no compiler signal that it is unreachable, and a real future regression in it would go unnoticed since nothing exercises it.

Fix: delete invocation_with_config (and the now-unnecessary #[allow(dead_code)]) unless there is a concrete near-term caller planned; if one is coming in a follow-up PR, say so explicitly instead of the generic "retained for callers that need a plain invocation" note, which as written applies to no one.

Verified via: sed -i 's/#\[allow(dead_code)\]//' src/engine.rs && cargo build --bin ado-aw → emits the never-used warning.

[jamesadevine]

@copilot Address feedback

[github-actions]

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

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Compiler contract review

Checked for drift between the runtime-model-control change and its generated artefacts / docs / schemas:

• Workflow lock drift: .github/workflows/*.md and their .lock.yml are in sync (latest commit 4cc781d regenerated locks alongside the .md edits).
• Codegen drift: no changes touched filter_ir.rs / the Fact enum, so types.gen.ts and fact-catalog.gen.json correctly did not move.
• Ado-script bundle sources: no scripts/ado-script/src/** changes in this PR, so no bundle drift to check.
• Front-matter grammar / safe-outputs schema: unchanged — no new Option<T> fields, no raw String identifier fields introduced.
• Typed IR: the new resolve_runtime_model / append_aw_info_field / capture_runtime_model fragments correctly use the existing shell_script! phases:/fragment_uses: registry mechanism rather than ad-hoc splicing, and go through Binding-typed bind calls — no format!-built bodies.
• Docs sync: docs/engine.md documents the new ADO_AW_MODEL_*_COPILOT precedence and COPILOT_MODEL blocking; docs/audit.md documents the new requested_model/observed_model/detection_model overview fields and the Detection aw_info.json enrichment/merge behavior. Both match the shipped code.
• Earlier review rounds on this PR already flagged (and the current tree has since fixed) the triplicated character-class validation, the invocation_with_config dead code, and the weak contains()-only test assertions — those are resolved as of the latest commits, so no new findings to raise there.

No compiler-contract drift found in the current diff. Nothing merge-blocking from this angle.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 103 AIC · ⌖ 11 AIC · ⊞ 12.5K
_{Comment /review to run again}

[github-actions]

Overall this PR is thoroughly tested: the runtime model precedence (engine.model > role-specific var > default var > CLI default) is covered for both Agent and Detection roles, the shared runtime_model_preamble bash is exercised directly via subprocess tests for every branch (role-specific wins, default fallback, unexpanded-macro-as-missing, invalid-character rejection), the append_aw_info_field helper has dedicated tests for key/value confusion and whitespace-preserving key matching, and tests/compiler_tests.rs adds a cross-target (standalone/1ES/job/stage) compile assertion. Prior bot findings on this PR (dead invocation_with_config, untested Agent-only gating, triplicated validation) have all since been addressed in later commits.

One small gap remains: copilot_detection_env's invalid-model rejection path is untested (see inline comment) — the equivalent Agent-side path is covered, but a regression on the Detection side (e.g. an injected COPILOT_MODEL value) would ship silently. Advisory only, not merge-blocking.

🧪 Test quality analysis by Test Quality Sentinel · auto · 212.7 AIC · ⌖ 1.73 AIC · ⊞ 11K
_{Comment /review to run again}

[github-actions]

Security review — runtime Copilot model controls

The core runtime-model feature is well-guarded: model names are validated with a strict character allowlist (validate_model_name, shared with runtime_model_preamble's bash case statement and the JSON-append shell helper), ADO's unresolved-macro literal form $(VAR) is explicitly treated as "unset" rather than as attacker-controlled data, user engine.env/engine.args cannot smuggle a raw --model/COPILOT_MODEL/ADO_AW_MODEL_*_COPILOT override (all three paths are explicitly rejected with tests), and the Agent/Detection runtime model resolution each stay scoped to their own task's pipeline-variable environment (no write-capable token or cross-stage leakage). The new ado_aw_append_info_field bash helper is a plain JSON-object field append gated by a grep dup-check and an input-shape guard (case "$json" in \{*\}) ;; *) exit 1 ;;), and it only ever receives the compiler's own validated model string or detection-model string, not raw agent output.

One real regression found: STAGE_CANDIDATE_ARTIFACT_PAYLOAD (the candidate-artifact staging script used for the AWF binary and compiler, under supply-chain.pipeline-artifact) had set -eo pipefail weakened to set -o pipefail, which silently defeats its own "exactly one file"/checksum/provenance verification on failure paths — see inline comment. This is unrelated to the runtime-model feature itself and looks like an accidental drop during refactoring rather than an intentional change (the sibling EXTRACT_PACKAGE_PAYLOAD script keeps -e).

No other injection, token-scope, network-allowlist, or validation-weakening issues found in this diff; the rest of the changes (audit analyzers, detection_invocation_with_config rename, docs) are read-only/refactor-only and security-neutral.

🔒 Security review by PR Security Reviewer · auto · 175.6 AIC · ⌖ 2.29 AIC · ⊞ 11.3K
_{Comment /review to run again}

Superseded by updated review from same workflow.

[jamesadevine]

/review

[github-actions]

⚠️ Rust Code Quality Reviewer failed during the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[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]

Security review — no regressions found

Reviewed this diff for the specific question: does it weaken the existing security posture?

Summary: security-neutral to security-positive. No new injection paths, no token exposure, no weakened validation found. Several changes are net hardening:

• is_valid_command_path (src/validate.rs) — tightened, not loosened: now also rejects ./.. traversal segments, trailing/doubled slashes, and relative paths with slashes, in addition to the existing character allowlist and shell-metacharacter rejection. New tests cover the added cases.
• Model plumbing (src/engine.rs) — --model can no longer be smuggled through engine.args (validate_user_arg now explicitly rejects --model/--model=), and COPILOT_MODEL/ADO_AW_MODEL_*_COPILOT were added to BLOCKED_ENV_KEYS so users can't override the compiler-selected model via engine.env. All model values (explicit and runtime) still pass through validate_model_name's strict character allowlist before reaching the invocation document or aw_info.json.
• copilot-invoker.js (new Stage-1 harness) — parses a closed-schema JSON invocation document with explicit unknown-key rejection, NUL-byte checks, and dedicated path/command validators (isSafeCommand, isAbsoluteContainerPath) before spawning. It receives no ADO bearer (correctly excluded from token_source_for/apply_bundle_auth in ado_bundle.rs, consistent with other non-REST bundles). The invocation document itself is embedded via Binding::document, which uses a quoted heredoc (<<'DELIM') — content cannot trigger shell expansion.
• ado_aw_append_info_field shell helper — only writes values that already passed validate_model_name's allowlist (no ", \, or shell metacharacters), and detects/rejects malformed aw_info.json shapes before appending, so the printf-based JSON append can't be corrupted by the model string.
• ado-script bundle gating removal (ado_script.rs, exec_context/mod.rs, types.rs) — replaces ~12 independent *_active boolean flags (each needing to stay in lock-step with a separate "is the consuming step emitted" predicate, a drift footgun the file's own comments called out) with always-on bundle staging. This removes an entire class of potential "step present, bundle not downloaded" drift, which is a hardening simplification.
• CI (copilot-cli-safeoutputs.yml) — adds a build step for the new bundle and wires COPILOT_INVOKER_BUNDLE into the existing AWF+Copilot E2E smoke test, which now also asserts the round-tripped requested model. No scope/permission changes.

Nothing here reaches the Stage 1 agent with a write-capable token, widens the AWF network allowlist, or introduces a raw-String path/identifier field in place of a validated newtype. No findings to post inline.

🔒 Security review by PR Security Reviewer · auto · 119.7 AIC · ⌖ 1.83 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

Reviewed the new scripts/ado-script/src/copilot-invoker/ module (the sandboxed Copilot process harness) and the supporting test/package.json changes. The ts-critic sub-agent's findings all checked out against the source; combined with my own pass, five advisory items are posted inline — unfiltered env forwarding to the Copilot child, no watchdog timeout on the spawned process, a missing .catch on the top-level main() invocation, an unchecked process.stdout.write in the read-result path, and a missing NUL-byte guard on the prompt content (inconsistent with every other string field in this file).

None are merge-blocking: the document/result parsing is strict and well-tested, signal forwarding and exit-code mapping look correct, and writeResultAtomic is a sound atomic-write pattern. All five are hardening suggestions for a module that runs inside a sandbox holding credentials, so I'd still like them addressed before this becomes the standard invocation path for every Agent/Detection run.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 118.6 AIC · ⌖ 2.36 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Checked drift (bundle registration in ado_bundle.rs, AGENTS.md, docs/engine.md, docs/ado-script.md all correctly updated for the new copilot-invoker bundle), front-matter grammar (no new fields), and typed IR (no new step variants) — all in sync.

One new finding not already covered by prior review rounds: Engine::command()/Engine::args() are now unreachable from production code (only #[cfg(test)] callers remain) after the invocation_document_with_config refactor, but both keep #[allow(dead_code)] instead of being moved under #[cfg(test)] or removed alongside the already-flagged invocation_with_config. Everything else of substance (duplicated model-validation logic across engine.rs/ado_aw_marker.rs, Detection-side test gap, weak invocation-string test assertions) is already captured in earlier review rounds on this PR.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 114.1 AIC · ⌖ 14.7 AIC · ⊞ 12.4K
_{Comment /review to run again}

[github-actions]

Solid coverage overall for the new Copilot invoker contract (strict schema parsing, model-resolution precedence, argv/env construction, signal forwarding, atomic result writes) and for the Rust-side invocation-document plumbing (role-scoped env mapping, compile-time vs runtime model precedence across all four targets, is_valid_command_path edge cases).

Two gaps worth closing before merge, both scoped to main()'s read-result mode — the path every pipeline run actually exercises via node copilot-invoker.js read-result <result> <role>:

1. Untested branch: the role-mismatch guard and the malformed/missing-result catch block in main() have zero unit or smoke coverage — only the run subcommand is exercised.
2. Validator parity gap: the TS command field validation table doesn't mirror the edge cases (bin/copilot, /tmp//copilot, /tmp/copilot/) that the parallel Rust is_valid_command_path test was extended with in this same PR, risking silent divergence between the two independently-maintained validators.

Neither blocks merge on its own, but closing them would meaningfully reduce the risk of the read-result path shipping a regression that only surfaces in a live ADO run.

🧪 Test quality analysis by Test Quality Sentinel · auto · 232.5 AIC · ⌖ 1.82 AIC · ⊞ 11K
_{Comment /review to run again}

[github-actions]

Reviewed the test diff (copilot-invoker, engine.rs runtime-model plumbing, ado_aw_marker.rs shared append helper, audit analyzers, compiler_tests.rs). Most of the prior findings (gating asymmetry, contains-based assertions, triplicated validation logic, dead invocation_with_config) are already tracked in existing review threads. Two additional gaps worth closing:\n\n1. The new writeResultAtomic permissions test never asserts the file mode it claims to verify.\n2. The shared ado_aw_append_info_field fail-closed branch for malformed aw_info.json has zero coverage despite being exercised from two call sites (Agent + Detection).\n\nBoth are advisory — no weakened assertions or missing coverage for new production behavior beyond these two gaps.

🧪 Test quality analysis by Test Quality Sentinel · auto · 143.4 AIC · ⌖ 1.8 AIC · ⊞ 10.9K
_{Comment /review to run again}

[github-actions]

Compiler contract review

Reviewed drift (bundle registry, docs, codegen), the front-matter/IR/safe-output contracts, and the new copilot-invoker.js + Rust engine.rs runtime-model-selection surface.

What's solid

• Bundle::CopilotInvoker is correctly registered in ado_bundle.rs (enum variant, ALL, path, and the no-ADO-bearer match arm), and AGENTS.md / docs/ado-script.md / docs/engine.md / docs/audit.md are all updated in lockstep with the new bundle and runtime-model feature.
• The exec-context "will-activate" duplication removal (pr_contributor_will_activate & friends deleted from exec_context/mod.rs and extensions/mod.rs, AdoScriptExtension's dozen *_active flags collapsed) is clean — declarations() now unconditionally stages the bundle because every Agent/Detection job needs copilot-invoker.js, and tests were updated to match (declarations_agent_prepare_always_stages_bundle, etc.). No stray references to the removed functions/fields remain anywhere in src/.
• FrontMatter::has_azure_authenticated_mcp_servers removal is clean — no dangling callers.
• The TypeScript copilot-invoker/index.ts schema validation (DOCUMENT_KEYS/RESULT_KEYS closed sets, isSafeCommand/isAbsoluteContainerPath path hardening, MODEL_PATTERN) is well-covered by index.test.ts, and the Rust side's validate_model_name uses the same character class — this is correctly a single validator per language, not duplicated as an earlier bot comment on this PR had flagged.
• tests/compiler_tests.rs assertions were fully migrated from string-matching the old --prompt=... bash invocation to the new JSON invocation-document shape; azure-wif-isolation.test.ts and smoke.test.ts were updated with real invoker fixtures.
• src/audit/* additions (Detection aw_info.json enrichment, observed_model vs. requested model) are thoroughly tested, including the warning-on-malformed-artifact path.

Finding (posted inline)

• src/engine.rs:370 — Engine::args() is now dead in production (only reachable via a #[cfg(test)] helper in src/compile/common.rs), kept alive solely by #[allow(dead_code)]. This matches an earlier bot review comment on this PR (invocation_with_config is now unreadable dead code kept alive with #[allow(dead_code)]) that doesn't appear to have been resolved for this sibling method.

No merge-blocking documentation or codegen drift found; the one dead-code item is advisory (comment, not a drift bug), so I'm not requesting changes.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 178.9 AIC · ⌖ 1.93 AIC · ⊞ 12.4K
_{Comment /review to run again}

[github-actions]

TypeScript review — scripts/ado-script/ only

Reviewed the one new bundle in this PR, src/copilot-invoker/index.ts (the Copilot process harness), plus its test file, package.json build wiring, and the two touched test fixtures (azure-wif-isolation.test.ts, smoke.test.ts).

Overall quality is high. Input validation is thorough and defense-in-depth (NUL-byte checks, path traversal guards, strict regex allowlists on model/command fields), the invocation/result documents are versioned and closed-schema (rejects unknown keys), secrets are kept out of error messages (verified by the test asserting the error path doesn't leak the raw env value), and the atomic result-write (tmp + rename, mode: 0o600) is a good pattern for a sandboxed script with no debugger.

Two findings posted inline, both in the process-lifecycle code path (runInvocation):

1. The top-level main().then() omits a rejection handler — every other bundle in this workspace (azure-wif-refresh, prepare-pr-base, ado-proxy) wires one explicitly, and its absence here is the one inconsistency with established convention.
2. Signal forwarding to the child has no timeout/SIGKILL escalation, so an unresponsive child can hang the pipeline stage past the point a deliberate timeout would catch it.

Neither is merge-blocking on its own — the first has no live trigger yet, and the second only manifests on a wedged child process — but both are worth tightening before this ships as the sole Copilot launcher for every Agent/Detection job. ts-critic (sub-agent) independently surfaced the same two issues; no discrepancy to adjudicate.

No duplicate findings against pr-review-comments.json — the existing comments on this PR are all in src/engine.rs / ado_aw_marker.rs, outside this reviewer's remit.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 129.5 AIC · ⌖ 2.2 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Reviewed the Copilot-invoker refactor (strict invocation-document protocol, runtime model resolution plumbing) against the Stage 1/2/3 trust boundary and token-scope rules.

Found one critical regression unrelated to the invoker feature itself: STAGE_CANDIDATE_ARTIFACT_PAYLOAD (the compiler supply-chain checksum/provenance verifier) had set -eo pipefail weakened to set -o pipefail, so a failed sha256sum -c - no longer halts staging before the binary is moved into place and made executable — see inline comment. This looks like it may have been accidentally swapped with the opposite (correct) tightening applied to RUN_AGENT in the same diff.

Everything else reviewed (invocation-document JSON schema + strict TS-side validation of command/paths/model characters, CopilotInvoker bundle correctly excluded from the ADO-bearer projection set, model-name/arg validation, --model conflict rejection) looks sound and doesn't weaken the Stage 1/2/3 boundary.

🔒 Security review by PR Security Reviewer · auto · 184 AIC · ⌖ 2.16 AIC · ⊞ 11.3K
_{Comment /review to run again}

[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]

Security review — no regressions found

Reviewed the diff against the Stage 1/2/3 trust boundary and the compiler's injection-safety invariants (generated bash:, token projection, validated newtypes, network allowlist, safe-output integrity).

Summary of what changed: adds runtime Copilot model overrides (ADO_AW_MODEL_*_COPILOT pipeline variables) and replaces the generated single-quoted Copilot command string with a versioned copilot-invoker.js process harness that receives a compiler-written JSON invocation document and resolves the model at runtime inside AWF.

What I checked and found sound:

• src/compile/ado_bundle.rs: the new Bundle::CopilotInvoker is correctly placed in the no-ADO-bearer group — it talks to neither ADO nor GitHub REST, consistent with its job.
• is_valid_command_path (src/validate.rs) is strengthened, not weakened: it now also rejects .., empty/. segments, double slashes, and trailing slashes, in addition to the existing char allowlist.
• Model-name validation (validate_model_name) is applied consistently on both the Rust compile side and mirrored in TypeScript (MODEL_PATTERN in copilot-invoker/index.ts), and runtime values read from ADO_AW_MODEL_*_COPILOT/ADO_AW_DEFAULT_MODEL_COPILOT env vars are validated before being written into COPILOT_MODEL for the child process — an attacker-controlled pipeline variable with unexpected characters is rejected rather than passed through.
• The new ADO_AW_MODEL_*_COPILOT/COPILOT_MODEL keys were added to BLOCKED_ENV_KEYS, and validate_user_arg now rejects user-supplied --model/--model= in engine.args, closing a path that could otherwise race the compiler-controlled model selection.
• The invocation document is passed into the generated Bash via Binding::document, which uses a quoted, collision-guarded heredoc ($(cat <<'DELIM' ... DELIM)), so no shell expansion of its JSON content occurs. The document's own fields (command, prompt_path, mcp_config_path, result_path, explicit_model) are produced by compiler code already subject to is_valid_command_path/validate_model_name, and the TS side independently re-validates every field (isSafeCommand, isAbsoluteContainerPath, MODEL_PATTERN, NUL-byte checks, unknown-key rejection) before spawning — defense in depth across the trust boundary.
• copilot-invoker.js spawns with stdio: "inherit" and an explicit env copy, deleting COPILOT_MODEL when no model was resolved rather than leaving a stale value; result file writes are atomic (tmp + rename) with 0o600 mode.
• The ado_aw_append_info_field shared bash phase (writing model/detection_model into aw_info.json) only runs when the model value is non-empty and the target isn't already a key, with accompanying tests guarding against false "key already present" matches on substrings (e.g. "agent_name":"model").
• Audit (src/audit/*) and ado_script.rs changes are read-only reporting/plumbing simplifications (always-stage-bundle instead of per-feature gating) with no change to trust/token scope.

I found no injection paths, no token-scope widening, no weakened path/identifier validation, and no new untrusted-content-to-prompt routing in this diff. This is advisory-only; no blocking issues identified.

🔒 Security review by PR Security Reviewer · auto · 93.9 AIC · ⌖ 1.87 AIC · ⊞ 11.4K
_{Comment /review to run again}

Superseded by updated review from same workflow.

[github-actions]

Compiler Contract Review

Checked PR #2246 (runtime model controls + copilot-invoker) against every contract I own: codegen drift, lock drift, smoke-source drift, front-matter/safe-output schema rules, typed IR, generated-shell discipline, and docs sync.

No new findings. This PR is unusually clean on contract hygiene:

• ado_bundle.rs registers Bundle::CopilotInvoker with the correct no-ADO-bearer auth classification.
• package.json wires build:copilot-invoker into build, clean, and test:smoke consistently.
• .gitignore covers the new copilot-invoker.js generated bundle.
• docs/ado-script.md, docs/engine.md, docs/audit.md, and AGENTS.md were all updated in lockstep with the Rust/TS changes (new bundle entry, Agent/Detection invocation protocol, runtime-model precedence table, audit observed_model semantics).
• The model-name character class (^[A-Za-z0-9._:-]+$ in src/validate.rs::validate_model_name / TS MODEL_PATTERN) and the command-path safety rules (is_valid_command_path / isSafeCommand+hasSafePathSegments) are kept byte-for-byte identical between the Rust compiler and the TypeScript invoker — this is exactly the kind of two-copies-of-a-contract drift risk this review focuses on, and it's correctly guarded by mirrored unit tests on both sides.
• The invocation document is delivered via Binding::document, so no interpolation path bypasses the typed shell-binding chokepoint.
• The previously-flagged shell-side duplicate of the model-resolution/validation algorithm (ado_aw_runtime_model in ado_aw_marker.rs, raised in earlier review rounds on this PR) has been removed entirely in the version I reviewed — resolution now lives solely in the Rust CopilotInvocationDocument path and the TS invoker, closing that drift surface.

I did not re-flag the dead-code (invocation_with_config) or test-coverage items already raised by github-actions[bot] on src/engine.rs lines 467/1007/1591 and ado_aw_marker.rs line 66 — those are still open in pr-review-comments.json and remain valid.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 105.9 AIC · ⌖ 2.41 AIC · ⊞ 12.4K
_{Comment /review to run again}

[github-actions]

Reviewed the Rust portion of the diff (src/audit/, src/compile/, src/engine.rs, src/validate.rs, tests/compiler_tests.rs) for panics, lossy conversions, missing error context, unwrap/expect on user-reachable paths, boundary bugs, non-deterministic iteration, tokio concurrency issues, and duplicated logic on changed lines only.

No new merge-blocking defects found beyond the five already-raised items (engine.rs:1469 string style, engine.rs:1016 Detection-side gating test gap, engine.rs:1591 .contains() assertions, ado_aw_marker.rs:66 duplicate precedence logic, engine.rs:467-468 dead invocation_with_config). Verified cargo build succeeds and cargo test --bin ado-aw engine:: passes (91/91). No dangling references to removed helpers; is_valid_command_path/validate_model_name/copilot-invoker.js validation paths are consistent and the new JSON-document invocation handoff avoids the old shell-string-embedding injection surface.

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

[github-actions]

Reviewed the new scripts/ado-script/src/copilot-invoker/index.ts bundle (the only in-scope TypeScript change — everything else in scripts/ado-script/ was test-only tweaks). Overall this is careful, well-tested code: strict schema validation with unknown-field rejection, NUL-byte checks, path-traversal guards, atomic result writes with 0600 perms, signal forwarding with proper listener cleanup, and a model-value error path that's explicitly tested to never leak the rejected value into logs.

No merge-blocking defects — all 4 findings below are advisory hardening/polish suggestions:

• isSafeCommand validates path shape but not that the binary is actually copilot (defense-in-depth gap, mirrors existing Rust-side validation)
• Two spots where JSON.parse failures discard the underlying SyntaxError, slightly hurting triage
• writeResultAtomic doesn't clean up its temp file if renameSync fails
• No test exercises the top-level main().catch safety net

ts-critic (sub-agent) findings were incorporated above; a couple of its lower-confidence items (non-null assertions on already-length-checked argv[1], unknown variable shadowing) were triaged out as non-issues given the existing invariants.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 111.4 AIC · ⌖ 7.06 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Overall the test suite for this PR is strong — the copilot-invoker TypeScript module, engine.rs model precedence, and the invoker bundle-placement compiler tests are thorough and behavior-focused (not implementation-mirroring). I found four gaps, all advisory (non-blocking):

1. The "writes results atomically with private permissions" test never actually asserts the file permissions it's named after.
2. The new MODEL_RESULT_STATUS != 0 error path in both the Agent and Detection run_agent_step/run_threat_analysis_step bash (invoker result missing/malformed) is untested — this is the one place a regression could silently mask a real agent failure.
3. mcp_config_path is missing a path-traversal malformed-input case, unlike its sibling fields prompt_path/result_path.
4. RECORD_DETECTION_RUNTIME_MODEL's "could not find copied aw_info.json" failure branch has no corresponding test.

None of these touch already-covered happy paths, and nothing in the diff weakens or removes an existing assertion.

🧪 Test quality analysis by Test Quality Sentinel · auto · 231.4 AIC · ⌖ 2.1 AIC · ⊞ 10.9K
_{Comment /review to run again}

[github-actions]

Reviewed the Rust-only portion of this PR (engine.rs, agentic_pipeline.rs, ado_aw_marker.rs, ado_script.rs, exec_context, audit analyzers, validate.rs). Overall solid: error handling uses anyhow with context throughout, the new copilot-invoker JSON-document protocol replaces fragile string concatenation with typed structs, and test coverage for the new runtime-model precedence paths is thorough. rust-critic sub-agent ran in parallel and its findings were incorporated.

Themes

• Shell error-handling inconsistency (medium): RUN_AGENT's new set +e after set -eo pipefail is never re-enabled, silently swallowing failures in the new metadata-append path; the sibling RUN_THREAT_ANALYSIS script has different, undocumented failure semantics for the same sequence — flagged inline.
• Shared JSON-append helper has no value escaping (low): ado_aw_append_info_field is correct today only because callers are constrained by validate_model_name's charset; the helper itself has no defensive escaping — flagged inline.
• No panics/unwraps on user-reachable paths, no lossy casts, no determinism regressions spotted in the HashMap/sorted-pairs code (copilot_detection_env sorts before returning). The always-on Agent bundle staging simplification (removing a dozen per-feature *_active flags) is a clean reduction in duplicated activation-predicate logic.

Neither inline finding is merge-blocking given the current charset constraints, so this is a COMMENT rather than REQUEST_CHANGES.

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