feat(safe-outputs): allow model-selected PR reviewers - #2099
feat(safe-outputs): allow model-selected PR reviewers#2099jamesadevine with Copilot wants to merge 7 commits into
Conversation
|
Azure Pipelines: 2 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Use generated temporary PR references so update-pr can safely apply model-selected reviewers and other follow-up operations after PR creation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93d15887-5c0e-4975-98a2-82ca84d35d5f
Exercise ordered create-pull-request and update-pr NDJSON execution through the shared temporary PR registry and mocked Azure DevOps APIs. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93d15887-5c0e-4975-98a2-82ca84d35d5f
Update existing create-pull-request fixtures for the required temporary ID and add a deterministic create-then-update handoff scenario in one executor process. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93d15887-5c0e-4975-98a2-82ca84d35d5f
|
Azure Pipelines: Successfully started running 1 pipeline(s). 1 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
✅ Compiler Contract Reviewer completed the compiler contract review.
|
|
✅ TypeScript Code Quality Reviewer completed the TypeScript code quality review.
|
|
✅ Test Quality Sentinel completed the test quality analysis.
|
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
|
✅ PR Security Reviewer completed the security review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
TypeScript review — scripts/ado-script/
Reviewed the new create-pull-request-temporary-id-handoff executor-e2e scenario (the only change under scripts/ado-script/ in this diff).
No blocking issues found:
executedRecordForToolthrows with a clear, contextual error (no executed record found for prior tool ...) rather than silently returningundefined.- ID/type validation goes through the existing
numResult/strResulthelpers, which already guard againstnull/""/falsecoercing to a false-positive0and reject non-string values — noanyor unchecked casts were introduced. Teardown.run()aggregates step failures with per-step context and always attempts every step, consistent with existing usage elsewhere in this file.- The new scenario is registered in the test registry (
index.test.ts), so it has coverage for the new branch.
Note: the ts-critic sub-agent was started in the background as required, but returned no parseable findings after two prompts and the full wait budget; I proceeded with my own pass only.
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 35.7 AIC · ⌖ 2.81 AIC · ⊞ 10.6K
Comment /review to run again
There was a problem hiding this comment.
Test Quality Sentinel review
The new temporary-PR-reference machinery is well covered for the happy path (id parsing, reviewer allowlist/dedup/limit, identity-lookup exact-match, warning-vs-failure structuring, and an end-to-end create→update NDJSON test in execute.rs). Three gaps in error-path coverage for genuinely security-relevant new logic:
- Reviewer input validation (max 100 entries, length cap, pipeline-injection rejection in
UpdatePrParams::validate) has no test for itsErrarms. resolve_update_pr_target's temporary-reference failure branches (unresolved ID, repository mismatch, disallowed repository) are untested — only the success path has a unit test.- The duplicate-
temporary_idguard increate_pull_request.rs(rejecting reuse of a temporary ID within the same run) is never exercised by any test.
None of these are merge-blocking on their own — the happy-path and structural tests are solid — but each is a new fail-closed safety check whose regression would silently ship broken security behavior.
🧪 Test quality analysis by Test Quality Sentinel · auto · 105.3 AIC · ⌖ 1.98 AIC · ⊞ 10.3K
Comment /review to run again
| ); | ||
| ensure!( | ||
| reviewers.len() <= 100, | ||
| "reviewers list must contain at most 100 entries" |
There was a problem hiding this comment.
New reviewer validation (max 100 entries, 0-char cap, pipeline-injection rejection) has no test exercising the Err arm — every existing add-reviewers test uses a single short, clean reviewer string. A regression that silently drops the length cap or injection check would ship undetected.
💡 suggested tests
Add cases to mod tests asserting params.validate() returns Err for: (1) a reviewers list with 101 entries, (2) a reviewer string longer than MAX_REVIEWER_LEN, and (3) a reviewer containing a pipeline-command pattern (e.g. ##vso[task.setvariable...]). These are new, non-trivial guard rails introduced in this PR and are currently only exercised by inspection.
| 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; \ |
There was a problem hiding this comment.
resolve_update_pr_target's three failure branches for a Temporary reference — unresolved ID, repository mismatch, and repository-not-allowed — have no direct unit test. Only the successful-resolution path (temporary_reference_resolves_exact_registered_target) is covered.
💡 why this matters
These are exactly the paths a model-facing agent hits when it reuses a stale or mistyped #aw_ reference, or targets the wrong repo alias. A regression here (e.g. accidentally returning Ok instead of the failure, or a typo in the mismatch condition) would silently let an update-pr operation apply against the wrong PR/repo instead of failing closed, and nothing in the suite would catch it.
Suggested additions: call resolve_update_pr_target directly with (a) an unregistered temporary ID and assert the "has not been resolved" failure, (b) a registered temporary ID plus a requested_repository that does not match resolved.target.alias, and (c) a resolved target whose alias is excluded by config.allowed_repositories.
| Err(failure) => return Ok(failure), | ||
| }; | ||
| debug!("Resolved repository ID: {}", target.repository_locator()); | ||
| if ctx.has_resolved_pull_request(&self.temporary_id)? { |
There was a problem hiding this comment.
The new duplicate-temporary_id guard (rejecting a second create-pull-request call reusing an already-registered temporary ID within the same run) has no test. All fixture executions in this file use distinct #aw_test1..5 IDs, so this branch is never exercised.
💡 suggested test
Run the executor twice against an ExecutionContext where resolved_pull_requests already contains the temporary ID used by the second CreatePrResult, and assert the second call returns ExecutionResult::failure mentioning "already used in this run" rather than proceeding to open a second PR under a colliding ID.
There was a problem hiding this comment.
Compiler Contract Review
Reviewed the temporary PR-reference handoff (create-pull-request → update-pr). This mirrors the existing WorkItemTemporaryId/GithubTemporaryId contract exactly: same validated_string! macro in src/secure.rs, same impl_temporary_reference_deserialize! macro reuse, same registration/resolution pattern in ExecutionContext (resolved_pull_requests alongside resolved_work_items/resolved_github_issues), and the deny_unknown_fields + schema-exclusion tests match the create-work-item precedent.
No drift found:
- No codegen/gate IR touched, so
types.gen.ts/fact-catalog.gen.jsonare correctly untouched. - No
.github/workflows/*.mdchanged, so no lock-file drift. docs/safe-outputs.mdwas updated with the newupdate-prconfig keys (allowed-reviewers,max-reviewers), the temporary-ID handoff semantics, and an example call sequence.AGENTS.mdarchitecture tree needs no changes (no new module/file added, only existing files modified).- The new
PullRequestTemporaryIdcorrectly reuses thesrc/secure.rsvalidated-newtype pattern rather than a rawString, satisfying the safe-output identifier-typing contract.
No compiler-contract violations found. cargo build and the targeted update_pr/create_pull_request test suites pass.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 79 AIC · ⌖ 14.2 AIC · ⊞ 11.8K
Comment /review to run again
There was a problem hiding this comment.
Rust review: PASS with one advisory nitpick
Well-structured change: anyhow context is preserved throughout, u64 used consistently for PR IDs (fixing a prior i32/lossy-cast risk), the temporary-ID registry uses a poison-safe Mutex with duplicate-registration rejection, reviewer resolution requires exact case-insensitive matches with ambiguity rejection, and safe-output execution is sequential/ordered so the create→update temporary-ID handoff has no concurrency hazard. Test coverage is thorough (unit + wiremock + TS executor-e2e scenario).
One non-blocking nitpick posted inline: the ad-hoc GUID-shape sniff in lookup_reviewer_id duplicates existing UUID validation elsewhere in the codebase instead of reusing it.
Note: the rust-critic sub-agent did not return usable output after two attempts (idle turns with an empty response), so this review reflects my own pass only.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 108.3 AIC · ⌖ 15.9 AIC · ⊞ 10.5K
Comment /review to run again
| token: &str, | ||
| connection_type: Option<crate::compile::types::WriteConnectionType>, | ||
| ) -> Option<String> { | ||
| if reviewer.len() == 36 |
There was a problem hiding this comment.
Nitpick, not blocking: this ad-hoc GUID sniff (len()==36 + hyphen/hex check) accepts any 36-char hex-and-hyphen string as a resolved reviewer ID without validating it's a plausible UUID version/variant or actually a known identity — a malformed-but-GUID-shaped reviewer string will be PUT straight to the reviewers endpoint and only fail (or silently succeed against an unintended identity) at the ADO API. Consider reusing the project's existing Guid/is_uuid_like validated type (see src/secure.rs / src/ado/mod.rs::is_uuid_like) instead of re-deriving UUID-shape checks inline.
💡 details
src/ado/mod.rs already has is_uuid_like and src/secure.rs has a Guid newtype used elsewhere in the codebase for exactly this kind of identifier validation. Duplicating the shape-check here means the two implementations can drift (e.g. if one becomes stricter about version/variant nibbles) and a reviewer passing this check is treated as a literal identity GUID and skips the identity-lookup safeguards (exact-match dedup, ambiguity rejection) added elsewhere in this same PR for the name/email path.
There was a problem hiding this comment.
🔒 Security review by PR Security Reviewer · auto · 182.7 AIC · ⌖ 2.59 AIC · ⊞ 10.7K
Comment /review to run again
| token: &str, | ||
| connection_type: Option<crate::compile::types::WriteConnectionType>, | ||
| ) -> Option<String> { | ||
| if reviewer.len() == 36 |
There was a problem hiding this comment.
🟡 Low-severity: GUID reviewer value bypasses identity verification (confidence: medium)
Any syntactically GUID-shaped reviewer string (36 chars, 4 dashes, hex) is accepted and used directly as the ADO identity ID with no lookup or existence check against vssps_base, unlike the email/display-name path which requires an exact match against a real identity returned by the API.
💡 details
With allowed-reviewers: ["*"] (a supported, documented config), a model influenced by untrusted content (prompt injection from an issue/PR it is summarizing) could pick an arbitrary GUID as a reviewer, and it would be PUT directly to the reviewers endpoint with no confirmation the GUID corresponds to a real, intended identity in this organization — unlike the exact-match path for emails/names, which is validated against the VSSPS identities API first.
Impact is bounded (adding a PR reviewer is not privilege-granting), so this is informational rather than blocking. Consider still routing GUID reviewers through a lookup/validation call (e.g. GET _apis/identities/{id}) before use, to keep the verification guarantee documented for other reviewer forms ("Reviewer identities require an unambiguous case-insensitive exact email, account, display-name, or GUID match").
Keep temporary PR producers and consumers in one approval lane, align reviewer allowlists with gh-aw, encode identity queries safely, and make executor E2E cleanup reclaim created PRs after consumer failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93d15887-5c0e-4975-98a2-82ca84d35d5f
Create a real PR, resolve its temporary ID through update-pr add-reviewers, verify the configured Azure DevOps identity is present, and clean up safely on failures. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 93d15887-5c0e-4975-98a2-82ca84d35d5f
|
/review |
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
There was a problem hiding this comment.
TypeScript review — scripts/ado-script/ (this PR's TS diff)
Scope: scripts/ado-script/src/executor-e2e/** only (test-harness module, explicitly documented as "not shipped in ado-script.zip") — no other scripts/ado-script/ files changed in this PR.
Sub-agent note: the ts-critic sub-agent was started on the diff but remained idle with no output after two blocking waits (~130s total); its findings are unavailable for this review and are not reflected below.
My own pass:
ado-rest.ts::resolveIdentityId— GUID passthrough, single-match, and ambiguous-match branches are all covered by the newado-rest.test.tscases.authedFetchcentralizes auth header injection and already applies a per-requestAbortSignal.timeout, so the new identity-lookup call inherits timeout/retry-adjacent safety without extra work.runner.tscorrectly threadsexecutedRecordsintocleanup()even on execute-phase failure, and the new runner test exercises exactly that path (prior success + primary failure).create-pull-request.ts's newresolveExecutorE2eReviewerguards against an unexpanded ADO macro literal ($(Build.RequestedForEmail)) reaching the harness, with matching tests.- No unhandled promise rejections, swallowed errors,
anyleaks, or token/secret log exposure found in the changed lines. Case-insensitive reviewer/added-list comparisons and error propagation all look correct.
No merge-blocking defects found in this file set. Deeper identity-verification and GUID-bypass concerns for the production update-pr Rust implementation are already covered by other reviewers' existing comments on src/safe_outputs/update_pr.rs (out of this reviewer's TypeScript-only scope).
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 58.2 AIC · ⌖ 11 AIC · ⊞ 10.5K
Comment /review to run again
There was a problem hiding this comment.
Security review
Reviewed this diff against main for regressions in the trust boundaries this repo cares about (Stage 1 read-only token isolation, Stage 3 write-token scoping, injection into generated shell/YAML, validated newtypes, path traversal, network allowlist, safe-output integrity).
No new security regressions found. This PR is security-neutral to net-positive:
CreatePrParamsnow has#[serde(deny_unknown_fields)]and drops the inlinereviewersfield, so the agent can no longer smuggle reviewers (or a self-chosentemporary_id) past theupdate-pr.allowed-reviewers/max-reviewerspolicy checks — reviewer selection is forced through the policy-checkedupdate-pr add-reviewerspath.- The new
PullRequestReference::Temporaryhandoff is scoped to one in-processExecutionContext(an in-memoryHashMapkeyed bytemporary_id.canonical()), and the compiler (common.rs::validate_pull_request_outputs_config) now requirescreate-pull-requestandupdate-prto share the same effectiverequire-approvallane, so a temporary ID can never cross the Stage-3-automatic / Stage-3-reviewed job boundary. - Repository re-validation is re-applied on the resolved target for temporary references (
resolved.target.aliasis checked against both the caller'srepositoryselector andallowed-repositories), so a temporary PR reference can't be used to write into an unintended or cross-org repository. - Reviewer strings continue to go through
reject_pipeline_injectionbefore being embedded in ADO log/error output, and the identity lookup moved from manual string interpolation (filterValue={encoded}) toreqwest's.query()builder, a hardening rather than a regression. - Token handling is unchanged:
SYSTEM_ACCESSTOKEN/AZURE_DEVOPS_EXT_PATstill flows only through the existingauthenticate_ado_requesthelper into Stage 3 REST calls; no new token projection into Stage 1 or into logs.
The one pre-existing weakness touched by this diff — the ad-hoc 36-char/4-hyphen/hex "looks like a GUID" heuristic in lookup_reviewer_id/resolveIdentityId that skips identity verification for any GUID-shaped reviewer string — is already flagged by existing review comments on this PR (low severity, medium confidence), so it is not repeated here.
No blocking findings; nothing further to add beyond what is already tracked in existing comments.
🔒 Security review by PR Security Reviewer · auto · 111 AIC · ⌖ 2.9 AIC · ⊞ 10.7K
Comment /review to run again
There was a problem hiding this comment.
Compiler Contract Review — no blocking findings
Reviewed the new PR temporary-ID handoff (create-pull-request → update-pr) against the ado-aw compiler contracts:
- Newtype pattern:
PullRequestTemporaryId(src/secure.rs) andPullRequestReference(src/safe_outputs/update_pr.rs) correctly follow the existingWorkItemTemporaryId/impl_temporary_reference_deserialize!convention used by create-work-item/update-work-item. - Schema boundary:
CreatePrParamsgained#[serde(deny_unknown_fields)], correctly blocking the internaltemporary_idand the removed inlinereviewersfield from agent input (verified bytest_create_pr_schema_excludes_internal_and_inline_reviewer_fieldsand the params-level test). - Approval-lane contract:
validate_pull_request_outputs_configinsrc/compile/common.rsreusesrequire_same_approval_laneto forcecreate-pull-requestandupdate-printo the same Stage 3 job when temporary IDs are shared — consistent with the work-item precedent, and wired intoagentic_pipeline.rs::validate_pipeline_front_matter. - Registry:
ResolvedPullRequest/resolved_pull_requestsinsrc/safe_outputs/result.rsmirrors the existingresolved_work_itemsmap (lock-poison handling, duplicate-registration rejection,Defaultwiring). - Docs:
docs/safe-outputs.mdwas updated in the same commit with the temporary-ID description,allowed-reviewers/max-reviewerssemantics, and the same-approval-lane requirement. - No drift:
pr-meta.json's file list shows no.lock.yml,*.gen.ts/*.gen.json, orscripts/ado-script/*.jsbundle changes needed — nothing in this diff touches the gate/fact IR or ado-script bundle sources, so no codegen/bundle drift applies here.
The five pre-existing review comments (missing-test coverage for the reject arm, temporary-reference failure branches, duplicate-temporary_id guard, and the GUID-sniff reviewer bypass) already cover the test-completeness and identity-verification concerns I would otherwise raise, and are outside compiler-contract scope proper — no new findings to add on top of those.
No inline comments posted (no new contract-drift findings on changed lines).
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 110.3 AIC · ⌖ 2.86 AIC · ⊞ 11.7K
Comment /review to run again
There was a problem hiding this comment.
Test Quality Review
Overall the new test suite for the temporary-ID handoff is thorough: reviewer policy (allowlist/wildcard/dedup/limit), identity exact-match/ambiguity/injection-safe query encoding, the create→update handoff end-to-end (both Rust unit test and TS executor-e2e scenarios), and cleanup-on-failure recovery are all covered.
Two untested error-path gaps I found beyond what prior review passes already flagged (reviewer count/length/injection validation, resolve_update_pr_target's temporary-reference failure branches, and the GUID-sniff reviewer bypass are already covered by existing comments on this PR):
PullRequestReference's deserializer negative-number and quoted-out-of-range arms are untested (inline comment onupdate_pr.rs).CreatePrResult::execute'sregister_resolved_pull_requestfailure branch — the one path that can leave a real ADO PR created but itstemporary_idunregistered — has no test proving thefailure_with_datapayload is correct (inline comment oncreate_pull_request.rs).
Neither is merge-blocking; both are advisory gaps in otherwise strong coverage.
🧪 Test quality analysis by Test Quality Sentinel · auto · 151.8 AIC · ⌖ 9.28 AIC · ⊞ 10.3K
Comment /review to run again
There was a problem hiding this comment.
🟡 Changes recommended
Four unresolved moderate findings remain in validation, GUID handling, and persisted warning data.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds same-run temporary PR references so update-pr can operate on PRs created earlier in the SafeOutputs job, including dynamic reviewer selection.
Changes:
- Adds temporary PR ID generation, persistence, routing, and approval-lane validation.
- Adds reviewer allowlists, limits, exact identity matching, and structured outcomes.
- Expands Rust, TypeScript, E2E, and documentation coverage.
Review findings:
src/compile/common.rs:2987— Moderate, 3 votes: validatemax-reviewersas a positiveusizeduring compilation.src/safe_outputs/update_pr.rs:509— Moderate, 1 vote: reuse full reviewer validation for execution records.src/safe_outputs/update_pr.rs:1056— Moderate, 1 vote: use canonical GUID validation.src/safe_outputs/update_pr.rs:487— Moderate, 1 vote: persist structured warning data.
File summaries
| File | Reviewed change |
|---|---|
tests/executor-e2e/README.md |
Documents reviewer scenario configuration. |
tests/executor-e2e/azure-pipelines.yml |
Passes reviewer configuration to E2E runs. |
tests/compiler_tests.rs |
Tests approval-lane validation. |
src/secure.rs |
Adds validated temporary PR IDs. |
src/safe_outputs/upload_build_attachment.rs |
Updates execution-context fixtures. |
src/safe_outputs/update_pr.rs |
Supports temporary references and reviewer operations. |
src/safe_outputs/result.rs |
Stores PR registry and warning results. |
src/safe_outputs/mod.rs |
Exposes repository target types. |
src/safe_outputs/create_pull_request.rs |
Registers temporary PR references. |
src/mcp.rs |
Generates and returns temporary PR IDs. |
src/execute.rs |
Tests ordered create/update execution. |
src/compile/common.rs |
Validates PR configuration and reviewer limits. |
src/compile/agentic_pipeline.rs |
Runs PR configuration validation. |
scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts |
Adds temporary-ID and reviewer scenarios. |
scripts/ado-script/src/executor-e2e/scenario.ts |
Extends scenario cleanup contracts. |
scripts/ado-script/src/executor-e2e/runner.ts |
Propagates cleanup records. |
scripts/ado-script/src/executor-e2e/ado-rest.ts |
Adds exact identity lookup. |
scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts |
Tests cleanup record propagation. |
scripts/ado-script/src/executor-e2e/__tests__/index.test.ts |
Tests scenario registration. |
scripts/ado-script/src/executor-e2e/__tests__/create-pull-request-scenarios.test.ts |
Tests reviewer handoff scenarios. |
scripts/ado-script/src/executor-e2e/__tests__/ado-rest.test.ts |
Tests identity lookup behavior. |
docs/safe-outputs.md |
Documents temporary references and reviewer policies. |
Review details
Suppressed comments (3)
src/safe_outputs/update_pr.rs:516
- The MCP-side
UpdatePrParams::validateenforces the 100-entry, non-empty, 256-character, and pipeline-injection checks, but Stage 3 deserializesUpdatePrResultdirectly and this execution-side validator only applies the allowlist/deduplication/max checks. An untrusted execution record can therefore bypass those new bounds and drive an excessive number of identity/API requests or oversized query values. Reuse the same reviewer validation before the write loop.
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))
src/safe_outputs/update_pr.rs:1060
- This predicate only counts four hyphens anywhere in a 36-character hex string, so malformed GUID-shaped reviewer values can bypass identity lookup and be sent directly as the reviewer ID. That violates the exact identity-resolution contract and turns a valid display name/email into a failed PUT in this edge case; use the shared canonical 8-4-4-4-12 GUID validator instead.
if reviewer.len() == 36
&& reviewer
.chars()
.filter(|character| *character == '-')
.count()
src/safe_outputs/update_pr.rs:490
- Although this builds the required
addedandfailedarrays,append_execution_recordcurrently writesresult: Nonefor every warning status, sowarning_with_datais discarded fromsafe-outputs-executed.ndjson. The structured reviewer outcome is therefore unavailable to executor-E2E/audit consumers; preserve warning data in the execution record (or expose it through an equivalent persisted path) while keeping the warning status.
if has_failures {
ExecutionResult::warning_with_data(message, data)
} else {
ExecutionResult::success_with_data(message, data)
- Files reviewed: 22/22 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| if let Some(config) = front_matter.safe_outputs.get("update-pr") | ||
| && let Some(max_reviewers) = config | ||
| .as_object() | ||
| .and_then(|object| object.get("max-reviewers")) | ||
| .and_then(serde_json::Value::as_u64) | ||
| { | ||
| anyhow::ensure!( | ||
| max_reviewers > 0, | ||
| "safe-outputs.update-pr.max-reviewers must be greater than zero" | ||
| ); | ||
| } |
Summary
Model-selected reviewers need the PR created by the same run, but Azure DevOps assigns its numeric ID only during SafeOutputs execution. Rather than making reviewer selection a special inline field on
create-pull-request, this change adds composable temporary PR references:create-pull-requestreturns and persists an MCP-generated#aw_...temporary ID; it is internal execution metadata, not model-supplied input.update-properation accepts either a numeric PR ID or a same-job temporary ID.update-pr add-reviewersoperation. Omittedallowed-reviewerspermits any valid reviewer, matching gh-aw; a non-empty list restricts selection, andmax-reviewersprovides a separate bound.addedandfailedarrays, while policy and reference failures remain hard failures.create-pull-request.reviewersremain unchanged.Temporary IDs resolve in NDJSON proposal order and only within one SafeOutputs job. Automatic and manually reviewed outputs run in separate jobs and therefore cannot share temporary references; compilation requires
create-pull-requestandupdate-prto use the same effectiverequire-approvalsetting.Test plan
update-prrequest.cargo test --all-targets.cargo clippy --all-targets.Review follow-ups
The branch review identified four actionable gaps, all addressed:
create-pull-requestandupdate-prconfigurations that resolve to different approval lanes, because temporary references are process-local to one SafeOutputs job.allowed-reviewerslist is restrictive. Exact and unambiguous Azure DevOps identity resolution remains fail-closed.update-properation fails.Cross-version replay of historical post-MCP NDJSON remains intentionally unsupported. Supported
audit,trace, MCP-author audit, and approval-summary debugging paths parse proposal records generically and are unaffected by the required internaltemporary_id.Validation
cargo check --all-targetscargo test --all-targetscargo clippy --all-targets1495015eb4ac4717915c6ffc47a94fd0ca98c3bf639050passed, includingcreatePullRequestTemporaryIdHandoffcreate-pull-request→update-pr add-reviewerstemporary-ID scenario that resolves the configured reviewer, verifies real PR membership by identity ID, and cleans up on failure639292rancreate-pull-request-add-reviewersagainst a real PR, addeddevinejames@microsoft.com, verified reviewer membership by identity ID, and completed cleanup