feat(safe-outputs): split Azure DevOps PR tools with safe migration

[Copilot]

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.

Review resolution highlights (October 6, 2026)

Current branch tip: c66b8cdb, a normal merge of main at a7d6582a
into the independently reviewed feature tip 4b439842. No history rewrite
or PR-to-main merge was performed.

The single conflict was in scripts/ado-script/src/shared/ado-remote.ts.
Resolution retains main's legacy discovery-URL normalization and exported
segment decoder while preserving this PR's strict HTTPS/collection validation
and triggering-PR identity checks. Added cross-consumer regressions prevent
discovery URLs being mistaken for collection identity. The auto-merged HTTP
test now asserts the branch's deliberate HTTPS-only contract.

Integration validation: 1,464 TypeScript tests passed, typecheck passed,
and all four affected bundles rebuilt. GitHub reports MERGEABLE, and fresh
integration checks on c66b8cdb have completed successfully: Rust build/tests,
Linux TypeScript/build/drift, Windows executor harness, prompt and SafeOutputs
contracts, ADO script E2E 645955, and executor E2E 645956. The current PR
check rollup has no unfinished or failed checks. Skipped reviewer jobs are not
counted as new independent review coverage.

The preceding d9b25ef0 added the requested abandonment result assertions;
4b439842 addresses the documentation, test, harness and review-delivery
follow-ups below. Their review/live evidence remains pinned to those revisions.

The four previously open GitHub review threads have evidence-backed replies
and are resolved, based on current code and regression tests rather than their
outdated-diff markers:

Review concern	Verified resolution
Module ordering	Declaration and re-export now place abandon_pull_request before add_build_tag; fixed in ba005bd2. Reply
Missing repository-denial coverage	rejects_disallowed_repository_before_network explicitly denies the default/self destination with allowed-repositories: ["other"] and asserts zero HTTP requests. Reply
Missing abandonment branches	Regressions cover title-prefix rejection, already-abandoned no-op, completed-PR refusal and comment failure after successful abandonment. d9b25ef0 adds the requested explicit already_abandoned/abandoned/comment-result assertions. Reply
Unreachable compiler validation	The canonical builder calls the dedicated PR-family validator, which invokes typed abandonment validation. Empty required-label configuration is rejected by the regression test. Reply

The later holistic findings are also addressed: reviewer assignment skips
observed existing membership without writing (749d2fa2, with the explicitly
accepted concurrent required-status race); native Git space-header delimiters
are handled; and staged executable-mode intent is preserved while
unrepresentable REST changes fail (94b5a633).

Revalidated on d9b25ef0:
cargo test --quiet --bin ado-aw abandon_pull_request — 19 passed.
The prior 63/63 executor run 645720 and successful real-agent smoke
645885 remain evidence for 2cda595f, not newly claimed runs on the test-only
tip. Current-tip validation is tracked separately;
closing old threads is not a maintainer approval or a claim that no new
findings can be raised.

Fresh review outcomes on d9b25ef0

Four reviewers completed and their summary reviews are confirmed published:

Reviewer	Outcome
Compiler contract	Two advisory documentation gaps: missing new PR modules in the AGENTS.md architecture map, and no standalone budget-groups reference/example.
Security	Reported no new security regressions; this is a review assessment, not a guarantee or maintainer approval.
Test quality	Requested malformed quoted-path escape cases and additional offline tool-entrypoint coverage for update-pull-request-comment. The claim of no behavioral coverage is too broad: shared ownership tests and successful live update/manual-edit-refusal scenarios already exist.
TypeScript	One confirmed test-harness issue: the inline-comment read-back fetch in scenarios/pr-comments.ts has no timeout.
Rust	Incomplete: the agent exceeded its 20-minute execution limit and produced no review proposal. It is not counted as a clean review.

Review publication also has a confirmed workflow gap: inline comment proposals
were skipped under the centralized workflow_dispatch command because the
handler's target: triggering did not recognize a PR context. Summary submission
found PR #2222 and succeeded, but direct review-comment API reads confirm zero
inline comments on those four reviews. Consequently, zero open threads must
not be interpreted as zero new findings.

Resolutions in 4b439842

Follow-up	Implementation and evidence
Architecture map	Added the missing PR modules and pr_patch parser/tests to AGENTS.md.
Shared-budget reference	Documented author-written budget-groups, its max/tools schema, a minimal example, zero/failed/no-op counting, member restrictions and matching approval/staged lanes.
Quoted-path decoder coverage	Added direct malformed quote, truncated/invalid octal, unsupported escape and invalid UTF-8 cases, plus exact token-boundary tests.
Owned-comment tool coverage	Added real execute_safe_output tests for successful owned updates, 100/101-byte key boundary, ID validation, wrong actor/pipeline/key, missing ownership, manual edits, replies and mismatched IDs; denied cases assert no writes.
Inline read-back timeout	Added the missing 30-second signal and tests proving the exact signal is passed and cancellation propagates.
Dispatch publication	Inline findings and summaries share a fixed PR number derived from trusted event/router context and a pinned commit. Each reviewer has a root PR-only activation guard; missing numbers fail as invalid zero. No wildcard target or extra write permission. All five locks regenerated with matching gh-aw v0.86.2.
Rust completion	Added a 12-minute investigation/submission budget, bounded three-file/six-minute critic, one at-most-60-second blocking read, and explicit scope/coverage reporting. No hard runtime limit increase or model override.

Local validation: 3,913 Rust tests passed (2 ignored), 1,452 TypeScript
tests passed, strict Clippy/typecheck, harness build and strict five-workflow
compilation passed. Eleven prompt/lock contract tests verify the actual
generated activation guards, fixed targets and read-only agent permissions.
A one-off test of the pinned upstream inline handler reproduced the old dispatch
skip and verified that fixed PR 2222 overrides an agent-supplied PR 9999; invalid
target zero performs no API read or buffering.

All five repaired reviewer runs completed successfully on exact 4b439842,
and one published COMMENT review per run has been verified through REST:

