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

[Copilot]

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-request returns and persists an MCP-generated #aw_... temporary ID; it is internal execution metadata, not model-supplied input.
• Every update-pr operation accepts either a numeric PR ID or a same-job temporary ID.
• Temporary references retain the exact organization, project, and repository target, including cross-organization routing.
• Model-selected reviewers remain an update-pr add-reviewers operation. Omitted allowed-reviewers permits any valid reviewer, matching gh-aw; a non-empty list restricts selection, and max-reviewers provides a separate bound.
• Reviewer identities require an unambiguous case-insensitive exact email, account, or display-name match. GUID inputs use the shared validated type and must resolve to a matching real VSSPS identity before assignment.
• Reviewer resolution and API failures return structured warnings with added and failed arrays, while policy and reference failures remain hard failures.
• Static operator-configured create-pull-request.reviewers remain unchanged.

safe-outputs:
  create-pull-request: {}
  update-pr:
    allowed-operations:
      - add-reviewers
    allowed-reviewers:
      - user@example.com
    max-reviewers: 3
    max: 2

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-request and update-pr to use the same effective require-approval setting.

Test plan

• Verify create-PR MCP responses return and persist generated temporary IDs.
• Verify create-PR input excludes internal temporary IDs and inline model reviewer fields.
• Verify numeric, quoted numeric, and temporary PR reference parsing and routing.
• Verify exact cross-organization target retention and repository mismatch rejection.
• Verify gh-aw-aligned permissive reviewer defaults, restrictive non-empty allowlists, wildcard compatibility, limits, and deduplication.
• Verify exact identity matching rejects fuzzy and ambiguous results.
• Verify partial and total reviewer failures produce structured warnings.
• Verify ordered executor NDJSON creates a PR, registers its temporary ID, then resolves it in a following update-pr request.
• Add the same create→update temporary-ID handoff to the deterministic Azure DevOps executor E2E suite.
• Verify failed handoff cleanup still abandons the producer-created scratch PR.
• Run cargo test --all-targets.
• Run cargo clippy --all-targets.

Review follow-ups

The branch review identified four actionable gaps, all addressed:

• The compiler rejects create-pull-request and update-pr configurations that resolve to different approval lanes, because temporary references are process-local to one SafeOutputs job.
• Reviewer authorization matches gh-aw: omission is permissive, while a non-empty allowed-reviewers list is restrictive. Exact and unambiguous Azure DevOps identity resolution remains fail-closed.
• Reviewer identity searches use URL query parameters, preserving values such as plus-addressed emails and names containing query delimiters.
• The test-only executor E2E harness passes parsed producer records into cleanup, allowing a created scratch PR to be abandoned even when the following update-pr operation fails.

Latest feedback addressed (4dbcee44)

• Added regression coverage for reviewer count, length, and pipeline-command validation.
• Added fail-closed coverage for unresolved temporary PR IDs, repository mismatches, and repository-policy exclusions.
• Added duplicate temporary-ID executor coverage proving no second PR request is issued.
• Replaced ad-hoc GUID detection with the shared validated Guid type and verified GUID identities through VSSPS before reviewer assignment.
• max-reviewers now must deserialize as a positive usize during compilation; negative, fractional, quoted, zero, and overflowing values are rejected.
  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 internal temporary_id.

Validation

• cargo check --all-targets
• cargo test --all-targets
• cargo clippy --all-targets
• All Rust targets passed after review feedback commit 4dbcee44 (cargo test, clippy with warnings denied, and cargo check)
• TypeScript typecheck and all 124 executor-E2E harness tests
• Azure DevOps executor E2E build 639050 passed, including createPullRequestTemporaryIdHandoff
• Add a live create-pull-request → update-pr add-reviewers temporary-ID scenario that resolves the configured reviewer, verifies real PR membership by identity ID, and cleans up on failure
• Run all 140 executor-E2E harness tests, TypeScript typecheck, and the executor bundle build
• Azure DevOps build 639292 ran create-pull-request-add-reviewers against a real PR, added devinejames@microsoft.com, verified reviewer membership by identity ID, and completed cleanup

Final integration review fixes (4d98c8d0)

