fix(compile): graceful-degradation bug + cleanup per PR review

Address two findings from the Rust PR Reviewer bot on PR #873.

1. CRITICAL — AW_AZ_MOUNTS undefined when az is missing:

The runtime-detection step in AzureCliExtension only set the
AW_AZ_MOUNTS pipeline variable in the detected branch. In the
missing-az branch the variable was left undefined. ADO leaves an
undefined $(VAR) as the LITERAL STRING "$(VAR)" in subsequent bash
steps (it does NOT expand to empty). Bash sees $(AW_AZ_MOUNTS),
interprets it as a $(...) command substitution, tries to execute
a program named AW_AZ_MOUNTS, gets exit 127, and the AWF
invocation step dies under `set -e` — the exact 1ES failure mode
the refactor set out to prevent.

Fix: always emit `##vso[task.setvariable variable=AW_AZ_MOUNTS]`,
with an empty value in the missing branch. ADO then expands
$(AW_AZ_MOUNTS) to nothing and the trailing `\` line becomes a
harmless continuation no-op.

Regression guards (both lock this in):
  - azure_cli.rs::test_azure_cli_prepare_steps_defines_aw_az_mounts_in_else_branch
    counts `setvariable` occurrences (must be 2) and asserts the
    else block contains an empty-value setvariable line.
  - compiler_tests.rs::test_default_pipeline_mounts_az_and_allows_azure_hosts
    asserts the same 2× count on the compiled lock.yml.

2. Cleanup — delete WRITE_REQUIRING_SAFE_OUTPUTS:

The const was retained with #[allow(dead_code)] after the
removal of `validate_write_permissions`, but its only consumers
left were the two tests that exercised the const itself. Each
`*Result` type already carries `REQUIRES_WRITE: bool` for any
caller (compiler, audit, runtime) that needs the same info.
Deleting the const removes a dead-code annotation and one
otherwise-purposeless list to maintain when adding new tools.

Test cleanup: removed `test_write_requiring_subset_of_all_known`
(purely exercised the deleted const) and rewrote
`test_all_known_completeness` to use a HashSet-based duplicate
check on ALL_KNOWN_SAFE_OUTPUTS plus the ALWAYS_ON/NON_MCP
disjointness check (preserves the meaningful invariants).

Validation:
  - cargo build: clean
  - cargo test: 1748 unit + 119 compiler + all integration pass
  - cargo clippy --all-targets --all-features -- -D warnings: clean
  - tests/safe-outputs/azure-cli.lock.yml regenerated (+1 line)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Author: jamesadevine

src/compile/extensions/azure_cli.rs

@@ -26,9 +26,16 @@ use super::{AwfMount, CompilerExtension, CompileContext, ExtensionPhase};
 ///   sets the ADO pipeline variable `AW_AZ_MOUNTS` to
 ///   `--mount /opt/az:/opt/az:ro --mount /usr/bin/az:/usr/bin/az:ro`
 ///   via `##vso[task.setvariable]`.
-/// * Otherwise, the step emits a `##vso[task.logissue type=warning]`
-///   explaining `az` won't be available inside the agent sandbox and
-///   leaves `AW_AZ_MOUNTS` unset (expands to the empty string).
+/// * Otherwise, the step sets `AW_AZ_MOUNTS` to the **empty string**
+///   (still via `##vso[task.setvariable]`) and emits a
+///   `##vso[task.logissue type=warning]` explaining `az` won't be
+///   available inside the agent sandbox. Setting the variable to empty
+///   is important: ADO leaves an *undefined* `$(VAR)` as the literal
+///   string `$(VAR)` in later bash steps, where bash would interpret
+///   it as a command substitution (`$(...)`) and fail under
+///   `set -e` with exit 127. An empty-but-defined variable expands to
+///   nothing, and the `$(AW_AZ_MOUNTS) \` line in the AWF chain
+///   becomes a harmless `\`-continuation no-op.
 ///
 /// The AWF invocation in `base.yml`/`1es-base.yml`/etc. then includes a
 /// `$(AW_AZ_MOUNTS) \` line (injected by