Reviewer	Run	Published result
Rust	37478733234	Review 5430136756; two actual inline comments published. The CLI completed in 15m36s under its existing 20-minute hard limit, though it exceeded the prompt's 12-minute soft target.
TypeScript	37478742539	Review 5430006955; no merge-blocking defects. One branch-prefix constant nit was preserved in the summary because the proposed line 2190 could not be inline-anchored.
Test quality	37478751617	Review 5430012543; no blocking coverage/assertion findings.
Security	37478760108	Review 5429987225; no merge-blocking security regressions reported.
Compiler contract	37478769622	Review 5429971352; documentation/registration/drift checks found no new blocking issue.

The Rust safe-output log confirms target: 2222, both handlers pinned to
4b439842, two buffered findings, and successful review submission. This is
actual inline-publication evidence, not just a green setup/evaluation job.
Its free-text heading mistakenly names d9b25ef0; checkout, prefetch, output
configuration and GitHub's review commit_id all identify 4b439842.

Review limits and new advisory notes remain explicit:

• Rust examined abandonment, patch parsing and shared mutation code, with a
  lighter target/HTTP pass. It explicitly did not cover every Rust module; this
  is not an exhaustive all-files sign-off.
• Its transport-error finding is a valid structured-diagnostic improvement for
  uncertain abandonment writes; the suggested snippet does not itself restore
  structured result data, and adjacent paths do not universally preserve data.
• Its VSSPS comment mixes two cases: a URL with no organization is not a complete
  ADO organization URL, but mixed-case hosts can reach the raw URL path through
  CLI/environment overrides. Structural host handling merits a targeted check.
• TypeScript's repeated ownership-prefix literal is a small maintainability
  follow-up, not a demonstrated production failure.

The two Rust threads and the TypeScript summary note have not been
auto-resolved or silently changed. All verdicts are COMMENT, not maintainer
approval. CI blocker resolved: the earlier ACTION_REQUIRED checks were
caused by branch conflicts, not human approval gates. Merge commit c66b8cdb
clears that conflict state; GitHub reports MERGEABLE and fresh integration
checks have passed. This does not automatically resolve the advisory notes or
constitute maintainer approval.

Implemented capabilities

• Closed PR configuration/proposal schemas; unsupported policy fields no longer
  appear to work while being ignored.
• Common triggering/fixed/wildcard targets, default repository, allowlists and
  label/title filters across PR mutations. Triggering means the complete trusted
  collection/project/repository/PR identity, not just a numeric ID.
• Proven legacy explicit-ID scope and original aggregate budgets migrate
  without granting extra authority. Imports preserve consumer precedence,
  custom-job ownership and source/cache bytes. Old runtime names are rejected.
• Content updates retain final UTF-16 limits and strict managed-section rules.
• Non-voting comment reviews preserve existing votes; explicit reset clears
  the authenticated actor's vote. This intentional new behavior has no legacy
  toggle; prompts are warned about, not rewritten.
• Bounded label add/remove/replace policies: allow/block controls, default
  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, exact
  one-field mutation, persisted read-back and repeat no-op.
• update-pull-request-comment: verified same-pipeline/same-actor root-comment
  updates with immutable ownership and a checked content hash. Unmarked history,
  manual edits and conversations with replies are protected.
• Opt-in non-destructive comment supersession: preserve text, mark superseded
  and close older eligible threads only after replacement succeeds. No deletion,
  automatic vote reset or review dismissal.
• Exact-source-head left/right inline positioning, including deleted files.
  ADO diff metadata, not local file presence, determines anchors.
• One self-contained review proposal with summary and inline findings.
  max-comments defaults to 0; the whole batch is preflighted, comments precede
  votes, 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-index
  MCP capture, only the agent delta, patch/path/file/binary bounds, source-ref
  allowlists, optimistic oldObjectId guard 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.
• Updated authoring guidance, previews, catalog, audit records, registries,
  compiler targets, approval/staged constraints and migration documentation.

Validation infrastructure repairs

• JavaScript process fixtures run through Node on Linux and Windows.
• PR label read-back uses the dedicated endpoint; cleanup is idempotent.
• Exact required selections, prerequisite preflight, JSON result artifacts,
  cleanup reporting and diagnostic failure-issue suppression.
• Bounded transient status-read recovery without write replay or weakened
  terminal-state cleanup proof.
• Actual ADO timeline handling: a skipped Phase without an allocated Job is
  explicit skip evidence, while missing records alone are not.
• Large-PR reviewer prefetch falls back from GitHub's 20,000-line diff API limit
  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.
• Real-agent validation caught a pre-agent PATH assumption; source preparation
  now invokes the compiler already staged at /tmp/awf-tools/ado-aw.
  The corrected full pipeline passed on the published head.

Holistic review follow-up (October 5, 2026)

Production fix: 749d2fa2; production-validation candidate 2cda595f
additionally corrects the optional-reviewer fixture. The later review tip
d9b25ef0 changes only test assertions. The three later findings are addressed:

• Existing reviewer state: read complete membership immediately before each
  addition and make existing identities no-write no-ops. Missing identities use
  an ID-only collection POST followed by membership read-back. The focused tool
  and creation's configured-reviewer follow-up share this behavior. Results
  distinguish added, already_present, and failed; creation retains its PR
  identity and emits a warning for unconfirmed reviewer follow-ups.
• Native Git paths: 94b5a633 separates Git's terminal tab delimiter from
  old/new header filenames. Real Git-generated spaced-path and rename-with-edit
  fixtures cover MCP capture and Stage 3 without weakening path validation.
• Executable modes: 94b5a633 preserves staged mode intent in private-index
  capture and rejects modes that content-only REST cannot represent before
  remote writes. Existing executable-content edits remain supported and are
  checked using tree modes, not just blob IDs. No new publication transport.

Explicitly accepted residual risk: membership read and addition are not
atomic. If another actor adds the same reviewer and marks them required between
those requests, the collection POST can clear required status. The October 5
decision accepts this narrow race rather than disabling new assignments.
There is no stale-state restoration or blind write retry.

