feat(safe-outputs): support cross-organization repository writes

[jamesadevine]

Summary

• add expanded permissions.write with connection-type: azureRM | azureDevOps and deny-by-default cross-org repository scopes
• migrate compiler-generated ADO token acquisition to typed AzureCLI@3
• support cross-organization create-pull-request, create-branch, and create-git-tag through a shared target resolver
• isolate cross-org patch-base credentials to trusted preparation steps with exact remote identity matching
• add readiness diagnostics, Bearer authentication, documentation, and deterministic E2E scenarios

Validation

• cargo test --all-targets — 3,298 tests passed
• cargo clippy --all-targets -- -D warnings
• full scripts/ado-script test suite and TypeScript typecheck
• generated-shell registry tests
• final code review: GO

Live cross-org E2E infrastructure

The branch includes optional deterministic cross-org branch/tag/PR scenarios. They currently skip because executor E2E definition 2550 has no EXECUTOR_E2E_CROSS_ORG_* variables and AgentPlayground exposes no Azure DevOps WIF service connection to this session. Enabling the canonical lane requires separate administrative provisioning of the connection, target organization identity, repository ACLs, variables, and token-mint step.

Addresses #1934 follow-up after #1935.

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

[jamesadevine]

/review

[github-actions]

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

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Test Quality Review — PR #2087

Overall the test suite for this cross-org write-permissions feature is strong: resolve_repository_write_target/canonical_repository_alias in src/safe_outputs/mod.rs has a good table-driven matrix (self, same-org, allowed cross-org, alias-vs-name collision, ambiguous collision, and several rejection paths), WritePermissionOptions/validate_ado_scope_tree in src/compile/types.rs covers duplicate-organization/project/repository detection and unknown connection types, and the new AzureCliV3 builder + create_branch.rs/create_git_tag.rs dry-run tests exercise both the happy and rejected paths end to end, including the compiled-YAML-level assertions in tests/compiler_tests.rs.

Three gaps worth addressing (inline comments below):

1. authenticate_ado_request's test only covers AzureDevOps and AzureRm connection types, not the default None case that most existing (non-permissions.write) pipelines take through this exact function.
2. The refactor from reject_cross_organization_repository (with its own dedicated tests) to the shared resolve_repository_write_target dropped the create_pull_request.rs-specific coverage for the plain "not in allow-list" and "trailing-name match" cases — those are now only proven generically in mod.rs, not through this call site's own execute_sanitized wiring.
3. The new cross-org scenario definitions (createCrossOrgBranch/createCrossOrgGitTag in scenarios/cross-org.ts) have no unit tests of their own logic — cross-org-scenarios.test.ts only covers the env-resolution helper, leaving the scenario assertions exercised solely by the currently-skipped live E2E lane.

None of these are merge-blocking; they're coverage gaps rather than incorrect or weakened assertions.

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

[github-actions]

Security review — no regressions found

Reviewed the cross-organization repository-write feature end to end against the two trust boundaries this repo cares about: (1) the Stage 1 Agent sandbox never receiving a write-capable token, and (2) cross-org writes being deny-by-default with independent compile-time and runtime enforcement.

Findings: none blocking. The design is sound and consistently applied:

• resolve_repository_write_target() in src/safe_outputs/mod.rs is the single runtime gate for every ADO write (create_branch, create_git_tag, create_pull_request). It fails closed on every ambiguous case checked: endpoint-without-organization, endpoint-matching-current-org, and cross-org targets missing from ctx.write_allowed_repositories. This re-verification is independent of the compiler's own gating in agentic_pipeline.rs::create_pr_prepare_repos (which only ever passes org/project/repository through when WritePermissionConfig::allows_repository() is true), so a compiler bug alone can't silently grant a cross-org write.
• authenticate_ado_request() is now the single chokepoint for auth-header selection (bearer vs PAT) across all three ADO-writing tools, replacing several call sites that previously built .basic_auth() calls independently — reduces the chance of one call site missing the cross-org bearer-token path.
• scripts/ado-script/src/prepare-pr-base/index.ts cross-checks the compiler-resolved cross-org target against the actual checked-out git remote identity and refuses to proceed on any mismatch, including the case where a "cross-org" target actually resolves to the current org (privilege/scope confusion). Good defense-in-depth against a compiler/runtime disagreement.
• Token isolation is preserved: SC_WRITE_TOKEN is only minted in SafeOutputs/Conclusion (Stage 3) job builders, never the Agent job; the Agent's new cross-org AzureCLI@3 prepare step mints its own separately-scoped token for patch-base diffing only, and uses visibleAzLogin: false to keep it out of logs.
• Path/identifier fields feeding these new code paths (project, repository, organization) are validated through AdoProject/AdoRepository/AdoOrganization newtypes at parse time (src/secure.rs), including the new cross-organization name: project/repository splitting in validate_repo_organization. URL-building helpers (repository_api_base, identity_picker_url) percent-encode path segments via the widened PATH_SEGMENT allowlist (now also escaping %), so crafted project/repo names can't break out of the path segment.

I did not find any new injection path into generated bash:/##vso[...] content, weakened validation, path traversal, or token leakage introduced by this diff. Comment-only, no blocking issues.

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

[github-actions]

Reviewed the cross-organization repository-writes changes for compiler-contract concerns (front-matter grammar, typed IR, safe-output targets, codegen/doc drift, generated shell).

What I checked