• GUID reviewer values now use the documented VSSPS identityIds query and require exactly one matching real identity.
• Temporary-ID producer/consumer tools must have matching effective staged settings across pull requests, work items, and GitHub issues, in addition to matching approval lanes.
• Warning execution records now preserve machine-readable result data while retaining the human-readable warning in error.
• Live executor coverage separately exercises raw email/name lookup and resolved-GUID lookup for update-pr add-reviewers.
• The general-lookup scenario uses validated temporary ID #aw_prreviewgen (220878c4).
• Azure DevOps executor E2E build 641376 passed both live reviewer paths: GUID identityIds on Azure DevOps scratch PR ID 41799 and raw email/name searchFilter on Azure DevOps scratch PR ID 41800; both verified membership and completed cleanup.

Final local validation: 3,670 Rust tests and 1,205 TypeScript tests passed, with clean clippy, cargo check, TypeScript typecheck, executor bundle build, and diff checks.

• Fixes [agent-issue]: Allow model-selected reviewers in create-pull-request #2090

[azure-pipelines]

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.

[azure-pipelines]

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.

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

✅ TypeScript Code Quality Reviewer completed the TypeScript code quality review.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

🧪 Test quality analysis by Test Quality Sentinel

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

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:

• executedRecordForTool throws with a clear, contextual error (no executed record found for prior tool ...) rather than silently returning undefined.
• ID/type validation goes through the existing numResult/strResult helpers, which already guard against null/""/false coercing to a false-positive 0 and reject non-string values — no any or 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}

[github-actions]

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:

1. Reviewer input validation (max 100 entries, length cap, pipeline-injection rejection in UpdatePrParams::validate) has no test for its Err arms.
2. 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.
3. The duplicate-temporary_id guard in create_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}