The API investigation did not prove atomic preservation: ID-only collection
POST preserved the vote but cleared required status; individual PUT, including
If-None-Match: *, reset both and returned HTTP 200. Those failed capability
hypotheses remain recorded and locally tested, but are not registered as routine
production-success scenarios.

Local integration: 3,907 Rust tests passed (2 ignored); 1,450 TypeScript
tests passed; strict Clippy/typecheck and both harness builds passed.
Mode/path run 645444 passed all 16 required cases, with no remaining refs.
Required executor 645706 finished 62/63 passing, no skips, including both
required-reviewer vote cases and configured creation. The only failure was
fixture setup: ADO forbids a PR creator declining their own review.
2cda595f uses supported optional/flagged creator state and adds a setup
regression; all state-preservation assertions remain. Already-declined reviewer
preservation is covered deterministically, not claimed as live coverage.
The corrected run 645720 passed all 63 required cases, with no skips, on
2cda595f26a9e617b95ff8e5b7c10f5771125652. Its downloaded result artifact
confirms the exact selected cases and candidate revision. No run-owned refs
remain from either 645706 or 645720. This includes existing negative/positive
required reviewers, optional/flagged membership, new assignments and configured
creation/follow-up no-ops, as well as the full path/mode and earlier PR matrix.

Actions recovered (October 6, 2026): all seven outage-affected workflows
passed on attempt 2 at unchanged 2cda595f: Rust, Linux TypeScript/build/drift,
prompt contracts, prompt evaluation, title lint, PR prefetch and the TypeScript
reviewer. Annotations confirmed that the original cancelled jobs never acquired
hosted runners. Only those jobs and their dependents were retried; previously
successful checks were left untouched. Those checks passed on 2cda595f;
fresh checks on the later test-only review tip are tracked separately.

Actual-agent authentication recovered after token rotation: following the
user's token refresh, smoke 645885 passed on unchanged 2cda595f:
canary 645887 and actual-agent PR push 645886 both succeeded.
Agent, Detection and SafeOutputs succeeded; all three proposed outputs were
confirmed. Persisted Git read-back verifies commit
83d5c20dc8d804fdbadfae2cc07deb27ddfc2c3b, its exact prepared parent
03cdf57da8a6ac6601843621419de848382d5d09, and exact proof-file content.
The expected build tag and safe-output artifact exist. Disposable PR 43841
is abandoned and all three run-owned source/target refs are absent.
The authentication validation hold is closed; no source changes were needed.

Historical authentication failures: smoke 645707 failed:
children 645714/645715 stopped before producing proposals
with GitHub 401 Bad credentials. This is not proven to be caused by the
Actions incident. No credentials/permissions were changed and no agentic smoke
rerun was queued as part of Actions recovery.

The explicitly requested retry 645873 on unchanged 2cda595f also failed
on October 6. Canary 645875 failed and PR-push 645874 was canceled;
both agent logs show "Authentication token found but could not be validated"
and GitHub 401 Bad credentials before any proposals were produced.
Credentials were not changed for that attempt. These failed runs are not counted
as passing code-validation evidence; the post-rotation run above supplies it.

Cleanup for the retry is complete: disposable PR 43840 is confirmed
abandoned, and all three refs in its source/target namespaces are absent.
It introduced no additional retained refs. Existing historical retained refs
remain separately tracked.

The smoke's retained PR 43686 is now confirmed abandoned after its source
child became terminal. Its two unchanged refs remain: expected-SHA deletion
returned forcePushRequired. The exact refs/SHAs are recorded separately from
the 24 historical refs below; none are represented as successful cleanup.

Review remediation (October 1, 2026)

All ten divided-review findings now have production-path fixes and regression
coverage. That milestone was 873a05d4, following the shared patch milestone
c5dac7b5 and filtered-preimage preservation fix c9f9881e.
CI and required live evidence for that milestone passed. Historical failed-run
ref cleanup remains permission-blocked, as recorded below.

Finding	Response
Legacy boolean shorthand	3172f1f6: normalize legacy true like null at the source-migration boundary; canonical runtime schemas remain strict.
Native copy expansion	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.
Exclusion mismatch	c5dac7b5: one application-glob selection; omit/report whole copy or rename operations and reject retained dependencies. Commit preimages handle rename swaps/chains correctly.
Checkout byte conversion	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.
Stale boundary cleanup	d890eb8a: separate target namespace, corroborated legacy ownership, all-definition source-child terminal proof, PR-before-ref cleanup and SHA leases. Ambiguity retains resources.
Auto-complete race	6b07d65b: only the auto-complete scenario accepts confirmed completion; uncertain abandonment gets one read-back, never blind replay.
Mixed-org preflight	6b07d65b: independently resolve every selected local/cross-org reviewer prerequisite before any setup writes.
Unbounded PR transport	b915f392: shared 30-second request / 8 MiB streamed-response bounds across the ADO PR family, including policy reads; incomplete metadata fails closed.
Obsolete test algorithms	2f97a8a4: port valuable assertions to production paths and remove retired request/result and inline-builder substitutes.
Repeated inline reads	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-size defaults 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-version
claim. 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. 873a05d4 corrects
all 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 c5dac7b5 passed and cleaned both ref namespaces.
The final-head repeat 644659 on 873a05d4 also 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 exact
proof-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 confirmed
abandoned. The exact retained refs/SHAs were recorded; conditional deletion
returned forcePushRequired for the local identity. No permissions were
changed 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.