• permissions.write grammar migration (String -> WritePermissionConfig) is backward compatible (scalar form still parses, #[serde(deny_unknown_fields)] on the expanded form) and every new identifier (AdoOrganization, AdoProject, AdoRepository) uses the validated src/secure.rs newtypes rather than raw String — matches the safe-output security contract.
• resolve_repository_write_target in src/safe_outputs/mod.rs is a fail-closed authorization chokepoint: endpoint-backed checkout without organization: is rejected, an organization: matching the current org is rejected, and cross-org targets are matched case-insensitively against permissions.write.allow before any REST call. It is wired from both src/main.rs call sites (Stage 3 + Conclusion), not just tests.
• New PREPARE_PR_BASE_AZURE_DEVOPS shell script and AzureCliV3 task builder both go through the typed ShellScript/Binding machinery — no format!-built bodies, no bearer landing anywhere but a shell-local ADO_TOKEN var (never SC_WRITE_TOKEN/task.setvariable in that path). prepare-pr-base/index.ts adds fail-closed remote-identity matching (explicitIdentity vs actual git remote) before ever using fetched credentials for a cross-org target.
• Deleted checkout_cross_organization_repo_aliases/reject_cross_organization_repository are superseded by the more general resolve_repository_write_target path — confirmed no dangling caller remains.
• Docs (docs/front-matter.md, docs/safe-outputs.md, docs/network.md, docs/ado-script.md, site/.../ir.mdx) and AGENTS.md were all updated in step with the code; no codegen (types.gen.ts, fact-catalog.gen.json) or .lock.yml drift — none of the changed files touch the gate/fact IR or workflow sources.

No compiler-contract violations found. This is thorough, well-tested (new case-insensitive-duplicate / missing-repositories / remote-mismatch tests), and internally consistent across the Rust/TypeScript boundary (ADO_AW_ACCESS_TOKEN_KIND contract, repository_write_scope_key normalization). Nothing to block on.

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

[github-actions]

Reviewed the scripts/ado-script/ TypeScript changes for cross-org support (bearer-token auth, cross-org executor scenarios). Note: the ts-critic sub-agent returned no usable output across two turns/attempts despite an explicit re-prompt, so this review reflects only my own pass.

Overall the change is solid: the git-auth-via-env (not argv) pattern, JSON-stringified YAML front-matter, oneLine() log sanitization, and path-traversal guard in execute-cli.ts are all deliberate, well-documented security patterns — no new unhandled rejections, any leakage, or missing test coverage found for the new branches (bearer auth, cross-org scenarios, source/writePermissions rendering are all covered by new tests).

Two advisory findings posted inline:

1. auth.ts — the WebApi cache key doesn't include the token, so a refreshed short-lived bearer token could be masked by a stale cached client for the same org URL.
2. cross-org.ts — non-null assertions after a grouped validation check could be replaced with a narrowing guard to remove reliance on assertion-by-convention.

Neither is merge-blocking for this PR.

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

[github-actions]

Rust code quality review — no merge-blocking defects found

Reviewed the Rust-only portion of this diff (src/compile/, src/safe_outputs/, src/main.rs — ~4600 lines). The cross-org ADO write support is well-tested (unit tests accompany essentially every new function) and follows a deliberate deny-by-default policy for cross-organization writes, with the bearer token kept shell-local rather than leaked via pipeline variables. I found one maintainability nit, posted inline, and no defects rising to REQUEST_CHANGES.

Note: the rust-critic sub-agent did not return output after repeated blocking waits (~6 minutes, 2 turns, no response); its input was discarded per the fallback contract, and this review relies solely on my own manual pass.

Themes considered but not flagged (verified as intentional/safe)

• canonical_repository_alias's new fail-closed behavior on ambiguous alias/self-name matches — a deliberate hardening, documented and tested.
• create_pull_request.rs dry-run check reordering — still gated by ctx.dry_run internally, covered by dedicated tests.
• No HashMap iteration non-determinism in new code — write_allowed_repositories is a HashSet used only via .contains().
• Cross-org PR-prepare partitioning logic in agentic_pipeline.rs correctly splits by organization presence and gates the cross-org task on connection-type: azureDevOps.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 252.6 AIC · ⌖ 2.03 AIC · ⊞ 11.5K
_{Comment /review to run again}

[jamesadevine]

/review

[github-actions]

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

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

Test quality review

Reviewed the test diffs for the cross-organization repository write feature (src/safe_outputs/{create_branch,create_git_tag,create_pull_request,mod}.rs, scripts/ado-script/src/{executor-e2e,prepare-pr-base,shared}).

Overall this is strong test coverage for a security-sensitive feature:

• resolve_repository_write_target / configure_repository_write_context in src/safe_outputs/mod.rs have dedicated unit tests for the happy path (self, same-org checkout, allowed cross-org) and every rejection path: missing repos.organization, wrong connection-type, repository not in permissions.write.allow, endpoint pointing at the current org, and name-collision ambiguity between self and an alias.
• create_branch.rs / create_git_tag.rs / create_pull_request.rs each add execute_sanitized dry-run tests that exercise the new resolver end-to-end, including negative cases (branch pattern rejection, unauthorized cross-org endpoint, unknown repository selector, label-policy rejection).
• prepare-pr-base/index.ts tests cover the explicit --organization/--project/--repository matching against the git remote, including a same-organization rejection, a mismatched-remote rejection, and a locale/code-point-distinct name rejection — good adversarial coverage for identity spoofing.
• auth.test.ts covers both bearer (Entra) and PAT credential handler selection, plus cache invalidation on token rotation.
• No existing assertions were weakened or removed; the diff only replaces obsolete cross-org-rejection tests with tests for the new resolver, consistent with the behavior change.
• test_cross_org_create_pull_request_preparation_is_credential_isolated in tests/compiler_tests.rs gives full-pipeline coverage of the credential-isolation contract, which is the highest-risk part of this change.

No blocking findings. A couple of very minor observations, not worth blocking on:

• The ambiguity test repository_name_collision_between_self_and_alias_is_ambiguous only asserts canonical_repository_alias returns None; there is no test asserting the actual user-facing error message text produced by resolve_repository_write_target for this specific collision case (as opposed to the parametrized cases in repository_write_target_rejects_incomplete_or_unauthorized_cross_org).
• scripts/ado-script/src/executor-e2e/scenarios/cross-org.ts scenarios are only unit-tested with mocked AdoRest; the real cross-org REST behavior remains exercised solely by the (currently skipped, per the PR description) live E2E lane — acceptable given the documented provisioning limitation.

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

[github-actions]

Compiler contract review — clean

Reviewed the cross-organization repository-write feature end-to-end against the compiler contracts this reviewer owns:

• Front-matter grammar (permissions.write, repos.organization): backward compatible — WritePermissionConfig is #[serde(untagged)] over the old scalar String and a new expanded object, so every existing write: <sc-name> workflow still parses; no codemod needed, and it is documented in docs/front-matter.md and docs/network.md with matching site/src/content/docs/...mdx updates.
• Validation wiring: validate_permissions_write_policy is called from agentic_pipeline.rs::validate_pipeline_front_matter; validate_repo_organization is called from lower_repos; deny-by-default scope validation (duplicate org/project/repo, empty allow lists) is exercised by targeted unit tests.
• Typed IR: AzureCliV3/AzureCliV3Connection correctly make a mixed azureSubscription/azureDevOpsServiceConnection task unrepresentable, and are covered by both unit tests and tests/compiler_tests.rs compiled-output assertions (AzureCLI@3, connectionType, absence of addSpnToEnvironment).
• Generated shell: the new token-mint and prepare-pr-base cross-org bodies go through shell_script!/ShellScript, not raw format!; bearer stays shell-local per cross_org_prepare_task_keeps_bearer_shell_local.
• Safe-output tools: create-branch/create-git-tag/create-pull-request now share resolve_repository_write_target, and drop the old reject_cross_organization_repository special case in favor of a general resolver — reduces duplication and is well covered by dry-run tests.
• Docs sync: docs/safe-outputs.md, docs/front-matter.md, docs/network.md, docs/safe-output-permissions.md, and their site/ mirrors all update together; AGENTS.md prose for prepare-pr-base/ was updated to reflect the new isolated-credential behavior.
• No codegen/lock/bundle drift: no changes touch filter_ir.rs/Fact, .github/workflows/*.md, or tests/safe-outputs/.

No merge-blocking findings from a compiler-contract perspective. Nice work keeping the scalar/expanded permissions.write forms compatible and pairing every schema change with docs.

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

[github-actions]

Security review of this diff — no exploitable regressions found.

This PR adds cross-organization Azure Repos write support via an expanded permissions.write (connection-type: azureRM|azureDevOps + deny-by-default allow scope tree) and migrates ADO token minting to typed AzureCLI@3. I traced the full write path end to end:

• Deny-by-default enforcement is consistent across all three writers. resolve_repository_write_target (src/safe_outputs/mod.rs) is the single chokepoint: it requires connection-type: azureDevOps for any cross-org target and checks the resolved organization/project/repository against write_allowed_repositories before returning a target. create_pull_request.rs, create_branch.rs, and create_git_tag.rs all route through it — none bypass the check.
• Cross-org identity now correctly follows the resolved target, not the pipeline's own org: add_reviewers_to_pr/identity_picker_url use ctx.target.organization, and every REST call is built from repository_api_base(&target) rather than the old fixed org_url/project.
• Token dispatch (bearer vs PAT/basic) is correctly threaded via ctx.write_connection_type into authenticate_ado_request, matching the connection type actually used to mint the token.
• prepare-pr-base.ts's cross-org path requires exact remote-identity matching (identityMatchesExplicit) before projecting the fetch token into a repo directory, and fails closed if the checked-out remote doesn't match the compiler-resolved target or if it unexpectedly points at the current org — good defense against directory/target confusion.
• Token minting now uses typed AzureCLI@3 builder with visible_az_login(false) and a masked ##vso[task.setvariable ...;issecret=true] publish; no bearer ever appears in argv or logs I could find.
• Reviewer IDs embedded in REST URLs are either regex-validated GUIDs (is_reviewer_guid) or come from ADO's own identity-picker response, so no injection surface there.

Nothing here weakens an existing control — the compile-time advisories (repository_write_readiness_warnings) are a net-new safety improvement, and the old cross-org-reject logic in create_pull_request.rs was replaced by the new allow-scoped resolver rather than simply removed.

This diff is security-neutral-to-positive; no blocking findings.

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

[github-actions]

Reviewed the scripts/ado-script/ portion of this PR (cross-org executor-e2e scenarios, prepare-pr-base explicit-coordinate matching, and shared/auth.ts bearer-vs-PAT caching). The ts-critic sub-agent finished but returned no output/findings after the blocking wait, so this review reflects only my own pass.

Overall the change is well-structured: authedFetch/request in ado-rest.ts already have per-request timeouts, withRetry is reused for the modified getCommitDiffMetadata call, the getWebApi cache correctly keys on token-kind + org + token value (so rotation invalidates), and the new explicit-identity matching in prepare-pr-base/index.ts fails closed (returns false/warns) rather than silently falling back to an untrusted remote. Test coverage for the new cross-org matching branches (explicit-match, mismatch, same-org rejection, locale-equivalent repo name rejection) is thorough.

Two minor items posted inline — neither blocking:

1. The new multi-repo failure aggregation (requiredTargetFailed) has no test with more than one repos entry.
2. Couldn't visually confirm two `Bearer ...` template literals render correctly in this session's file view (likely a redaction artifact of this review pipeline, not a real bug — flagged for awareness only).

No merge-blocking findings.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 117 AIC · ⌖ 2.72 AIC · ⊞ 11.6K
_{Comment /review to run again}

[github-actions]

Rust engineering review — no blocking issues found

Reviewed the Rust changes (src/safe_outputs/{mod,create_branch,create_git_tag,create_pull_request}.rs, src/compile/{common,types,agentic_pipeline}.rs, src/compile/ir/tasks/azure_cli.rs, src/secure.rs usage) for the new cross-organization write support.

Summary of what I checked:

• resolve_repository_write_target is a single well-designed chokepoint consumed identically by all three write tools (create-branch, create-git-tag, create-pull-request), each now sharing execute_sanitized/dry-run/target-resolution logic — no divergence found.
• Cross-org policy checks (connection-type must be azureDevOps, explicit permissions.write.allow membership) are enforced before any network call, with clear, actionable error messages.
• repository_write_scope_key lowercases consistently at both insertion (write_allowed_repositories) and lookup time — no case-sensitivity mismatch.
• authenticate_ado_request correctly branches Bearer vs Basic auth based on WriteConnectionType, threaded through every REST call site (push, refs, PR create/update, reviewer add, identity lookup).
• Organization/project/repo values used in URL construction are either compiler-validated (AdoOrganization newtype forbids /, controls charset) or percent-encoded (utf8_percent_encode with PATH_SEGMENT), so no injection into ADO REST URLs.
• No unwrap()/expect() reachable from agent-controlled input in the reviewed diff; the one panic! in acquire_ado_token_step guards an internal compiler-only invariant (only ever called with two hardcoded literal variable names), not user input.
• AzureCliV3/AzureCliV3Connection builder makes the ARM-vs-DevOps connection distinction a type-level enum, avoiding a mixed/invalid-state bug class.

I ran a background rust-critic sub-agent in parallel on the same diff; it returned no retrievable findings after two attempts (empty response both times), so I'm relying entirely on my own manual pass — noting this per the review contract.

No inline comments posted; nothing rose to a postable, diff-scoped defect. Nice, careful refactor.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 207.4 AIC · ⌖ 0.625 AIC · ⊞ 11.5K
_{Comment /review to run again}

[github-actions]

Reviewed the test changes for the cross-org repository writes feature (executor-e2e cross-org scenarios, prepare-pr-base cross-org coordinate matching, auth.ts bearer-vs-PAT caching).

Overall the new test coverage is strong: the security-critical identity-matching logic in prepare-pr-base/index.ts (explicit-coordinates-vs-remote mismatch, current-org rejection, locale-distinct repo names) is well exercised with both positive and negative cases, and auth.test.ts correctly covers bearer/PAT handler selection plus cache-key rotation on token change. Found three gaps, none blocking:

1. createCrossOrgBranch lacks the failed-verification test that its tag sibling createCrossOrgGitTag has — asymmetric coverage of the same assert contract.
2. A pre-existing REST-forbidden-fallback test now captures rc (added by this PR) but never asserts it, missing a chance to pin the non-fatal return code for that path.
3. The locale-distinct-repository-name test only covers a literal-byte mismatch, not a case where naive .toLowerCase() comparison in sameAdoName could produce a false positive across genuinely distinct identifiers.

These are advisory — the diff doesn't weaken or remove any existing assertion, and all new behavior has at least baseline coverage.

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

[github-actions]

Rust Code Quality Review — no merge-blocking issues found

Reviewed the Rust-only portion of this diff (compiler config/types, the AzureCliV3 typed-IR migration, and the cross-org repository-write resolution/auth path in safe_outputs/). The cross-org credential isolation is well-designed and well-tested (test_cross_org_create_pull_request_preparation_is_credential_isolated explicitly proves the Agent-side bearer never touches SC_WRITE_TOKEN), error handling is consistent with anyhow, new config validation (validate_ado_scope_tree) has solid duplicate/case-insensitivity/empty-scope test coverage, and I found no new unwrap()/expect()/panics reachable from user input, no lossy casts, and no non-deterministic iteration affecting generated output.

One minor maintainability note posted inline (stringly-typed dispatch with a panic! fallback in acquire_ado_token_step — not reachable today, but worth hardening against future misuse).

Note on rust-critic sub-agent

The rust-critic sub-agent was launched in the background but returned empty output on two attempts (including an explicit follow-up prompt), so its findings were discarded; this review reflects my own manual pass only.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 117.9 AIC · ⌖ 2.23 AIC · ⊞ 11.5K
_{Comment /review to run again}

[jamesadevine]

/review

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[jamesadevine]

/azp run ado-aw candidate compiler smoke

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[github-actions]

TypeScript review — scripts/ado-script/ (cross-org executor E2E + prepare-pr-base)

Reviewed the non-generated changes in ado-rest.ts, execute-cli.ts, runner.ts, scenario.ts, scenarios/create-pull-request.ts, scenarios/cross-org.ts, prepare-pr-base/index.ts, shared/ado-client.ts, and shared/auth.ts.

No merge-blocking defects found:

• Auth/token handling: resolveCrossOrgEnv validates all five required env vars before building CrossOrgEnv, throws SkipError (not a hard failure) when infrastructure isn't provisioned, and never logs the token itself. Bearer/basic auth header construction in AdoRest and create-pull-request.ts is inline, not persisted or logged.
• Error handling: preparePatchBase/prepareTargetWorktree correctly distinguish "explicit target resolves to current org" vs. "checkout remote mismatch" failure modes, both surfaced via warnRepo (uses ##vso[task.logissue], not a raw throw), and both paths are covered by dedicated tests in prepare-pr-base/__tests__/index.test.ts (lines ~300+).
• REST client: getCommitDiffMetadata validates SHA/branch-commit shape (requireSha) and count bounds (requireCount) before trusting REST responses; AdoRest.request guards non-JSON bodies instead of blindly casting.
• Cross-org scenario tests: cross-org-scenarios.test.ts covers both the missing-env skip path and the happy-path source/permissions shape.
• flushPending's all-or-none --organization/--project/--repository validation is correct and exercised by both the "complete" and "rejects partial" test cases.

Note: my ts-critic sub-agent invocation returned empty output on two attempts, so this review reflects my own pass only — flagging per the review contract rather than silently omitting it.

No comments to post inline; nothing here blocks merge.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 105.2 AIC · ⌖ 2.04 AIC · ⊞ 11.6K
_{Comment /review to run again}

[github-actions]

Security review: no regressions found

Reviewed this diff (cross-org repository writes for create-pull-request/create-branch/create-git-tag, expanded permissions.write, AzureCLI@3 token minting) against the threat model: injection into generated pipelines, weakened validation, path traversal, token scope/projection, network boundary, safe-output integrity, prompt injection.

Findings: none that weaken the security posture relative to main.

• Cross-organization repository targets are deny-by-default: permissions.write.allow requires an explicit organization/project/repository triple (validate_ado_scope_tree in src/compile/types.rs), and resolve_repository_write_target (src/safe_outputs/mod.rs) rejects any cross-org target not in that allow-list, rejects a cross-org endpoint that resolves to the current org, and requires connection-type: azureDevOps before any cross-org write is attempted.
• authenticate_ado_request correctly switches between bearer_auth (Entra/OAuth tokens from AzureCLI@3 azureDevOps connections) and basic_auth (PAT-style System.AccessToken/ARM tokens) based on connection_type, applied consistently across create-branch, create-git-tag, and create-pull-request REST calls.
• All organization/project/repository values reaching interpolated URLs (repository_api_base, identity_picker_url) are validated newtypes (AdoOrganization, AdoProject, AdoRepository from src/secure.rs), which reject slashes and other structurally dangerous characters, so no path/URL injection.
• prepare-pr-base.ts's cross-org fetch path requires the checkout origin remote to exactly match the compiler-resolved organization/project/repository (case-insensitively) before honoring the write-scoped fetch token, and refuses when the resolved target actually points at the current org (preventing scope confusion between organizations).
• Read- and write-permission scope validation now share one hardened tree validator (validate_ado_scope_tree) that also rejects duplicate organizations/projects/repositories (case-insensitively) — tightening, not loosening, prior read-policy validation.
• No new domains added to the AWF network allowlist; docs/network.md changes are documentation only.
• Stage boundary preserved: token minting/write execution remains confined to Stage 3 (SafeOutputs/Conclusion); the Agent (Stage 1) continues to receive only the read-scoped token via acquire_ado_token_step.

Advisory-only; no merge-blocking issues found.

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

[github-actions]

Rust Code Quality Review 🦀

Reviewed the Rust changes in src/compile/{ado_bundle,agentic_pipeline,common,custom_tools,mod,types}.rs, src/compile/ir/tasks/azure_cli.rs, src/compile/extensions/ado_script.rs, src/main.rs, src/safe_outputs/{create_branch,create_git_tag,create_pull_request,mod,result,upload_build_attachment}.rs, and tests/compiler_tests.rs.

The rust-critic sub-agent completed but returned no parseable findings, so I relied on my own pass.

Verdict: no merge-blocking defects found.

💡 Notes

• resolve_repository_write_target / configure_repository_write_context (src/safe_outputs/mod.rs) cleanly consolidate same-org and cross-org repository resolution with a deny-by-default posture (explicit permissions.write.allow scope required, connection-type: azureDevOps required for cross-org), and are well covered by unit tests including the ambiguous-name-collision case.
• authenticate_ado_request correctly switches between basic_auth("", token) (ARM-minted PAT-style) and bearer_auth(token) (Azure DevOps connection-type), consistently threaded through every REST call site in create_pull_request.rs/create_branch.rs.
• All new unwrap()/expect() calls found in the diff are confined to #[test] blocks — none reachable from user/agent input.
• AzureCliV3/AzureCliV3Connection (azure_cli.rs) correctly makes a mixed azureSubscription/azureDevOpsServiceConnection task unrepresentable via the enum, with tests confirming only the relevant input is set per variant.
• Public helper methods added to WritePermissionConfig (allows_repository, supports_cross_organization_writes, etc.) are all exercised by call sites in common.rs/agentic_pipeline.rs — no dead code.
• No lossy casts, no blocking calls in async paths, no non-deterministic HashMap iteration affecting generated YAML output found in the changed lines.

Everything ado-aw-specific (front-matter schema, safe-output contracts, generated YAML shape, bundle/codegen drift) is out of scope here and left to the Compiler Contract reviewer.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 180.5 AIC · ⌖ 1.95 AIC · ⊞ 11.5K
_{Comment /review to run again}

[github-actions]

Reviewed test coverage for this cross-org write-permissions PR. Overall the new behavior is well covered: resolve_repository_write_target/configure_repository_write_context have thorough unit tests for same-org, allowed cross-org, unauthorized cross-org, and ambiguous-alias cases (src/safe_outputs/mod.rs); create_branch.rs/create_git_tag.rs/create_pull_request.rs each add dry-run tests for the cross-org success and rejection paths; the TypeScript cross-org.ts scenario has a dedicated, well-structured test file exercising env resolution, source/config/ndjson/assert/cleanup and both branch/tag failure paths; auth.ts's bearer-vs-PAT caching is directly tested; prepare-pr-base gets extensive new tests for cross-org remote matching, mismatch rejection, and Unicode/locale repository-name spoofing.

One gap: the new execute-cli.test.ts case only asserts fragments of the permissions: block and never checks that the corresponding repos: entry (name/alias/organization/endpoint) is actually emitted, even though renderSourceMarkdown gained a new branch specifically for source.repositories. Left an inline comment with a suggested addition.

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

[github-actions]

Compiler contract review — no findings

Reviewed this expansion of permissions.write (scalar to typed WritePermissionConfig/WriteConnectionType), the new cross-organization repository-write path (repos.organization, resolve_repository_write_target, AdoRepositoryTarget), and the AzureCLI@3 migration.

Contract checks:

• Front matter: new write object form and Repository.organization are additive (Option<T>/#[serde(default)]); no breaking change to existing .md sources, so no codemod is required. docs/front-matter.md, docs/safe-outputs.md, docs/network.md, and the site mirrors were all updated in step.
• Deleted helper: FrontMatter::checkout_cross_organization_repo_aliases was removed along with both of its unit tests and its only caller in src/main.rs; confirmed no dangling references remain in the tree.
• Renamed types: AdoReadOrganizationScope/AdoReadProjectScope -> AdoOrganizationScope/AdoProjectScope keep a pub type AdoReadOrganizationScope = AdoOrganizationScope alias, so this is not a breaking rename for any external consumer of the type name.
• Identifier fields: the new organization/project/repository fields on AdoOrganizationScope/AdoProjectScope/WritePermissionOptions all use the existing validated src/secure.rs newtypes (AdoOrganization, AdoProject, AdoRepository, ServiceConnection) rather than raw String, consistent with the security contract for identifier fields.
• Generated shell: the new PREPARE_PR_BASE_AZURE_DEVOPS script and prepare_pr_base_azure_devops_step_typed go through ShellScript/Bindings as required; the minted bearer token stays a shell-local variable, never landing in EnvValue/committed YAML, and a test asserts SYSTEM_ACCESSTOKEN is not present on the task env.
• AGENTS.md / docs sync: the architecture-tree entry for prepare-pr-base/ and the ado-proxy-design.md/ado-script.md references to generate_acquire_ado_token/AzureCLI@2 were updated to the new acquire_ado_token_step/AzureCLI@3 names.
• Codegen/lock drift: no changes touched filter_ir.rs/Fact, and no .github/workflows/*.md changed, so the gate/fact codegen and compiled-workflow-lock invariants do not apply here.

Ran cargo build and the targeted compile::/safe_outputs:: test suites (2393 passed) against the branch; all green.

No merge-blocking issues found.

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

[jamesadevine]

/review

[github-actions]

✅ PR Security Reviewer completed the security review.

🔒 Security review by PR Security Reviewer

[github-actions]

Compiler contract review

Reviewed for the invariants this repo does not enforce at the type level: front-matter grammar compatibility, the permissions.write typed IR migration (String → WritePermissionConfig), the AzureCLI@2 → AzureCLI@3 token-acquisition migration, the read/write AdoOrganizationScope/AdoProjectScope type merge, and doc/AGENTS.md sync.

No blocking contract violations found. Specifically verified:

• permissions.write migrated from Option<String> to Option<WritePermissionConfig> (untagged enum). The scalar shorthand still deserializes via the ServiceConnection variant, so existing write: my-sc front matter keeps parsing — no codemod is required for this change.
• The read-only AdoReadOrganizationScope/AdoReadProjectScope types were renamed to the shared AdoOrganizationScope/AdoProjectScope and re-exported as type aliases (pub type AdoReadOrganizationScope = AdoOrganizationScope;), so no call site outside types.rs needed updates (confirmed no residual references in src/compile/ado_proxy/).
• New organization: field on Repository/RepoEntry is Option<...> with #[serde(default)], and cross-field validation (validate_repo_organization in common.rs) rejects organization without endpoint and non-git/self misuse — the invariant is enforced, not just documented.
• docs/front-matter.md, docs/network.md, docs/safe-outputs.md, docs/ado-proxy-design.md, and AGENTS.md (prepare-pr-base bundle description) were all updated in step with the code change removing the previous cross-org limitation notice.
• New AzureCliV3 typed builder (src/compile/ir/tasks/azure_cli.rs) is documented in site/src/content/docs/reference/ir.mdx's builder table alongside the existing AzureCli (AzureCLI@2) entry.
• resolved_execution_config_json (custom_tools.rs) was extended with organization and writePermissions fields consumed by main.rs's ResolvedExecutionConfig/ResolvedWritePermissions, keeping the compile-time and custom-job runtime paths in sync.
• Generated shell for the new PREPARE_PR_BASE_AZURE_DEVOPS and ACQUIRE_ADO_READ_TOKEN/ACQUIRE_ADO_WRITE_TOKEN scripts goes through shell_script!/ShellScript with typed bindings, not format!; the short-lived cross-org bearer stays a shell-local ADO_TOKEN variable passed only via env into the bundle child, never written to a pipeline variable.

Nothing rose to the level of a codegen/lock/bundle drift finding. Good adherence to the typed-IR and generated-shell contracts for a change of this size (49 files).

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

[github-actions]

Security review — no blocking findings

Reviewed the cross-organization Azure DevOps write path added in this PR (permissions.write expansion, AzureCliV3/azureDevOps connection type, resolve_repository_write_target, authenticate_ado_request, and the prepare-pr-base TypeScript identity checks).

The design is security-conscious and consistent with the existing trust model:

• Deny-by-default cross-org scope: resolve_repository_write_target requires connection-type: azureDevOps and an explicit permissions.write.allow entry before treating any repository as cross-organization, with a compile-time same-org guard (config.organization must not equal the pipeline's own org) and a fail-closed same-org-target rejection.
• prepare-pr-base.ts verifies the actual checkout remote identity (parseAdoRepoUrl + sameAdoName) against the compiler-resolved --organization/--project/--repository flags before allowing a fetch with the bearer token — preventing a mismatched worktree from silently receiving cross-org credentials.
• authenticate_ado_request correctly switches between bearer_auth (Entra/OAuth via azureDevOps connection) and basic_auth (PAT/ARM-minted) based on connection_type, verified by unit tests including the default-None fallback.
• Token scope: the new AzureCLI@3-based token acquisition/prepare-pr-base-azure-devops step only reaches steps that make REST calls (Stage 1 patch-base prep and Stage 3 executor), consistent with ado_bundle.rs's chokepoint model; no write-capable token appears to leak into a non-REST step.
• Identifier validation: organization/project/repository values flow through the validated AdoOrganization/AdoProject/AdoRepository newtypes (src/secure.rs) before being shell-quoted with sh_single_quote, so shell injection via front-matter values is not possible.
• PATH_SEGMENT fix: adding % to the percent-encoded set (src/safe_outputs/mod.rs) closes a double-encoding gap and is a net hardening, confirmed by the new Project%2FArchive test.

Nothing in this diff weakens an existing security control relative to main. The removed reject_cross_organization_repository/advisory-only warning is superseded by the more complete resolve_repository_write_target runtime check plus repository_write_readiness_warnings compile-time diagnostics, so the net effect is stricter, not looser.

No merge-blocking findings. Nice work threading the new cross-org write path through the existing token/scope model without widening the sandbox's trust boundary.

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

[github-actions]

TypeScript review (scripts/ado-script/ only)

Reviewed the cross-org additions: executor-e2e/ado-rest.ts, execute-cli.ts, runner.ts, scenario.ts, scenarios/create-pull-request.ts, scenarios/cross-org.ts (new), scenarios/index.ts, prepare-pr-base/index.ts, shared/ado-client.ts, shared/auth.ts.

No merge-blocking defects found:

• Bearer/PAT auth-header construction is confined to typed authKind branches; the token itself never reaches a log line or thrown Error (git auth flows through GIT_CONFIG_* env, not argv/logs, and the harness comment at create-pull-request.ts correctly documents this).
• shared/auth.ts's new token-kind cache keys correctly on ${tokenKind}:${orgUrl} and re-mints when the token value rotates, avoiding stale-credential reuse across bearer/PAT boundaries.
• prepare-pr-base/index.ts's new --organization/--project/--repository triad is validated as all-or-nothing in flushPending, and both preparePatchBase/prepareTargetWorktree refuse to send the pipeline's own-org fetch credential to a remote whose identity doesn't exactly match the compiler-resolved target (case-folded comparison, explicit early-return on org self-match) — this is the security-sensitive path and it's well guarded and unit-tested (prepare-pr-base/__tests__/index.test.ts covers partial coordinates, org self-pointing, and mismatched repo name).
• executor-e2e/** remains test-only (not shipped in ado-script.zip), so its error handling has appropriately lower stakes than the shipped prepare-pr-base/shared modules, and both are held to a good standard here.

Note: the ts-critic sub-agent I started for a second pass did not return usable output after a blocking wait and a follow-up nudge; I completed the review directly instead and did not find additional issues worth flagging.

No inline comments to add.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 125.9 AIC · ⌖ 2.01 AIC · ⊞ 11.6K
_{Comment /review to run again}

[github-actions]

Rust code-quality review — no merge-blocking issues found

Reviewed all 16 in-scope Rust files (src/compile/**, src/safe_outputs/**, src/main.rs, tests/compiler_tests.rs) covering the cross-org Azure DevOps write support: WritePermissionConfig/WriteConnectionType, resolve_repository_write_target deny-by-default enforcement, authenticate_ado_request bearer/basic dispatch, the new AzureCliV3 typed builder, and the create_branch.rs/create_git_tag.rs/create_pull_request.rs integration of these helpers.

rust-critic sub-agent returned no findings (advisory pass); its output added nothing beyond my own review.

💡 Notes (non-blocking, no action needed)

• resolve_repository_write_target correctly gates cross-org targets on connection_type == AzureDevOps and a case-insensitive org/project/repo allowlist before any network call; no bypass found. Backed by dedicated dry-run tests in all three executor files.
• canonical_repository_alias was tightened to reject ambiguous name matches (return None) instead of silently picking one — a genuine security hardening, well covered by repository_name_collision_between_self_and_alias_is_ambiguous.
• AzureCliV3Connection's AzureRm(String) | AzureDevOps(String) enum makes emitting both azureSubscription and azureDevOpsServiceConnection simultaneously a type error rather than a runtime bug.
• PATH_SEGMENT now also encodes %, closing a double-encoding surface (e.g. Project%2FArchive), and is exercised by path_segment_encodes_literal_percent_sequences.
• Dry-run ordering in create_branch.rs/create_git_tag.rs/create_pull_request.rs validates tool config, branch/tag pattern, and label policy before the dry-run short-circuit but defers the token fetch until after — correct, and each path has a matching test.
• Most remaining hunks are cargo fmt reformatting noise (collapsed multi-line calls/method chains); not flagged.

Nothing rose to a KEEP/HARDEN defect worth an inline comment.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 122 AIC · ⌖ 2.53 AIC · ⊞ 11.5K
_{Comment /review to run again}

[github-actions]

Test coverage on this PR is strong overall — resolve_repository_write_target has thorough table-driven tests for the cross-org authorization matrix (unauthorized, wrong connection-type, missing organization, endpoint-equals-current-org), authenticate_ado_request has a direct unit test for both auth schemes, AzureCliV3 has parity tests for both connection variants, and the TypeScript side (cross-org.ts, prepare-pr-base, auth.ts token-kind caching) all get dedicated new-behavior tests including several deliberately adversarial cases (locale-folding, Unicode identifier confusables). No weakened or removed assertions found — the two AzureCLI@2→AzureCLI@3 / reject_cross_organization_repository→resolve_repository_write_target test rewrites are consistent renames/refactors that track the corresponding production change and add rather than reduce assertions.

One small gap flagged inline: the "too many path segments" error arm of split_repository_target_name (new in this PR) has no direct test. Non-blocking.

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

[jamesadevine]

/review

[github-actions]

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

🦀 Rust code quality review by Rust Code Quality Reviewer

[github-actions]

This is a well-executed compiler-contract change — I checked the areas most likely to drift silently and found none:

• permissions.write scalar→object migration (WritePermissionConfig/WritePermissionOptions in src/compile/types.rs) keeps backward compat via #[serde(untagged)], has deny_unknown_fields, and validation (validate_ado_scope_tree) is wired through common::validate_permissions_write_policy at compile time — not just unit-tested in isolation.
• New repos.organization field is a validated AdoOrganization newtype (not a raw String), documented in docs/front-matter.md (front-matter table + prose), and cross-checked at runtime by resolve_repository_write_target (connection-type + allow scope enforcement) before any cross-org REST call.
• AzureCliV3/AzureCliV3Connection typestate builder in src/compile/ir/tasks/azure_cli.rs makes an inconsistent azureSubscription/azureDevOpsServiceConnection pairing unrepresentable, with tests for both connection types.
• The removed checkout_cross_organization_repo_aliases / reject_cross_organization_repository functions have no dangling references anywhere in src/ or docs/.
• Docs (docs/front-matter.md, docs/safe-outputs.md, docs/network.md, site mirrors) are all updated in step with the new connection-type/allow schema.
• pr-meta.json's files list shows no unexplained *.gen.ts/*.gen.json or .lock.yml drift, and the missing scripts/ado-script/*.js bundles are expected (gitignored build artifacts).

No merge-blocking findings from a compiler-contract perspective.

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

[github-actions]

Reviewed the scripts/ado-script/ TypeScript changes for cross-org repository writes. The ts-critic sub-agent produced no usable output (empty response after the blocking wait), so this review reflects only my own pass.

Overall the code is solid: resolveCrossOrgEnv validates all required env vars and throws SkipError cleanly, the bearer-vs-basic auth branching in AdoRest/getWebApi is well-contained, and prepare-pr-base/index.ts's explicit-identity matching (sameAdoName, identityMatchesExplicit) guards against cross-org/same-org confusion with clear warnings and fail-closed behavior. One caching concern flagged inline in auth.ts around mixing PAT and bearer auth kinds within a single process via a global env var rather than a per-call parameter — worth a second look, otherwise no merge-blocking issues found.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · auto · 52.5 AIC · ⌖ 1.49 AIC · ⊞ 11.6K
_{Comment /review to run again}

[github-actions]

Rust review — no merge-blocking issues found

Reviewed the Rust portions of this PR (src/compile/{ado_bundle,agentic_pipeline,common,custom_tools,types,mod}.rs, src/compile/ir/tasks/azure_cli.rs, src/compile/extensions/ado_script.rs, src/main.rs, src/safe_outputs/{create_branch,create_git_tag,create_pull_request,mod,result,upload_build_attachment}.rs, tests/compiler_tests.rs).

What I checked closely, given the security-sensitive nature of this change:

• resolve_repository_write_target (mod.rs): exact-scope enforcement via repository_write_scope_key, connection-type: azureDevOps gating for cross-org, and rejection of endpoint-declared-as-current-org misconfigurations — all correctly fail closed with actionable errors.
• authenticate_ado_request: correctly switches Bearer vs Basic auth based on WriteConnectionType, consistently threaded through create_pull_request.rs, create_branch.rs, create_git_tag.rs.
• canonical_repository_alias: name-collision ambiguity between self and an alias is now explicitly rejected rather than silently preferring one (verified by repository_name_collision_between_self_and_alias_is_ambiguous).
• validate_ado_scope_tree: deny-by-default duplicate/empty-scope checks are case-insensitive and applied consistently to both read and write policies.
• AzureCliV3Connection enum design makes an invalid azureRM+azureDevOps mixed task unrepresentable — good use of the type system.
• Error handling throughout uses anyhow::Context consistently; no new unwrap()/expect() on user-reachable paths were introduced.

I did not find any correctness, error-handling, or credential-isolation defects in the changed lines. The rust-critic sub-agent produced no findings after reviewing the same diff (twice, given no output on the initial invocation), so there's nothing to adjudicate from it either.

One pre-existing note (non-blocking, not introduced by this PR): the new test ado_request_auth_matches_connection_type in src/safe_outputs/mod.rs asserts the Bearer-auth header value as the literal string "******" rather than "Bearer entra-token". This looks like it may be an artifact of secret-redaction tooling touching the PR diff/description rather than the actual committed test — worth a quick sanity check that the committed source doesn't literally contain a masked assertion, but I can't confirm from the diff alone and it doesn't block correctness of the reviewed logic.

🦀 Rust code quality review by Rust Code Quality Reviewer · auto · 129.4 AIC · ⌖ 9.87 AIC · ⊞ 11.5K
_{Comment /review to run again}

[github-actions]

Security review: no regressions found

Reviewed the full diff against the threat model: injection into generated pipelines, weakened validation, path traversal, token scope/exposure, network allowlist, safe-output write integrity, and prompt injection.

This PR is a hardening change, not a weakening one. The previous blanket reject_cross_organization_repository() guard is replaced by a stronger, allow-listed, deny-by-default gate (resolve_repository_write_target in src/safe_outputs/mod.rs) that:

• Requires connection-type: azureDevOps for any cross-org write, and an explicit permissions.write.allow entry matching the exact org/project/repo scope (no wildcards).
• Rejects (fail-closed) any repository alias with no declared organization, and anti-spoofing-rejects an alias whose declared organization equals the current org.
• Validates identity match between the compiler-resolved cross-org target and the actual checkout remote in prepare-pr-base/index.ts before rendering diff/PR-base data.
• Tightens PATH_SEGMENT escaping to also encode %, closing a path-segment smuggling vector, and tightens canonical_repository_alias() to reject ambiguous alias/self collisions instead of silently preferring self.
• Keeps cross-org bearer tokens shell-local (never assigned to a pipeline variable or SC_WRITE_TOKEN, never logged) — verified by the new typed AzureCliV3 builder and existing tests asserting no task.setvariable/env exposure.
• Reuses existing validated newtypes (AdoOrganization, etc.) rather than introducing raw String fields for org/project/repo identifiers.

No injection into generated YAML/bash was found (all new pipeline steps use the typed AzureCliV3 builder rather than string concatenation), no token scope widening onto the Stage 1 agent, and no safe-output write bypassing Stage 3. Test/e2e-only additions (executor-e2e/scenarios/cross-org.ts) require explicit environment configuration and skip otherwise, and are not part of the production trust boundary.

No merge-blocking findings. Nothing to flag inline.

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

[github-actions]

Test Quality Review

Thorough test coverage for this change. Highlights:

• resolve_repository_write_target / canonical_repository_alias (the core cross-org authorization logic in src/safe_outputs/mod.rs) has dedicated unit tests for every branch: self resolution, same-org checkout, authorized cross-org, missing organization, wrong connection-type, not-in-allow, endpoint-declared-as-current-org, and the new self/alias name-collision ambiguity rule. This is exactly the kind of behavior-level coverage a security-sensitive resolver needs.
• create_branch.rs / create_git_tag.rs / create_pull_request.rs each gained matching dry-run tests for cross-org success and rejection paths, plus pattern/label-policy enforcement.
• The old reject_cross_organization_repository tests were fully replaced (not just deleted) by equivalent-or-stronger coverage for the new resolver — no assertion was silently weakened.
• AzureCliV3Connection builder has both-variant tests asserting the ARM and Azure DevOps inputs are mutually exclusive on the emitted task, matching the docs claim that a mixed task is unrepresentable.
• TypeScript: auth.test.ts covers bearer vs PAT handler selection and cache invalidation on token rotation; prepare-pr-base tests cover matching/mismatched/same-org/Unicode-confusable cross-org remote rejection (good adversarial coverage); cross-org-scenarios.test.ts covers the SkipError paths including the unexpanded-pipeline-macro case.

No missing-test or weakened-assertion findings to flag; nothing here duplicates a linter/compiler check. No inline comments needed.

One minor observation (non-blocking): repository_write_target_rejects_incomplete_or_unauthorized_cross_org (src/safe_outputs/mod.rs) is a good table-driven test for the three rejection reasons — no changes needed there, just calling it out as a positive example other reviewers might want to reuse as a pattern.

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