feat(compile): adopt AWF strict network isolation

[jamesadevine]

Summary

• migrate generated Agent and Detection jobs to rootless AWF strict network isolation
• attach bridge-mode awmg-mcpg to the Agent topology while keeping host SafeOutputs bridge-scoped and authenticated
• adopt AWF's always-on API proxy and explicit versioned image selection, including internal registry mirrors
• replace the compiler-derived live contract with a handwritten AWF + MCPG + Copilot + SafeOutputs workflow
• update compiler coverage, committed pipeline locks, and network architecture documentation

Test plan

• cargo test
• cargo clippy --all-targets --all-features -- -D warnings
• cargo test --test bash_lint_tests
• shellcheck tests/awf-copilot-safeoutputs/run.sh
• validate .github/workflows/copilot-cli-safeoutputs.yml as YAML
• run the live handwritten contract through this PR's Copilot CLI SafeOutputs Contract workflow

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured security improvement with clean separation of concerns. One shell-injection concern worth verifying, a few minor observations.

Findings

🔒 Security Concerns

• src/compile/agentic_pipeline.rs — awf_image_flags: registry.name is interpolated into a bash script via format!(" --image-registry \"{}\" \\\n", registry.name.as_str()). If this field accepts arbitrary user input without a strict character allowlist, a value like foo" --some-flag " would break out of the shell quoting. Please confirm registry.name is typed with a validated newtype from src/secure.rs (e.g. StrictRelativePath or similar) that rejects shell-special characters. If not, this should go through src/validate.rs before interpolation.

⚠️ Suggestions

• tests/awf-copilot-safeoutputs/run.sh:203 — AWF is invoked with --env-all and no --exclude-env flags. In production pipelines, BYOM/BYOK credential keys are explicitly excluded; here COPILOT_GITHUB_TOKEN and the full environment flow into the agent sandbox. This is intentional (the agent needs the token), but a brief comment explaining the deliberate difference from production compiled pipelines would help future readers understand it's not an oversight.

• src/engine.rs — copilot_byom_active demoted to #[cfg(test)]: The function is now test-only. The comment says "AWF's API proxy is always enabled; production compilation only needs the exact credential-key list." The production caller path (via awf_exclude_env_flags + copilot_byom_credential_keys) is clear, but the old public predicate was also used in at least one other callsite for the pre-pull decision. Worth a quick grep to confirm no call site was silently dropped rather than migrated.

✅ What Looks Good