Coverage	Evidence
October 5 pragmatic reviewer/path/mode correction	645720 on 2cda595: 63/63 required cases passed, no skips; zero current-run refs. Existing reviewer state is preserved by no-write no-ops; the documented concurrent required-status race remains accepted. Actions CI and actual-agent validation recovered October 6.
October 6 Actions recovery	All seven targeted attempt-2 reruns passed on 2cda595. Only runner-starved jobs and dependents retried, with no code changes. Later test-only tip CI is tracked separately.
October 6 post-rotation real-agent smoke	645885 on 2cda595: 2/2 cases passed, canary645887 and PR-push645886. Verified successful Agent/Detection/SafeOutputs, exact push parent/content, three confirmed outputs, expected tag/artifact, PR43841 abandoned and three-ref cleanup.
Final required remediation executor matrix	644654 on 873a05d: 45/45 passed, no skips; native copy/rename, whole-operation exclusions, CRLF/binary fidelity, pre-application expansion rejection, broad PR operations and current-run cleanup verified
Final full-pipeline boundaries and real-agent push	644659 on 873a05d: 4/4 passed; children 644660–644663, expected manual-review timeout, exact push parent/content, tags/artifacts, three abandoned PRs and seven-ref cleanup verified
Final local and CI validation	3,893 Rust passes (2 ignored), 1,429 TypeScript passes, strict Clippy/typecheck and enforced shell checks; final affected TS rerun 82 passed. All checks green on 873a05d
Original required PR executor matrix	643035 on d4f7f948: 20 passed, no failures/skips
Synthetic automatic and timeout-rejected boundaries	643206 on 7957fa6a: passed; canary 643210, automatic 643211, expected rejection 643212; exact PR state, tags/artifacts, reviewed skip and five-ref cleanup verified
ADO lifecycle API prerequisite probes	643209 on c8645de8: 4 passed, no skips; these establish platform prerequisites, not substitute executor coverage
Non-voting review/reset and exact-target regressions	643219 on 34c307d5: 10 passed, no skips
Labels and draft publication	643228 on a9be3c30: 6 passed, no skips, including forbidden-batch no-write and repeat-publication no-op
Owned comments, left/right anchors and review batches	643257 on 2726a751: 10 passed, no skips. The earlier run exposed immutable thread properties and deleted-file item.path=null; both corrected and re-proven
Deterministic guarded PR pushes	643919 on ef74722c: 6 passed, no skips; exact source update, stale head, forbidden branch, protected file, hash mismatch and empty patch
Real-agent source preparation / MCP capture / push	643927 on c4aa0506: passed. Push child 643932 and canary 643933 succeeded; 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. Earlier 643920 exposed the PATH bug and was cleaned up; it is not counted as a pass
Full local regression	3,542 Rust unit passes, 1 ignored, plus integration suites; 1,342 TypeScript tests before the four additional prefetch regressions; strict Clippy/typecheck and shell registry/emission guards passed
Published-head CI	c4aa0506: Rust, Linux TypeScript/build/bundle/drift, Windows process harness and reviewer prefetch passed. The prefetch fix was also verified on 86e552b4 with a complete 28,672-line filtered diff
Additional-project/organization live coverage	Blocked: approved destination inputs absent; no resources/grants provisioned
Native PR full-pipeline boundary	Blocked: existing mirror has no native build-validation policy
Human-approved reviewed path	On demand only: never auto-approved

Diagnostic 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.

[azure-pipelines]

Azure Pipelines:
Successfully started running 2 pipeline(s).
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract 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]

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 calls queue_sanitized_output — it never touches the write-capable token. The actual PATCH/POST happens in execute.rs/abandon_pull_request.rs's Executor::execute_impl, which runs in Stage 3 with ctx.access_token (the write-capable token the agent never sees), consistent with update-pr and submit-pr-review.
• Sanitization: the agent-supplied body is passed through sanitize_text in sanitize_content_fields before being reused as the abandonment comment, and repository through sanitize_config — matching the existing pattern for other PR-comment safe outputs.
• Repository resolution: uses the same resolve_repo_name + allowed-repositories allowlist gate already used by update-pr, submit-pr-review, and resolve-pr-thread. Repository/project segments are percent-encoded (utf8_percent_encode with PATH_SEGMENT) before being placed in the URL, so no path/URL injection via repository.
• State check before mutation: the PR is fetched and its status validated (active required, abandoned short-circuited) and title/label filters applied before the PATCH that 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 no ado-aw-debug gate 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}

[github-actions]

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}

[github-actions]

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:

1. Likely dead validation code (comment on src/compile/common.rs): the new "abandon-pull-request" match arm lives inside validate_github_issue_outputs_config, which only iterates GITHUB_ISSUE_SAFE_OUTPUT_TOOLS — a list abandon-pull-request is not part of. If that reading is correct, validate_abandon_pull_request_config is never invoked at compile time and an invalid config (e.g. an empty required-labels entry) will silently compile. No test exercises this path, which is exactly why it slipped through.
2. Untested control-flow branches in abandon_pull_request.rs: the allowed-repositories rejection, the required-title-prefix mismatch, 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}

[github-actions]

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-critic sub-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 anyhow context consistently, no unwrap/expect on user-input paths, repository/target resolution mirrors resolve_repo_name/resolve_repository_write_target conventions used elsewhere.
• One clippy derivable_impls hint on the manual Default for AbandonPullRequestTarget impl — already caught by cargo 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}

[jamesadevine]

/review

[github-actions]

⚠️ Rust Code Quality Reviewer failed during the Rust code quality review.

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Compiler Contract Review

Checked for the usual drift classes: ado-script bundle/codegen drift (none — no filter_ir.rs/Fact changes), compiled-workflow drift (.lock.yml files move alongside their .md sources, pr-sous-chef/review-rust/review-typescript all consistent), tests/safe-outputs/ smoke sources (markdown-only, correctly registered via tests/smoke/cases.json, no stray lock files), front-matter grammar additions (Option/defaulted, codemods 0009–0011 properly registered in CODEMODS), and safe-output identifier typing (PrLabelName newtype correctly added to src/secure.rs and used across add_pr_labels.rs/remove_pull_request_labels.rs/replace_pull_request_label.rs; push_to_pull_request_branch.rs correctly uses GitRefName/CommitSha).

Two documentation-sync gaps found (both non-blocking):

