fix: align tool allow lists with gh-aw

[jamesadevine]

Summary

Aligns the Copilot CLI tool allow list generation (generate_copilot_params()) with gh-aw's implementation.

Changes

Default behavior → --allow-all-tools

gh-aw's applyDefaultTools() sets bash: ["*"] when sandbox is enabled and bash isn't specified. Since ado-aw agents always run in AWF sandbox, our default should match. Previously we defaulted to 12 specific shell commands (cat, date, echo, ...); now we default to --allow-all-tools.

--allow-all-tools for wildcard bash

When tools.bash is set to [":*"] or ["*"], emit a single --allow-all-tools flag and drop ALL individual --allow-tool flags. This matches gh-aw's computeCopilotToolArguments() which returns early with just ["--allow-all-tools"].

--allow-all-paths when edit is enabled

When the edit tool is enabled (default), emit --allow-all-paths to allow the agent to write to any file path. This matches gh-aw's GetExecutionSteps().

"*" wildcard support

Accept both "*" and ":*" as bash wildcards, matching gh-aw's dual-check pattern.

Before / After

Config	Before	After
No tools: (default)	--allow-tool github --allow-tool safeoutputs --allow-tool write --allow-tool "shell(cat)" ...	--allow-all-tools --allow-all-paths
bash: [":*"]	--allow-tool github --allow-tool safeoutputs --allow-tool write --allow-tool "shell(:*)"	--allow-all-tools --allow-all-paths
bash: ["cat", "ls"]	--allow-tool github ... --allow-tool "shell(cat)" --allow-tool "shell(ls)"	--allow-tool github ... --allow-tool "shell(cat)" --allow-tool "shell(ls)" --allow-all-paths
edit: false	No --allow-tool write	No --allow-tool write, no --allow-all-paths

Testing

• All 919 tests pass (840 unit + 68 integration + 3 init + 8 MCP HTTP)
• Updated 4 existing tests, added 5 new tests
• Verified compiled output with tools: azure-devops and restricted bash configs

[github-actions]

🔍 Rust PR Review

Summary: Looks good — the logic is correct and well-structured. One subtle behavioral note worth double-checking, and one minor inconsistency in the restricted-bash path.

Findings

⚠️ Suggestions

• src/compile/common.rs:480 — When use_allow_all_tools = false and edit_enabled = true, both --allow-tool write AND --allow-all-paths are emitted. This is probably fine (they govern different dimensions: tool identity vs. path scope), but it's a slight divergence from the --allow-all-tools path where only --allow-all-paths is emitted. Worth a comment confirming this is intentional, especially since the before/after table in the PR description doesn't explicitly show --allow-tool write being present alongside --allow-all-paths in the restricted-bash case.

• src/compile/common.rs:478 — The None arm in the inner bash_commands match is logically unreachable (correctly noted in the comment), but Rust's exhaustiveness checker can't verify this invariant. A cleaner pattern would be unreachable!("bash=None implies use_allow_all_tools=true") instead of vec![], which would panic loudly in debug builds if the invariant is ever broken rather than silently generating an empty allow list. Either approach is defensible — just noting it.

• src/compile/common.rs:464 — The wildcard detection only fires when cmds.len() == 1. A user writing bash: [":*", "cat"] would get shell(:*) emitted literally as an --allow-tool arg instead of --allow-all-tools. This was also the case in the old code, so it's not a regression — just worth a comment warning that mixing wildcards and commands is not supported.

✅ What Looks Good

• The restructuring is clean: collecting extensions/MCPs only in the !use_allow_all_tools branch is logically correct and avoids unnecessary work.
• The single-quote injection check is correctly preserved inside if !use_allow_all_tools — no injection risk is introduced by the new wildcard path.
• Tests are thorough: the added tests cover default, ":*", "*", edit-disabled, and the interaction between wildcards and --allow-all-paths. Updating the MCP-sorting/container tests to explicitly set bash is the right call.
• The test_lean_runtime_compiled_output integration test correctly pivots from asserting shell(lean) is present to asserting --allow-all-tools is present — the behavior change is accurately reflected.
• The security trade-off (default → unrestricted) is intentional and well-motivated by the AWF sandbox always being present. The AWF is the primary isolation boundary, and matching gh-aw's applyDefaultTools() behavior reduces per-ecosystem drift.
• AGENTS.md and the {{ copilot_params }} section are kept in sync with the implementation.

Generated by Rust PR Reviewer for issue #279 · ● 978.3K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Logic is correct and well-structured; one stale documentation section and one weak test assertion worth noting.

Findings

⚠️ Suggestions

