feat(safe-outputs): allow model-selected PR reviewers

docs/safe-outputs.md

@@ -30,6 +30,13 @@ safe-outputs:
       - agent-created
     work-items:
       - 12345
+  update-pr:
+    allowed-operations:
+      - add-reviewers
+    allowed-reviewers:
+      - "user@example.com"
+    max-reviewers: 3
+    max: 2
 ```
 
 Safe output configurations are passed to Stage 3 execution and used when processing safe outputs.
@@ -1102,6 +1109,11 @@ This hybrid approach combines:
 
 Note: The source branch name is auto-generated from a sanitized version of the PR title plus a unique suffix (e.g., `agent/fix-bug-in-parser-a1b2c3`). This format is human-readable while preventing injection attacks.
 
+The tool response includes a generated temporary PR ID such as `#aw_a1b2c3`.
+The agent can pass that value as `pull_request_id` to later `update-pr` calls in
+the same SafeOutputs job. The ID is generated by the MCP server and is not an
+input to `create-pull-request`.
+
 **Configuration options (front matter):**
 - `target-branch` - Target (base) branch the PR merges into (default: "main"). A
   plain literal branch name, applied to every repo unless overridden below.
@@ -1146,7 +1158,7 @@ Note: The source branch name is auto-generated from a sanitized version of the P
 - `protected-files` - Controls whether manifest/CI files (e.g., `package-lock.json`, `.github/`, `*.lock`) can be modified: `"blocked"` (default, reject changes to these files) or `"allowed"` (permit all files)
 - `excluded-files` - Glob patterns for files to strip from the patch before applying (e.g., `["*.lock", "dist/**"]`)
 - `allowed-labels` - Allowlist of labels the agent is permitted to apply. If empty (default), any labels are accepted.
-- `reviewers` - List of reviewer emails to add
+- `reviewers` - List of reviewer emails or Azure DevOps user IDs to add
 - `labels` - List of labels to apply
 - `work-items` - List of work item IDs to link
 - `fallback-record-branch` - When PR creation fails, record the pushed branch name and target branch in the failure response so operators can manually create the PR (default: true)
@@ -1277,7 +1289,7 @@ safe-outputs:
 Updates pull request metadata (reviewers, labels, auto-complete, vote, description).
 
 **Agent parameters:**
-- `pull_request_id` - The PR ID to update (required)
+- `pull_request_id` - A positive numeric PR ID, a quoted positive numeric ID, or a temporary ID (`#aw_...`) returned by an earlier `create-pull-request` call in the same SafeOutputs job (required)
 - `operation` - Update operation: `add-reviewers`, `add-labels`, `set-auto-complete`, `vote`, or `update-description` (required)
 - `reviewers` - Reviewer emails (required for `add-reviewers`)
 - `labels` - Label names (required for `add-labels`)
@@ -1291,12 +1303,37 @@ safe-outputs:
   update-pr:
     allowed-operations: []          # Optional — restrict which operations are permitted (empty = all)
     allowed-repositories: []        # Optional — restrict which repos can be updated