@@ -107,6 +114,15 @@ impl CompilerExtension for AzureCliExtension {
         // contains only path chars, `:`, and spaces — no shell
         // metachars — so unquoted expansion is safe.
         //
+        // Both branches MUST set the variable (the else branch sets it
+        // to empty string). If left undefined, ADO leaves the literal
+        // `$(AW_AZ_MOUNTS)` in subsequent bash steps, where bash
+        // interprets it as a `$(...)` command substitution, tries to
+        // run a program named `AW_AZ_MOUNTS`, gets exit 127, and the
+        // AWF invocation step dies under `set -e` — the opposite of
+        // graceful degradation. Defining the variable as empty makes
+        // ADO expand it to nothing, leaving a harmless `\`-continuation.
+        //
         // Warning text is intentionally short and operator-facing.
         // Agents that don't invoke `az` are unaffected; agents that do
         // will get a normal "command not found" inside the sandbox.
@@ -116,6 +132,7 @@ impl CompilerExtension for AzureCliExtension {
       echo "##vso[task.setvariable variable=AW_AZ_MOUNTS]--mount /opt/az:/opt/az:ro --mount /usr/bin/az:/usr/bin/az:ro"
       echo "Azure CLI detected on host; mounting /opt/az and /usr/bin/az into AWF sandbox."
     else
+      echo "##vso[task.setvariable variable=AW_AZ_MOUNTS]"
       echo "##vso[task.logissue type=warning]Azure CLI not detected on this runner (missing /usr/bin/az or /opt/az). The az command will not be available inside the agent sandbox. Install azure-cli on the runner image to enable it."
     fi
   displayName: "Detect Azure CLI on host (for AWF mount)"
@@ -240,6 +257,51 @@ mod tests {
         );
     }
 
+    #[test]
+    fn test_azure_cli_prepare_steps_defines_aw_az_mounts_in_else_branch() {
+        // Regression guard for the graceful-degradation bug:
+        // if the `else` branch doesn't explicitly setvariable on
+        // AW_AZ_MOUNTS, ADO leaves the literal `$(AW_AZ_MOUNTS)` in
+        // the subsequent AWF bash step, bash interprets it as a
+        // `$(...)` command substitution, tries to execute a program
+        // named AW_AZ_MOUNTS, gets exit 127, and `set -e` kills the
+        // step — exactly the failure mode this PR set out to prevent.
+        let ext = AzureCliExtension;
+        let fm = fm();
+        let ctx = CompileContext::for_test(&fm);
+        let step = ext.prepare_steps(&ctx).into_iter().next().unwrap();
+
+        // Count setvariable occurrences — must be 2 (one per branch).
+        let setvar_count = step
+            .matches("##vso[task.setvariable variable=AW_AZ_MOUNTS]")
+            .count();
+        assert_eq!(
+            setvar_count, 2,
+            "AW_AZ_MOUNTS must be set in BOTH branches of the if/else (got {setvar_count}); \
+             leaving it undefined in the missing-az branch causes bash to interpret \
+             the literal `$(AW_AZ_MOUNTS)` as command substitution and fail under set -e. \
+             Step:\n{step}"
+        );
+
+        // Verify the else branch sets it to empty (no `--mount` chars
+        // after the `]`). We slice the step from "else" to "fi" and
+        // assert the else block contains a setvariable line that ends
+        // with `]"` (closing-bracket-then-quote = empty value).
+        let else_start = step.find("else").expect("must have else branch");
+        let fi_end = step[else_start..].find("fi").expect("must have fi");
+        let else_block = &step[else_start..else_start + fi_end];
+        assert!(
+            else_block.contains("##vso[task.setvariable variable=AW_AZ_MOUNTS]\""),
+            "else branch must set AW_AZ_MOUNTS to empty string (line must end with `]\"`), got:\n{else_block}"
+        );
+        // And the else branch must NOT include any --mount arg (would
+        // mean we're accidentally setting non-empty when az is missing).
+        assert!(
+            !else_block.contains("--mount"),
+            "else branch must not contain --mount args (those belong to the detected branch only): {else_block}"
+        );
+    }
+
     #[test]
     fn test_azure_cli_prepare_steps_uses_pipefail() {
         // Bash steps in this repo's lint policy require `set -eo

src/safeoutputs/mod.rs

@@ -26,41 +26,6 @@ pub const ALWAYS_ON_TOOLS: &[&str] = tool_names![
     ReportIncompleteResult,
 ];
 
-/// Safe-output tools that perform write operations against ADO.
-/// Compile-time derived from tool types via `ToolResult::NAME`.
-///
-/// **Informational only.** The Stage 3 executor always receives a
-/// `SYSTEM_ACCESSTOKEN` (sourced from the pipeline's built-in
-/// `$(System.AccessToken)` by default, or from a `permissions.write` ARM
-/// service connection when one is configured). This list is kept so other
-/// compiler/runtime code (e.g. audit) can identify write-bearing tools, but
-/// the compiler no longer fails when one is configured without an ARM SC.
-///
-/// Adding a new write-requiring tool: create the struct with `tool_result!{ write = true, ... }`,
-/// then add its type to this list.
-#[allow(dead_code)]
-pub const WRITE_REQUIRING_SAFE_OUTPUTS: &[&str] = tool_names![
-    CreateWorkItemResult,
-    CommentOnWorkItemResult,
-    UpdateWorkItemResult,
-    CreatePrResult,
-    CreateWikiPageResult,
-    UpdateWikiPageResult,
-    AddPrCommentResult,
-    LinkWorkItemsResult,
-    QueueBuildResult,
-    CreateGitTagResult,
-    AddBuildTagResult,
-    CreateBranchResult,
-    UpdatePrResult,
-    UploadBuildAttachmentResult,
-    UploadPipelineArtifactResult,
-    UploadWorkitemAttachmentResult,
-    SubmitPrReviewResult,
-    ReplyToPrCommentResult,
-    ResolvePrThreadResult,
-];
-
 /// Non-MCP safe-output keys handled by the compiler/executor, not the MCP server.
 /// These must not appear in `--enabled-tools` or they cause real MCP tools to be
 /// filtered out (the router has no route for them).
@@ -810,17 +775,6 @@ pub use upload_workitem_attachment::*;
 mod tests {
     use super::*;
 
-    #[test]
-    fn test_write_requiring_subset_of_all_known() {
-        for name in WRITE_REQUIRING_SAFE_OUTPUTS {
-            assert!(
-                ALL_KNOWN_SAFE_OUTPUTS.contains(name),
-                "WRITE_REQUIRING_SAFE_OUTPUTS entry '{}' is missing from ALL_KNOWN_SAFE_OUTPUTS",
-                name
-            );
-        }
-    }
-
     #[test]
     fn test_always_on_subset_of_all_known() {
         for name in ALWAYS_ON_TOOLS {
@@ -876,40 +830,32 @@ mod tests {
         const { assert!(!ReportIncompleteResult::REQUIRES_WRITE); }
     }
 
-    /// Verify ALL_KNOWN_SAFE_OUTPUTS has exactly the right count:
-    /// write tools + diagnostics + non-MCP keys.
+    /// Verify ALL_KNOWN_SAFE_OUTPUTS contains no duplicate entries, and
+    /// that the always-on and non-MCP sub-lists are disjoint.
     #[test]
     fn test_all_known_completeness() {
-        // The three sub-lists must be disjoint — a tool in multiple lists would
-        // be duplicated in ALL_KNOWN and the count would mismatch.
-        for name in WRITE_REQUIRING_SAFE_OUTPUTS {
+        // No duplicates: a tool name appearing twice in ALL_KNOWN would
+        // mean `all_safe_output_names!` was given the same type twice
+        // and would silently break tool routing.
+        let mut seen = std::collections::HashSet::new();
+        for name in ALL_KNOWN_SAFE_OUTPUTS {
             assert!(
-                !ALWAYS_ON_TOOLS.contains(name),
-                "Tool '{}' appears in both WRITE_REQUIRING and ALWAYS_ON — lists must be disjoint",
-                name
-            );
-            assert!(
-                !NON_MCP_SAFE_OUTPUT_KEYS.contains(name),
-                "Tool '{}' appears in both WRITE_REQUIRING and NON_MCP — lists must be disjoint",
+                seen.insert(*name),
+                "ALL_KNOWN_SAFE_OUTPUTS contains duplicate entry '{}'",
                 name
             );
         }
+
+        // ALWAYS_ON and NON_MCP must be disjoint — a diagnostic tool
+        // that also appears as a non-MCP key would be both routed and
+        // intercepted, giving inconsistent behaviour.
         for name in ALWAYS_ON_TOOLS {
             assert!(
                 !NON_MCP_SAFE_OUTPUT_KEYS.contains(name),
                 "Tool '{}' appears in both ALWAYS_ON and NON_MCP — lists must be disjoint",
                 name
             );
         }
-
-        let expected = WRITE_REQUIRING_SAFE_OUTPUTS.len()
-            + ALWAYS_ON_TOOLS.len()
-            + NON_MCP_SAFE_OUTPUT_KEYS.len();
-        assert_eq!(
-            ALL_KNOWN_SAFE_OUTPUTS.len(),
-            expected,
-            "ALL_KNOWN_SAFE_OUTPUTS should be the union of write + diagnostic + non-MCP lists"
-        );
     }
 
     // ─── validate_git_ref_name ──────────────────────────────────────────────

tests/compiler_tests.rs

@@ -4577,6 +4577,24 @@ fn test_default_pipeline_mounts_az_and_allows_azure_hosts() {
          Compiled:\n{compiled}"
     );
 
+    // (1a) Regression guard: `setvariable` for AW_AZ_MOUNTS must appear
+    // TWICE — once per branch of the if/else. If the missing-az branch
+    // skips the setvariable, ADO leaves the literal `$(AW_AZ_MOUNTS)`
+    // in the AWF bash step, where bash interprets it as a `$(...)`
+    // command substitution, attempts to run a program named
+    // `AW_AZ_MOUNTS`, gets exit 127, and `set -e` kills the pipeline —
+    // the exact failure mode this PR set out to prevent on runners
+    // without azure-cli installed.
+    let setvar_count = compiled
+        .matches("##vso[task.setvariable variable=AW_AZ_MOUNTS]")
+        .count();
+    assert_eq!(
+        setvar_count, 2,
+        "AW_AZ_MOUNTS must be set in BOTH branches of the detection step (got {setvar_count} \
+         occurrences); leaving it unset in the missing-az branch breaks `set -e` in the \
+         AWF invocation. See AzureCliExtension::prepare_steps for the rationale."
+    );
+
     // (2) The AWF invocation must reference $(AW_AZ_MOUNTS) so the
     // pipeline-variable value (the two --mount args, or empty) is
     // word-split into the docker run command at runtime. Unquoted on