• AGENTS.md line 637 — --allow-all-tools description is incomplete

  The {{ copilot_params }} section still says:

  --allow-all-tools - When bash has wildcard (":*" or "*"), allows all tools...

  But after this PR, --allow-all-tools is also emitted when bash is omitted entirely (the new default). The line in the Tools Configuration section (L396) was correctly updated, but the {{ copilot_params }} marker docs weren't. Suggested fix:

  --allow-all-tools - When bash is omitted (default) or has a wildcard (":*" or "*"), allows all tools instead of individual --allow-tool flags

• AGENTS.md line 638 — references the deleted DEFAULT_BASH_COMMANDS list

  The line still says:

  ...explicitly allows specific tools (github, safeoutputs, write, shell commands like cat, date, echo, grep, head, ls, pwd, sort, tail, uniq, wc, yq)

  DEFAULT_BASH_COMMANDS was removed in this PR — that hardcoded list no longer exists. In restricted mode, the shell commands come from the user's explicit bash: list plus runtime extensions. The example list is now misleading. Suggested fix:

  ...explicitly allows configured tools (github, safeoutputs, write, and shell commands from the bash: field plus any runtime-required commands)

• src/compile/common.rs line 551 — debug_assert!(false, ...) vs unreachable!()

  The code correctly proves this None arm can't be reached (invariant: bash=None → use_allow_all_tools=true → block skipped), and the comment explains it well. Using debug_assert!(false, ...) with a silent vec![] fallback in release builds is unusual for a "proven unreachable" path — idiomatic Rust uses unreachable!() here, which panics in both debug and release. The current approach degrades gracefully, but the intent is clearer with unreachable!(). Low priority since the path truly cannot be reached.

• src/compile/common.rs line 2507 — weak test assertion in test_copilot_params_custom_mcp_no_mcp_flag

  The test uses minimal_front_matter() (no explicit bash → use_allow_all_tools=true) and asserts !params.contains("--mcp my-tool"). The --mcp flag pattern never existed in the new code, so this assertion passes trivially regardless. A more meaningful assertion would be:

  assert!(!params.contains("--allow-tool my-tool"),
      "default (all-tools) mode should not emit individual --allow-tool for MCPs");

✅ What Looks Good

• The core logic refactor is clean and well-motivated: the guard if !use_allow_all_tools correctly gates all individual tool collection, and the cmds.len() == 1 check for wildcard detection is a pragmatic safeguard against mixed lists.
• --allow-all-paths being emitted independently of --allow-all-tools correctly matches gh-aw's behavior and is well-commented.
• The invariant comment at line 536–551 (explaining why None can't be reached in the inner block) is excellent.
• The Lean runtime test update (shell(lean) → --allow-all-tools) is correct and reflects the new default.
• New tests cover all four meaningful combinations: wildcard :*, wildcard *, explicit list, and empty list — plus the edit-enabled/disabled cross-product.

Generated by Rust PR Reviewer for issue #279 · ● 1.9M · ◷

[github-actions]

🔍 Rust PR Review

Summary: Looks good — clean implementation, well-tested. One minor silent-failure concern worth a compile-time warning.

Findings

⚠️ Suggestions

• src/compile/common.rs, wildcard detection (line ~110): When a user writes bash: [":*", "cat"] (wildcard mixed with explicit commands), the code silently falls through to restricted mode — emitting --allow-tool "shell(:*)" and --allow-tool "shell(cat)" literally. The comment documents this limitation, but a compile-time eprintln! warning would save users from a confusing runtime experience. This follows the existing pattern used elsewhere in the file for workspace misconfigurations:

  // After computing `use_allow_all_tools = false`:
  if cmds.iter().any(|c| c == ":*" || c == "*") {
      eprintln!(
          "Warning: bash wildcard ('{}') mixed with other commands — \
           wildcard is ignored. Use a single-entry list for --allow-all-tools.",
          cmds.iter().find(|c| *c == ":*" || *c == "*").unwrap()
      );
  }

✅ What Looks Good

• unreachable!() invariant (line ~213): Structurally sound — bash=None → use_allow_all_tools=true → outer if-block never entered → unreachable arm never executed. The comment explaining the invariant is clear.
• Assertion correctness: params.contains("--allow-tool") is a safe negative check — "--allow-all-tools" does not contain "--allow-tool" as a substring (confirmed: --allow-all-tools[8] = 'a', not 't'), so no false-positive risk.
• --allow-all-paths emitted independently: Correctly follows gh-aw's GetExecutionSteps() — paths scope is orthogonal to tool scope, so it's always emitted when edit_enabled regardless of --allow-all-tools.
• Test coverage: 5 new unit tests covering all relevant combinations (default, "*", ":*", edit-disabled, mixed). Existing integration tests updated to match new expected output.
• Breaking change documented: AGENTS.md and {{ copilot_params }} section properly updated to reflect the new default behavior.

Generated by Rust PR Reviewer for issue #279 · ● 821K · ◷