+    allowed-reviewers: []           # REQUIRED for add-reviewers — empty rejects all reviewers; ["*"] permits any valid reviewer
+    max-reviewers: 3                # Maximum reviewers in one add-reviewers call (default: 3)
     allowed-votes: []               # REQUIRED for vote operation — empty rejects all votes
     delete-source-branch: true      # For set-auto-complete (default: true)
     merge-strategy: "squash"        # For set-auto-complete: squash, noFastForward, rebase, rebaseMerge
     max: 1                          # Maximum per run (default: 1)
 ```
 
+Reviewer allowlisting uses case-insensitive exact matching. Non-GUID reviewer
+values must also exactly match an Azure DevOps identity email, account name, or
+display name; fuzzy Identity Picker results are not selected. Reviewer identity
+or API failures return a warning with structured `added` and `failed` arrays.
+Invalid configuration, disallowed reviewers, and unresolved PR references fail
+before reviewer writes begin.
+
+Temporary PR references are resolved in safe-output proposal order, so
+`create-pull-request` must appear before its `update-pr` entries. They are
+in-memory references scoped to one SafeOutputs job: automatic and manually
+reviewed safe outputs execute in separate jobs and cannot share a temporary ID.
+Each follow-up call counts against `update-pr.max`.
+
+Example agent call sequence:
+
+```json
+{"title":"Update dependencies","description":"Refresh dependencies and related tests."}
+{"pull_request_id":"#aw_a1b2c3","operation":"add-reviewers","reviewers":["user@example.com"]}
+```
+
+The first line represents the `create-pull-request` call; use the actual
+temporary ID returned by that call in the later `update-pr` call.
+
 ### link-work-items
 Links two Azure DevOps work items together.
 

scripts/ado-script/src/executor-e2e/__tests__/index.test.ts

@@ -26,6 +26,7 @@ describe("scenario registry", () => {
     expect(ids).toContain("create-pull-request");
     expect(ids).toContain("create-pull-request-self-multi-checkout");
     expect(ids).toContain("create-pull-request-cross-org");
+    expect(ids).toContain("create-pull-request-temporary-id-handoff");
     expect(ids).toContain("create-branch-cross-org");
     expect(ids).toContain("create-git-tag-cross-org");
   });

scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts

@@ -26,9 +26,14 @@ import { createHash } from "node:crypto";
 import { mkdir, rm, writeFile } from "node:fs/promises";
 import { join } from "node:path";
 
-import type { Scenario, ScenarioContext } from "../scenario.js";
+import type {
+  ExecutedRecord,
+  PriorEntry,
+  Scenario,
+  ScenarioContext,
+} from "../scenario.js";
 import { partialOutput } from "../execute-cli.js";
-import { detBody, numResult, Teardown } from "./common.js";
+import { detBody, numResult, strResult, Teardown } from "./common.js";
 import {
   crossOrgSource,
   resolveCrossOrgEnv,
@@ -62,6 +67,9 @@ interface CreatePrScenarioOptions {
   readonly changedFileSuffix?: string;
 }
 
+const CREATE_PR_TEMPORARY_ID = "#aw_prcreate";
+const HANDOFF_TEMPORARY_ID = "#aw_prhandoff";
+
 function runGit(
   args: string[],
   cwd: string,
@@ -270,6 +278,7 @@ function createPullRequestScenario(
       patch_file: state.patchRelPath,
       repository: state.repositorySelector,
       agent_labels: [],
+      temporary_id: CREATE_PR_TEMPORARY_ID,
       base_commit: state.baseCommit,
       patch_sha256: state.patchSha256,
     }),
@@ -325,8 +334,120 @@ export const createPullRequestCrossOrg = createPullRequestScenario({
   changedFileSuffix: "-cross-org",
 });
 
+function executedRecordForTool(
+  records: ExecutedRecord[],
+  tool: string,
+): ExecutedRecord {
+  const recordName = tool.replaceAll("-", "_");
+  const record = records.find((candidate) => candidate.name === recordName);
+  if (!record) {
+    throw new Error(`no executed record found for prior tool '${tool}'`);
+  }
+  return record;
+}
+
+/**
+ * Runs create-pull-request and update-pr in one executor process. This is the
+ * production handoff shape: the create result registers the real PR under a
+ * temporary ID, then the following update resolves that ID without the model
+ * ever knowing Azure DevOps' numeric PR ID.
+ */
+export const createPullRequestTemporaryIdHandoff: Scenario<CreatePrState> = {
+  id: "create-pull-request-temporary-id-handoff",
+  tool: "update-pr",
+  targetsAdoRepo: true,
+  setup: (ctx) =>
+    setupCreatePullRequest(ctx, {
+      id: "create-pull-request-temporary-id-handoff",
+      repositorySelector: "named",
+      patchRelPath: "create-pr-temporary-id-handoff.patch",
+      changedFileSuffix: "-temporary-id-handoff",
+    }),
+  config: (_ctx, state) => ({
+    "allowed-operations": ["update-description"],
+    "allowed-repositories": [state.repo],
+    max: 1,
+  }),
+  priorEntries: async (ctx, state): Promise<PriorEntry[]> => [
+    {
+      tool: "create-pull-request",
+      config: {
+        "target-branch": state.targetBranch,
+        "allowed-repositories": [state.repo],
+        "delete-source-branch": true,
+        "if-no-changes": "error",
+        "include-stats": false,
+      },
+      entry: {
+        title: `${ctx.prefix("create-pull-request-temporary-id-handoff")} (do not merge)`,
+        description: detBody(ctx, "create-pull-request-temporary-id-handoff"),
+        source_branch: state.sourceBranch,
+        patch_file: state.patchRelPath,
+        repository: state.repositorySelector,
+        agent_labels: [],
+        temporary_id: HANDOFF_TEMPORARY_ID,
+        base_commit: state.baseCommit,
+        patch_sha256: state.patchSha256,
+      },
+    },
+  ],
+  files: async (_ctx, state) => ({ [state.patchRelPath]: state.patchContent }),
+  env: async (_ctx, state) => ({
+    BUILD_SOURCESDIRECTORY: state.sourcesDir,
+  }),
+  ndjson: async (ctx) => ({
+    pull_request_id: HANDOFF_TEMPORARY_ID,
+    operation: "update-description",
+    description: `${detBody(ctx, "create-pull-request-temporary-id-handoff")} Updated through temporary ID.`,
+  }),
+  assert: async (ctx, state, record, records) => {
+    const created = executedRecordForTool(records, "create-pull-request");
+    const createdPrId = numResult(created, "pull_request_id");
+    state.prId = createdPrId;
+
+    if (strResult(created, "temporary_id") !== HANDOFF_TEMPORARY_ID) {
+      throw new Error(
+        `create-pull-request reported temporary_id '${strResult(created, "temporary_id")}', expected '${HANDOFF_TEMPORARY_ID}'`,
+      );
+    }
+    const updatedPrId = numResult(record, "pull_request_id");
+    if (updatedPrId !== createdPrId) {
+      throw new Error(
+        `temporary_id '${HANDOFF_TEMPORARY_ID}' resolved to PR #${updatedPrId}, but create-pull-request filed #${createdPrId}`,
+      );
+    }
+
+    const expectedDescription =
+      `${detBody(ctx, "create-pull-request-temporary-id-handoff")} Updated through temporary ID.`;
+    const pr = await state.rest.getPullRequest(state.repo, createdPrId);
+    if (pr.description !== expectedDescription) {
+      throw new Error(
+        `PR #${createdPrId} description was not updated through temporary ID`,
+      );
+    }
+  },
+  cleanup: async (_ctx, state) => {
+    const teardown = new Teardown();
+    if (state.prId !== undefined) {
+      const prId = state.prId;
+      teardown.add("abandon PR", () =>
+        state.rest.abandonPullRequest(state.repo, prId),
+      );
+    }
+    await teardown
+      .add("delete source branch", () =>
+        state.rest.deleteRef(state.repo, `refs/heads/${state.sourceBranch}`),
+      )
+      .add("remove local checkout", () =>
+        rm(state.sourcesDir, { recursive: true, force: true }),
+      )
+      .run();
+  },
+};
+
 export const createPullRequestScenarios: Scenario<unknown>[] = [
   createPullRequest,
   createPullRequestSelfMultiCheckout,
   createPullRequestCrossOrg,
+  createPullRequestTemporaryIdHandoff,
 ];