[github-actions]

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.json are correctly untouched.
• No .github/workflows/*.md changed, so no lock-file drift.
• docs/safe-outputs.md was updated with the new update-pr config keys (allowed-reviewers, max-reviewers), the temporary-ID handoff semantics, and an example call sequence.
• AGENTS.md architecture tree needs no changes (no new module/file added, only existing files modified).
• The new PullRequestTemporaryId correctly reuses the src/secure.rs validated-newtype pattern rather than a raw String, 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}

[github-actions]

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}

[github-actions]

🔒 Security review by PR Security Reviewer · auto · 182.7 AIC · ⌖ 2.59 AIC · ⊞ 10.7K
_{Comment /review to run again}

[jamesadevine]

/review

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

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 new ado-rest.test.ts cases.
• authedFetch centralizes auth header injection and already applies a per-request AbortSignal.timeout, so the new identity-lookup call inherits timeout/retry-adjacent safety without extra work.
• runner.ts correctly threads executedRecords into cleanup() even on execute-phase failure, and the new runner test exercises exactly that path (prior success + primary failure).
• create-pull-request.ts's new resolveExecutorE2eReviewer guards against an unexpanded ADO macro literal ($(Build.RequestedForEmail)) reaching the harness, with matching tests.
• No unhandled promise rejections, swallowed errors, any leaks, 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}

[github-actions]

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:

• CreatePrParams now has #[serde(deny_unknown_fields)] and drops the inline reviewers field, so the agent can no longer smuggle reviewers (or a self-chosen temporary_id) past the update-pr.allowed-reviewers / max-reviewers policy checks — reviewer selection is forced through the policy-checked update-pr add-reviewers path.
• The new PullRequestReference::Temporary handoff is scoped to one in-process ExecutionContext (an in-memory HashMap keyed by temporary_id.canonical()), and the compiler (common.rs::validate_pull_request_outputs_config) now requires create-pull-request and update-pr to share the same effective require-approval lane, 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.alias is checked against both the caller's repository selector and allowed-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_injection before being embedded in ADO log/error output, and the identity lookup moved from manual string interpolation (filterValue={encoded}) to reqwest's .query() builder, a hardening rather than a regression.
• Token handling is unchanged: SYSTEM_ACCESSTOKEN/AZURE_DEVOPS_EXT_PAT still flows only through the existing authenticate_ado_request helper 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}

[github-actions]

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) and PullRequestReference (src/safe_outputs/update_pr.rs) correctly follow the existing WorkItemTemporaryId/impl_temporary_reference_deserialize! convention used by create-work-item/update-work-item.
• Schema boundary: CreatePrParams gained #[serde(deny_unknown_fields)], correctly blocking the internal temporary_id and the removed inline reviewers field from agent input (verified by test_create_pr_schema_excludes_internal_and_inline_reviewer_fields and the params-level test).
• Approval-lane contract: validate_pull_request_outputs_config in src/compile/common.rs reuses require_same_approval_lane to force create-pull-request and update-pr into the same Stage 3 job when temporary IDs are shared — consistent with the work-item precedent, and wired into agentic_pipeline.rs::validate_pipeline_front_matter.
• Registry: ResolvedPullRequest/resolved_pull_requests in src/safe_outputs/result.rs mirrors the existing resolved_work_items map (lock-poison handling, duplicate-registration rejection, Default wiring).
• Docs: docs/safe-outputs.md was updated in the same commit with the temporary-ID description, allowed-reviewers/max-reviewers semantics, and the same-approval-lane requirement.
• No drift: pr-meta.json's file list shows no .lock.yml, *.gen.ts/*.gen.json, or scripts/ado-script/*.js bundle 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}

[github-actions]

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):

1. PullRequestReference's deserializer negative-number and quoted-out-of-range arms are untested (inline comment on update_pr.rs).
2. CreatePrResult::execute's register_resolved_pull_request failure branch — the one path that can leave a real ADO PR created but its temporary_id unregistered — has no test proving the failure_with_data payload is correct (inline comment on create_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}

[Copilot]

🟡 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: validate max-reviewers as a positive usize during 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::validate enforces the 100-entry, non-empty, 256-character, and pipeline-injection checks, but Stage 3 deserializes UpdatePrResult directly 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 added and failed arrays, append_execution_record currently writes result: None for every warning status, so warning_with_data is discarded from safe-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.

[jamesadevine]

/review

[github-actions]

✅ Rust Code Quality Reviewer completed the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Compiler-contract review

Checked drift (ado-script bundles/codegen/lock files), front-matter grammar, safe-output newtype usage, typed IR, and docs sync for this PR.

• PullRequestTemporaryId follows the existing GithubTemporaryId/WorkItemTemporaryId pattern correctly (validated newtype in src/secure.rs, impl_temporary_reference_deserialize! macro reuse).
• New update-pr.max-reviewers / allowed-reviewers fields are Option/defaulted, so existing workflows still parse — no codemod needed.
• update-pr.max-reviewers is validated at compile time as a positive usize (rejects negative/fractional/overflow) in src/compile/common.rs, matching Stage 3 deserialization.
• GUID reviewer identities are verified against VSSPS via a Guid-typed lookup instead of being trusted on shape alone; src/ado/mod.rs::is_uuid_like now delegates to the same shared crate::validate::is_valid_guid.
• require_same_approval_lane and the new require_same_staged_lane correctly gate create-pull-request/update-pr pairing so temporary PR IDs cannot cross a SafeOutputs-job boundary.
• docs/safe-outputs.md documents the new fields, the temporary-ID handoff, and the staged/approval-lane requirements.
• No .lock.yml / codegen / bundle drift: no .github/workflows/*.md changed, and types.gen.ts / fact-catalog.gen.json are untouched (consistent with the filter IR being unchanged in this PR).

All findings raised by earlier automated reviews on this PR (missing tests for validation error arms, duplicate-temporary-ID guard, GUID reviewer bypass) were addressed in commit 4dbcee4. No new compiler-contract issues found.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 69.8 AIC · ⌖ 1.84 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

Security review

This diff (temporary PR-reference handoff between create-pull-request and update-pr, plus reviewer allowlisting) is security-neutral to positive:

• New PullRequestReference / PullRequestTemporaryId handling is exact and validated (Guid::parse, reject_pipeline_injection on every reviewer string, ##vso[/$(/{{ all rejected).
• Temporary-ID resolution enforces repository-target and allowed-repositories checks even on the temporary path, and rejects duplicate registration before any PR is created (tested).
• Reviewer resolution now requires an unambiguous case-insensitive exact match on email/account/display-name (rejecting fuzzy/multi-match results), and GUID-shaped input is verified against a real VSSPS identity before being trusted — closing the gap flagged in earlier review rounds.
• execute.rs/update_pr.rs now route every ADO call through the shared authenticate_ado_request chokepoint instead of ad-hoc basic_auth, so token-scheme selection (Bearer vs Basic) stays centralized — a hardening, not a regression.
• max-reviewers deserializes strictly to a positive usize at compile time (rejecting negative/fractional/quoted/overflow values), addressing the prior Copilot finding on that field.
• No new domains, no widened token scope, no bypass of Stage 3 write-token isolation — update-pr/create-pull-request writes remain confined to Stage 3, and staged/require-approval lane-matching is extended consistently to the new temporary-ID family.

No exploitable regression found in this diff; all previously-flagged items in review comments appear addressed. No blocking findings.

🔒 Security review by PR Security Reviewer · auto · 87.2 AIC · ⌖ 9.39 AIC · ⊞ 10.2K
_{Comment /review to run again}

[github-actions]

This PR's test additions are notably thorough. Reviewing beyond the prior feedback thread (all of which has since been addressed in 4dbcee44):

• Error-path coverage is strong: resolve_update_pr_target has direct unit tests for all three Temporary failure branches (unresolved ID, repository mismatch, allowlist exclusion), reviewer validation covers length/count/injection limits, and the GUID identity lookup has five distinct rejection-path tests (missing, duplicate, mismatched, HTTP error, malformed JSON) plus an exact-match ambiguity test.
• No assertions were weakened or removed — the two -assert lines in the diff are pure reformatting (multi-line wrap), not semantic changes.
• No tests/safe-outputs/*.lock.yml drift and no per-case ADO definition additions in the smoke suite — both invariants this repo cares about are respected.
• TS and Rust coverage are symmetric: the executor-e2e scenario tests exercise the same create→update temporary-ID handoff, duplicate-ID rejection, and reviewer resolution edge cases as the Rust unit tests, including a rejected-live-ADO-state case.

Minor, non-blocking observation: the five reviewer_guid_lookup_rejects_* tests (update_pr.rs) are structurally identical (mock a GET /_apis/identities response shape, assert None) and could be collapsed into one table-driven test over the five response bodies, which would make it easier to add a sixth case later without copy-pasting ~30 lines of wiremock boilerplate each time. Not a blocker.

No new test-quality issues found.

🧪 Test quality analysis by Test Quality Sentinel · auto · 147.2 AIC · ⌖ 1.78 AIC · ⊞ 10.9K
_{Comment /review to run again}

[github-actions]

Verdict: COMMENT (no merge-blocking defects found)

This is a well-tested, carefully implemented feature. Temporary-PR-ID plumbing (lock scoping, collision retry, duplicate-registration guard) mirrors the existing create-work-item pattern correctly, the u64 PR-ID conversion and Guid-based reviewer verification are sound, and the approval/staged-lane cross-validation is thorough. I did not find any correctness, panic-risk, or error-handling regressions in the changed lines worth a merge-blocking comment.

Note: the rust-critic sub-agent failed to start (400 model "gpt-5.4-mini" is not accessible), so this review reflects only my own manual pass over the Rust diff (src/ado, src/audit, src/compile/{agentic_pipeline,common}, src/execute, src/mcp, src/safe_outputs/{create_pull_request,mod,result,update_pr,upload_build_attachment}, src/secure, tests/compiler_tests).

Themes checked

• Error handling: anyhow context preserved throughout; no new unwrap/expect on request-reachable paths (pr_id, reviewer JSON, etc. all use safe fallbacks with explicit zero/none handling).
• Correctness: pullRequestId now read via as_u64 (was as_i64().unwrap_or(0)), consistent with the new u64 PR-ID type end-to-end; GUID reviewer path now round-trips through the shared Guid validated type and a real identity lookup rather than a shape-only check.
• Concurrency: create_pr_proposal_lock correctly serializes temp-ID allocation + NDJSON append, matching the pre-existing create_work_item_proposal_lock pattern; no .await held across a std::sync::Mutex guard (the resolved_pull_requests map uses std::sync::Mutex only for synchronous read/insert).
• Maintainability: UpdatePrContext/PrContext param-object refactors reduce per-function argument counts; new public items (ResolvedPullRequest, PullRequestReference, reviewer validation helpers) are documented.
• Sequential execution of the NDJSON queue in execute_safe_outputs (confirmed via src/execute.rs) guarantees a create-pull-request proposal is fully resolved before a same-job update-pr consumer can reference its temporary ID, so the new approval/staged-lane validation is not just defense-in-depth but load-bearing.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 189.4 AIC · ⌖ 2.34 AIC · ⊞ 10.1K
_{Comment /review to run again}