1. AGENTS.md's src/safe_outputs/ tree is missing ~10 of this PR's new files (mark_pull_request_as_ready_for_review.rs, pr_comments.rs, pr_http.rs, pr_inline.rs, pr_labels.rs, pr_patch.rs, push_to_pull_request_branch.rs, remove_pull_request_labels.rs, replace_pull_request_label.rs, update_pull_request_comment.rs) — only 6 of the ~16 new modules were added to the tree.
2. safe-outputs.budget-groups is a new author-writable front-matter key (validated in pr_migration.rs::validate_budget_groups, constructed directly in front matter by tests/codemod_tests.rs) but is documented only as a migration side effect, not as a standalone feature authors can use.

Everything else checked (codegen, schemas, registries, extension registration, generated-shell patterns) is consistent with the existing contracts.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 137.6 AIC · ⌖ 1.98 AIC · ⊞ 12.5K
_{Comment /review to run again}

[github-actions]

Security review — no regressions found

This diff is a large, security-conscious refactor of the Azure DevOps PR safe-output tools (new pr_patch, pr_http, pr_mutations, pr_common modules; new abandon-pull-request / push-to-pull-request-branch tools). I focused on the threat model for this repo: injection into generated pipelines, weakened validation, path traversal, token scope/projection, network boundary, and safe-output integrity.

Findings: none that weaken the existing security posture. Highlights that strengthen it:

• Bounded HTTP everywhere — pr_http.rs introduces a shared 8 MiB response bound and continuation-token/partial-page rejection; create_pull_request.rs's old raw resp.json() call was migrated to bounded_json().
• New validated newtype — PrLabelName in src/secure.rs runs reject_pipeline_injection plus length/control-char checks at deserialization time, consistently used across add_pr_labels.rs, remove_pull_request_labels.rs, replace_pull_request_label.rs instead of raw String.
• push-to-pull-request-branch enforces an exact expected-head guard, rejects fork-backed PRs, non-branch refs, completed/abandoned PRs, and verifies the ADO push response's ref/commit/parent before declaring success — a solid anti-TOCTOU design including a re-check for the source moving during patch preparation.
• Patch parsing (pr_patch/parse.rs) is a from-scratch, bounds-checked unified-diff/binary-delta parser (size caps, symlink rejection, path validation via RelativeSafePath, conflicting-destination/removal detection) rather than shelling out to an unbounded git apply.
• Git subprocess hygiene — git_command() uses env_clear() with an explicit allow-list and GIT_TERMINAL_PROMPT=0/GIT_NO_LAZY_FETCH=1, and the PR-diff-prefetch GitHub Actions step scopes its credential header to the exact GITHUB_SERVER_URL host with http.followRedirects=false before fetching pinned base/head SHAs.
• Token/target integrity — PrMutationPolicy/resolve_pr_policy_target validate every policy string through reject_pipeline_injection, and source_state() cross-checks repository/project/PR identity against the authorized target before any write.

I did not find any new unsanitized value reaching a bash:/##vso[...]/template-expression sink, any newtype-to-String downgrade, or any token reaching a Stage 1 (Agent) step beyond the existing SC_READ_TOKEN/System.AccessToken pattern already used elsewhere in the compiler. Nothing here is security-neutral filler either — several of the above are genuine hardening over the previous implementation (e.g., replacing the old format-patch/synthetic-commit flow in mcp.rs with an isolated private-index capture).

No blocking issues from a security standpoint.

🔒 Security review by PR Security Reviewer · auto · 125.2 AIC · ⌖ 3.27 AIC · ⊞ 11.4K
_{Comment /review to run again}

[github-actions]

Reviewed the PR-safe-outputs test suite (the Rust src/safe_outputs/pr_* family, pr_patch/parse.rs, and related). Overall the test quality here is strong: ownership/provenance in pr_comments.rs, label-transition policy in pr_labels.rs, and inline-review supersession in submit_pr_review.rs all have thorough, behavior-focused test coverage with real adversarial cases (self-approval, concurrent edits, forged ownership markers, head-moved races).

Two gaps worth closing before this consolidates further:

1. unquote() in the new pr_patch/parse.rs — this git-style octal/backslash path-escape decoder for untrusted patch content has no direct unit test; only one happy-path octal sequence is exercised indirectly through inspected_paths. Malformed-escape edge cases (unterminated quote, truncated octal, invalid escape char) aren't covered.
2. update-pull-request-comment — this new 168-line tool has zero tests of its own. The only coverage is a shared cross-tool smoke test that checks the generic required-labels gate, not this tool's ownership verification, comment-key bounds, or ID validation.

Both findings are posted inline. Nothing else in the diff showed weakened assertions — files like update_pr.rs and the inline-comment helpers in add_pr_comment.rs had code removed, but the corresponding behavior and assertions moved to new modules (pr_inline.rs, abandon_pull_request.rs, etc.) with equivalent or stronger coverage, which is a legitimate refactor rather than a regression.

🧪 Test quality analysis by Test Quality Sentinel · auto · 212.5 AIC · ⌖ 1.83 AIC · ⊞ 11K
_{Comment /review to run again}

[github-actions]

TypeScript review (scripts/ado-script/)

Reviewed the production TS diff (28 files, test/generated files excluded per workspace convention). The ts-critic sub-agent ran in the background and returned one finding after a brief retry (first turn had no diff attached; it answered once I pointed it at the on-disk file).