src/execute.rs

@@ -1353,6 +1353,149 @@ mod tests {
         assert_eq!(manifest[1]["status"], "succeeded");
     }
 
+    #[tokio::test]
+    async fn test_execute_safe_outputs_creates_then_updates_temporary_pr_reference() {
+        use std::process::Command;
+        use wiremock::matchers::{method, path};
+        use wiremock::{Mock, MockServer, ResponseTemplate};
+
+        let server = MockServer::start().await;
+        let api_base = "/Target%20Project/_apis/git/repositories/repo-id";
+        Mock::given(method("GET"))
+            .and(path(format!("{api_base}/refs")))
+            .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
+                "value": []
+            })))
+            .expect(1)
+            .mount(&server)
+            .await;
+        Mock::given(method("POST"))
+            .and(path(format!("{api_base}/pushes")))
+            .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({
+                "pushId": 1
+            })))
+            .expect(1)
+            .mount(&server)
+            .await;
+        Mock::given(method("POST"))
+            .and(path(format!("{api_base}/pullrequests")))
+            .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({
+                "pullRequestId": 42,
+                "url": "https://example.test/pr/42",
+                "createdBy": {"id": "creator-id"}
+            })))
+            .expect(1)
+            .mount(&server)
+            .await;
+        Mock::given(method("PATCH"))
+            .and(path(format!("{api_base}/pullRequests/42")))
+            .respond_with(ResponseTemplate::new(200))
+            .expect(1)
+            .mount(&server)
+            .await;
+
+        let temp_dir = tempfile::tempdir().unwrap();
+        let repo_dir = temp_dir.path().join("repo");
+        let safe_outputs_dir = temp_dir.path().join("safe-outputs");
+        std::fs::create_dir_all(&repo_dir).unwrap();
+        std::fs::create_dir_all(&safe_outputs_dir).unwrap();
+        let run_git = |args: &[&str]| {
+            let output = Command::new("git")
+                .args(args)
+                .current_dir(&repo_dir)
+                .output()
+                .expect("git command should run");
+            assert!(
+                output.status.success(),
+                "git {:?} failed: {}",
+                args,
+                String::from_utf8_lossy(&output.stderr)
+            );
+            output
+        };
+        run_git(&["init", "-b", "main"]);
+        run_git(&["config", "user.email", "test@example.com"]);
+        run_git(&["config", "user.name", "Test User"]);
+        std::fs::write(repo_dir.join("file.txt"), "before\n").unwrap();
+        run_git(&["add", "file.txt"]);
+        run_git(&["commit", "-m", "initial"]);
+        std::fs::write(repo_dir.join("file.txt"), "after\n").unwrap();
+        run_git(&["add", "file.txt"]);
+        run_git(&["commit", "-m", "update file"]);
+        let patch = run_git(&["format-patch", "HEAD~1", "--stdout"]).stdout;
+        run_git(&["reset", "--hard", "HEAD~1"]);
+        run_git(&["update-ref", "refs/remotes/origin/main", "HEAD"]);
+        let base_commit = String::from_utf8(run_git(&["rev-parse", "HEAD"]).stdout)
+            .unwrap()
+            .trim()
+            .to_string();
+        let patch_file = safe_outputs_dir.join("change.patch");
+        std::fs::write(&patch_file, &patch).unwrap();
+        let patch_sha256 = crate::hash::sha256_hex(&patch);
+
+        let create = serde_json::json!({
+            "name": "create-pull-request",
+            "title": "Update test file",
+            "description": "Update the test file before following up.",
+            "source_branch": "agent/update-test-file-abc123",
+            "patch_file": "change.patch",
+            "repository": "self",
+            "agent_labels": [],
+            "temporary_id": "#aw_pr123",
+            "base_commit": base_commit,
+            "patch_sha256": patch_sha256
+        });
+        let update = serde_json::json!({
+            "name": "update-pr",
+            "pull_request_id": "#aw_pr123",
+            "operation": "update-description",
+            "description": "Updated through the temporary reference."
+        });
+        let ndjson = format!(
+            "{}\n{}\n",
+            serde_json::to_string(&create).unwrap(),
+            serde_json::to_string(&update).unwrap()
+        );
+        tokio::fs::write(safe_outputs_dir.join(SAFE_OUTPUT_FILENAME), ndjson)
+            .await
+            .unwrap();
+
+        let mut tool_configs = HashMap::new();
+        tool_configs.insert(
+            "create-pull-request".to_string(),
+            serde_json::json!({"max": 1, "include-stats": false}),
+        );
+        tool_configs.insert("update-pr".to_string(), serde_json::json!({"max": 1}));
+        let ctx = ExecutionContext {
+            ado_org_url: Some(server.uri()),
+            ado_organization: Some("target-org".to_string()),
+            ado_project: Some("Target Project".to_string()),
+            access_token: Some("test-token".to_string()),
+            working_directory: safe_outputs_dir.clone(),
+            source_directory: repo_dir.clone(),
+            self_repository_directory: repo_dir,
+            repository_id: Some("repo-id".to_string()),
+            repository_name: Some("target-repo".to_string()),
+            repository_provider: Some("TfsGit".to_string()),
+            tool_configs,
+            ..Default::default()
+        };
+
+        let results = execute_safe_outputs(&safe_outputs_dir, &ctx, &ToolFilter::default())
+            .await
+            .unwrap();
+        assert_eq!(results.len(), 2);
+        assert!(results[0].success, "{}", results[0].message);
+        assert_eq!(
+            results[0].data.as_ref().unwrap()["temporary_id"],
+            "#aw_pr123"
+        );
+        assert_eq!(results[0].data.as_ref().unwrap()["pull_request_id"], 42);
+        assert!(results[1].success, "{}", results[1].message);
+        assert_eq!(results[1].data.as_ref().unwrap()["pull_request_id"], 42);
+        server.verify().await;
+    }
+
     #[tokio::test]
     async fn test_execute_safe_outputs_empty_file_returns_empty() {
         let temp_dir = tempfile::tempdir().unwrap();