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 · ◷