feat(safe-outputs): split Azure DevOps PR tools with safe migration - #2222
jamesadevine with Copilot wants to merge 46 commits into
Conversation
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com>
|
Azure Pipelines: Successfully started running 2 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
|
✅ PR Security Reviewer completed the security review.
|
|
✅ Compiler Contract Reviewer completed the compiler contract review.
|
|
✅ Test Quality Sentinel completed the test quality analysis.
|
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Security review: no regressions found
Reviewed the new abandon-pull-request Azure DevOps safe output end-to-end:
- Boundary respected: the Stage 1 MCP tool (
mcp.rs) only callsqueue_sanitized_output— it never touches the write-capable token. The actual PATCH/POST happens inexecute.rs/abandon_pull_request.rs'sExecutor::execute_impl, which runs in Stage 3 withctx.access_token(the write-capable token the agent never sees), consistent withupdate-prandsubmit-pr-review. - Sanitization: the agent-supplied
bodyis passed throughsanitize_textinsanitize_content_fieldsbefore being reused as the abandonment comment, andrepositorythroughsanitize_config— matching the existing pattern for other PR-comment safe outputs. - Repository resolution: uses the same
resolve_repo_name+allowed-repositoriesallowlist gate already used byupdate-pr,submit-pr-review, andresolve-pr-thread. Repository/project segments are percent-encoded (utf8_percent_encodewithPATH_SEGMENT) before being placed in the URL, so no path/URL injection viarepository. - State check before mutation: the PR is fetched and its
statusvalidated (activerequired,abandonedshort-circuited) and title/label filters applied before thePATCHthat abandons it — no TOCTOU-style bypass of the configured guards. - No validated newtype was downgraded to a raw
String, no new domain was added to the AWF allowlist, and noado-aw-debuggate was touched.
This diff does not weaken the compiler's trust boundary or the Stage 1/Stage 3 credential isolation. No inline findings to post.
🔒 Security review by PR Security Reviewer · auto · 56.7 AIC · ⌖ 1.8 AIC · ⊞ 10.2K
Comment /review to run again
There was a problem hiding this comment.
Compiler contract review — abandon-pull-request
New safe output is wired consistently with the existing ADO-repository-write
contracts (typed_safe_output_config, ALL_KNOWN_SAFE_OUTPUTS, mcp.rs tool
registration, dispatch_pr_tools, resolve_repo_name same-org-only resolution
matching update_pr.rs's precedent). No codegen, gate/fact IR, or lock-file
drift here.
Documentation sync — missing (docs half of the contract)
AGENTS.md's src/safe_outputs/ architecture tree (around line 212) lists
every safe-output module file but does not include the new
abandon_pull_request.rs. Per docs/extending.md, a new safe-output tool
needs an entry there so future agents can find it by scanning the tree. There's
no diff line to attach this to since the omission is what's missing — fix is a
one-line addition to the tree, alphabetically between assign_work_item.rs
and close_github_issue.rs.
Minor: module ordering nit
Flagged inline on src/safe_outputs/mod.rs — new mod/pub use entries
landed out of the file's alphabetical convention.
Everything else — Option<String> for repository/target_repo (an alias,
not a path/ref/sha, so the src/secure.rs newtype rule doesn't apply),
sanitization via SanitizeContent, and cross-org write gating being
out-of-scope for a same-org-only tool (matches update_pr.rs) — looks correct.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 79.4 AIC · ⌖ 2.67 AIC · ⊞ 11.3K
Comment /review to run again
| mod assign_github_issue_to_user; | ||
| mod assign_work_item; | ||
| mod close_github_issue; | ||
| mod abandon_pull_request; |
There was a problem hiding this comment.
mod abandon_pull_request; is out of alphabetical order (lands between assign_work_item/close_github_issue, and assign_work_item itself got reordered around it). ado-aw's safe_outputs module list is otherwise kept alphabetical — worth a quick fixup so future diffs on this list stay minimal.
💡 Fix
Move both the mod declaration and the matching pub use line so abandon_pull_request sorts before add_build_tag.
There was a problem hiding this comment.
Test Quality review — abandon-pull-request safe output
Good coverage of the happy path (with/without comment, triggering-vs-explicit target) and the missing-label rejection. Two things need attention before merge:
- Likely dead validation code (comment on
src/compile/common.rs): the new"abandon-pull-request"match arm lives insidevalidate_github_issue_outputs_config, which only iteratesGITHUB_ISSUE_SAFE_OUTPUT_TOOLS— a listabandon-pull-requestis not part of. If that reading is correct,validate_abandon_pull_request_configis never invoked at compile time and an invalid config (e.g. an emptyrequired-labelsentry) will silently compile. No test exercises this path, which is exactly why it slipped through. - Untested control-flow branches in
abandon_pull_request.rs: theallowed-repositoriesrejection, therequired-title-prefixmismatch, the already-abandoned short-circuit, the non-active status rejection, and the comment-post-failure warning are all real branches with distinct user-facing messages, but only the missing-label case has a test. These are the core guardrails for a destructive action (abandoning a PR) — a regression in any of them should not be able to ship silently.
Requesting changes mainly for (1), since it means a documented, seemingly-validated config option may not actually be validated.
🧪 Test quality analysis by Test Quality Sentinel · auto · 110.2 AIC · ⌖ 1.86 AIC · ⊞ 9.8K
Comment /review to run again
| .as_ref() | ||
| .ok_or_else(|| anyhow::anyhow!("SYSTEM_TEAMPROJECT not set"))?; | ||
| let token = ctx.access_token.as_ref().ok_or_else(|| { | ||
| anyhow::anyhow!( |
There was a problem hiding this comment.
allowed-repositories rejection path is untested
No test configures allowed-repositories with a repository selector that is not in the list. The three async tests all use the implicit "self" repository selector with an empty allowed_repositories, so resolve_repository's deny branch never executes.
💡 Why this matters
This is a security-relevant guard — it exists specifically to stop an agent-controlled repository param from targeting an unlisted repo. A regression here (e.g. an accidental == → != flip, or the emptiness check being inverted) would silently allow writes to any repository and no test would catch it.
Suggested case: configure allowed-repositories: ["other"], pass repository: "self" (or omit it so it defaults to "self"), and assert the execution fails with the not in the allowed-repositories list message before any HTTP mock is hit (mount GET/PATCH with .expect(0)).
| Ok(repo_name) => repo_name, | ||
| Err(result) => return Ok(result), | ||
| }; | ||
| let client = reqwest::Client::new(); |
There was a problem hiding this comment.
required-title-prefix mismatch and the already-abandoned short-circuit are both untested
missing_label_rejects_before_patch covers a missing label, but no test exercises a title-prefix mismatch (this branch, lines 461-467), and none exercises the PR-already-abandoned short-circuit (around line 670) or the not-active/non-abandoned status rejection (around line 683). The comment-post-failure warning path (around line 706) is also unexercised.
💡 Why this matters
Each of these is a distinct control-flow branch with its own user-facing message, and together they are most of the business logic guarding this destructive action (aside from label filtering, which is covered). A regression in any of them — e.g. the already-abandoned branch accidentally falling through to call PATCH again, or the status check being inverted so a completed PR gets abandoned — would ship untested.
At minimum, add: (1) a title-prefix-mismatch case mirroring missing_label_rejects_before_patch; (2) a case where pr("abandoned") is returned by the GET mock and PATCH is asserted with .expect(0), verifying the success message and already_abandoned: true; (3) a case with pr("completed") asserting failure.
| crate::safe_outputs::validate_close_github_issue_config(&config)?; | ||
| } | ||
| } | ||
| "abandon-pull-request" => { |
There was a problem hiding this comment.
This match arm is unreachable — no test would catch that validate_abandon_pull_request_config is never invoked here
validate_github_issue_outputs_config only iterates front_matter.github_issue_tool_names(), which filters GITHUB_ISSUE_SAFE_OUTPUT_TOOLS (src/compile/types.rs:710). "abandon-pull-request" is not in that list, so this match arm can never be reached for any front matter — validate_abandon_pull_request_config is dead code from this call site, and a workflow with an invalid abandon-pull-request config (e.g. an empty required-labels entry) will compile successfully instead of failing validation.
💡 Why this matters
There is no compiler test that asserts an invalid abandon-pull-request config is rejected at compile time (the existing mod tests in abandon_pull_request.rs only unit-tests the config parsing and the executor in isolation, never through validate_github_issue_outputs_config or the compile pipeline). Had one existed — e.g. validate_github_issue_outputs_config(&fm_with_bad_abandon_config).is_err() — it would have caught that this branch is unreachable.
Likely fix: call validate_abandon_pull_request_config from wherever abandon-pull-request is actually compiled/validated (it is not a GitHub-issue-style tool), and add a fixture/unit test that exercises compile-time rejection of an invalid config (e.g. an empty string in required-labels).
There was a problem hiding this comment.
Rust review: no merge-blocking issues
Solid implementation, consistent with existing safe-output patterns (update_pr.rs, close_github_issue.rs). Builds cleanly; all 6 new tests pass.
Notes
- The
rust-criticsub-agent failed with a model-access error (400, gpt-5.4-mini not accessible); its output was discarded per contract and this review is my own manual pass. - Error handling uses
anyhowcontext consistently, nounwrap/expecton user-input paths, repository/target resolution mirrorsresolve_repo_name/resolve_repository_write_targetconventions used elsewhere. - One clippy
derivable_implshint on the manualDefault for AbandonPullRequestTargetimpl — already caught bycargo clippy, so not posted as a separate comment per review-signal guidance. - Tests cover target-form parsing, filter rejection, triggering-context resolution, and the abandon+comment happy path against a mock ADO server.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 151.4 AIC · ⌖ 16.5 AIC · ⊞ 10.1K
Comment /review to run again
# Conflicts: # src/execute.rs # src/mcp.rs
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
jamesadevine
changed the title
There was a problem hiding this comment.
Rust code quality review
Note: the rust-critic sub-agent failed to start (model access error, not a timeout), so this review reflects only my own manual pass over the Rust diff (src/**, tests/**, Cargo.toml).
The new PR-mutation safe-output modules (pr_common.rs, pr_mutations.rs, push_to_pull_request_branch.rs, pr_inline.rs, add_pr_labels.rs, etc.) are carefully guarded: exact-head checks, bounded pagination, byte-size limits, and typed newtypes for paths/SHAs/GUIDs are used consistently, and error handling uses anyhow::Context/ensure! throughout with no user-input-reachable unwrap()/expect() found. Casts I checked (i32::MAX as u32, MAX_PATCH as u64, value as u64 after an i64 <= 0 guard) are all safe.
One concurrency concern found and posted inline: Worktree's Drop fallback in push_to_pull_request_branch.rs runs a blocking std::process::Command on the async runtime thread if an early ? return skips the explicit async cleanup().
Themes considered
- Error handling / anyhow context: clean, no issues found.
- Unwrap/expect on user-input paths: none found outside
#[cfg(test)]blocks and compiler-owned constant.expect()calls. - Lossy casts: checked all
as u32/u64/usizeconversions in the diff; all guarded. - HashMap iteration affecting generated YAML determinism: new HashMap usage is confined to execution-time budget-group counters and test fixtures, not codegen output ordering.
- Async/blocking: one finding (see inline comment on
push_to_pull_request_branch.rs). - Existing bot comments already cover the
mod.rsalphabetical ordering,abandon_pull_request.rsuntested rejection paths, and the unreachablecommon.rsmatch arm — not duplicated here.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 92.8 AIC · ⌖ 1.78 AIC · ⊞ 10.1K
Comment /review to run again
There was a problem hiding this comment.
Security review
This is a large, security-focused PR (new push-to-pull-request-branch safe-output tool, PR-source-identity capture/projection, prefetch workflow token handling, label identity handling, ADO remote-URL parsing hardening). I reviewed it end-to-end against the ado-aw threat model (Stage 1/2/3 boundary, token scoping via ado_bundle.rs, sandbox network isolation, path/ref validation via secure.rs newtypes) and found no security regressions:
- Token scoping: The new PR-push source-preparation step correctly reuses the existing SC_READ_TOKEN vs System.AccessToken selection pattern (agentic_pipeline.rs), and the actual write (the ADO pushes REST call) happens only in push_to_pull_request_branch.rs::execute_impl, which runs in Stage 3 with the write-capable token — the agent never sees it, matching the three-stage model.
- Path/ref validation: New fields (repository, expected_head_sha, patch_file) use the existing validated newtypes (RelativeSafePath, CommitSha, StrictRelativePath) rather than raw strings, and the new PrLabelName newtype in secure.rs runs reject_pipeline_injection at deserialization time and is applied consistently across the label tools.
- Patch application hardening: push_to_pull_request_branch.rs disables git smudge/clean/process filters during patch application (git_without_filters), which reduces attack surface (blocks LFS/custom-filter code execution) rather than weakening it; it also rejects fork-backed PRs, merge/synthetic-merge history, non-branch refs, mismatched source/target, protected files, symlink/submodule changes, and enforces exact-head optimistic-concurrency checks before and after the push, with size/aggregate bounds (5 MB/file, 10 MB aggregate).
- Credential handling in git fetch fallback: Both the ADO source-commit fetch (ensure_source_commit) and the GitHub Actions PR-diff fallback (pr-data-prefetch.yml / pr-diff-data-fetch.md) pass bearer tokens via GIT_CONFIG_KEY/VALUE env vars (never argv), scrub them from the config after building the header, and validate the origin's scheme/host/path shape before use.
- ado-remote.ts hardening: The diff tightens URL parsing (rejects control characters, path separators, embedded credentials, ports, unexpected path-segment counts) rather than loosening it.
No instances of a validated newtype being downgraded to a raw String, no new network-allowlist entries, no secret logging, and no write bypassing Stage 3 were found. Nothing here is merge-blocking from a security perspective.
🔒 Security review by PR Security Reviewer · auto · 112.7 AIC · ⌖ 1.8 AIC · ⊞ 10.3K
Comment /review to run again
There was a problem hiding this comment.
Test Quality Sentinel 🧪
This is a very large PR (100 files, ~20k additions) that overall has strong test discipline: shared PR-target resolution (resolve_configured_pr_target, PrMutationPolicy) is exercised centrally in pr_common.rs across all 14 mutation tools, race/fail-closed paths in push_to_pull_request_branch.rs and pr_labels.rs are covered with wiremock, and the previously-flagged gaps in abandon_pull_request.rs (allowed-repositories rejection, title-prefix mismatch, already-abandoned short-circuit) and the unreachable validate_abandon_pull_request_config call in compile/common.rs have since been fixed and tested — this looks like a later iteration of a PR already under review.
One coverage gap found (see inline comment): the new has_current_pr_policy / PR_POLICY_HEADER recompile-provenance marker in src/compile/common.rs, which gates whether the explicit_pr_policy codemod re-pins PR target defaults on every future recompile, has no test anywhere in the diff or repo. A regression there would silently change PR-mutation authority scope on recompiles without any test catching it.
No weakened or deleted assertions found; the one large assertion change (CONFIGURED_ONLY_TOOLS.len() 14→24, plus new negative assertions for update-pr/abbreviated routes) is a legitimate strengthening, not a weakening.
🧪 Test quality analysis by Test Quality Sentinel · auto · 198.2 AIC · ⌖ 1.77 AIC · ⊞ 9.8K
Comment /review to run again
There was a problem hiding this comment.
TypeScript Code Quality Review 🔵
Reviewed the scripts/ado-script/** portion of this diff (approval-summary, shared/ado-remote, executor-e2e, compiler-smoke-e2e, exec-context-pr-synth/pr-checks). The ts-critic sub-agent failed at startup (400 model "gpt-5.4-mini" is not accessible via the /chat/completions endpoint), so this reflects my own manual pass only, not an adjudicated combination.
Findings posted inline (2):
preserveLargeIntegersinrender.tssilently coerces decimal/exponent numeric tokens (e.g.3.5) into JSON strings, not just full-u64 integers, contrary to its stated purpose — a real correctness bug on the proposal-rendering path.- A minor defense-in-depth note on unsanitized string interpolation in
renderPrTarget's triggering-destination message; the value is subsequently escaped as a whole bysanitizeInlinewhen rendered, so it isn't currently exploitable, but it's worth tightening for future-proofing.
What else I checked and found solid:
- New validation in
shared/ado-remote.ts(parseAdoRepoUrl,parseTriggeringPrIdentity,positivePrId) rejects malformed hosts, ports, credentials, control characters, and injection markers ($(,##vso[,{{) before trusting any triggering-PR identity — good hardening. compiler-smoke-e2e's new retry/cancel state machine inrunner.tsbounds transient read failures (max 3) against the existing deadline/cancel-grace-period logic without introducing unbounded retries or losing the original fail-closed abort semantics.AdoRestREST helpers added in this PR (listPullRequestLabels,boundaryTimeline,boundaryArtifacts,verifyBoundaryPush) validate response shape before use and throw descriptive errors rather than silently returningundefined/empty arrays.- No unhandled-rejection patterns (async
forEach, floating promises) introduced in the reviewed files;Promise.allusages are properly awaited. - No secrets/tokens observed reaching a log line or thrown
Errormessage in the reviewed diff.
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 157 AIC · ⌖ 2.12 AIC · ⊞ 10.1K
Comment /review to run again
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
|
/review |
|
✅ Rust Code Quality Reviewer completed the Rust code quality review.
|
There was a problem hiding this comment.
Security review
Reviewed this PR's diff (~28.7k lines, 124 files) for security regressions against the code it replaces. Focused on the highest-risk new surface: the new push-to-pull-request-branch safe output (src/safe_outputs/push_to_pull_request_branch.rs), PR target/identity resolution (pr_common.rs, pr_mutations.rs), the triggering-PR identity plumbing (ado-remote.ts, agentic_pipeline.rs), and the PR-policy codemods.
No security regression found. Notable positives:
- The new patch-apply write path is heavily bounded: exact-head verification before and after push, 5 MB/10 MB size caps, max-files, symlink/submodule/LFS/file-mode-change rejection, protected-file checks, and a worktree-isolated
git apply --checkdry run before the real apply. - New identifier fields (
repository,expected_head_sha, file paths) use validatedsecure.rsnewtypes (RelativeSafePath,CommitSha,StrictRelativePath) rather than rawString. - The write-capable token still only flows through the existing
authenticate_ado_request/UpdatePrContextchokepoint; no new token widened to Stage 1. ado-remote.tsURL/identity parsing was tightened, not loosened (rejects path separators, control characters,##vso[/$(/{{injection markers, embedded credentials/ports/query/fragment).- No changes to
src/allowed_hosts.rs/src/ecosystem_domains.rs— no new AWF network allowlist entries. - Sanitization (
reject_pipeline_injection,SanitizeContent,neutralize_pipeline_commands) is consistently applied to new agent-controlled fields (reviewer names, labels, repository selectors, PR descriptions).
This PR is security-neutral relative to main; no blocking findings to request changes on.
This review is scoped to the diff only, per the narrow security-regression mandate — a full vulnerability sweep is out of scope here.
🔒 Security review by PR Security Reviewer · auto · 94.3 AIC · ⌖ 2.24 AIC · ⊞ 10.3K
Comment /review to run again
There was a problem hiding this comment.
Compiler contract review
Reviewed this PR for ado-aw-specific compiler contracts: front-matter grammar changes, safe-output tool registrations, codemod coverage, typed IR, generated shell, and documentation sync.
No contract violations found. Specifically verified:
- All three new codemods (
0009_split_update_pr,0010_pull_request_tool_names,0011_explicit_pr_policy) are registered inCODEMODSand documented indocs/codemods.mdwith detailed migration semantics. - New/renamed safe-output tools (
abandon-pull-request,add-pull-request-labels,add-pull-request-reviewers,update-pull-request,set-pull-request-auto-complete, etc.) are wired intoALL_KNOWN_SAFE_OUTPUTS/CONFIGURED_ONLY_TOOLSinsrc/safe_outputs/mod.rs, anddocs/safe-outputs.mdwas updated with matching sections and renamed examples throughout. - Identifier fields follow the
src/secure.rsnewtype convention where new fields were introduced (e.g. newPrLabelNamenewtype used byadd_pr_labels.rs;CommitSha/GitRefNamereused inpush_to_pull_request_branch.rs). Existingrepository/allowed-repositoriesString/Vec<String>fields follow the pre-existing repo-wide convention rather than introducing a new gap. AGENTS.md's architecture tree and module list were updated to include every newsrc/safe_outputs/*.rsfile.- No
Fact/gate IR orPipelineSummarychanges in this PR, so no codegen-drift risk there;scripts/ado-script/src/approval-summary/render.tswas updated consistently with the renamed tool keys. - No stray
.lock.ymladded undertests/safe-outputs/, and the workflow.mdfiles that changed (review-rust.md,review-typescript.md,pr-sous-chef.md) all have matching.lock.ymlupdates. - Generated shell in
push_to_pull_request_branch.rs/pr_mutations.rsis Stage 3 runtimetokio::process::Command/HTTP code, not compiler-emittedShellScriptpipeline YAML, so theShellScript/Bindingcontract doesn't apply there; credentials are passed viaCommand::env, not interpolated into any generated script body.
This is a large, well-scoped migration with strong test and documentation coverage. Deferring to the other specialist reviewers for general Rust/TypeScript/test-quality/security concerns.
🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 117.3 AIC · ⌖ 1.89 AIC · ⊞ 11.3K
Comment /review to run again
There was a problem hiding this comment.
TypeScript reviewer (scripts/ado-script/ only)
Reviewed the ado-script diff directly plus a ts-critic sub-agent pass (its single finding is duplicated below; agreed and posted).
The production shared/ado-remote.ts, approval-summary/ and exec-context-pr-synth/ changes are solid: inputs from JSON/env are validated with explicit shape/format checks before use (positivePrId, GUID regex, injection-marker rejection on project/repository_name), errors from JSON.parse are caught, and the new retry/timeout logic in compiler-smoke-e2e/ado-rest.ts and runner.ts is well-reasoned (bounded consecutive-failure tolerance, explicit transient-vs-permanent classification).
Only one advisory finding survived triage — everything else in the diff either has adequate error handling/tests already, or is test-harness-only code exercised against a real ADO project where the existing patterns (thrown errors, AbortSignal.timeout where present) are consistent with the rest of the codebase.
Findings
- Missing fetch timeout (low) —
executor-e2e/scenarios/pr-comments.ts:191: inconsistent with siblingfetch()calls added in this PR.
Nothing here blocks merge.
🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 228.3 AIC · ⌖ 2.7 AIC · ⊞ 10.2K
Comment /review to run again
There was a problem hiding this comment.
🧪 Test quality analysis by Test Quality Sentinel · auto · 167.8 AIC · ⌖ 1.71 AIC · ⊞ 9.8K
Comment /review to run again
There was a problem hiding this comment.
Rust code quality review — advisory
Reviewed the Rust portion of the diff (src/**, tests/**, Cargo.toml) directly, plus a rust-critic background pass; its 4 candidates were triaged and 3 were kept as inline comments (1 dropped as a defensive-only, already-unreachable panic surface not worth the noise).
Themes
src/execute.rs: the executed-manifest now persistsresult.dataunconditionally (was gated to succeeded/warning) — likely intentional given the matching test rename, but worth confirming everyfailure_with_datacall site in the new PR tools only attaches data safe to retain on failure.src/safe_outputs/push_to_pull_request_branch.rs:Worktree'sDropfallback runs a blocking subprocess call from async execution paths.src/compile/agentic_pipeline.rs: unguardedserde_json::Valueindex access onresolved_execution_config_jsonwould silently fall back tonullrather than failing the compile if the JSON shape ever drifts.
The large volume of new .unwrap()/.expect() in the diff (abandon_pull_request.rs, pr_mutations.rs, pr_common.rs, push_to_pull_request_branch.rs, etc.) was checked line-by-line against each file's #[cfg(test)] boundary — all of it is confined to test code or compiler-owned static/constant values consistent with the project's documented .expect("...") pattern for values that cannot fail. No unwraps on untrusted/external input were found.
🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 664.5 AIC · ⌖ 2.65 AIC · ⊞ 10.2K
Comment /review to run again
…aces Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
This comment has been minimized.
This comment has been minimized.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: da8711de-7251-47cd-a006-6e4ece913b49
Prompt evaluationNote This is an advisory static review. Only Prompt Contracts is merge-blocking. Suites selected:
Observation (not a regression)Both candidate prompts add new PR comment/review/push-branch tool-selection Potential regressionsNone found. Per-case scores
|
Summary
Consolidates #2221 into #2222 and provides focused, ADO-native PR safe outputs
with safe source migration. Public names use pull-request, and public
labels terminology is retained. This is not a drop-in gh-aw schema adapter.
Implemented
appear to work while being ignored.
label/title filters across PR mutations. Triggering means the complete trusted
collection/project/repository/PR identity, not just a numeric ID.
without granting extra authority. Imports preserve consumer precedence,
custom-job ownership and source/cache bytes. Old runtime names are rejected.
commentreviews preserve existing votes; explicitresetclearsthe authenticated actor's vote. This intentional new behavior has no legacy
toggle; prompts are warned about, not rewritten.
10-label batch cap, authoritative label identities, non-atomic add/verify/remove
replacement and truthful partial results.
mark-pull-request-as-ready-for-review: active draft publication, exactone-field mutation, persisted read-back and repeat no-op.
update-pull-request-comment: verified same-pipeline/same-actor root-commentupdates with immutable ownership and a checked content hash. Unmarked history,
manual edits and conversations with replies are protected.
and close older eligible threads only after replacement succeeds. No deletion,
automatic vote reset or review dismissal.
ADO diff metadata, not local file presence, determines anchors.
max-commentsdefaults to 0; the whole batch is preflighted, comments precedevotes, stale heads stop further writes, and partial/uncertain outcomes persist.
Standalone comments are independent: no hidden buffering or duplicate posting.
push-to-pull-request-branch: exact source snapshot preparation, isolated-indexMCP capture, only the agent delta, patch/path/file/binary bounds, source-ref
allowlists, optimistic
oldObjectIdguard and persisted-head verification.No forks, force-push, rebase-on-race, fallback PR, policy bypass or immediate merge.
Failed/unconfirmed pushes block same-PR publication/review/auto-complete follow-ups.
compiler targets, approval/staged constraints and migration documentation.
Validation infrastructure repairs
cleanup reporting and diagnostic failure-issue suppression.
terminal-state cleanup proof.
explicit skip evidence, while missing records alone are not.
to pinned bare Git objects with identical exclusions and no checkout of PR code.
All five reviewer locks were regenerated with the pinned gh-aw compiler.
now invokes the compiler already staged at
/tmp/awf-tools/ado-aw.The corrected full pipeline passed on the published head.
Review remediation (October 1, 2026)
All ten divided-review findings now have production-path fixes and regression
coverage. Current head is
873a05d4, following the shared patch milestonec5dac7b5and filtered-preimage preservation fixc9f9881e.Final-head CI and required live evidence have passed. Historical failed-run
ref cleanup remains permission-blocked, as recorded below.
3172f1f6: normalize legacytruelike null at the source-migration boundary; canonical runtime schemas remain strict.c5dac7b5: parse native operations, inspect source/intermediate objects and bound expansion before Git application; covers the 99-copy amplification case even at the maximum configured limit.c5dac7b5: one application-glob selection; omit/report whole copy or rename operations and reject retained dependencies. Commit preimages handle rename swaps/chains correctly.c5dac7b5: isolated push index and exact bounded Git-blob serialization shared with creation. Creation applies and parents at the same verified captured base; original index/worktree/refs remain unchanged.d890eb8a: separate target namespace, corroborated legacy ownership, all-definition source-child terminal proof, PR-before-ref cleanup and SHA leases. Ambiguity retains resources.6b07d65b: only the auto-complete scenario accepts confirmed completion; uncertain abandonment gets one read-back, never blind replay.6b07d65b: independently resolve every selected local/cross-org reviewer prerequisite before any setup writes.b915f392: shared 30-second request / 8 MiB streamed-response bounds across the ADO PR family, including policy reads; incomplete metadata fails closed.2f97a8a4: port valuable assertions to production paths and remove retired request/result and inline-builder substitutes.b915f392: fetch/scan each immutable(commit,path)once, preserve proposal order, explicit comment authority and later head checks.Intentional size tightening: per-tool
max-patch-sizedefaults to 4096 KiB(previously 5 MiB), valid integer range 1–10240. Creation gains expanded-content
and full encoded-payload bounds. Both retain separate 10 MiB source-processing
and encoded REST ceilings. There is no automatic larger-limit migration or
agent override.
The remaining gh-aw comparison is documented against v0.89.21 /
856e7fa3ca4f1597f9adbd519eec415ce92320e2, not an unqualified latest-versionclaim. Native inputs and size defaults align; ADO-native schemas,
max-files,REST full-blob transport, exact-head guards and independent resource bounds
remain intentional differences.
Local integration: 3,893 Rust tests passed (2 ignored),
1,429 TypeScript tests passed, strict Clippy, typecheck, 59 registry shell
tests and 2 emitted-shell tests passed; both live harnesses built. The final
creation corrections also passed the complete Rust suite/strict Clippy and
82 directly affected TypeScript tests plus harness rebuild.
Required executor run 644640 finished 40/45 passing, no skips. All
guarded-push cases and the broader comment/review/label/publication matrix
passed. Five creation cases exposed a prefix-ref collision check, ADO's
one-operation-per-path restriction, and a fixture whose intended retained
content was also detected as a copy of the excluded source.
873a05d4correctsall three: exact complete ref matching, final-tree add/edit/delete serialization,
and independent fixture content. 644654 passed all 45 required cases, with
zero failures/skips, on
873a05d45640bc24483a4434339989c1e69658e1.The downloaded result artifact confirms the exact revision and selected IDs;
no run-owned refs remain.
Smoke run 644641 on
c5dac7b5passed and cleaned both ref namespaces.The final-head repeat 644659 on
873a05d4also passed all four cases:canary 644660, automatic boundary 644661, expected timeout rejection
644662, and actual-agent push 644663. The rejected child's one-minute
Manual Review failure is the expected result, not a suite failure: ungated
SafeOutputs ran, while reviewed writes/artifacts were withheld.
Persisted read-back verifies push commit
65a34c4487f76f3e41e9aff2a50e6a1e6ed794ba, its exact prepared parent, and exactproof-file bytes. All three disposable PRs 43213–43215 are abandoned,
required tags/artifacts exist, and all seven source/target refs are absent.
Final PR checks are green, including Linux Rust/TypeScript/drift, Windows
executor harness, prompt contracts and ADO integration checks.
No active run was replaced or counted as a pass. Failure-issue filing remained
disabled; no gate was auto-approved.
Historical cleanup blocker: the failed manual run 644640 and automatic
validation runs 644639/644644 left 24 refs (eight per run, under
refs/heads/ado-aw-det-<buildId>-). All nine associated owned PRs are confirmedabandoned. The exact retained refs/SHAs were recorded; conditional deletion
returned
forcePushRequiredfor the local identity. No permissions werechanged and no unconditional retry was made. These retained refs are not
counted as successful cleanup; current-candidate cleanup is verified separately.
Test plan
Evidence is tied to immutable revisions. Mocks, registered cases, cancellations,
and skipped prerequisites are not counted as live-service passes.
643035ond4f7f948: 20 passed, no failures/skips643206on7957fa6a: passed; canary643210, automatic643211, expected rejection643212; exact PR state, tags/artifacts, reviewed skip and five-ref cleanup verified643209onc8645de8: 4 passed, no skips; these establish platform prerequisites, not substitute executor coverage643219on34c307d5: 10 passed, no skips643228ona9be3c30: 6 passed, no skips, including forbidden-batch no-write and repeat-publication no-op643257on2726a751: 10 passed, no skips. The earlier run exposed immutable thread properties and deleted-fileitem.path=null; both corrected and re-proven643919onef74722c: 6 passed, no skips; exact source update, stale head, forbidden branch, protected file, hash mismatch and empty patch643927onc4aa0506: passed. Push child643932and canary643933succeeded; source preparation, Agent, Detection and SafeOutputs all succeeded. Observer verified direct-parent source commit, exact proof-file content, PR update, tags/artifacts and deletion of all three owned refs. Earlier643920exposed the PATH bug and was cleaned up; it is not counted as a passc4aa0506: Rust, Linux TypeScript/build/bundle/drift, Windows process harness and reviewer prefetch passed. The prefetch fix was also verified on86e552b4with a complete 28,672-line filtered diffDiagnostic runs explicitly disable GitHub failure-issue filing and report cleanup
outcomes. No line/branch coverage percentage is claimed. No main merge or release
is part of this work.