feat(safeoutputs): add GitHub issue outputs

[jamesadevine]

Summary

• promote GitHub issue filing from ado-aw-debug to a configured-only public safe output named create-github-issue
• add gh-aw-aligned set-github-issue-type with same-run temporary issue IDs
• names are GitHub-qualified so agents cannot confuse them with the Azure DevOps create-work-item surface at runtime
• isolate PAT and permission-scoped GitHub App credentials to Stage 3, including GHES API routing
• migrate legacy front matter with the promote_debug_create_github_issue codemod (ado-aw-debug.create-issue -> safe-outputs.create-github-issue) and update documentation and dogfood workflows

Closes #1621

Test plan

• cargo test (2,774 tests)
• cargo clippy --all-targets
• cargo test --test compiler_tests
• cargo test --test codemod_tests
• npm test -- --maxWorkers=1 (789 tests, 75 files)
• npm run typecheck
• npm run build:github-app-token

Added review-requested coverage

Two security-relevant gates flagged in review as having no regression protection:

• engine.github-app-token.permissions write rejection — GitHub issue safe outputs may inherit the engine App credentials, and that token is also handed to Agent/Detection, so a write scope there would leak write-capable GitHub credentials into Stage 1. The sibling empty-permissions branch was already tested; the write branch was not. Adds the negative test plus a read-only positive counterpart.
• set-github-issue-type.allowed default-allow — verified intentional, not a bug: issue types are a closed set defined by the repository owner, so an empty list is already bounded by configuration the agent cannot influence, whereas create-github-issue.allowed-labels is default-deny because labels are free-form strings the agent can invent. Adds three tests pinning the documented behaviour, plus a note in docs/safe-outputs.md so the asymmetry is not "fixed" for consistency later.

Both gates were mutation-checked: disabling the write guard, and flipping the empty-allowed branch to default-deny, each fail the corresponding new test.

Known gap: no executor E2E coverage (follow-up)

Neither new tool is exercised end to end against real GitHub. Deferred deliberately — the smoke suite is being reworked separately, so adding a scenario now would collide with that work.