• Security improvement is solid: Removing host.docker.internal from the agent-accessible allowlist (generate_allowed_domains) and the corresponding test inversion in standalone.rs is exactly right under the strict topology model. The separation into MCPG_DOMAIN (agent-facing awmg-mcpg) vs MCPG_HOST_DOMAIN (host.docker.internal, MCPG-to-host only) is clean and well-named.
• --bind-address flag defaults to 127.0.0.1: backward-compatible, and std::net::IpAddr parsing gives free input validation before it reaches SocketAddr::new.
• Shellcheck disable comments are well-targeted with explanations (# ADO macros are substituted before bash, # AWF expands the engine command inside the sandbox).
• Replacing copilot_cli_safeoutputs_tests.rs with run.sh: The Rust test couldn't exercise AWF integration; the shell script tests the actual end-to-end path including topology attachment. Better signal.
• awf_image_flags / awf_exclude_env_flags helpers: factoring these out from run_agent_step and run_threat_analysis_step removes the duplication that existed before and makes the AWF flag contract easy to see and audit in one place.
• Port change 80 → 8080 and constant rename MCPG_PORT/MCPG_DOMAIN are consistently propagated through constants, tests, fixture lock files, and docs.

Generated by Rust PR Reviewer for #1534 · 50.2 AIC · ⌖ 5.78 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-structured security upgrade with solid test coverage. A couple of observations worth calling out.

Findings

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — awf_image_flags() trailing indentation coupling: The function ends with block.push_str(" ") to act as a 2-space indent prefix for the next --flag in the caller format string. This works, but it is a hidden coupling between the helper and the exact Rust line-continuation layout at the call sites. If a future caller omits the \ continuation or adds extra indentation, the generated bash gets double-indented flags silently. A small comment in the function (or even just in the format string) would make the contract explicit. Cosmetic, not a correctness issue.

• tests/mcp_http_tests.rs:test_explicit_bind_address_is_used — 127.0.0.2 assumption: The test binds the server to 127.0.0.2. On Linux the whole 127.0.0.0/8 range is aliased by the loopback interface so this works in practice, but on some stripped-down CI environments the alias may be absent. A one-line comment noting the Linux loopback range assumption would prevent future readers treating failures as flaky.

• src/compile/agentic_pipeline.rs — Detection job has --network-isolation but no --topology-attach: Intentional (threat analysis doesn't need MCP tools), but without a comment it looks like an oversight next to the Agent invocation which has both. A short inline note such as # no --topology-attach: detection run needs domain allowlist only, not MCP tools would clarify the deliberate omission.

✅ What Looks Good

• Removing sudo -E from both AWF invocations and dropping --enable-host-access is a clear security hardening win; the agent sandbox now has no route to the host.
• The SAFE_OUTPUTS_BIND_ADDRESS resolution (docker network inspect bridge | jq -er ... // empty) is robustly guarded: jq -e + // empty exits non-zero when the gateway is absent, and the if ! block emits an actionable ##vso[task.logissue] error before exiting.
• copilot_byom_active correctly narrowed to #[cfg(test)] now that the byom_active flag is removed from StandaloneCtx.
• The new test_docker_host_alias_is_allowed_for_bridge_bind test directly exercises the rmcp allowed_hosts whitelist without requiring a live Docker environment — good unit-level coverage of the DNS-rebinding protection.
• Removing the :latest retag dance for squid/agent images in favour of passing --image-tag explicitly to AWF is cleaner and avoids the implicit contract that required those local alias tags to match AWF's internal resolution.

Generated by Rust PR Reviewer for #1534 · 64.7 AIC · ⌖ 5.78 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — well-structured security improvement with clean code changes; a few minor points worth verifying.

Findings

✅ What Looks Good

• Rootless AWF (sudo -E dropped, --network-isolation + --topology-attach replace --enable-host-access) is a meaningful security hardening — the agent process no longer runs as root.
• host.docker.internal removed from agent allowlist (generate_allowed_domains, CORE_ALLOWED_HOSTS) — the sandboxed agent can no longer reach host-side services, which was the main attack surface concern.
• set -euo pipefail added to start_safeoutputs_server_step — previously the script had no failure guard; the -u flag also turns an unset SAFE_OUTPUTS_BIND_ADDRESS into a hard error.
• Error handling for bridge gateway detection is correct: jq -er '...[0].Gateway // empty' with the if ! guard produces a clear ##vso[task.logissue] error message and exit 1 rather than a silent misconfiguration.
• copilot_byom_active demoted to #[cfg(test)] correctly — it's no longer needed in production now that AWF always enables its API proxy.
• :latest aliasing removed from prepull_images_step in favour of --image-tag — the old :latest dance was a fragile workaround that broke when --skip-pull was passed; using explicit version tags is strictly better.
• Test updated in standalone.rs to assert host.docker.internal is not in the allowlist — the regression coverage is in the right place.

⚠️ Suggestions

• src/compile/agentic_pipeline.rs — jq availability assumed: start_safeoutputs_server_step now depends on jq to resolve the bridge gateway. ADO hosted agents have it, but if a consumer uses a custom pool image without jq, the step will fail with jq: command not found (the 2>/dev/null on the docker network inspect pipe would swallow it, making the error message slightly misleading — jq not found would cause the condition to trigger). Low risk in practice, but worth a comment or a command -v jq >/dev/null || { echo "jq required"; exit 1; } guard.

• src/compile/agentic_pipeline.rs:awf_image_flags — trailing " " indent is fragile: The function ends with block.push_str(" "); to inject 2-space indentation for the --skip-pull line that immediately follows in the format string. This coupling between the function's output and the call site's template is easy to break silently if either side changes. A brief comment (// caller expects 2-space indent for the following flag) or encoding it in the return type would help.

• src/compile/agentic_pipeline.rs:run_threat_analysis_step — --topology-attach intentionally absent: This is correct (detection has no MCPG), but it's the only AWF invocation with --network-isolation and no topology attachment. A one-line comment (// detection does not need MCPG; no --topology-attach) would prevent a future reader from adding it by mistake.

• src/mcp.rs:run_http — allowed_hosts mutation after default construction: StreamableHttpServerConfig::default() then config.allowed_hosts.push(...) works, but if the default config ever initialises allowed_hosts to a non-empty list that already includes "host.docker.internal", the push would duplicate it. Minor, but worth using .contains() before pushing or constructing the list explicitly.

Generated by Rust PR Reviewer for #1534 · 39.2 AIC · ⌖ 5.88 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Well-structured security improvement — the host-network isolation removal is a meaningful hardening, and the implementation is largely clean. Two items warrant attention before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/agentic_pipeline.rs — awf_image_flags trailing-space contract The function always appends " " (two bare spaces) as its last push to serve as the indentation prefix for whatever flag follows it in the format string. This implicit contract isn't documented and is fragile — if a future caller wraps the interpolation differently (e.g. adds a newline before {image_flags_block} in the format template), the generated bash will have malformed indentation that silently produces a broken AWF invocation. Consider returning the block without the trailing spaces and letting the format string supply the leading indent:

  // in format template:
  "  {image_flags_block}\n  --skip-pull \\\n"

• src/compile/agentic_pipeline.rs — run_threat_analysis_step format template alignment The detection AWF invocation uses {exclude_env_block} \ where the trailing \ was the old indentation left over from {api_proxy_block} \. With the new function awf_exclude_env_flags returning an empty string for non-BYOM runs, the two trailing spaces become a bare \ continuation that's harmless in bash but visually inconsistent with the rest of the command block.

⚠️ Suggestions

• src/engine.rs — implicit AWF version coupling for BYOM isolation awf_exclude_env_flags no longer emits --enable-api-proxy, relying instead on AWF 0.27.32+ always enabling its API proxy unconditionally. The comment documents this, but there's no guard. If AWF_VERSION is ever rolled back (e.g. for a hotfix) or a supply-chain: config points to an older pinned AWF, BYOM/BYOK credentials will be passed directly into the agent sandbox without proxy interception — a silent security regression with no compile-time or runtime warning. A // INVARIANT: AWF_VERSION must be >= "0.27.32" for API-proxy-always-on comment adjacent to the constant would make the constraint auditable, and a debug_assert!(AWF_VERSION >= "0.27.32", ...) in awf_exclude_env_flags would catch the regression in tests.

• src/mcp.rs — 0.0.0.0 in allowed_hosts When bind_address is 0.0.0.0 (the bridge-binding case), config.allowed_hosts.push("0.0.0.0") adds a wildcard-looking entry for rmcp's DNS-rebinding check. This has no practical effect (Host: 0.0.0.0 would be unusual), but it's noise alongside the intentional host.docker.internal entry. Worth either not adding the 0.0.0.0 entry, or adding a comment that it's included for completeness.

• tests/copilot_cli_safeoutputs_tests.rs deleted (444 lines) The live contract workflow replacement is a meaningful improvement for real-world validation, but the deleted file also covered a handful of unit-level behaviors (key generation, auth rejection, noop round-trip) that now only run as live E2E. The new mcp_http_tests.rs entries cover the --bind-address surface well — it's worth confirming the auth rejection and tool-filter tests are captured either there or in the handwritten contract.

✅ What Looks Good

• The host.docker.internal removal from generate_allowed_domains is correct and is validated by the test loop checking that no --allow-domains line contains it.
• start_safeoutputs_server_step now uses set -euo pipefail and emits a structured ADO error (##vso[task.logissue type=error]) before exiting on bridge gateway failure — clean fail-fast pattern.
• copilot_byom_active correctly scoped to #[cfg(test)] since production paths only need the key list.
• The NO_PROXY/no_proxy synchronization for the topology-attached MCPG container name is a correct and well-commented solution for Node HTTP clients that respect only lowercase no_proxy.
• Container name hardened from the hardcoded string "mcpg" to the constant MCPG_CONTAINER_NAME throughout — good consistency.

Generated by Rust PR Reviewer for #1534 · 63.8 AIC · ⌖ 5.95 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid architectural migration — moves SafeOutputs from a host-side HTTP server to a hardened stdio container, drops --enable-host-access/host.docker.internal, and adopts AWF strict network isolation. No critical bugs found.

Findings

✅ What Looks Good

• Hardened SafeOutputs container (--network none, --cap-drop ALL, --no-new-privileges, --read-only, --tmpfs /tmp, --pids-limit 256) is excellent defence-in-depth for the MCP write-intent collector.
• ${MCP_RUNNER_UID}:${MCP_RUNNER_GID} substitution is correct: the Rust \\${{...}} escaping emits \${...} into the shell script, which sed treats as a literal pattern and replaces with the expanded $VAR value at runtime. No off-by-one in the escaping.
• host.docker.internal removal from generate_allowed_domains is correct — the agent no longer needs a route to the host, closing an exfiltration path.
• sudo -E removal aligns with rootless AWF; no privilege escalation needed or desirable.
• copilot_byom_active scoped to #[cfg(test)] — clean way to retire a now-dead production predicate while keeping its test coverage.
• image_ref moved to common.rs eliminates the drift risk between the pre-pull step and the docker run invocation.
• NO_PROXY injection in routed_engine_run (${NO_PROXY:+$NO_PROXY,}awmg-mcpg) correctly bypasses proxy for the MCPG container name without clobbering an existing NO_PROXY.

⚠️ Suggestions

• src/compile/common.rs — generate_mcpg_config calls compute_effective_workspace independently: build_pipeline_context already computes the working directory and stores it in StandaloneCtx. generate_mcpg_config re-derives it from FrontMatter with a separate compute_effective_workspace call. The two paths should always agree, but if they ever diverge (e.g. a future refactor changes one), the SafeOutputs container would silently get the wrong -w and bind-mount path while the AWF command uses a different one. Consider threading the pre-computed working_directory string into generate_mcpg_config as a parameter, or at minimum add an assertion in tests that the two values match.

• src/compile/agentic_pipeline.rs — prepull_images_step always pulls api-proxy: the old docker tag ... :latest aliases for squid and agent were removed, and --image-tag is now passed to AWF. Confirm that AWF 0.27.32 with --skip-pull --image-tag <ver> correctly locates the versioned images rather than falling back to :latest. If AWF still resolves :latest internally for any sub-image, the missing tag alias would cause a silent cache miss on air-gapped pools (supply-chain mirror path).

• tests/copilot_cli_safeoutputs_tests.rs deleted: the in-process contract tests are replaced by a live ADO workflow. That trade-off is reasonable for the architectural change, but the new test_generate_mcpg_config_safeoutputs_is_hardened_stdio_container unit test in common.rs is a good partial replacement that comprehensively covers the new container shape.

Generated by Rust PR Reviewer for #1534 · 57.2 AIC · ⌖ 5.88 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean, well-scoped migration with a real security improvement. One functional bug worth addressing before merge, plus a few minor observations.

Findings

🐛 Bugs / Logic Issues

• src/mcp.rs:1579 — allowed_hosts broken for non-loopback bind addresses

  config.allowed_hosts.push(bind_address.to_string());

  StreamableHttpServerConfig::allowed_hosts is a DNS-rebinding guard that validates the HTTP Host: header sent by clients. When --bind-address 0.0.0.0 is passed (expected usage inside a container), pushing "0.0.0.0" to the list is semantically wrong — no client will ever send Host: 0.0.0.0. Connections to the container's actual IP or hostname will be rejected with a 421/403 from the rebinding check.

  For the default 127.0.0.1 case this works correctly. For 0.0.0.0 you'd want either a wildcard or no host restriction at all (since the network binding itself already scopes access). Suggested fix:

  // Only add a Host restriction when binding to a specific address.
  // Binding to 0.0.0.0 / :: means "all interfaces"; the caller controls
  // access via the network layer, so skip the rebinding guard.
  if !bind_address.is_unspecified() {
      config.allowed_hosts.push(bind_address.to_string());
  }

⚠️ Suggestions

• src/compile/common.rs — MCPG_CONTAINER_NAME and MCPG_DOMAIN are identical aliases

  pub const MCPG_DOMAIN: &str = "awmg-mcpg";
  pub const MCPG_CONTAINER_NAME: &str = MCPG_DOMAIN;

  Two names for the same value at the same scope adds cognitive load. The split is conceptually clean (DNS name vs Docker --name arg), but since AWF's topology model deliberately makes these identical, a single constant with a comment ("container name == DNS name within awf-net") would read better. Not a bug — just a minor clarity point.

• src/compile/agentic_pipeline.rs — api-proxy unconditionally pre-pulled

  The old code conditionally pulled api-proxy only when byom_active. Now it's always pulled, which adds an unconditional image pull to every pipeline run (including non-BYOM). The commit message says AWF 0.27.32+ always enables the proxy, so the pull is correct — but a comment to that effect in prepull_images_step (matching the docstring on awf_exclude_env_flags) would make future reviewers' lives easier.

✅ What Looks Good

• The rootless migration is correct and coherent: sudo -E dropped, --enable-host-access removed, MCPG switched from --network host to bridge with loopback port-binding, SafeOutputs hardened to --network none --cap-drop ALL --read-only --no-new-privileges.
• image_ref correctly promoted from agentic_pipeline.rs private fn to common::pub(crate) — single source of truth for image reference rewriting.
• copilot_byom_active correctly demoted to #[cfg(test)] since production paths only need copilot_byom_credential_keys now.
• The NO_PROXY/no_proxy injection in routed_engine_run is the right approach to bypass Squid for the trusted MCPG container name (awmg-mcpg does not need to traverse the AWF L7 filter).
• Detection job intentionally omits --topology-attach — correct, detection has no MCPG dependency.
• Test updates are thorough: inverted host.docker.internal assertion, new SafeOutputs hardening checks, allowed_hosts assertion for MCPG port/domain constants.

Generated by Rust PR Reviewer for #1534 · 78.3 AIC · ⌖ 5.92 AIC · ⊞ 6.2K · ◷

[jamesadevine]

Merged current main into this branch twice; final head is 10e1fb5. Validation summary:\n\n- GitHub Build & Test / drift / contract: passed on the previous merged head; refreshed checks are running.\n- Azure-native ado-script E2E: build 625553 passed.\n- Executor E2E: build 625558 passed.\n- Strict-network agentic runtime (temporary validation branch differing only by removal of the release-compiler integrity step): canary 625566, Azure CLI 625567, noop 625568, janitor 625569 all passed.\n- Local merged-tree validation: 1,313 compile tests passed (1 ignored), 185 compiler integration tests passed, bash lint passed.\n\nThe automatic noop PR check fails only at Verify pipeline integrity: it downloads released ado-aw v0.45.1, which cannot reproduce an unreleased compiler PR's generated lock. The equivalent runtime pipeline passed when that single step was omitted. A manual-run-only integrity override is preserved as a follow-up prototype, not included in this PR.

[github-actions]

🔍 Rust PR Review

Summary: Solid security improvement — looks good with a couple of minor observations worth noting before merge.

Findings

⚠️ Suggestions

• src/mcp.rs:1574 — allowed_hosts push is incomplete for non-loopback bind addresses

  config.allowed_hosts.push(bind_address.to_string());

  When bind_address is 0.0.0.0, the pushed entry "0.0.0.0" won't match the Host header that HTTP clients send (e.g. 127.0.0.1:8100 or a machine hostname). The default 127.0.0.1 case works correctly, but if someone runs mcp-http --bind-address 0.0.0.0 for intra-container use, connections may be rejected by the StreamableHttpServerConfig host-header check. Consider either also pushing "localhost" / "127.0.0.1" when binding to 0.0.0.0, or documenting this limitation.

• src/compile/common.rs — MCPG_CONTAINER_NAME is an alias for MCPG_DOMAIN with no semantic distinction

  pub const MCPG_DOMAIN: &str = "awmg-mcpg";
  pub const MCPG_CONTAINER_NAME: &str = MCPG_DOMAIN;

  Both constants hold the same value and are used interchangeably at call sites (--topology-attach, stop_mcpg_step, NO_PROXY). If there is a future scenario where they could diverge, the split makes sense — but if not, a single constant with a clear doc-comment would reduce confusion. Minor nit; not worth blocking on.

• src/compile/common.rs:generate_mcpg_config — generate_enabled_tools_args output is split on whitespace

  generate_enabled_tools_args(front_matter)
      .split_whitespace()
      .map(str::to_string)

  This works correctly for the current implementation (flag + space-separated tool names with no embedded spaces). If generate_enabled_tools_args ever emits quoted or multi-word tokens this would silently mis-parse. Fragility is low given the controlled function, but the coupling is worth a brief comment.

✅ What Looks Good

• Security posture: Removing host.docker.internal from the allowed-domain list, running SafeOutputs with --network none --cap-drop ALL --read-only --security-opt no-new-privileges, and dropping sudo + --enable-host-access from AWF are all meaningful, concrete threat-surface reductions. The strict topology model is clearly the right direction.
• copilot_byom_active scoped to #[cfg(test)]: Clean removal of a dead production code path.
• image_ref moved to common.rs: Correct deduplication — the function was already documented as a single chokepoint; it now actually is one.
• NO_PROXY/no_proxy dual-export: Correctly handles both POSIX and Node/curl conventions so both uppercase and lowercase env checks work inside the AWF sandbox.
• Test coverage: The updated fixture locks, the new copilot_cli_safeoutputs_tests.rs, and the hardened run.sh contract test together give good regression coverage for the topology change.
• mcp-http --bind-address default 127.0.0.1: Tightening the bind from implicit 0.0.0.0 to an explicit loopback default is a good defence-in-depth improvement for local use.

Generated by Rust PR Reviewer for #1534 · 63.3 AIC · ⌖ 5.8 AIC · ⊞ 6.2K · ◷