Overall: solid. The new ADO REST helpers (ado-rest.ts boundary-PR/ref APIs), ado-remote.ts identity parsing, and approval-summary PR-target rendering are thoroughly defensive — strict shape validation on every REST response, AdoHttpError with typed retry classification, force-with-lease SHA-pinned ref deletion with reconciliation on failure, bounded integer parsing (positivePrId, preserveLargeIntegers for u64 IDs), and injection-safe marker sanitization ($(, ##vso[, {{ rejected in trusted identity fields). No unhandled rejections, no any/unchecked casts on external data, no secret leakage into logs or errors.

One minor finding (inline, test-harness scope): a readback fetch() in pr-comments.ts is missing the AbortSignal.timeout its sibling mutation call uses.

Not merge-blocking.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 376.4 AIC · ⌖ 2.67 AIC · ⊞ 11.4K
_{Comment /review to run again}

[github-actions]

Compiler contract review — no blocking findings

Checked the areas in my mandate against this PR:

• Codegen/lock drift: no Fact/filter_ir changes, so fact-catalog.gen.json correctly wasn't touched. No .github/workflows/*.md changed without its matching .lock.yml. tests/safe-outputs/ smoke sources remain lock-free, consistent with the lane model.
• Breaking front-matter changes: the update-pr → focused PR tools split and the abbreviated-name rename are both covered by new codemods 0009_split_update_pr, 0010_pull_request_tool_names, and 0011_explicit_pr_policy (registered in codemods/mod.rs), including idempotency/version-gating tests.
• Safe-output identifier typing: new PR tools consistently use src/secure.rs newtypes where expected — PrLabelName for label fields (add-pull-request-labels, remove-pull-request-labels, replace-pull-request-label), and RelativeSafePath/CommitSha/StrictRelativePath in push-to-pull-request-branch. No raw-String identifier fields found where a newtype was warranted.
• Generated shell: the new PREPARE_PR_PUSH_SOURCE script in agentic_pipeline.rs is correctly registered via shell_script! with declared bindings/externals, and the SYSTEM_ACCESSTOKEN credential goes through .with_env(..., EnvValue::secret(...)) rather than a binding.
• Public IR/graph contracts: no PipelineSummary/GraphSummary field renames; new dependency wiring (e.g. budgetGroups in resolved_execution_config_json) doesn't touch graph cycle detection.
• Docs sync: AGENTS.md's safe_outputs/ module tree is updated for every new/renamed file (abandon_pull_request.rs, pr_labels.rs, pr_patch/, etc.), and docs/safe-outputs.md documents the new tools, shared budget groups, and label policies in depth.
• Binary patch decompression (pr_patch/parse.rs) bounds flate2 output with .take(MAX_SOURCE_BYTES + 1) before read_to_end, avoiding a zip-bomb amplification path.

Given the PR description's extensive prior review-resolution history, this pass found nothing new to add on top of the already-resolved threads.

🏗️ Compiler contract review by Compiler Contract Reviewer · auto · 96.4 AIC · ⌖ 3.15 AIC · ⊞ 12.8K
_{Comment /review to run again}

[github-actions]

Security review — no merge-blocking findings

Reviewed this PR's diff against main for erosion of the Agent/Detection/SafeOutputs trust boundary: token projection (ado_bundle.rs-adjacent flows), path/ref validation on the new push-to-pull-request-branch + shared pr_patch module, repository allow-listing in pr_common.rs::resolve_pr_policy_target, and the new AW_PR_TRIGGERING_IDENTITY propagation from the Agent/Setup job into SafeOutputs.

Findings: none merge-blocking. The diff is security-neutral-to-positive:

• push-to-pull-request-branch's prepare-pr-push step only ever receives a read token (SC_READ_TOKEN or System.AccessToken), asserted by its own compiler test (never SC_WRITE_TOKEN) — the write-capable token projection boundary is preserved.
• The new src/safe_outputs/pr_patch/ patch parser validates every path through RelativeSafePath::parse, rejects control characters and literal backslashes, bounds binary-frame/compressed sizes against MAX_SOURCE_BYTES, and explicitly rejects newly-introduced symlinks (mode 120000) in both the push and create-PR code paths.
• secure.rs adds a new validated PrLabelName newtype (routed through reject_pipeline_injection) rather than loosening an existing one.
• pr_http.rs adds a bounded-response wrapper (8 MiB cap, continuation-token rejection, explicit timeouts) used uniformly by the new PR mutation paths — this is new defense-in-depth, not a removal.
• resolve_pr_policy_target/resolve_pr_target enforce allowed-repositories membership and cross-check temporary-ID-resolved repositories against the caller's requested selector before any mutation.
• The new AW_PR_TRIGGERING_IDENTITY value is sourced only from trusted ADO predefined variables (Build.Repository.*, System.CollectionUri) in Setup/native builds or from the compiler-owned synthetic-PR step, validated with a strict GUID/control-character/pipeline-injection regex on the TypeScript side (parseTriggeringPrIdentity), and is never derived from agent-authored content.
• A new executor-e2e test (pr-boundary/ado-rest tests) explicitly asserts a fake access token never leaks into the executor's report output.

No added network-allowlist domains, no sanitize.rs/validate.rs weakening (neither file is touched), and no new raw-String identifier fields replacing a secure.rs newtype were found. I did not flag anything already covered in the four existing review threads from github-actions[bot].

This review is scoped to security regressions only; correctness/test-coverage questions are owned by the other reviewers.

🔒 Security review by PR Security Reviewer · auto · 136.3 AIC · ⌖ 1.85 AIC · ⊞ 11.7K
_{Comment /review to run again}

[github-actions]

TypeScript Code Quality Review — scripts/ado-script/

Reviewed the diff scoped to scripts/ado-script/src/** (tests excluded from scoring per contract). The ts-critic sub-agent ran in parallel and returned successfully; its findings were all low-severity maintainability nits in test-harness code (executor-e2e/, compiler-smoke-e2e/), triaged below.

Production bundle code (approval-summary/, exec-context-pr-synth/, shared/ado-remote.ts) is solid:

• No unhandled promise rejections or forEach-with-async patterns found.
• External data (NDJSON proposals, env vars, REST responses) is consistently shape-validated before use — parseTriggeringPrIdentity, parsePrPolicies, positivePrId all reject malformed input rather than trusting it.
• as Foo casts are limited to already-validated Record<string, unknown> narrowing, not blind trust of external shapes.
• No tokens/secrets reach log lines or thrown Error messages.
• The one real catch { return undefined } (in readTriggeringPrIdentity) is a safe fail-closed fallback on malformed trusted JSON, not an error swallow.

Test-harness code (executor-e2e/, compiler-smoke-e2e/) is new E2E infrastructure, not shipped in ado-script.zip. It's unusually defensive for test code (exact-match REST response validation, conditional ref deletion with lease semantics, bounded retry/cancellation). ts-critic's findings here were all low-severity:

• A hardcoded branch-prefix literal in verifyBoundaryPush instead of reusing CANDIDATE_BRANCH_PREFIX — posted inline (the one finding I kept, since it's a real maintainability trap on an ownership-proving function).
• A lane definition 0 sentinel-collision in a log message, a reviewer-value guard relying on an implicit upstream check, and a missing test branch for one artifact-verification failure path — all triaged as DROP: cosmetic, test-only, or redundant with existing coverage, not worth a review comment budget slot.

No merge-blocking defects found.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 262.9 AIC · ⌖ 3.27 AIC · ⊞ 11.7K
_{Comment /review to run again}

Comments that could not be inline-anchored

scripts/ado-script/src/compiler-smoke-e2e/ado-rest.ts:2190

verifyBoundaryPush hardcodes the literal "refs/heads/ado-aw-smoke-candidate/" instead of reusing CANDIDATE_BRANCH_PREFIX from ./config.js, which every other owned-ref check in this file (and git.ts) derives from.

<details><summary>💡 Why this matters</summary>

If CANDIDATE_BRANCH_PREFIX is ever renamed, this ownership guard silently stops matching real candidate refs (or starts matching the wrong ones) while every sibling check (parseCandidateRef, createBoundaryTarget, `listCa…

[github-actions]

Test Quality Sentinel — no blocking findings

Reviewed the test diffs in this PR (new abandon_pull_request.rs, pr_migration.rs, pr_policy.rs, pr_patch.rs/parse.rs, codemods 0009-0011, and the exec-context-pr-synth / ado-remote.ts identity plumbing) against the production changes they cover.

This diff is unusually well-tested for its size:

• Every new module ships a dedicated #[cfg(test)]/tests suite that exercises error paths explicitly (e.g. rejects_title_prefix_before_patch, rejects_disallowed_repository_before_network, handles_already_abandoned_and_rejects_completed, fixed_target_rejects_mismatched_request_id in abandon_pull_request.rs).
• Migration/codemod logic (pr_migration.rs, pr_policy.rs, codemods 0009-0011) is tested for idempotency (a second migrate call returns false and leaves state unchanged) and for conflicting/ambiguous states that must fail atomically without rewriting source files — directly protecting against the riskiest bug class in a migration tool: partial rewrites.
• One assertion-shape change that looked suspicious at first glance (Condition::And(...) to Condition::Succeeded in src/compile/extensions/ado_script.rs tests) is a legitimate simplification tied to the matching production condition change, not a weakened regression check, and a new AW_PR_TRIGGERING_IDENTITY assertion was added alongside it.
• nativeTriggeringPrIdentity/parseTriggeringPrIdentity in ado-remote.ts lack a dedicated direct unit test for every branch, but are exercised end-to-end through approval-summary and exec-context-pr-synth consumer tests covering both the synthetic (ADO_AW_TRIGGERING_PR_IDENTITY) and native (ADO_AW_TRIGGER_*) paths, including malformed and foreign-org rejection — acceptable indirect coverage given the thin, well-typed surface of that parser.

I did not find an untested new behavior, a weakened or deleted assertion, or a reintroduced tests/safe-outputs/*.lock.yml / per-case ADO definition in this diff that would warrant a blocking comment. No inline comments posted.

🧪 Test quality analysis by Test Quality Sentinel · auto · 184.3 AIC · ⌖ 1.73 AIC · ⊞ 11.3K
_{Comment /review to run again}

[github-actions]

Rust review — d9b25ef0

Reviewed the Rust-specific diff: src/safe_outputs/abandon_pull_request.rs (new), src/safe_outputs/pr_patch/parse.rs (new native patch parser), src/safe_outputs/pr_mutations.rs (new, shared PR mutation ops: set-auto-complete, add-reviewers, add-labels, update-description), plus a lighter pass over pr_common.rs/pr_http.rs for the shared target-resolution and HTTP helpers these depend on.

Overall this is careful, well-tested code: every fallible HTTP path returns a structured ExecutionResult rather than panicking, checked_add/checked_sub guard the patch-parsing arithmetic throughout pr_patch/parse.rs, reviewer-identity resolution re-checks membership after a POST rather than trusting the write blindly, and the AbandonPullRequestTarget serde visitor correctly rejects zero/negative IDs on every input variant (u64, i64, string).

Two inline findings, both low/medium and non-blocking:

• An abandon_pr transport failure loses the PR-ID context that every sibling error path in the same function preserves (inconsistency, not a crash).
• The VSSPS endpoint derivation in pr_mutations.rs is a literal substring match rather than a structural URL check — not reachable today given how organization_url is constructed, but fragile for future callers.

rust-critic ran in the background and returned 5 candidate findings; I incorporated its two highest-value ones above (the abandon_pr context loss, adjudicated as HARDEN with added detail, and the VSSPS substring fragility). Its other three (an unwrap_or_else swallowing the original read error, a theoretical raw.len() - 1 underflow on a single-byte hunk line) were triaged DROP: the unwrap_or_else pattern is intentional and repeated 20× across this module family as the established error-formatting convention, and the raw.len() - 1 lines are only reached after lines are confirmed non-empty via the preceding split_inclusive(b'\n') producing at least a \n-terminated byte, so raw.len() >= 1 always holds there.

Scope not covered: the Rust diff also touches create_pull_request.rs, push_to_pull_request_branch.rs, pr_patch.rs/pr_patch/tests.rs, pr_labels.rs, submit_pr_review.rs, update_pull_request.rs, and several codemods — these were out of this run's 3-file bounded scope and were not inspected.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 340.2 AIC · ⌖ 1.97 AIC · ⊞ 12.1K
_{Comment /review to run again}

[github-actions]

A transport failure in abandon_pr (e.g. network timeout, DNS failure) propagates via the outer ? as a generic anyhow::Error, losing the pull_request_id/repository context that every sibling failure path in this function preserves through ExecutionResult.

💡 Why this matters

Every other failure branch in execute_impl — fetch, label, filter, and the comment-post error paths just below — returns Ok(ExecutionResult::failure(...)) or abandoned_comment_warning(...) carrying pull_request_id/repository in structured data. When abandon_pr's HTTP send itself fails (not just a non-2xx response), the ? instead surfaces as a bare anyhow::Error to the Stage 3 caller, which wraps it as "Failed to execute entry: {e}" with no PR ID — audit tooling and operators then can't tell which PR abandonment attempt failed from transport-level errors, unlike every adjacent branch here.

Consider mapping the transport error the same way fetch_pr/post_comment do internally, but surfacing it as an ExecutionResult::failure with the pull_request_id embedded, e.g.:

let abandon_result = abandon_pr(&client, &pr_url, pull_request_id, token, ctx)
    .await
    .map_err(|error| anyhow::anyhow!("Failed to abandon PR #{pull_request_id}: {error}"))?;
if let Err(result) = abandon_result {
    return Ok(result);
}

This at least keeps the PR ID in the error text reaching the operator/audit log.

[github-actions]

VSSPS base derivation only matches the exact substring "://dev.azure.com/", so a valid org URL without a trailing path segment (just https://dev.azure.com) or any case variant silently fails add-reviewers with "Cannot derive VSSPS identity endpoint" even though the org is fully supported.

💡 Why this matters

organization_url is stored as e.g. "https://dev.azure.com/org" per the test fixtures in result.rs/pr_common.rs, so trimmed_org.replace("://dev.azure.com/", ...) works for the common shape. But the match is a literal string substitution, not a URL-structural check: a value without the trailing slash before the org segment, or with mixed case (Dev.Azure.com), produces vssps_base == trimmed_org and trips the "Legacy *.visualstudio.com organizations are not currently supported" error message — which is misleading for a dev.azure.com URL that simply didn't match the exact substring.

A more robust approach parses the authority and swaps the host rather than doing substring replacement, e.g.:

let vssps_base = if let Some(rest) = trimmed_org.strip_prefix("https://dev.azure.com") {
    format!("https://vssps.dev.azure.com{rest}")
} else { ... };

This isn't reachable with current construction paths (the URL is always /-suffixed with the org), so it's low severity today, but it's an easy correctness trap for future callers who build organization_url differently.

[github-actions]

Prompt evaluation

Note

This is an advisory static review. Only Prompt Contracts is merge-blocking.

Suites selected: create (prompts/create-ado-agentic-workflow.md changed), update (prompts/update-ado-agentic-workflow.md changed). debug and prompt-contract.md are byte-identical between base and head, so that suite was not evaluated.

Diff summary: Both changed prompts add the same new guidance block: how to choose among Azure DevOps PR safe-output tools (add-pull-request-comment vs submit-pull-request-review, comment vs reset voting semantics, update-pull-request vs update-pull-request-comment, push-to-pull-request-branch source-branch constraints, and target: "*" scoping). No other prompt text changed.

None of the 6 synthetic cases in the create/update suites exercise PR review/comment safe-output tooling — they cover a manual read-only workflow, a needs-clarification request, a scheduled work-item comment workflow, a body-only formatting edit, a PR trigger-filter mode change, and a work-item comment safe-output addition. The new guidance is additive and scoped to a part of each prompt that these cases never traverse, so base and candidate produce identical guidance for every case here.

Prompt	Cases	Improved	Unchanged	Regressed	Inconclusive
create/update/debug	6	0	6	0	0

Potential regressions

None found. The added PR-tool-selection guidance is net-additive (new deterministic intent boundaries for review vs. comment vs. vote/reset vs. push tools) and was not observed to remove, weaken, or contradict any existing instruction relevant to the evaluated cases.

Per-case scores

Case	Prompt	Base	Candidate	Result
create-minimal-manual (common criteria)	create	2/2/2/2	2/2/2/2	unchanged
create-minimal-manual (create criteria)	create	2/2/2/2	2/2/2/2	unchanged
create-needs-clarification (common criteria)	create	2/2/2/2	2/2/2/2	unchanged
create-needs-clarification (create criteria, n/a: no workflow invented)	create	2/2/2/2	2/2/2/2	unchanged
create-scheduled-workitem-report (common criteria)	create	1/2/2/2	1/2/2/2	unchanged
create-scheduled-workitem-report (create criteria)	create	2/1/1/2	2/1/1/2	unchanged
update-body-only (common criteria)	update	2/2/2/2	2/2/2/2	unchanged
update-body-only (update criteria)	update	2/2/2/2	2/2/2/2	unchanged
update-pr-filter-mode (common criteria)	update	2/2/2/2	2/2/2/2	unchanged
update-pr-filter-mode (update criteria)	update	2/2/2/2	2/2/2/2	unchanged
update-safe-output (common criteria)	update	1/2/2/2	1/2/2/2	unchanged
update-safe-output (update criteria)	update	2/2/1/2	2/2/1/2	unchanged

Evidence for the recurring 1 scores (identical in base and candidate, unaffected by this PR's diff):

• create-scheduled-workitem-report / task_completion & create_trigger_scope & create_tools_outputs_permissions: the create prompt's References list omits docs/schedule-syntax.md and gives no inline comment-on-work-item example, so fuzzy-schedule and work-item safe-output shaping rely on an external doc link rather than inline guidance.
• update-safe-output / task_completion & update_privilege_discipline: the update prompt's Validate checklist only generically says "no accidental privilege expansion" — it does not explicitly instruct withholding direct MCP write-tool allow-list entries the way the case's reference.md does, so correct behavior again depends on external reference material rather than the prompt itself.

These gaps are pre-existing and orthogonal to this PR's change (new PR comment/review tool-selection guidance); recommend tracking separately rather than blocking this PR.

Generated by Prompt Evaluator for #2222 · auto · 54.8 AIC · ⌖ 3.11 AIC · ⊞ 9.8K · ◷