• executor-e2e excludes it by design today — tests/executor-e2e/README.md: "Excluded (out of scope or GitHub-only): the GitHub-only create-github-issue." There is no github-issue scenario in scenarios/index.ts. (Note the confusable: executor-e2e/github-issue.ts is the harness's own failure reporter, not a scenario under test.)
• smoke-failure-reporter (def 2549) is the only pipeline that calls the tool at runtime, but its release-owned lock still runs the legacy path ("create-issue", ADO_AW_DEBUG_GITHUB_TOKEN). It only switches to create-github-issue once a release regenerates the lock — at which point ADO_AW_GITHUB_TOKEN must be provisioned on that definition (the rotation script is updated in this PR to match).
• set-github-issue-type has no runtime coverage at all; its only non-src/ appearance is a compile fixture.

Residual risk while this is open: the same-run temporary-ID handoff (create-github-issue -> set-github-issue-type) is proven only by a wiremock unit test, so a wrong REST shape or App-token scoping failure would not be caught until the reworked smokes land.

Feasibility note for whoever picks this up: definition 2550 already has EXECUTOR_E2E_GITHUB_TOKEN provisioned, so a scenarios/github-issue.ts covering create -> set-type -> clear-type needs no new secret provisioning.

Addressed review feedback (2026-08-03)

The three reviewers each reported findings with "see inline comment", but no inline comments were actually posted (0 review threads exist on this PR via both REST and GraphQL) — the create-pull-request-review-comment safe outputs were dropped, most likely because the target lines fell outside the pre-fetched diff, which the Rust reviewer noted was truncated at 3000 lines. Findings were therefore taken from the review bodies and are resolved here rather than on threads.

Finding	Reviewer	Resolution
Silent permission-key collision after -→_ normalization in the App-token mint step	Rust Code Quality	Fixed
Two untested reachable error paths	Test Quality Sentinel	Fixed (3 tests)
target-repo should be a src/secure.rs newtype	Compiler Contract	Declined, with rationale + test

1. Permission-key collision (defect)

ado_script.rs normalizes -→_ before serializing --permissions-json (the GitHub API spells permissions with underscores) and collects into a BTreeMap, so pull-requests and pull_requests collapse onto one entry. The source map is also a BTreeMap, so _ sorts after - and deterministically wins — an author declaring both spellings could have an intended read silently replaced by write.

GithubAppTokenConfig::validate_for now rejects two keys that normalize to the same identifier, failing at compile time with a clear message. A single dashed spelling stays valid, so existing front matter is unaffected.

2. Untested reachable error paths

Both were in resolve_target_repo: a GitHub Enterprise pipeline source still pointing at api.github.com, and a GitHub-backed build where ADO did not surface BUILD_REPOSITORY_NAME. Added both negative tests plus a GHE positive counterpart.

3. target-repo newtype — declined

Applying this would introduce a fail-open. ExecutionContext::get_tool_config deserializes with .ok().unwrap_or_default(), so a value rejected at deserialization time collapses the entire config to Default (target_repo: None) — which resolve_target_repo then resolves to the current repository. The newtype would convert today's loud failure into a silent redirect that files issues against the wrong repo.

target-repo is already validated at compile time by validate_github_issue_outputs_config (validate_target_repo + reject_pipeline_injection) and re-validated at every Stage 3 call site. Added malformed_target_repo_does_not_redirect_to_current_repository, which pins the no-silent-redirect behaviour with a usable current repository deliberately present in the context, and records the rationale on the test so this is not "fixed" later.

Revisiting this properly means hardening get_tool_config so malformed configs fail loudly for all safe outputs, rather than only re-typing this one field. That is a cross-cutting change and out of scope here.

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

🔍 Rust PR Review

Summary: looks good — solid promotion from debug-only to public safe output, well-structured; one correctness concern and two suggestions worth addressing.

Findings

🐛 Bugs / Logic Issues

• src/safe_outputs/set_issue_type.rs:20-23 — #[serde(untagged)] on GithubIssueNumber may mis-parse
  GithubIssueNumber is #[serde(untagged)], so serde tries variants in declaration order. Number(u64) is listed first, and Temporary(GithubTemporaryId) second. A JSON string like "#aw_bug1" won't parse as u64, so it falls through correctly — but an integer-valued string like "42" will also fail u64 and fall through to GithubTemporaryId, which will then reject it with a confusing error about the #aw_ prefix rather than "expected a positive integer". More concretely: if the MCP layer ever serialises the number as a quoted string (some clients do), the error message will mislead the agent. Consider tagging the enum (e.g. #[serde(rename_all = "lowercase")] + an outer-object {type, value} shape) or explicitly testing that "42" (string) produces a clear error. This is a latent UX bug, not a security issue.

• src/safe_outputs/create_issue.rs — duplicate-temporary_id check runs before resolve_target_repo
  The has_resolved_github_issue guard fires before the target-repo is resolved. If resolve_target_repo would fail (e.g. non-GitHub pipeline with no configured target-repo), a second call with the same temporary_id returns "already used" rather than the config error. Not a security issue, but can confuse diagnostics — the guard and the repo resolution could swap order.

⚠️ Suggestions

• src/compile/common.rs — github_safe_outputs_auth() called twice at compile time
  validate_github_issue_outputs_config (called in build_pipeline_context) ends with let _ = front_matter.github_safe_outputs_auth()?; to surface any auth-config errors early, and then build_safeoutputs_job calls it again. The duplication is harmless but it means the validation logic and the usage logic have to stay in sync. Extracting the auth result into BuiltPipelineContext (like executor_ado_env was) would make the data flow explicit and prevent future drift.

• src/safe_outputs/set_issue_type.rs:369 — response.text() on non-success path uses .unwrap_or_else

  .unwrap_or_else(|_| "<unable to read response body>".to_string())

  This is fine as-is and consistent with create_issue.rs, just flagging it was reviewed and is intentional.

✅ What Looks Good

• Security isolation is well-preserved: ADO_AW_GITHUB_TOKEN / ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN are injected into Stage 3 only (via generate_executor_ado_env) and the github_token field never flows into Agent or Detection env. The old debug_enabled_tools NDJSON gate has been cleanly replaced by the standard tool_configs.contains_key pattern already used by all other tools.
• GithubTemporaryId validated newtype follows project conventions from src/secure.rs exactly — parse-don't-validate, consistent #aw_<id> normalisation, and the canonical() helper avoids double-# bugs.
• Codemod is robust: the errors_without_mutation_when_new_key_already_exists test verifies the snapshot equality on failure, ensuring atomicity is maintained. The github-token bridging of $(ADO_AW_DEBUG_GITHUB_TOKEN) is a good compatibility shim.
• require-approval parity validation in validate_github_issue_outputs_config correctly enforces that create-issue and set-issue-type must share the same job so temporary-ID resolution works; the error message clearly explains why.
• engine.github-app-token write-permission guard (permissions.{name} is write → bail!) prevents the Agent token (which is read-only by design) from inadvertently being reused as a write credential for Stage 3, which would break the security model.

Generated by Rust PR Reviewer for #1670 · 60.2 AIC · ⌖ 5.9 AIC · ⊞ 6.2K · ◷

[github-actions]

🔍 Rust PR Review

Summary: Solid promotion of GitHub issue outputs from debug-only to production; a few items worth addressing before merge.

Findings

🐛 Bugs / Logic Issues

• src/safe_outputs/create_issue.rs:311-318 — TOCTOU on temporary ID duplicate check
  The pre-flight check calls has_resolved_github_issue (releases the lock), makes the HTTP call, then calls register_resolved_github_issue (re-acquires the lock). Two concurrent safe-output executors for the same run could both pass the pre-flight check and both file a GitHub issue. register_resolved_github_issue does have a second check inside the lock and bail!s, but that propagates as an anyhow::Error, not an ExecutionResult::failure — so it surfaces as a hard error rather than a graceful operator-facing failure message. Consider converting the inner bail! path to return ExecutionResult::failure at the call site in execute_impl, or document that the duplicate-registration case is intentionally a hard error.

• src/safe_outputs/set_issue_type.rs — empty issue_type sends {"type": ""} to GitHub
  The GitHub Issues REST API for clearing an issue type expects "type": null, not "type": "". Sending an empty string is likely to return a 422 Unprocessable Entity rather than clearing the type. The dry-run path and docstring both describe this as a "clear" operation, so the intent is correct — the payload should use serde_json::Value::Null when resolved_type is empty.

• src/compile/common.rs:1501 — ADO_AW_GITHUB_API_URL written unquoted into generated YAML
  generate_executor_ado_env emits ADO_AW_GITHUB_API_URL: {value} as a bare literal. The URL is validated at compile time via url::Url::parse (https-only), but a URL with a #fragment component — which url::Url permits — would produce malformed YAML. The parse_safe_outputs_github_api_url helper returns parsed.to_string() which preserves fragments. Reject or strip fragment components in that function, or quote the emitted value.

⚠️ Suggestions

• src/safe_outputs/set_issue_type.rs — no allowlist enforcement when allowed is empty
  When the operator omits safe-outputs.set-issue-type.allowed, any agent-supplied issue_type passes through verbatim to GitHub (after reject_pipeline_injection). The analogous create-issue.allowed-labels works the same way, but issue types have no extra structural validation. Consider documenting this clearly in docs/safe-outputs.md so operators know an empty allowed list is fully permissive, not deny-all.

• src/safe_outputs/result.rs:get_tool_config — silent require-approval removal
  The new code strips require-approval from the config value before deserialisation to avoid deny_unknown_fields errors. This is correct, but a brief comment explaining the rationale would help future readers avoid confusion if a config struct ever legitimately needs this field.

✅ What Looks Good

• Credential isolation is well-designed: ADO_AW_GITHUB_TOKEN is strictly Stage 3-only and never injected into Agent or Detection jobs. The compile-time wiring in agentic_pipeline.rs enforces this boundary cleanly.
• GithubTemporaryId as a validated newtype (parse-don't-validate) is exactly the right pattern — format errors fail at deserialisation rather than runtime.
• parsePermissions in TypeScript has solid prototype-pollution guards (__proto__, prototype, constructor blocklist), and the Rust mirror in GithubAppTokenConfig::validate_for is consistent.
• Codemod 0006 is idempotent, preserves the legacy token variable as a migration aid, handles all edge cases (existing auth, conflicting keys) with clear error messages, and is well tested.
• Error body sanitization via neutralize_pipeline_commands on GitHub API error responses is a good defence-in-depth addition.

Generated by Rust PR Reviewer for #1670 · 105.7 AIC · ⌖ 5.83 AIC · ⊞ 6.2K · ◷

[github-actions]

@copilot This PR has a failing check:

• detection failed: https://github.com/githubnext/ado-aw/actions/runs/30165338843/job/89697724683

Please investigate the failure and push a fix.

Generated by 👨‍🍳 PR Sous Chef · aut00 · 39.4 AIC · ⌖ 3.9 AIC · ⊞ 12.5K · ◷
_{Comment /souschef to run again}

[jamesadevine]

@copilot resolve the merge conflicts in this pull request

[Copilot]

@copilot resolve the merge conflicts in this pull request

Resolved in fc90d6f.

[jamesadevine]

/review

[github-actions]

✅ PR Security Reviewer completed the security review.

[jamesadevine]

/review

[github-actions]

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

No review performed: pre-fetched PR data files were empty (pr-meta.json = {}, pr-diff.patch = 0 lines) for PR #1670, so there were no Rust changes to analyze.

[jamesadevine]

/review

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

Test Quality Sentinel: pre-fetched PR diff/metadata files (pr-diff.patch, pr-meta.json, pr-review-comments.json) for PR #1670 were empty/blank when this review ran, so no test-quality review could be performed. Likely a cache-warming/prefetch step failure or timing issue in the pr-data-prefetch workflow — no findings to report.

[jamesadevine]

/review

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

[github-actions]

Reviewed the scripts/ado-script/ portion of this PR (mainly github-app-token/index.ts's new parsePermissions helper and the ADO_AW_DEBUG_GITHUB_TOKEN → ADO_AW_GITHUB_TOKEN env-var rename across the e2e harnesses).

The new permission-parsing code is solid: prototype-pollution guards (__proto__/prototype/constructor), a strict name regex, validated read/write levels, and matching unit tests for both the happy path and each rejection case. mintInstallationToken's optional permissions param defaults safely and is only added to the request body when non-empty. main()'s try/catch already wraps the new parsePermissions call so a bad flag fails the pipeline step cleanly rather than crashing unhandled.

One minor advisory comment posted inline about giving JSON-syntax parse errors more context. Nothing merge-blocking.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · aut00 · 37.3 AIC · ⌖ 3.85 AIC · ⊞ 10.9K
_{Comment /review to run again}

[github-actions]

Compiler contract review — no drift found

Checked the areas that matter most for this PR's scope:

• Bundle drift: scripts/ado-script/src/approval-summary/render.ts and scripts/ado-script/src/github-app-token/index.ts changed; both bundle directories map to gitignored .js outputs that are rebuilt by CI (npm run build), not committed — no drift to flag here.
• Tool registration: CreateGithubIssueResult and SetGithubIssueTypeResult are correctly wired into ALL_KNOWN_SAFE_OUTPUTS, the new CONFIGURED_ONLY_TOOLS registry, and src/mcp.rs's gating logic (DEBUG_ONLY_TOOLS.contains(...) || CONFIGURED_ONLY_TOOLS.contains(...)).
• Codemod: 0006_promote_debug_create_github_issue is registered in both the mod m0006_... declaration and the CODEMODS static array in src/compile/codemods/mod.rs, matches the numbering convention, and has idempotency + conflict-detection tests.
• Typed identifiers: the new temporary_id / issue_number fields correctly use the validated GithubTemporaryId newtype from src/secure.rs rather than a raw String.
• Docs: docs/safe-outputs.md has a full "GitHub issue safe outputs" section (auth, target-repo resolution, temporary IDs, approval-gating parity requirement) and both tools are documented individually. AGENTS.md's architecture tree reflects the renamed create_github_issue.rs, new set_github_issue_type.rs, and the new codemod file.

No merge-blocking contract gaps found in this diff.

🏗️ Compiler contract review by Compiler Contract Reviewer · aut00 · 95.3 AIC · ⌖ 3.83 AIC · ⊞ 11.9K
_{Comment /review to run again}

[github-actions]

Reviewed test coverage for the GitHub issue safe-output promotion. The new set_github_issue_type.rs and the codemod (0006_promote_debug_create_github_issue.rs) are well-tested, including idempotency, no-mutation-on-error, and a full wiremock-backed temporary-ID resolution flow. tests/compiler_tests.rs::test_compile_github_issue_app_fixture_scopes_tokens_by_stage is a strong integration test for the stage-isolated-credential claim, and its fixture (tests/fixtures/github-issue-app-agent.md) genuinely exercises the scenario it asserts on.

However, github_safe_outputs_auth() in src/compile/types.rs has several bail! branches with no matching tests — specifically the "github-app.permissions not supported for issue outputs" rejection, the write-permission rejection for an inherited engine.github-app-token, and both "github-api-url applies only to PAT auth" conflict checks (explicit github-app and inherited engine.github-app-token). These are exactly the kind of security-relevant auth-resolution branches this PR's description is built around, so leaving them untested is a real regression risk. resolve_target_repo's GitHub-Enterprise-without-api-url branch in create_github_issue.rs is similarly untested (only github/tfsgit providers are covered). Finally, create_github_issue.rs's executor tests are all rejection-path only — no test exercises the actual HTTP success path or the "filed but failed to register temporary_id" partial-failure branch, unlike its sibling set_github_issue_type.rs which does mock the real request/response cycle.

None of these are merge-blocking on their own (the existing coverage is otherwise solid), but I'd like to see at least the auth-resolution branches covered before merge given their security relevance.

🧪 Test quality analysis by Test Quality Sentinel · aut00 · 238.4 AIC · ⌖ 4.35 AIC · ⊞ 10.7K
_{Comment /review to run again}

[jamesadevine]

/review

[github-actions]

✅ Compiler Contract Reviewer completed the compiler contract review.

Compiler-contract review of PR #1670 complete. Checked: bundle drift (approval-summary.js, github-app-token.js are release-built artifacts, not committed to git — confirmed via .gitignore and .github/workflows/ado-script.yml; no drift possible), codegen drift (types.gen.ts/fact-catalog.gen.json correctly untouched since no Fact/gate IR changed), compiled workflow drift (no .github/workflows/*.md changed), release-owned fixtures (smoke-failure-reporter.lock.yml untouched, correct), front-matter grammar (new github-token/github-api-url/github-app fields are optional with proper validation), safe-output tools (create-github-issue and set-github-issue-type properly registered in catalog.rs, docs/safe-outputs.md, custom_tools reserved keys), codemod 0006 correctly registered in CODEMODS registry, AGENTS.md tree fully updated with both new safe-output files and the codemod. No merge-blocking contract violations found — this is a well-executed, thoroughly cross-referenced change. No inline comments needed.

[github-actions]

TypeScript review — scripts/ado-script/

Reviewed the scripts/ado-script/ changes: parsePermissions/mintInstallationToken additions in github-app-token/index.ts, the approval-summary TOOL_SPECS rename/addition, and the ADO_AW_DEBUG_GITHUB_TOKEN → ADO_AW_GITHUB_TOKEN env rename in the executor/trigger e2e harnesses.

No defects found:

• parsePermissions validates permission names against an allowlist regex and explicitly rejects __proto__/prototype/constructor before use (the map is also built with Object.create(null), so prototype pollution isn't reachable regardless).
• JSON.parse and all new network/error paths are covered by main()'s existing outer try/catch, which logs err.message and returns a non-zero exit rather than throwing unhandled.
• No secrets are logged — the new logInfo call only reports the owner, installation id, and repo count, not permissions or the token.
• New behavior (permission parsing, minting with permissions) has matching unit tests in github-app-token/__tests__/index.test.ts.

No inline comments needed.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · aut00 · 54.1 AIC · ⌖ 6.33 AIC · ⊞ 10.9K
_{Comment /review to run again}

[github-actions]

Security review — no findings

Reviewed the diff for token-scope and injection regressions in the new create-github-issue / set-github-issue-type safe outputs and the GitHub App permission-scoping work.

• Token isolation is preserved: ADO_AW_GITHUB_TOKEN / minted App tokens are wired only into the Stage 3 SafeOutputs job env (generate_executor_ado_env), never into the Agent or Detection stage. safe-outputs.github-token explicitly rejects referencing the Agent-visible GITHUB_TOKEN or Copilot engine token variable, closing a plausible confusion vector.
• App token minting is now permission-scoped (--permissions-json) rather than inheriting full installation scope, and repository scoping (scope_github_app_to_issue_targets) enforces owner match and rejects mixing implicit/explicit targets, preventing the token from being minted broader than the configured target repo(s).
• GHES api-url is validated (https-only, no query/fragment) in both the GitHub App config path and the new github-api-url field, and resolve_target_repo requires an explicit github-api-url/App api-url for enterprise sources rather than silently falling back to api.github.com.
• Agent-supplied content (title, body, labels, assignees, issue_type) passes through reject_pipeline_injection and sanitize_text/neutralize_pipeline_commands before being embedded in requests, logs, or failure messages; label allowlisting is default-deny with an explicit "*" opt-out.
• HTTP calls use reqwest with .bearer_auth(token) (never string-interpolated into a header or URL) and owner/repo path segments are percent-encoded before building the request URL.
• MCP-layer gating (CONFIGURED_ONLY_TOOLS) keeps create-github-issue / set-github-issue-type unreachable unless the operator declares the safe-outputs key, consistent with the existing DEBUG_ONLY_TOOLS pattern, and is covered by tests for both the default-stripped and explicitly-enabled cases.

No exploitable regression found in this diff relative to main. Nice work isolating the PAT/App credential to Stage 3 and adding the permission-subset minting.

🔒 Security review by PR Security Reviewer · aut00 · 64.2 AIC · ⌖ 3.89 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Reviewed the Rust portions of the diff (src/compile/agentic_pipeline.rs, src/compile/codemods/0006_promote_debug_create_github_issue.rs, src/compile/common.rs, src/compile/custom_tools.rs, src/compile/extensions/ado_script.rs, src/compile/imports/merge.rs, src/compile/mod.rs, src/compile/types.rs) — note the diff was truncated at 3000 lines so types.rs and later files (src/execute.rs, src/mcp.rs, src/safe_outputs/*, src/secure.rs) were not fully visible.

One defect found: a silent permission-key collision after -→_ normalization in the GitHub App token mint step (inline comment). Error handling, anyhow context usage, and overall structure otherwise look solid — the new GithubSafeOutputsAuth enum, codemod, and validation paths are well-tested and free of panics on user-input paths. rust-critic sub-agent returned no output after one retry attempt per protocol, so this review reflects my own pass only.

🦀 Rust code quality review by Rust Code Quality Reviewer · aut00 · 92.6 AIC · ⌖ 5.58 AIC · ⊞ 11K
_{Comment /review to run again}

[github-actions]

Solid test coverage overall for the new create-github-issue/set-github-issue-type tools and the App-token permission scoping (positive/negative cases for token vs. App auth, mixed implicit/explicit targets, empty-permissions rejection, reserved permission keys). Two gaps worth closing before merge: an untested default-allow behavior in set-github-issue-type when allowed: is empty (asymmetric with the sibling tool's default-deny for labels), and no test for the write-scoped engine App-token rejection branch — both are security-relevant gates that currently have no regression protection. Neither is merge-blocking on its own but the second in particular protects the PR's core "isolate credentials to Stage 3" invariant.

🧪 Test quality analysis by Test Quality Sentinel · aut00 · 119.6 AIC · ⌖ 4 AIC · ⊞ 10.7K
_{Comment /review to run again}

[jamesadevine]

/review

[github-actions]

The test additions visible in the fetched diff are solid: github-app-token gains focused positive/negative unit tests for parsePermissions (valid read/write JSON, rejected permission names, rejected levels, reserved __proto__/constructor keys) and an integration-style assertion that mintInstallationToken actually sends the scoped permissions body — this directly protects the write-scope-leak concern called out in the PR description. The new codemod (0007_promote_debug_create_github_issue.rs) has good behavioral coverage: happy path, empty-debug-section cleanup, existing-auth preservation, the conflict-error branch (with a snapshot equality check confirming no partial mutation on error), and an explicit idempotency test.

No weakened or removed assertions, and no untested new behavior, in the portion of the diff I could inspect.

Scope note: the fetched pr-diff.patch is capped at 3000 lines and truncates partway through src/compile/types.rs, before it reaches several test-relevant additions mentioned in this PR (the new src/safe_outputs/set_github_issue_type.rs, and the modified tests/codemod_tests.rs, tests/compiler_tests.rs, src/safe_outputs/create_github_issue.rs tests). I could not assess test quality for that code in this pass and have no diff lines to anchor inline comments to for it — flagging so a follow-up pass (or a reviewer with the full diff) covers that portion, particularly the two review-requested security-gate tests the PR body says were added (engine.github-app-token.permissions write-rejection, set-github-issue-type.allowed default-allow).

🧪 Test quality analysis by Test Quality Sentinel · aut00 · 40.9 AIC · ⌖ 3.82 AIC · ⊞ 10.7K
_{Comment /review to run again}

[github-actions]

Rust engineering review

No merge-blocking defects found in the reviewed portion.

Reviewed the Rust-relevant hunks in src/compile/agentic_pipeline.rs, src/compile/codemods/0007_promote_debug_create_github_issue.rs, src/compile/codemods/mod.rs, src/compile/common.rs, src/compile/custom_tools.rs, src/compile/extensions/ado_script.rs, src/compile/imports/merge.rs, src/compile/mod.rs, and the start of src/compile/types.rs. The pre-fetched diff is capped at 3000 lines and cuts off mid-types.rs, so src/execute.rs, src/mcp.rs, src/safe_outputs/create_github_issue.rs, src/safe_outputs/set_github_issue_type.rs, src/safe_outputs/result.rs, src/secure.rs, and the remainder of types.rs were not visible to this pass.

💡 What was checked

• Error handling: new validation paths (validate_github_issue_outputs_config, GithubAppTokenConfig::validate_for, parse_safe_outputs_github_token/_api_url) consistently use anyhow::bail!/Context with actionable messages, no bare unwrap/expect on user-reachable input.
• Determinism: the new permissions map uses BTreeMap (not HashMap), so --permissions-json serialization is stable for generated YAML.
• The permission-key normalization collision check (- vs _) in validate_for is a nice defensive addition catching a real silent-collapse bug before it reaches the mint step.
• Removed executor_ado_env field from StandaloneCtx/tests and its recomputation per-variant in build_safeoutputs_job is consistent with the new per-job GitHub auth requirement.
• Codemod (0007_promote_debug_create_github_issue.rs) mutation is guarded (existing-key conflict bails without mutating), and idempotency is explicitly tested.

The rust-critic sub-agent invocation in this run did not receive diff content due to a tooling issue on my side, so its findings could not be incorporated; this review reflects only my own pass.

Note: given the truncated diff, a full pass over the untruncated src/safe_outputs/create_github_issue.rs, set_github_issue_type.rs, and src/mcp.rs changes (largest additions in this PR) would need a follow-up review once the full diff is available.

🦀 Rust code quality review by Rust Code Quality Reviewer · aut00 · 56.7 AIC · ⌖ 4.24 AIC · ⊞ 11K
_{Comment /review to run again}

[github-actions]

Security review — no regressions found

Reviewed the visible portion of the diff (capped at 3000 lines; several newer/larger files — src/mcp.rs, src/secure.rs, src/safe_outputs/create_github_issue.rs, src/safe_outputs/set_github_issue_type.rs, src/safe_outputs/result.rs — fell outside the cap and were not inspected here).

What was reviewable looks like a genuine hardening pass rather than a weakening:

• Token isolation preserved: generate_executor_ado_env still routes SYSTEM_ACCESSTOKEN through the shared token_source_for() chokepoint, and the new ADO_AW_GITHUB_TOKEN / ADO_AW_GITHUB_API_URL env is only emitted for the SafeOutputs (Stage 3) job — Agent/Detection are untouched.
• App-token scoping: scope_github_app_to_issue_targets rejects mixing implicit (current-repo) and explicit target-repo values across the two tools, and cross-checks the App's owner against the resolved target, closing an ambiguous-scope path.
• Permission-collision defect fixed: the -→_ normalization collision in GithubAppTokenConfig::validate_for (which could silently let a declared read be overridden by write) is now rejected at compile time, with a mutation-checked test, matching what the PR description states.
• Injection guards extended: validate_github_issue_outputs_config runs reject_pipeline_injection over target-repo, title-prefix, labels, allowed-labels, and assignees for both new tools; API URLs are validated as https://, host-present, no query/fragment.
• safe-outputs.github-token guard: explicitly rejects referencing GITHUB_TOKEN or the engine App-token variable, so a Stage-3-only credential can't be redirected to double as the Agent/Detection credential.

The declined target-repo newtype (documented in the PR body) is a defensible call given get_tool_config's .ok().unwrap_or_default() fail-open behavior — converting to a hard deserialization failure there would swap a loud error for a silent redirect. Compile-time + Stage-3 re-validation already cover it, and the tradeoff is explained and tested.

No exploitable findings in the reviewable portion of the diff. Not requesting changes.

🔒 Security review by PR Security Reviewer · aut00 · 52.8 AIC · ⌖ 3.91 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Compiler-contract review — no findings

Checked the areas this reviewer owns for PR #1670 (promoting create-issue → create-github-issue, adding set-github-issue-type):

• Codemod registration: 0007_promote_debug_create_github_issue.rs is correctly registered in codemods/mod.rs's CODEMODS array — the breaking front-matter rename (ado-aw-debug.create-issue → safe-outputs.create-github-issue) has a migration path.
• Bundle drift: scripts/ado-script/src/approval-summary/render.ts and src/github-app-token/index.ts changed, but their compiled .js outputs are .gitignored (confirmed via scripts/ado-script/.gitignore and docs/ado-script.md), not committed artifacts — so there is no drift to detect here, unlike a typical committed-bundle scenario.
• Codegen drift: no changes touched filter_ir.rs or the Fact enum, so types.gen.ts/fact-catalog.gen.json are correctly untouched.
• Front-matter grammar: new GithubAppTokenConfig.permissions, safe-outputs.github-token/github-api-url/github-app are all #[serde(default...)]/Option, non-breaking for existing pipelines.
• New safe-output tools: create-github-issue / set-github-issue-type are registered in ALL_KNOWN_SAFE_OUTPUTS, flow through the generic all_safe_output_tool_names() tool-config plumbing (the old special-cased debug_create_issue/ado_aw_debug.create_issue insertion logic in src/main.rs was correctly deleted rather than left as dead code), and are documented in docs/safe-outputs.md, docs/front-matter.md, docs/ado-script.md, AGENTS.md, and src/inspect/catalog.rs.
• Identifiers: target-repo stays a String in CreateGithubIssueConfig/SetGithubIssueTypeConfig, but it is validated at compile time via validate_target_repo (owner/repo regex) and again at Stage 3 — not an unvalidated raw string reaching an untrusted boundary.
• New GithubTemporaryId in src/secure.rs correctly follows the validated_string! newtype convention for the #aw_... temporary-ID format.

No bundle/codegen/lock drift, no unregistered extensions, no missing documentation found. Nice, thorough PR description covering the deliberate allowed/allowed-labels asymmetry and the known executor-e2e gap.

🏗️ Compiler contract review by Compiler Contract Reviewer · aut00 · 122 AIC · ⌖ 3.93 AIC · ⊞ 12K
_{Comment /review to run again}

[jamesadevine]

/review

[github-actions]

✅ Test Quality Sentinel completed the test quality analysis.

[github-actions]

Compiler contract review

Good contract hygiene overall: the breaking ado-aw-debug.create-issue -> safe-outputs.create-github-issue rename is covered by a properly-registered codemod (0007_promote_debug_create_github_issue), and the new temporary_id field uses a validated GithubTemporaryId newtype from src/secure.rs rather than a raw String.

Bundle drift (blocking): two scripts/ado-script/src/** sources change runtime-relevant behavior but their committed bundles are absent from this PR's file list:

• src/github-app-token/index.ts adds parsePermissions/--permissions-json scoping (security-relevant — this is the very mechanism isolating write-capable App credentials to Stage 3), but github-app-token.js is not rebuilt.
• src/approval-summary/render.ts renames create-issue -> create-github-issue and adds set-github-issue-type to TOOL_SPECS, but approval-summary.js is not rebuilt.

Both need npm --prefix scripts/ado-script run build with the regenerated .js files committed — otherwise the shipped bundles silently keep running the old, unscoped/mislabeled behavior even though the source and tests say otherwise. Inline comments added on each source file.

🏗️ Compiler contract review by Compiler Contract Reviewer · aut00 · 37.7 AIC · ⌖ 6.5 AIC · ⊞ 11.9K
_{Comment /review to run again}

[github-actions]

TypeScript review — scripts/ado-script/

Reviewed the github-app-token permission-scoping addition and the ADO_AW_DEBUG_GITHUB_TOKEN to ADO_AW_GITHUB_TOKEN rename across executor-e2e/trigger-e2e.

No blocking findings. parsePermissions validates permission names against an allowlist regex, explicitly rejects proto/prototype/constructor (prototype-pollution guard), validates levels are read/write, and is called inside main's existing try/catch so failures are logged via logError and return a non-zero exit code without leaking the raw JSON or secret material. Test coverage for both valid and invalid permission JSON is present. No unhandled promises, any leakage, or unsafe casts introduced in the diff.

🟦 TypeScript code quality review by TypeScript Code Quality Reviewer · aut00 · 52.7 AIC · ⌖ 3.75 AIC · ⊞ 10.9K
_{Comment /review to run again}

[github-actions]

This diff is a well-scoped credential-isolation refactor and I did not find a security regression compared to main.

Threat-model check (Stage 1/2 read-only vs Stage 3 write-capable GitHub credential):

• ADO_AW_GITHUB_TOKEN/App token is only injected via generate_executor_ado_env() into the SafeOutputs (Stage 3) job env; agentic_pipeline.rs never wires github_auth into the Agent or Detection job builders. A new regression test (test_compile_github_issue_app_fixture_scopes_tokens_by_stage) explicitly asserts ADO_AW_GITHUB_TOKEN is absent from the Agent/Detection blocks — good coverage for exactly the leak a reviewer would worry about.
• github_safe_outputs_auth() in types.rs rejects any write permission when GitHub issue outputs would inherit the shared engine.github-app-token (which is also used in Stage 1/2), forcing a dedicated Stage-3-only credential whenever write access is needed.
• parse_safe_outputs_github_token() rejects reusing GITHUB_TOKEN or the engine App-token variable name as the safe-outputs token source, so an author cannot accidentally point Stage 3 write config back at a Stage 1 read-only var.
• Both new Stage 3 executors (create_github_issue.rs, set_github_issue_type.rs) independently gate on ctx.tool_configs.contains_key(...), so a forged NDJSON entry cannot bypass the MCP layer's CONFIGURED_ONLY_TOOLS default-deny even if that filter were somehow circumvented.
• set-github-issue-type's temporary-ID resolution validates the resolved issue's repository against the configured target-repo when both are set, preventing a temp-ID mixup from redirecting a type change to an unintended repo.
• GitHub API error bodies are passed through neutralize_pipeline_commands() before being embedded in ExecutionResult messages, preventing ##vso[...] injection via a reflected/compromised API response.
• GithubTemporaryId (new validated newtype in secure.rs) uses a closed-form regex, consistent with the rest of the secure.rs pattern.

No injection sinks, weakened validation, path-traversal issues, or network-allowlist widening were found in the reviewed portions (compiler front-matter validation, IR/pipeline wiring, MCP tool gating, Stage 3 executors, the TS App-token minting script, and the migration codemod). Nice defense-in-depth design overall.

🔒 Security review by PR Security Reviewer · aut00 · 137.1 AIC · ⌖ 3.91 AIC · ⊞ 11.2K
_{Comment /review to run again}

[github-actions]

Rust Code Quality Review — no merge-blocking findings

Reviewed the Rust changes (src/safe_outputs/create_github_issue.rs, set_github_issue_type.rs, result.rs, secure.rs, compile/types.rs, compile/extensions/ado_script.rs, compile/agentic_pipeline.rs, execute.rs, mcp.rs, main.rs) plus accompanying tests, cross-referenced with a rust-critic sub-agent pass.

Verdict: no defects found. Both passes came back empty.

💡 What was checked

• Error handling: all fallible executor paths return anyhow::Result/ExecutionResult::failure, no reachable unwrap()/expect() outside tests (the one non-test expect() in common.rs::generate_executor_ado_env is on serde_json::to_string of a &str, which cannot fail).
• Credential isolation: github_token/github_api_url are correctly confined to the Stage 3 ExecutionContext, never wired into Agent/Detection env.
• Concurrency: resolved_github_issues: Arc<Mutex<HashMap<...>>> in result.rs uses poison-safe .lock().map_err(...) and a proper check-then-insert under a single lock in register_resolved_github_issue, avoiding a TOCTOU race on the mutation path.
• Determinism: the new permissions: BTreeMap<String, GithubAppPermissionLevel> in GithubAppTokenConfig plus the validate_for collision guard (rejecting -/_ spelling collisions before they silently collapse in --permissions-json) is correctly ordered and covered by github_app_permissions_reject_separator_only_key_collision.
• Fallback safety: resolve_target_repo's current-repo fallback is validated end-to-end (malformed_target_repo_does_not_redirect_to_current_repository), confirming no silent redirect to the wrong repository when target-repo is malformed.
• Cross-file consistency: renames (create_issue.rs -> create_github_issue.rs, CreateIssue* -> CreateGithubIssue*, ADO_AW_DEBUG_GITHUB_TOKEN -> ADO_AW_GITHUB_TOKEN) are complete and consistent across mcp.rs, execute.rs, mod.rs, main.rs, and tests.

Nothing to add beyond what's already covered by the compiler-contract and test-quality reviewers.

🦀 Rust code quality review by Rust Code Quality Reviewer · aut00 · 138.5 AIC · ⌖ 3.9 AIC · ⊞ 11K
_{Comment /review to run again}

[github-actions]

Test Quality Sentinel review

This is a very well-tested PR overall — the author's own PR description calls out mutation-checked coverage for the two most security-relevant gates (write-scope rejection for engine App tokens, and the set-github-issue-type default-allow asymmetry), and both are indeed exercised by focused positive/negative test pairs (github_issue_outputs_reject_write_scoped_engine_app_permissions / _accept_read_only_engine_app_permissions, empty_allowed_list_permits_any_issue_type / non_empty_allowed_list_rejects_unlisted_type_before_http). The rename/promotion of create-issue → create-github-issue is mechanically mirrored across the Rust and TypeScript test suites, and the new set_github_issue_type.rs module has solid wiremock-backed coverage of its create→resolve→PATCH flow, unresolved temporary IDs, and case-insensitive type matching.

One gap: the new GithubTemporaryId newtype in src/secure.rs breaks the file's established pattern of pinning every validated-string type's accept/reject boundary with a dedicated unit test (see inline comment). Everything else checked out — no weakened or deleted assertions were found, and the removed test_execute_rejects_when_debug_tool_not_authorized test is legitimate since the underlying debug-gate mechanism it protected was intentionally removed as part of the promotion to a public safe output.

🧪 Test quality analysis by Test Quality Sentinel · aut00 · 142.7 AIC · ⌖ 4.01 AIC · ⊞ 10.7K
_{Comment /review to run again}

incorrect prompt - built bundles are not committed