diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index c93a0793..bff3f978 100644 --- a/docs/safe-outputs.md +++ b/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. diff --git a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts index 2c266930..4680f9ee 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts +++ b/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"); }); diff --git a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts index 16fafb4d..2090f6d2 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts +++ b/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 = { + 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 => [ + { + 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[] = [ createPullRequest, createPullRequestSelfMultiCheckout, createPullRequestCrossOrg, + createPullRequestTemporaryIdHandoff, ]; diff --git a/src/execute.rs b/src/execute.rs index 7740caa5..7f34b850 100644 --- a/src/execute.rs +++ b/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(); diff --git a/src/mcp.rs b/src/mcp.rs index 17af63e4..358f50f0 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -25,18 +25,18 @@ use crate::safe_outputs::{ MissingToolParams, MissingToolResult, NoopParams, NoopResult, PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE, QueueBuildParams, QueueBuildResult, RemoveGithubIssueLabelsParams, RemoveGithubIssueLabelsResult, ReplyToPrCommentParams, - ReplyToPrCommentResult, ReportIncompleteParams, ReportIncompleteResult, - ResolvePrThreadParams, ResolvePrThreadResult, SetGithubIssueFieldParams, - SetGithubIssueFieldResult, SetGithubIssueTypeParams, SetGithubIssueTypeResult, - SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UnassignGithubIssueFromUserParams, - UnassignGithubIssueFromUserResult, UpdateGithubIssueParams, UpdateGithubIssueResult, - UpdatePrParams, UpdatePrResult, UpdateWikiPageParams, UpdateWikiPageResult, - UpdateWorkItemParams, UpdateWorkItemResult, UploadBuildAttachmentParams, - UploadBuildAttachmentResult, UploadPipelineArtifactParams, UploadPipelineArtifactResult, - UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, Validate, anyhow_to_mcp_error, + ReplyToPrCommentResult, ReportIncompleteParams, ReportIncompleteResult, ResolvePrThreadParams, + ResolvePrThreadResult, SetGithubIssueFieldParams, SetGithubIssueFieldResult, + SetGithubIssueTypeParams, SetGithubIssueTypeResult, SubmitPrReviewParams, SubmitPrReviewResult, + ToolResult, UnassignGithubIssueFromUserParams, UnassignGithubIssueFromUserResult, + UpdateGithubIssueParams, UpdateGithubIssueResult, UpdatePrParams, UpdatePrResult, + UpdateWikiPageParams, UpdateWikiPageResult, UpdateWorkItemParams, UpdateWorkItemResult, + UploadBuildAttachmentParams, UploadBuildAttachmentResult, UploadPipelineArtifactParams, + UploadPipelineArtifactResult, UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, + Validate, anyhow_to_mcp_error, }; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_markdown}; -use crate::secure::WorkItemTemporaryId; +use crate::secure::{PullRequestTemporaryId, WorkItemTemporaryId}; /// Sanitize a title into a safe branch name slug. /// Only allows alphanumeric characters and dashes, collapses multiple dashes, @@ -220,6 +220,8 @@ pub struct SafeOutputs { custom_proposal_lock: Arc>, /// Serializes create-work-item temporary-ID allocation and proposal append. create_work_item_proposal_lock: Arc>, + /// Serializes create-pull-request temporary-ID allocation and proposal append. + create_pr_proposal_lock: Arc>, } /// Resolve which git directory to use for patch generation. @@ -537,8 +539,7 @@ impl SafeOutputs { WorkItemTemporaryId::parse(format!("#aw_{}", generate_short_id())).ok()?; let canonical = candidate.canonical(); let collision = existing.iter().any(|proposal| { - proposal.get("name").and_then(Value::as_str) - == Some(CreateWorkItemResult::NAME) + proposal.get("name").and_then(Value::as_str) == Some(CreateWorkItemResult::NAME) && proposal.get("temporary_id").and_then(Value::as_str) == Some(canonical.as_str()) }); @@ -636,6 +637,7 @@ impl SafeOutputs { tool_router, custom_proposal_lock: Arc::new(tokio::sync::Mutex::new(())), create_work_item_proposal_lock: Arc::new(tokio::sync::Mutex::new(())), + create_pr_proposal_lock: Arc::new(tokio::sync::Mutex::new(())), }) } @@ -1078,7 +1080,8 @@ and only the fields you want to update." name = "create-pull-request", description = "Create a new pull request to propose code changes. This tool captures all \ changes in the repository (both committed and uncommitted) and creates a PR from them. \ -Use 'self' for the pipeline's own repository, or a repository alias from the checkout list." +Use 'self' for the pipeline's own repository, or a repository alias from the checkout list. \ +Returns a generated temporary_id that can be passed as pull_request_id to later update-pr calls." )] async fn create_pr( &self, @@ -1131,7 +1134,31 @@ Use 'self' for the pipeline's own repository, or a repository alias from the che format!("agent/{}-{}", title_slug, short_id) }; - // Create the result with patch file reference and integrity hash + const MAX_ID_ATTEMPTS: usize = 16; + let _guard = self.create_pr_proposal_lock.lock().await; + let existing = self + .read_safe_output_file() + .await + .map_err(anyhow_to_mcp_error)?; + let temporary_id = (0..MAX_ID_ATTEMPTS) + .find_map(|_| { + let candidate = + PullRequestTemporaryId::parse(format!("#aw_{}", generate_short_id())).ok()?; + let canonical = candidate.canonical(); + let collision = existing.iter().any(|proposal| { + proposal.get("name").and_then(Value::as_str) == Some(CreatePrResult::NAME) + && proposal.get("temporary_id").and_then(Value::as_str) + == Some(canonical.as_str()) + }); + (!collision).then_some(candidate) + }) + .ok_or_else(|| { + anyhow_to_mcp_error(anyhow::anyhow!( + "Failed to allocate a unique create-pull-request temporary ID" + )) + })?; + + // Create the result with patch file reference, temporary ID, and integrity hash let result = CreatePrResult { name: CreatePrResult::NAME.to_string(), title: sanitized.title.clone(), @@ -1140,17 +1167,25 @@ Use 'self' for the pipeline's own repository, or a repository alias from the che patch_file: patch_filename, repository: repository.to_string(), agent_labels: sanitized.labels, + temporary_id: temporary_id.clone(), base_commit: Some(merge_base), patch_sha256, }; // Write to safe outputs - let _ = self.write_safe_output_file(&result).await; + self.write_safe_output_file(&result) + .await + .map_err(anyhow_to_mcp_error)?; - Ok(CallToolResult::success(vec![Content::text(format!( - "PR request saved for repository '{}'. Patch file: {}. Changes will be pushed and PR created during safe output processing.", - repository, result.patch_file - ))])) + let canonical = temporary_id.canonical(); + let mut response = CallToolResult::success(vec![Content::text(format!( + "PR request saved for repository '{}'. Patch file: {}. Use temporary ID {} as pull_request_id in later update-pr calls.", + repository, result.patch_file, canonical + ))]); + response.structured_content = Some(serde_json::json!({ + "temporary_id": canonical, + })); + Ok(response) } #[tool( @@ -1837,6 +1872,31 @@ mod tests { (safe_outputs, temp_dir) } + fn initialize_git_repo_with_change(path: &std::path::Path) { + use std::process::Command; + + let run = |args: &[&str]| { + let output = Command::new("git") + .args(args) + .current_dir(path) + .output() + .expect("git command should run"); + assert!( + output.status.success(), + "git {:?} failed: {}", + args, + String::from_utf8_lossy(&output.stderr) + ); + }; + run(&["init", "-b", "main"]); + run(&["config", "user.email", "test@example.com"]); + run(&["config", "user.name", "Test User"]); + std::fs::write(path.join("file.txt"), "before\n").unwrap(); + run(&["add", "file.txt"]); + run(&["commit", "-m", "initial"]); + std::fs::write(path.join("file.txt"), "after\n").unwrap(); + } + fn valid_create_work_item_params(suffix: &str) -> CreateWorkItemParams { CreateWorkItemParams { title: format!("Create work item {suffix}"), @@ -2046,6 +2106,30 @@ mod tests { assert_eq!(proposals[0]["temporary_id"], temporary_id); } + #[tokio::test] + async fn create_pr_returns_and_persists_generated_temporary_id() { + let (safe_outputs, temp_dir) = create_test_safe_outputs().await; + initialize_git_repo_with_change(temp_dir.path()); + + let response = safe_outputs + .create_pr(Parameters(CreatePrParams { + title: "Update test file".to_string(), + description: "Update the test file through a generated pull request.".to_string(), + repository: None, + labels: Vec::new(), + })) + .await + .unwrap(); + + let structured = response.structured_content.expect("structured response"); + let temporary_id = structured["temporary_id"].as_str().unwrap(); + assert!(PullRequestTemporaryId::parse(temporary_id).is_ok()); + let proposals = safe_outputs.read_safe_output_file().await.unwrap(); + assert_eq!(proposals.len(), 1); + assert_eq!(proposals[0]["name"], "create-pull-request"); + assert_eq!(proposals[0]["temporary_id"], temporary_id); + } + #[tokio::test] async fn create_work_item_preserves_html_description_in_proposal() { let (safe_outputs, _temp_dir) = create_test_safe_outputs().await; @@ -2592,6 +2676,24 @@ safe-outputs: assert!(!properties.contains_key("temporary_id")); } + #[tokio::test] + async fn test_create_pr_schema_excludes_internal_and_inline_reviewer_fields() { + let temp_dir = tempfile::tempdir().unwrap(); + let enabled = vec!["create-pull-request".to_string()]; + let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled), None) + .await + .unwrap(); + let tools = so.tool_router.list_all(); + let tool = tools + .iter() + .find(|tool| tool.name.as_ref() == "create-pull-request") + .expect("create-pull-request should be enabled"); + let schema = serde_json::to_value(&tool.input_schema).unwrap(); + let properties = schema["properties"].as_object().unwrap(); + assert!(!properties.contains_key("temporary_id")); + assert!(!properties.contains_key("reviewers")); + } + #[tokio::test] async fn test_github_queue_propagates_ndjson_write_failures() { let temp_dir = tempfile::tempdir().unwrap(); diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 7d2bb145..e1ccae1a 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -8,6 +8,7 @@ use tokio::process::Command; use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, PATH_SEGMENT, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; +use crate::secure::PullRequestTemporaryId; use crate::tool_result; use crate::validate::reject_pipeline_injection; use ado_aw_derive::SanitizeConfig; @@ -231,6 +232,7 @@ fn identity_picker_url(organization: &str) -> String { /// Parameters for creating a pull request #[derive(Deserialize, JsonSchema)] +#[serde(deny_unknown_fields)] pub struct CreatePrParams { /// Title for the pull request; should be concise and descriptive pub title: String, @@ -284,6 +286,7 @@ struct CreatePrResultFields { repository: String, #[serde(default)] agent_labels: Vec, + temporary_id: PullRequestTemporaryId, #[serde(skip_serializing_if = "Option::is_none")] base_commit: Option, /// SHA-256 hex digest of the patch file, recorded at staging time. @@ -311,6 +314,8 @@ tool_result! { /// Agent-provided labels (validated against allowed-labels at execution time) #[serde(default)] agent_labels: Vec, + /// Temporary identifier for later safe outputs in the same run + temporary_id: PullRequestTemporaryId, /// Base commit SHA recorded at patch generation time (merge-base of HEAD and /// the upstream branch). When present, Stage 3 uses this as the parent commit /// for the ADO Push API, ensuring the patch applies cleanly even if the target @@ -695,6 +700,12 @@ impl Executor for CreatePrResult { Err(failure) => return Ok(failure), }; debug!("Resolved repository ID: {}", target.repository_locator()); + if ctx.has_resolved_pull_request(&self.temporary_id)? { + return Ok(ExecutionResult::failure(format!( + "temporary_id '{}' was already used in this run", + self.temporary_id.canonical() + ))); + } let resolved_target_branch = config.resolve_target_branch(&repository_alias, &ctx.repo_refs); @@ -1282,7 +1293,7 @@ impl Executor for CreatePrResult { } let pr_data: serde_json::Value = pr_response.json().await?; - let pr_id = pr_data["pullRequestId"].as_i64().unwrap_or(0); + let pr_id = pr_data["pullRequestId"].as_u64().unwrap_or(0); let pr_web_url = pr_data["url"].as_str().unwrap_or(""); info!("Pull request created: #{} - {}", pr_id, pr_web_url); @@ -1295,7 +1306,36 @@ impl Executor for CreatePrResult { pr_id, token, connection_type: ctx.write_connection_type, + reviewers: &config.reviewers, }; + if pr_id == 0 { + return Ok(ExecutionResult::failure( + "Azure DevOps create-pull-request response contained no positive pull request ID", + )); + } + if let Err(error) = ctx.register_resolved_pull_request( + &self.temporary_id, + crate::safe_outputs::ResolvedPullRequest { + id: pr_id, + url: pr_web_url.to_string(), + target: target.clone(), + }, + ) { + return Ok(ExecutionResult::failure_with_data( + format!( + "Created pull request #{} but failed to register temporary_id '{}': {}", + pr_id, + self.temporary_id.canonical(), + crate::sanitize::neutralize_pipeline_commands(&error.to_string()) + ), + serde_json::json!({ + "pull_request_id": pr_id, + "url": pr_web_url, + "temporary_id": self.temporary_id.canonical(), + "repository": target.display_name(), + }), + )); + } set_pr_completion_options(&pr_ctx, pr_data["createdBy"]["id"].as_str()).await; add_reviewers_to_pr(&pr_ctx).await; @@ -1314,7 +1354,8 @@ impl Executor for CreatePrResult { "url": pr_web_url, "source_branch": source_branch, "target_branch": target_branch, - "draft": config.draft + "draft": config.draft, + "temporary_id": self.temporary_id.canonical(), }), )) } @@ -1711,9 +1752,10 @@ struct PrContext<'a> { client: &'a reqwest::Client, config: &'a CreatePrConfig, target: &'a crate::safe_outputs::result::AdoRepositoryTarget, - pr_id: i64, + pr_id: u64, token: &'a str, connection_type: Option, + reviewers: &'a [String], } /// Set PR completion options (delete-source-branch, squash-merge) and optionally @@ -1776,11 +1818,11 @@ async fn set_pr_completion_options(ctx: &PrContext<'_>, pr_created_by_id: Option /// issues a `PUT` for each one. Logs a warning if a reviewer cannot be resolved or /// if the API call fails; does not abort the overall PR creation. async fn add_reviewers_to_pr(ctx: &PrContext<'_>) { - if ctx.config.reviewers.is_empty() { + if ctx.reviewers.is_empty() { return; } - debug!("Adding {} reviewers", ctx.config.reviewers.len()); - for reviewer in &ctx.config.reviewers { + debug!("Adding {} reviewers", ctx.reviewers.len()); + for reviewer in ctx.reviewers { debug!("Adding reviewer: {}", reviewer); // Resolve reviewer identity (email/name -> ID) @@ -2492,6 +2534,26 @@ mod tests { assert!(params.validate().is_err()); } + #[test] + fn test_params_reject_internal_and_inline_reviewer_fields() { + assert!( + serde_json::from_value::(serde_json::json!({ + "title": "Valid title", + "description": "A sufficiently long description.", + "temporary_id": "#aw_pr123" + })) + .is_err() + ); + assert!( + serde_json::from_value::(serde_json::json!({ + "title": "Valid title", + "description": "A sufficiently long description.", + "reviewers": ["owner@example.com"] + })) + .is_err() + ); + } + #[test] fn test_validate_params_rejects_repository_pipeline_command() { let params = CreatePrParams { @@ -2513,6 +2575,7 @@ mod tests { patch_file: "/tmp/test.patch".to_string(), repository: "##vso[task.setvariable variable=x]y".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test1").unwrap(), base_commit: None, patch_sha256: "0123456789abcdef0123456789abcdef0123456789abcdef0123456789abcdef" .to_string(), @@ -2689,6 +2752,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test1").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2720,6 +2784,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test2").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2746,6 +2811,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec!["unapproved".to_string()], + temporary_id: PullRequestTemporaryId::parse("#aw_test3").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2768,6 +2834,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "not-checked-out".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test4").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -2816,6 +2883,7 @@ mod tests { patch_file: "patch.diff".to_string(), repository: "cross-org-repo".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_test5").unwrap(), base_commit: None, patch_sha256: "deadbeef".to_string(), }; @@ -3404,6 +3472,7 @@ index 0000000..abcdefg patch_file: patch_file.to_string(), repository: "self".to_string(), agent_labels: vec![], + temporary_id: PullRequestTemporaryId::parse("#aw_hash1").unwrap(), base_commit: None, patch_sha256: wrong_hash, }; @@ -3446,6 +3515,9 @@ index 0000000..abcdefg resolved_work_items: std::sync::Arc::new(std::sync::Mutex::new( std::collections::HashMap::new(), )), + resolved_pull_requests: std::sync::Arc::new(std::sync::Mutex::new( + std::collections::HashMap::new(), + )), triggered_by_build_id: None, triggered_by_definition_name: None, triggered_by_build_number: None, diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index bd86a508..2dc4a49d 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -435,19 +435,21 @@ fn split_repository_target_name( pub(crate) fn resolve_repository_write_target( repository: Option<&str>, ctx: &ExecutionContext, -) -> Result { +) -> Result { let selector = repository.unwrap_or("self"); let Some(alias) = canonical_repository_alias(selector, ctx) else { return Err(ExecutionResult::failure(format!( "Repository '{selector}' is not in the allowed repository list" ))); }; - let current_org_url = ctx.ado_org_url.as_deref().ok_or_else(|| { - ExecutionResult::failure("Azure DevOps organization URL not configured") - })?; - let current_organization = ctx.ado_organization.as_deref().ok_or_else(|| { - ExecutionResult::failure("Azure DevOps organization name not configured") - })?; + let current_org_url = ctx + .ado_org_url + .as_deref() + .ok_or_else(|| ExecutionResult::failure("Azure DevOps organization URL not configured"))?; + let current_organization = ctx + .ado_organization + .as_deref() + .ok_or_else(|| ExecutionResult::failure("Azure DevOps organization name not configured"))?; let current_project = ctx .ado_project .as_deref() @@ -459,7 +461,7 @@ pub(crate) fn resolve_repository_write_target( .as_deref() .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set"))?; let (_, repository) = split_repository_target_name(name, current_project)?; - return Ok(crate::safe_outputs::result::AdoRepositoryTarget { + return Ok(AdoRepositoryTarget { alias, organization: current_organization.to_string(), organization_url: current_org_url.trim_end_matches('/').to_string(), @@ -507,8 +509,7 @@ pub(crate) fn resolve_repository_write_target( ))); } - let (project, repository_name) = - split_repository_target_name(&config.name, current_project)?; + let (project, repository_name) = split_repository_target_name(&config.name, current_project)?; let organization = config .organization .as_deref() @@ -534,7 +535,7 @@ pub(crate) fn resolve_repository_write_target( } } - Ok(crate::safe_outputs::result::AdoRepositoryTarget { + Ok(AdoRepositoryTarget { alias, organization: organization.to_string(), organization_url: if cross_organization { @@ -765,9 +766,9 @@ macro_rules! impl_temporary_reference_deserialize { mod add_build_tag; mod add_github_issue_labels; mod add_pr_comment; -mod assign_work_item; mod assign_github_issue_milestone; mod assign_github_issue_to_user; +mod assign_work_item; mod close_github_issue; mod comment_on_github_issue; mod comment_on_work_item; @@ -806,9 +807,9 @@ mod upload_workitem_attachment; pub use add_build_tag::*; pub use add_github_issue_labels::*; pub use add_pr_comment::*; -pub use assign_work_item::*; pub use assign_github_issue_milestone::*; pub use assign_github_issue_to_user::*; +pub use assign_work_item::*; pub use close_github_issue::*; pub use comment_on_github_issue::*; pub use comment_on_work_item::*; @@ -832,8 +833,8 @@ pub use reply_to_pr_comment::*; pub use report_incomplete::*; pub use resolve_pr_thread::*; pub use result::{ - ExecutionContext, ExecutionResult, Executor, ResolvedGithubIssue, ResolvedWorkItem, ToolResult, - Validate, anyhow_to_mcp_error, org_from_url, + AdoRepositoryTarget, ExecutionContext, ExecutionResult, Executor, ResolvedGithubIssue, + ResolvedPullRequest, ResolvedWorkItem, ToolResult, Validate, anyhow_to_mcp_error, org_from_url, }; pub use set_github_issue_field::*; pub use set_github_issue_type::*; @@ -1503,7 +1504,11 @@ mod tests { let error = resolve_repository_write_target(Some("target"), &ctx).unwrap_err(); - assert!(error.message.contains("declares the pipeline's current organization")); + assert!( + error + .message + .contains("declares the pipeline's current organization") + ); } #[test] diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index bd537595..f80c9d49 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -7,7 +7,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex}; use crate::sanitize::{SanitizeConfig, SanitizeContent}; -use crate::secure::{GithubTemporaryId, WorkItemTemporaryId}; +use crate::secure::{GithubTemporaryId, PullRequestTemporaryId, WorkItemTemporaryId}; /// Trait for tool results that include a name field pub trait ToolResult: Serialize { @@ -59,6 +59,14 @@ pub struct ResolvedWorkItem { pub url: String, } +/// An Azure DevOps pull request created earlier in the same Stage 3 execution. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedPullRequest { + pub id: u64, + pub url: String, + pub target: AdoRepositoryTarget, +} + /// Trusted compiler/source metadata for one checked-out repository alias. #[derive(Debug, Clone, PartialEq, Eq)] pub struct AdoRepositoryTargetConfig { @@ -274,6 +282,8 @@ pub struct ExecutionContext { pub resolved_github_issues: Arc>>, /// Temporary work-item IDs resolved by successful `create-work-item` calls. pub resolved_work_items: Arc>>, + /// Temporary pull-request IDs resolved by successful `create-pull-request` calls. + pub resolved_pull_requests: Arc>>, } impl ExecutionContext { @@ -382,6 +392,41 @@ impl ExecutionContext { .map_err(|_| anyhow::anyhow!("temporary work-item map lock poisoned"))?; Ok(work_items.get(&temporary_id.canonical()).cloned()) } + + pub fn has_resolved_pull_request( + &self, + temporary_id: &PullRequestTemporaryId, + ) -> anyhow::Result { + let pull_requests = self + .resolved_pull_requests + .lock() + .map_err(|_| anyhow::anyhow!("temporary pull-request map lock poisoned"))?; + Ok(pull_requests.contains_key(&temporary_id.canonical())) + } + + pub fn register_resolved_pull_request( + &self, + temporary_id: &PullRequestTemporaryId, + pull_request: ResolvedPullRequest, + ) -> anyhow::Result<()> { + register_resolved_reference( + &self.resolved_pull_requests, + temporary_id.canonical(), + pull_request, + "temporary pull-request map lock poisoned", + ) + } + + pub fn resolve_pull_request( + &self, + temporary_id: &PullRequestTemporaryId, + ) -> anyhow::Result> { + let pull_requests = self + .resolved_pull_requests + .lock() + .map_err(|_| anyhow::anyhow!("temporary pull-request map lock poisoned"))?; + Ok(pull_requests.get(&temporary_id.canonical()).cloned()) + } } /// Extract the organization name from an Azure DevOps org URL. @@ -496,6 +541,7 @@ impl ExecutionContext { uploaded_pipeline_artifact_keys: Arc::new(Mutex::new(HashSet::new())), resolved_github_issues: Arc::new(Mutex::new(HashMap::new())), resolved_work_items: Arc::new(Mutex::new(HashMap::new())), + resolved_pull_requests: Arc::new(Mutex::new(HashMap::new())), } } } @@ -579,6 +625,17 @@ impl ExecutionResult { } } + /// Create a warning result with additional data. + pub fn warning_with_data(message: impl Into, data: serde_json::Value) -> Self { + Self { + success: true, + warning: true, + budget_exhausted: false, + message: message.into(), + data: Some(data), + } + } + /// Create a failed execution result pub fn failure(message: impl Into) -> Self { Self { @@ -972,6 +1029,52 @@ mod tests { assert!(r.data.is_none()); } + #[test] + fn warning_with_data_preserves_structured_result() { + let r = ExecutionResult::warning_with_data( + "some reviewers failed", + serde_json::json!({"added": ["one"], "failed": ["two"]}), + ); + assert!(r.success); + assert!(r.is_warning()); + assert_eq!( + r.data.as_ref().unwrap()["failed"], + serde_json::json!(["two"]) + ); + } + + #[test] + fn pull_request_registry_resolves_and_rejects_duplicates() { + let ctx = ExecutionContext::default(); + let temporary_id = PullRequestTemporaryId::parse("aw_pr123").unwrap(); + let resolved = ResolvedPullRequest { + id: 42, + url: "https://example.test/pr/42".to_string(), + target: AdoRepositoryTarget { + alias: "self".to_string(), + organization: "org".to_string(), + organization_url: "https://dev.azure.com/org".to_string(), + project: "project".to_string(), + repository: "repo".to_string(), + repository_id: Some("repo-id".to_string()), + cross_organization: false, + }, + }; + + assert!(!ctx.has_resolved_pull_request(&temporary_id).unwrap()); + ctx.register_resolved_pull_request(&temporary_id, resolved.clone()) + .unwrap(); + assert!(ctx.has_resolved_pull_request(&temporary_id).unwrap()); + assert_eq!( + ctx.resolve_pull_request(&temporary_id).unwrap(), + Some(resolved.clone()) + ); + assert!( + ctx.register_resolved_pull_request(&temporary_id, resolved) + .is_err() + ); + } + #[test] fn test_execution_result_success_is_not_warning() { let r = ExecutionResult::success("all good"); diff --git a/src/safe_outputs/update_pr.rs b/src/safe_outputs/update_pr.rs index 67e44bde..2b25f043 100644 --- a/src/safe_outputs/update_pr.rs +++ b/src/safe_outputs/update_pr.rs @@ -5,10 +5,13 @@ use log::{debug, info, warn}; use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; +use std::fmt; -use super::{PATH_SEGMENT, resolve_repo_name}; +use super::result::AdoRepositoryTarget; +use super::{PATH_SEGMENT, canonical_repository_alias, resolve_repository_write_target}; use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text, sanitize_config}; +use crate::secure::PullRequestTemporaryId; use crate::tool_result; use crate::validate::reject_pipeline_injection; use anyhow::{Context, ensure}; @@ -33,6 +36,33 @@ const VALID_VOTES: &[&str] = &[ /// Valid merge strategy values accepted by ADO's completionOptions.mergeStrategy const VALID_MERGE_STRATEGIES: &[&str] = &["squash", "noFastForward", "rebase", "rebaseMerge"]; +const DEFAULT_MAX_REVIEWERS: usize = 3; +const MAX_REVIEWER_LEN: usize = 256; + +/// Positive Azure DevOps pull-request ID or a same-run temporary ID. +#[derive(Debug, Clone, PartialEq, Eq, Serialize, JsonSchema)] +#[serde(untagged)] +pub enum PullRequestReference { + Number(u64), + Temporary(PullRequestTemporaryId), +} + +impl fmt::Display for PullRequestReference { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Number(id) => write!(formatter, "{id}"), + Self::Temporary(temporary_id) => formatter.write_str(&temporary_id.canonical()), + } + } +} + +impl_temporary_reference_deserialize!( + PullRequestReference, + PullRequestTemporaryId, + expecting = "a positive pull-request ID or #aw_ temporary ID", + negative = "pull_request_id must be positive", + quoted_out_of_range = "quoted pull_request_id is outside the u64 range", +); /// Map a vote string to its ADO numeric value fn vote_to_ado_value(vote: &str) -> Option { @@ -49,8 +79,8 @@ fn vote_to_ado_value(vote: &str) -> Option { /// Parameters for updating a pull request #[derive(Deserialize, JsonSchema)] pub struct UpdatePrParams { - /// Pull request ID (must be positive) - pub pull_request_id: i32, + /// Positive pull request ID or a temporary ID from create-pull-request. + pub pull_request_id: PullRequestReference, /// Repository alias: "self" for the pipeline repo, or an alias from the checkout list #[serde(default)] @@ -74,10 +104,9 @@ pub struct UpdatePrParams { impl Validate for UpdatePrParams { fn validate(&self) -> anyhow::Result<()> { - ensure!( - self.pull_request_id > 0, - "pull_request_id must be a positive integer" - ); + if let PullRequestReference::Number(id) = self.pull_request_id { + ensure!(id > 0, "pull_request_id must be positive"); + } if let Some(repository) = &self.repository { reject_pipeline_injection(repository, "repository")?; } @@ -97,6 +126,19 @@ impl Validate for UpdatePrParams { !reviewers.is_empty(), "reviewers list must not be empty for add-reviewers operation" ); + ensure!( + reviewers.len() <= 100, + "reviewers list must contain at most 100 entries" + ); + for reviewer in reviewers { + let reviewer = reviewer.trim(); + ensure!(!reviewer.is_empty(), "reviewer must not be empty"); + ensure!( + reviewer.len() <= MAX_REVIEWER_LEN, + "reviewer must be {MAX_REVIEWER_LEN} characters or fewer" + ); + reject_pipeline_injection(reviewer, "update-pr.reviewer")?; + } } "add-labels" => { let labels = self @@ -141,7 +183,7 @@ tool_result! { params = UpdatePrParams, /// Result of updating a pull request pub struct UpdatePrResult { - pull_request_id: i32, + pull_request_id: PullRequestReference, repository: Option, operation: String, reviewers: Option>, @@ -203,6 +245,16 @@ pub struct UpdatePrConfig { #[serde(default, rename = "allowed-votes")] pub allowed_votes: Vec, + /// Case-insensitive exact allowlist for model-selected reviewers. + /// Empty rejects all reviewers; a literal "*" allows any valid reviewer. + #[serde(default, rename = "allowed-reviewers")] + pub allowed_reviewers: Vec, + + /// Maximum reviewers accepted by one add-reviewers operation. + #[serde(default = "default_max_reviewers", rename = "max-reviewers")] + #[sanitize_config(skip)] + pub max_reviewers: usize, + /// Whether to delete the source branch after merge (for set-auto-complete, default: true) #[serde(default = "default_true", rename = "delete-source-branch")] pub delete_source_branch: bool, @@ -220,18 +272,121 @@ fn default_merge_strategy() -> String { "squash".to_string() } +fn default_max_reviewers() -> usize { + DEFAULT_MAX_REVIEWERS +} + impl Default for UpdatePrConfig { fn default() -> Self { Self { allowed_operations: Vec::new(), allowed_repositories: Vec::new(), allowed_votes: Vec::new(), + allowed_reviewers: Vec::new(), + max_reviewers: default_max_reviewers(), delete_source_branch: true, merge_strategy: "squash".to_string(), } } } +struct UpdatePrContext<'a> { + client: &'a reqwest::Client, + target: AdoRepositoryTarget, + pr_id: u64, + token: &'a str, + connection_type: Option, +} + +impl UpdatePrContext<'_> { + fn repository_api_base(&self) -> String { + format!( + "{}/{}/_apis/git/repositories/{}", + self.target.organization_url, + utf8_percent_encode(&self.target.project, PATH_SEGMENT), + utf8_percent_encode(self.target.repository_locator(), PATH_SEGMENT), + ) + } +} + +fn repository_is_allowed(config: &UpdatePrConfig, alias: &str) -> bool { + config.allowed_repositories.is_empty() + || config + .allowed_repositories + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(alias)) +} + +fn resolve_update_pr_target( + reference: &PullRequestReference, + requested_repository: Option<&str>, + config: &UpdatePrConfig, + ctx: &ExecutionContext, +) -> anyhow::Result> { + match reference { + PullRequestReference::Number(id) => { + if *id == 0 { + return Ok(Err(ExecutionResult::failure( + "pull_request_id must be positive", + ))); + } + let selector = requested_repository.unwrap_or("self"); + let Some(alias) = canonical_repository_alias(selector, ctx) else { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + crate::sanitize::neutralize_pipeline_commands(selector) + )))); + }; + if !repository_is_allowed(config, &alias) { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + alias, + config.allowed_repositories.join(", ") + )))); + } + let target = match resolve_repository_write_target(Some(&alias), ctx) { + Ok(target) => target, + Err(error) => return Ok(Err(error)), + }; + Ok(Ok((*id, target))) + } + PullRequestReference::Temporary(temporary_id) => { + let Some(resolved) = ctx.resolve_pull_request(temporary_id)? else { + return Ok(Err(ExecutionResult::failure(format!( + "temporary pull-request ID '{}' has not been resolved; \ + create-pull-request must succeed earlier in the same SafeOutputs job", + temporary_id.canonical() + )))); + }; + if let Some(selector) = requested_repository { + let Some(alias) = canonical_repository_alias(selector, ctx) else { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + crate::sanitize::neutralize_pipeline_commands(selector) + )))); + }; + if !alias.eq_ignore_ascii_case(&resolved.target.alias) { + return Ok(Err(ExecutionResult::failure(format!( + "temporary pull-request ID '{}' resolved to repository '{}', which does \ + not match requested repository '{}'", + temporary_id.canonical(), + resolved.target.alias, + crate::sanitize::neutralize_pipeline_commands(selector) + )))); + } + } + if !repository_is_allowed(config, &resolved.target.alias) { + return Ok(Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed-repositories list: [{}]", + resolved.target.alias, + config.allowed_repositories.join(", ") + )))); + } + Ok(Ok((resolved.id, resolved.target))) + } + } +} + #[async_trait::async_trait] impl Executor for UpdatePrResult { fn dry_run_summary(&self) -> String { @@ -248,20 +403,10 @@ impl Executor for UpdatePrResult { self.pull_request_id, self.operation ); - let org_url = ctx - .ado_org_url - .as_ref() - .context("AZURE_DEVOPS_ORG_URL not set")?; - let project = ctx - .ado_project - .as_ref() - .context("SYSTEM_TEAMPROJECT not set")?; let token = ctx .access_token .as_ref() .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - debug!("ADO org: {}, project: {}", org_url, project); - let config: UpdatePrConfig = ctx.get_tool_config("update-pr")?; debug!("Config: {:?}", config); @@ -276,58 +421,35 @@ impl Executor for UpdatePrResult { ))); } - // Validate repository against allowed-repositories - let repo_alias = self.repository.as_deref().unwrap_or("self"); - if !config.allowed_repositories.is_empty() - && !config - .allowed_repositories - .contains(&repo_alias.to_string()) - { - return Ok(ExecutionResult::failure(format!( - "Repository '{}' is not in the allowed-repositories list: [{}]", - repo_alias, - config.allowed_repositories.join(", ") - ))); - } - - // Resolve repo name - let repo_name = match resolve_repo_name(self.repository.as_deref(), ctx) { - Ok(name) => name, + let (pr_id, target) = match resolve_update_pr_target( + &self.pull_request_id, + self.repository.as_deref(), + &config, + ctx, + )? { + Ok(target) => target, Err(failure) => return Ok(failure), }; - debug!("Resolved repository: {}", repo_name); + debug!("Resolved PR target: {} #{}", target.display_name(), pr_id); let client = reqwest::Client::new(); - let encoded_project = utf8_percent_encode(project, PATH_SEGMENT).to_string(); - let base_url = format!( - "{}/{}/_apis/git/repositories", - org_url.trim_end_matches('/'), - encoded_project, - ); + let operation_ctx = UpdatePrContext { + client: &client, + target, + pr_id, + token, + connection_type: ctx.write_connection_type, + }; match self.operation.as_str() { "set-auto-complete" => { - self.execute_set_auto_complete( - &client, &base_url, &repo_name, token, org_url, &config, - ) - .await - } - "vote" => { - self.execute_vote(&client, &base_url, &repo_name, token, org_url, &config) - .await - } - "add-reviewers" => { - self.execute_add_reviewers(&client, &base_url, &repo_name, token, org_url) - .await - } - "add-labels" => { - self.execute_add_labels(&client, &base_url, &repo_name, token) - .await - } - "update-description" => { - self.execute_update_description(&client, &base_url, &repo_name, token) + self.execute_set_auto_complete(&operation_ctx, &config) .await } + "vote" => self.execute_vote(&operation_ctx, &config).await, + "add-reviewers" => self.execute_add_reviewers(&operation_ctx, &config).await, + "add-labels" => self.execute_add_labels(&operation_ctx).await, + "update-description" => self.execute_update_description(&operation_ctx).await, _ => Ok(ExecutionResult::failure(format!( "Unknown operation: {}", self.operation @@ -342,6 +464,84 @@ enum ReviewerAddResult { Failed(String), } +fn reviewer_execution_result( + pr_id: u64, + added: Vec, + failed: Vec, +) -> ExecutionResult { + let mut message = format!("Added {} reviewer(s) to PR #{}", added.len(), pr_id); + if !failed.is_empty() { + message.push_str(&format!( + " ({} failed: {})", + failed.len(), + failed.join(", ") + )); + } + let has_failures = !failed.is_empty(); + let data = serde_json::json!({ + "pull_request_id": pr_id, + "operation": "add-reviewers", + "added": added, + "failed": failed, + }); + if has_failures { + ExecutionResult::warning_with_data(message, data) + } else { + ExecutionResult::success_with_data(message, data) + } +} + +fn validate_and_normalize_reviewers( + reviewers: &[String], + config: &UpdatePrConfig, +) -> Result, ExecutionResult> { + if config.max_reviewers == 0 { + return Err(ExecutionResult::failure( + "update-pr.max-reviewers must be greater than zero", + )); + } + let allow_any = config + .allowed_reviewers + .iter() + .any(|allowed| allowed == "*"); + if config.allowed_reviewers.is_empty() { + return Err(ExecutionResult::failure( + "add-reviewers requires allowed-reviewers to be configured; use \ + allowed-reviewers: [\"*\"] to permit any valid reviewer", + )); + } + + let mut normalized = Vec::new(); + for reviewer in reviewers { + let reviewer = reviewer.trim(); + if !allow_any + && !config + .allowed_reviewers + .iter() + .any(|allowed| allowed.eq_ignore_ascii_case(reviewer)) + { + return Err(ExecutionResult::failure(format!( + "Reviewer '{}' is not in update-pr.allowed-reviewers", + crate::sanitize::neutralize_pipeline_commands(reviewer) + ))); + } + if !normalized + .iter() + .any(|existing: &String| existing.eq_ignore_ascii_case(reviewer)) + { + normalized.push(reviewer.to_string()); + } + } + if normalized.len() > config.max_reviewers { + return Err(ExecutionResult::failure(format!( + "add-reviewers requested {} unique reviewers, exceeding max-reviewers: {}", + normalized.len(), + config.max_reviewers + ))); + } + Ok(normalized) +} + impl UpdatePrResult { /// Set auto-complete on a pull request. /// @@ -350,11 +550,7 @@ impl UpdatePrResult { /// Uses the agent's own identity (not the PR creator) for proper audit trail. async fn execute_set_auto_complete( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, - org_url: &str, + operation_ctx: &UpdatePrContext<'_>, config: &UpdatePrConfig, ) -> anyhow::Result { // Validate merge_strategy before any network I/O @@ -366,16 +562,19 @@ impl UpdatePrResult { ))); } - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); - // Resolve the agent's identity via connection data - let connection_url = format!("{}/_apis/connectiondata", org_url.trim_end_matches('/')); - let conn_response = client - .get(&connection_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to fetch connection data for auto-complete identity")?; + let connection_url = format!( + "{}/_apis/connectiondata", + operation_ctx.target.organization_url.trim_end_matches('/') + ); + let conn_response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.get(&connection_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .send() + .await + .context("Failed to fetch connection data for auto-complete identity")?; if !conn_response.status().is_success() { let status = conn_response.status(); @@ -403,8 +602,9 @@ impl UpdatePrResult { // PATCH to set auto-complete using the agent's identity let patch_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + "{}/pullRequests/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); let patch_body = serde_json::json!({ "autoCompleteSetBy": { @@ -416,22 +616,24 @@ impl UpdatePrResult { } }); - info!("Setting auto-complete on PR #{}", self.pull_request_id); - let response = client - .patch(&patch_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&patch_body) - .send() - .await - .context("Failed to set auto-complete on PR")?; + info!("Setting auto-complete on PR #{}", operation_ctx.pr_id); + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.patch(&patch_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&patch_body) + .send() + .await + .context("Failed to set auto-complete on PR")?; if response.status().is_success() { - info!("Auto-complete set on PR #{}", self.pull_request_id); + info!("Auto-complete set on PR #{}", operation_ctx.pr_id); Ok(ExecutionResult::success_with_data( - format!("Auto-complete set on PR #{}", self.pull_request_id), + format!("Auto-complete set on PR #{}", operation_ctx.pr_id), serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "set-auto-complete", }), )) @@ -443,7 +645,7 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); Ok(ExecutionResult::failure(format!( "Failed to set auto-complete on PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))) } } @@ -454,11 +656,7 @@ impl UpdatePrResult { /// PUTs the vote to the reviewers endpoint. async fn execute_vote( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, - org_url: &str, + operation_ctx: &UpdatePrContext<'_>, config: &UpdatePrConfig, ) -> anyhow::Result { let vote_str = self @@ -492,15 +690,20 @@ impl UpdatePrResult { // Resolve the current user identity. // Use the org URL for connection data — supports vanity domains and national clouds. - let connection_url = format!("{}/_apis/connectiondata", org_url.trim_end_matches('/')); + let connection_url = format!( + "{}/_apis/connectiondata", + operation_ctx.target.organization_url.trim_end_matches('/') + ); debug!("Connection data URL: {}", connection_url); - let conn_response = client - .get(&connection_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to fetch connection data")?; + let conn_response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.get(&connection_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .send() + .await + .context("Failed to fetch connection data")?; if !conn_response.status().is_success() { let status = conn_response.status(); @@ -530,17 +733,19 @@ impl UpdatePrResult { // Positive votes (approve=10, approve-with-suggestions=5) are blocked when // the authenticated user is also the PR author. if vote_value > 0 { - let encoded_repo_check = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let pr_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo_check, self.pull_request_id + "{}/pullRequests/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); - let pr_response = client - .get(&pr_url) - .basic_auth("", Some(token)) - .send() - .await - .context("Failed to fetch PR for self-approval check")?; + let pr_response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.get(&pr_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .send() + .await + .context("Failed to fetch PR for self-approval check")?; if pr_response.status().is_success() { let pr_body: serde_json::Value = pr_response @@ -557,7 +762,7 @@ impl UpdatePrResult { return Ok(ExecutionResult::failure(format!( "Self-approval blocked: the authenticated identity created PR #{} \ and cannot cast a positive vote ('{}') on it", - self.pull_request_id, vote_str + operation_ctx.pr_id, vote_str ))); } } else { @@ -568,17 +773,18 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); return Ok(ExecutionResult::failure(format!( "Failed to fetch PR #{} for self-approval check (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))); } } // PUT vote to reviewers endpoint - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let encoded_user_id = utf8_percent_encode(user_id, PATH_SEGMENT).to_string(); let vote_url = format!( - "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id, encoded_user_id + "{}/pullRequests/{}/reviewers/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id, + encoded_user_id ); let vote_body = serde_json::json!({ "vote": vote_value @@ -586,29 +792,31 @@ impl UpdatePrResult { info!( "Voting '{}' ({}) on PR #{}", - vote_str, vote_value, self.pull_request_id + vote_str, vote_value, operation_ctx.pr_id ); - let response = client - .put(&vote_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&vote_body) - .send() - .await - .context("Failed to submit vote")?; + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.put(&vote_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&vote_body) + .send() + .await + .context("Failed to submit vote")?; if response.status().is_success() { info!( "Vote '{}' submitted on PR #{}", - vote_str, self.pull_request_id + vote_str, operation_ctx.pr_id ); Ok(ExecutionResult::success_with_data( format!( "Vote '{}' submitted on PR #{}", - vote_str, self.pull_request_id + vote_str, operation_ctx.pr_id ), serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "vote", "vote": vote_str, "vote_value": vote_value, @@ -622,7 +830,7 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); Ok(ExecutionResult::failure(format!( "Failed to submit vote on PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))) } } @@ -633,23 +841,23 @@ impl UpdatePrResult { /// the reviewers endpoint with vote 0. async fn execute_add_reviewers( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, - org_url: &str, + operation_ctx: &UpdatePrContext<'_>, + config: &UpdatePrConfig, ) -> anyhow::Result { - let reviewers = self + let requested_reviewers = self .reviewers .as_ref() .context("reviewers list is required for add-reviewers operation")?; + let reviewers = match validate_and_normalize_reviewers(requested_reviewers, config) { + Ok(reviewers) => reviewers, + Err(failure) => return Ok(failure), + }; - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let mut added = Vec::new(); let mut failed = Vec::new(); // Derive VSSPS base URL once, before the loop. - let trimmed_org = org_url.trim_end_matches('/'); + let trimmed_org = operation_ctx.target.organization_url.trim_end_matches('/'); let vssps_base = trimmed_org.replace("://dev.azure.com/", "://vssps.dev.azure.com/"); if vssps_base == trimmed_org { return Ok(ExecutionResult::failure(format!( @@ -661,15 +869,15 @@ impl UpdatePrResult { ))); } - for reviewer in reviewers { + for reviewer in &reviewers { match resolve_and_add_reviewer( - client, + operation_ctx.client, &vssps_base, - base_url, - &encoded_repo, - self.pull_request_id, + &operation_ctx.repository_api_base(), + operation_ctx.pr_id, reviewer, - token, + operation_ctx.token, + operation_ctx.connection_type, ) .await { @@ -680,35 +888,11 @@ impl UpdatePrResult { } } - if added.is_empty() && !failed.is_empty() { - Ok(ExecutionResult::failure(format!( - "Failed to add any reviewers to PR #{}: {}", - self.pull_request_id, - failed.join(", ") - ))) - } else { - let mut message = format!( - "Added {} reviewer(s) to PR #{}", - added.len(), - self.pull_request_id - ); - if !failed.is_empty() { - message.push_str(&format!( - " ({} failed: {})", - failed.len(), - failed.join(", ") - )); - } - Ok(ExecutionResult::success_with_data( - message, - serde_json::json!({ - "pull_request_id": self.pull_request_id, - "operation": "add-reviewers", - "added": added, - "failed": failed, - }), - )) - } + Ok(reviewer_execution_result( + operation_ctx.pr_id, + added, + failed, + )) } /// Add labels to a pull request. @@ -716,20 +900,17 @@ impl UpdatePrResult { /// For each label, POSTs to the labels endpoint. async fn execute_add_labels( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, + operation_ctx: &UpdatePrContext<'_>, ) -> anyhow::Result { let labels = self .labels .as_ref() .context("labels list is required for add-labels operation")?; - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let labels_url = format!( - "{}/{}/pullRequests/{}/labels?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + "{}/pullRequests/{}/labels?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); let mut added = Vec::new(); @@ -740,18 +921,20 @@ impl UpdatePrResult { "name": label }); - debug!("Adding label '{}' to PR #{}", label, self.pull_request_id); - let response = client - .post(&labels_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&label_body) - .send() - .await; + debug!("Adding label '{}' to PR #{}", label, operation_ctx.pr_id); + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.post(&labels_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&label_body) + .send() + .await; match response { Ok(resp) if resp.status().is_success() => { - info!("Added label '{}' to PR #{}", label, self.pull_request_id); + info!("Added label '{}' to PR #{}", label, operation_ctx.pr_id); added.push(label.clone()); } Ok(resp) => { @@ -762,14 +945,14 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); warn!( "Failed to add label '{}' to PR #{} (HTTP {}): {}", - label, self.pull_request_id, status, error_body + label, operation_ctx.pr_id, status, error_body ); failed.push(format!("{} (HTTP {})", label, status)); } Err(e) => { warn!( "Request failed for label '{}' on PR #{}: {}", - label, self.pull_request_id, e + label, operation_ctx.pr_id, e ); failed.push(format!("{} (request error)", label)); } @@ -779,14 +962,14 @@ impl UpdatePrResult { if added.is_empty() && !failed.is_empty() { Ok(ExecutionResult::failure(format!( "Failed to add any labels to PR #{}: {}", - self.pull_request_id, + operation_ctx.pr_id, failed.join(", ") ))) } else { let mut message = format!( "Added {} label(s) to PR #{}", added.len(), - self.pull_request_id + operation_ctx.pr_id ); if !failed.is_empty() { message.push_str(&format!( @@ -798,7 +981,7 @@ impl UpdatePrResult { Ok(ExecutionResult::success_with_data( message, serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "add-labels", "added": added, "failed": failed, @@ -810,20 +993,17 @@ impl UpdatePrResult { /// Update the description of a pull request. async fn execute_update_description( &self, - client: &reqwest::Client, - base_url: &str, - repo_name: &str, - token: &str, + operation_ctx: &UpdatePrContext<'_>, ) -> anyhow::Result { let description = self .description .as_ref() .context("description is required for update-description operation")?; - let encoded_repo = utf8_percent_encode(repo_name, PATH_SEGMENT).to_string(); let patch_url = format!( - "{}/{}/pullRequests/{}?api-version=7.1", - base_url, encoded_repo, self.pull_request_id + "{}/pullRequests/{}?api-version=7.1", + operation_ctx.repository_api_base(), + operation_ctx.pr_id ); let patch_body = serde_json::json!({ "description": description @@ -831,24 +1011,26 @@ impl UpdatePrResult { info!( "Updating description on PR #{} ({} chars)", - self.pull_request_id, + operation_ctx.pr_id, description.len() ); - let response = client - .patch(&patch_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&patch_body) - .send() - .await - .context("Failed to update PR description")?; + let response = crate::safe_outputs::authenticate_ado_request( + operation_ctx.client.patch(&patch_url), + operation_ctx.token, + operation_ctx.connection_type, + ) + .header("Content-Type", "application/json") + .json(&patch_body) + .send() + .await + .context("Failed to update PR description")?; if response.status().is_success() { - info!("Description updated on PR #{}", self.pull_request_id); + info!("Description updated on PR #{}", operation_ctx.pr_id); Ok(ExecutionResult::success_with_data( - format!("Description updated on PR #{}", self.pull_request_id), + format!("Description updated on PR #{}", operation_ctx.pr_id), serde_json::json!({ - "pull_request_id": self.pull_request_id, + "pull_request_id": operation_ctx.pr_id, "operation": "update-description", }), )) @@ -860,7 +1042,7 @@ impl UpdatePrResult { .unwrap_or_else(|_| "Unknown error".to_string()); Ok(ExecutionResult::failure(format!( "Failed to update description on PR #{} (HTTP {}): {}", - self.pull_request_id, status, error_body + operation_ctx.pr_id, status, error_body ))) } } @@ -874,7 +1056,21 @@ async fn lookup_reviewer_id( vssps_base: &str, reviewer: &str, token: &str, + connection_type: Option, ) -> Option { + if reviewer.len() == 36 + && reviewer + .chars() + .filter(|character| *character == '-') + .count() + == 4 + && reviewer + .chars() + .all(|character| character.is_ascii_hexdigit() || character == '-') + { + return Some(reviewer.to_string()); + } + let identity_url = format!( "{}/_apis/identities?searchFilter=General&filterValue={}&api-version=7.1", vssps_base, @@ -882,20 +1078,51 @@ async fn lookup_reviewer_id( ); debug!("Resolving identity for '{}': {}", reviewer, identity_url); - match client - .get(&identity_url) - .basic_auth("", Some(token)) - .send() - .await + match crate::safe_outputs::authenticate_ado_request( + client.get(&identity_url), + token, + connection_type, + ) + .send() + .await { Ok(resp) if resp.status().is_success() => { let body: serde_json::Value = resp.json().await.unwrap_or_default(); - body.get("value") + let matching_ids = body + .get("value") .and_then(|v| v.as_array()) - .and_then(|arr| arr.first()) - .and_then(|entry| entry.get("id")) - .and_then(|id| id.as_str()) - .map(|s| s.to_string()) + .into_iter() + .flatten() + .filter(|identity| { + let direct_match = ["providerDisplayName", "customDisplayName", "displayName"] + .iter() + .filter_map(|field| identity.get(field).and_then(serde_json::Value::as_str)) + .any(|value| value.eq_ignore_ascii_case(reviewer)); + let property_match = ["Account", "Mail"] + .iter() + .filter_map(|field| { + identity + .get("properties") + .and_then(|properties| properties.get(field)) + .and_then(|property| property.get("$value")) + .and_then(serde_json::Value::as_str) + }) + .any(|value| value.eq_ignore_ascii_case(reviewer)); + direct_match || property_match + }) + .filter_map(|entry| entry.get("id").and_then(serde_json::Value::as_str)) + .collect::>(); + if matching_ids.len() == 1 { + matching_ids.into_iter().next().map(str::to_string) + } else { + if matching_ids.len() > 1 { + warn!( + "Identity lookup for '{}' returned multiple exact matches", + reviewer + ); + } + None + } } Ok(resp) => { warn!( @@ -917,27 +1144,29 @@ async fn lookup_reviewer_id( /// with a short reason string on any HTTP or transport error. async fn add_reviewer_to_pr( client: &reqwest::Client, - base_url: &str, - encoded_repo: &str, - pr_id: i32, + repository_api_base: &str, + pr_id: u64, reviewer_id: &str, reviewer: &str, token: &str, + connection_type: Option, ) -> ReviewerAddResult { let reviewer_url = format!( - "{}/{}/pullRequests/{}/reviewers/{}?api-version=7.1", - base_url, encoded_repo, pr_id, reviewer_id, + "{}/pullRequests/{}/reviewers/{}?api-version=7.1", + repository_api_base, pr_id, reviewer_id, ); let reviewer_body = serde_json::json!({ "vote": 0, "isRequired": false }); debug!("Adding reviewer '{}' to PR #{}", reviewer, pr_id); - let response = client - .put(&reviewer_url) - .header("Content-Type", "application/json") - .basic_auth("", Some(token)) - .json(&reviewer_body) - .send() - .await; + let response = crate::safe_outputs::authenticate_ado_request( + client.put(&reviewer_url), + token, + connection_type, + ) + .header("Content-Type", "application/json") + .json(&reviewer_body) + .send() + .await; match response { Ok(resp) if resp.status().is_success() => { @@ -972,24 +1201,26 @@ async fn add_reviewer_to_pr( async fn resolve_and_add_reviewer( client: &reqwest::Client, vssps_base: &str, - base_url: &str, - encoded_repo: &str, - pr_id: i32, + repository_api_base: &str, + pr_id: u64, reviewer: &str, token: &str, + connection_type: Option, ) -> ReviewerAddResult { - let Some(reviewer_id) = lookup_reviewer_id(client, vssps_base, reviewer, token).await else { + let Some(reviewer_id) = + lookup_reviewer_id(client, vssps_base, reviewer, token, connection_type).await + else { warn!("Could not resolve identity for '{}', skipping", reviewer); return ReviewerAddResult::Failed("identity not found".to_string()); }; add_reviewer_to_pr( client, - base_url, - encoded_repo, + repository_api_base, pr_id, &reviewer_id, reviewer, token, + connection_type, ) .await } @@ -1011,15 +1242,24 @@ mod tests { "operation": "set-auto-complete" }"#; let params: UpdatePrParams = serde_json::from_str(json).unwrap(); - assert_eq!(params.pull_request_id, 42); + assert_eq!(params.pull_request_id, PullRequestReference::Number(42)); assert_eq!(params.operation, "set-auto-complete"); assert!(params.repository.is_none()); } + #[test] + fn pull_request_reference_accepts_quoted_numbers_and_temporary_ids() { + let quoted: PullRequestReference = serde_json::from_str("\"42\"").unwrap(); + let temporary: PullRequestReference = serde_json::from_str("\"#aw_pr123\"").unwrap(); + assert_eq!(quoted, PullRequestReference::Number(42)); + assert!(matches!(temporary, PullRequestReference::Temporary(_))); + assert!(serde_json::from_str::("\"not-an-id\"").is_err()); + } + #[test] fn test_params_converts_to_result() { let params = UpdatePrParams { - pull_request_id: 42, + pull_request_id: PullRequestReference::Number(42), repository: Some("self".to_string()), operation: "set-auto-complete".to_string(), reviewers: None, @@ -1029,14 +1269,14 @@ mod tests { }; let result: UpdatePrResult = params.try_into().unwrap(); assert_eq!(result.name, "update-pr"); - assert_eq!(result.pull_request_id, 42); + assert_eq!(result.pull_request_id, PullRequestReference::Number(42)); assert_eq!(result.operation, "set-auto-complete"); } #[test] fn test_validation_rejects_zero_pr_id() { let params = UpdatePrParams { - pull_request_id: 0, + pull_request_id: PullRequestReference::Number(0), repository: None, operation: "set-auto-complete".to_string(), reviewers: None, @@ -1051,7 +1291,7 @@ mod tests { #[test] fn test_validation_rejects_invalid_operation() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: None, operation: "delete-pr".to_string(), reviewers: None, @@ -1067,7 +1307,7 @@ mod tests { #[test] fn test_validation_rejects_vote_without_value() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: None, operation: "vote".to_string(), reviewers: None, @@ -1082,7 +1322,7 @@ mod tests { #[test] fn test_validation_rejects_reviewers_without_list() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: None, operation: "add-reviewers".to_string(), reviewers: None, @@ -1097,7 +1337,7 @@ mod tests { #[test] fn test_validation_rejects_repository_pipeline_command() { let params = UpdatePrParams { - pull_request_id: 1, + pull_request_id: PullRequestReference::Number(1), repository: Some("##vso[task.setvariable variable=x]y".to_string()), operation: "set-auto-complete".to_string(), reviewers: None, @@ -1112,7 +1352,7 @@ mod tests { #[test] fn test_result_serializes_correctly() { let params = UpdatePrParams { - pull_request_id: 99, + pull_request_id: PullRequestReference::Number(99), repository: Some("self".to_string()), operation: "vote".to_string(), reviewers: None, @@ -1134,9 +1374,211 @@ mod tests { assert!(config.allowed_operations.is_empty()); assert!(config.allowed_repositories.is_empty()); assert!(config.allowed_votes.is_empty()); + assert!(config.allowed_reviewers.is_empty()); + assert_eq!(config.max_reviewers, DEFAULT_MAX_REVIEWERS); assert_eq!(config.merge_strategy, "squash"); } + #[test] + fn reviewer_policy_is_default_deny_and_supports_explicit_wildcard() { + let reviewers = vec!["owner@example.com".to_string()]; + let denied = validate_and_normalize_reviewers(&reviewers, &UpdatePrConfig::default()); + assert!(denied.unwrap_err().message.contains("allowed-reviewers")); + + let config = UpdatePrConfig { + allowed_reviewers: vec!["*".to_string()], + ..Default::default() + }; + assert_eq!( + validate_and_normalize_reviewers(&reviewers, &config).unwrap(), + reviewers + ); + } + + #[test] + fn reviewer_policy_deduplicates_and_enforces_limit() { + let config = UpdatePrConfig { + allowed_reviewers: vec![ + "Owner@example.com".to_string(), + "other@example.com".to_string(), + ], + max_reviewers: 2, + ..Default::default() + }; + let reviewers = validate_and_normalize_reviewers( + &[ + "owner@example.com".to_string(), + "OWNER@example.com".to_string(), + ], + &config, + ) + .unwrap(); + assert_eq!(reviewers, ["owner@example.com"]); + + let too_many = validate_and_normalize_reviewers( + &[ + "owner@example.com".to_string(), + "other@example.com".to_string(), + "third@example.com".to_string(), + ], + &UpdatePrConfig { + allowed_reviewers: vec!["*".to_string()], + max_reviewers: 2, + ..Default::default() + }, + ); + assert!(too_many.unwrap_err().message.contains("max-reviewers")); + } + + #[test] + fn reviewer_results_warn_for_partial_and_total_failures() { + let partial = reviewer_execution_result( + 42, + vec!["added@example.com".to_string()], + vec!["failed@example.com (HTTP 403)".to_string()], + ); + assert!(partial.success); + assert!(partial.is_warning()); + assert_eq!( + partial.data.as_ref().unwrap()["added"][0], + "added@example.com" + ); + + let total = reviewer_execution_result( + 42, + Vec::new(), + vec!["failed@example.com (identity not found)".to_string()], + ); + assert!(total.success); + assert!(total.is_warning()); + assert_eq!( + total.data.as_ref().unwrap()["failed"] + .as_array() + .unwrap() + .len(), + 1 + ); + + let success = + reviewer_execution_result(42, vec!["added@example.com".to_string()], Vec::new()); + assert!(success.success); + assert!(!success.is_warning()); + } + + #[test] + fn temporary_reference_resolves_exact_registered_target() { + let temporary_id = PullRequestTemporaryId::parse("#aw_pr123").unwrap(); + let ctx = ExecutionContext::default(); + let target = AdoRepositoryTarget { + alias: "tools".to_string(), + organization: "other-org".to_string(), + organization_url: "https://dev.azure.com/other-org".to_string(), + project: "Other Project".to_string(), + repository: "tools".to_string(), + repository_id: Some("repo-id".to_string()), + cross_organization: true, + }; + ctx.register_resolved_pull_request( + &temporary_id, + crate::safe_outputs::ResolvedPullRequest { + id: 42, + url: "https://example.test/pr/42".to_string(), + target: target.clone(), + }, + ) + .unwrap(); + + let resolved = resolve_update_pr_target( + &PullRequestReference::Temporary(temporary_id), + None, + &UpdatePrConfig::default(), + &ctx, + ) + .unwrap() + .unwrap(); + assert_eq!(resolved, (42, target)); + } + + #[tokio::test] + async fn reviewer_identity_lookup_requires_exact_match() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/_apis/identities")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [ + { + "id": "wrong-id", + "providerDisplayName": "Similar Person", + "properties": {"Mail": {"$value": "similar@example.com"}} + }, + { + "id": "exact-id", + "providerDisplayName": "Exact Person", + "properties": {"Mail": {"$value": "owner@example.com"}} + } + ] + }))) + .mount(&server) + .await; + + let id = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + "owner@example.com", + "token", + None, + ) + .await; + assert_eq!(id.as_deref(), Some("exact-id")); + + let missing = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + "missing@example.com", + "token", + None, + ) + .await; + assert!(missing.is_none()); + } + + #[tokio::test] + async fn reviewer_identity_lookup_rejects_ambiguous_exact_matches() { + use wiremock::matchers::{method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/_apis/identities")) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ + "value": [ + { + "id": "first-id", + "properties": {"Mail": {"$value": "owner@example.com"}} + }, + { + "id": "second-id", + "properties": {"Mail": {"$value": "owner@example.com"}} + } + ] + }))) + .mount(&server) + .await; + + let id = lookup_reviewer_id( + &reqwest::Client::new(), + &server.uri(), + "owner@example.com", + "token", + None, + ) + .await; + assert!(id.is_none()); + } + #[test] fn test_config_deserializes_from_yaml() { let yaml = r#" @@ -1148,6 +1590,9 @@ allowed-repositories: allowed-votes: - approve - reject +allowed-reviewers: + - owner@example.com +max-reviewers: 2 "#; let config: UpdatePrConfig = serde_yaml::from_str(yaml).unwrap(); assert_eq!(config.allowed_operations.len(), 2); @@ -1163,6 +1608,8 @@ allowed-votes: ); assert_eq!(config.allowed_repositories.len(), 1); assert_eq!(config.allowed_votes.len(), 2); + assert_eq!(config.allowed_reviewers, ["owner@example.com"]); + assert_eq!(config.max_reviewers, 2); } #[test] diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index 70dd486e..d0648f65 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -946,6 +946,9 @@ attachment-type: "agent-artifact" resolved_work_items: std::sync::Arc::new(std::sync::Mutex::new( std::collections::HashMap::new(), )), + resolved_pull_requests: std::sync::Arc::new(std::sync::Mutex::new( + std::collections::HashMap::new(), + )), triggered_by_build_id: None, triggered_by_definition_name: None, triggered_by_build_number: None, diff --git a/src/secure.rs b/src/secure.rs index 41e95c77..3c566ba0 100644 --- a/src/secure.rs +++ b/src/secure.rs @@ -304,6 +304,11 @@ validated_string! { WorkItemTemporaryId, "temporary_id", validate_temporary_id } +validated_string! { + /// A temporary Azure DevOps pull-request identifier used to link safe outputs in one run. + PullRequestTemporaryId, "temporary_id", validate_temporary_id +} + impl GithubTemporaryId { /// Canonical map/reference form with the leading `#`. pub fn canonical(&self) -> String { @@ -326,6 +331,17 @@ impl WorkItemTemporaryId { } } +impl PullRequestTemporaryId { + /// Canonical map/reference form with the leading `#`. + pub fn canonical(&self) -> String { + if self.as_str().starts_with('#') { + self.as_str().to_string() + } else { + format!("#{}", self.as_str()) + } + } +} + validated_string! { /// An Azure DevOps pipeline or variable-group variable name. ///