fix(safe-outputs): support GitHub App comment replacement

[jamesadevine]

Summary

• capture the GitHub App bot identity during JWT-authenticated installation-token minting and pass it only to SafeOutputs jobs using hide-older-comments
• bypass /user and /installation actor discovery for installation tokens while preserving PAT /user behavior and fail-closed hide-before-post ordering
• add compiler/runtime regressions, combined PAT-backed executor E2E coverage, and documentation for the actor handoff

Fixes #2079

Test plan

• cargo test
• cargo clippy --all-targets --all-features -- -D warnings
• npm test in scripts/ado-script
• npm run typecheck in scripts/ado-script
• npm run build:github-app-token in scripts/ado-script

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

✅ Compiler Contract Reviewer completed the compiler contract review.

🏗️ Compiler contract review by Compiler Contract Reviewer

[github-actions]

No merge-blocking Rust issues found in this diff.

Scope and method

Reviewed src/compile/{agentic_pipeline,common,types}.rs, src/compile/extensions/ado_script.rs, src/safe_outputs/{comment_on_github_issue,create_pull_request,github_api,result,upload_build_attachment}.rs, and tests/compiler_tests.rs.

• Error handling: the ? propagation through comment_on_github_issue_config()? and the new fail-closed empty-actor-login check (returns ExecutionResult::failure before any comment mutation) are both sound.
• Correctness: github_actor_required gating correctly combines the comment-on-github-issue tool check with hide_older_comments, and is threaded consistently into github_app_token_step_typed_for, generate_executor_ado_env, and the executor context (ExecutionContext::github_actor_login).
• The removed authenticated_comment_actor/GithubInstallation machinery is fully superseded by the compile-time actor handoff, and tests were updated in lockstep (mock servers now assert /user and /installation are never hit).
• No unsafe casts, no blocking-in-async, no HashMap-ordering nondeterminism introduced by these changes.
• I also ran a rust-critic sub-agent pass over the same diff in parallel; it returned no findings.

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

[github-actions]

Security review — no regressions found

This diff improves the security posture rather than weakening it: Stage 3's comment-on-github-issue executor no longer sends the write-capable GitHub App installation token to /user or /installation for actor discovery. Instead, the App bot login (<slug>[bot]) is derived once during JWT-authenticated token minting (which already calls /orgs/{owner}/installation and receives app_slug in the response) and passed to the executor as a non-secret ADO job variable (ADO_AW_GITHUB_ACTOR_LOGIN, set via setVar, not setSecretVar).

Checked and confirmed sound:

• executor_actor_var() only returns Some for the App auth variant — PAT-backed configs never get this env var and continue to resolve via GET /user, matching the docs.
• The new actor-login path fails closed: an empty/whitespace ADO_AW_GITHUB_ACTOR_LOGIN returns ExecutionResult::failure before any comment listing/write calls (verified by hide_older_rejects_empty_app_actor_before_comment_writes, which asserts only the initial issue-fetch request was made).
• --actor-output-var's value is a compiler-owned constant (ADO_AW_SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN), not user-controlled input, and is single-quoted via the existing sh_single_quote helper consistent with other argv flags — no injection surface introduced.
• No secret (JWT, installation token) is logged or exposed through the new variable; the login string itself is non-sensitive.
• Stage separation is preserved: the mint step (JWT auth) and comment-hiding write (installation token) both still run in Stage 3 (SafeOutputs), so no read-only Stage 1 boundary is touched.

No exploitable weaknesses identified. Approving is not permitted for this workflow, so filing as COMMENT.

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

[github-actions]

TypeScript review — scripts/ado-script/

Reviewed github-app-token/index.ts (install-metadata capture, --actor-output-var, [bot] login derivation) and the new comment-on-github-issue-hide-older executor-e2e scenario.

No high-confidence defects found:

• resolveInstallation validates data.id is numeric before use and safely narrows app_slug (string-checked + trimmed) before exposing it; both success and app_slug-missing failure paths are covered by new tests.
• The actor login is derived and validated before mintInstallationToken runs, so a missing app_slug fails closed without ever minting/exposing a token — good ordering.
• setSecretVar is still called before setVar/logInfo, preserving the existing secret-before-log-line invariant.
• setVar/setSecretVar already escape name and value (escapeProperty/escapeMessage) so the new non-secret actorOutputVar variable emission is not an injection vector.
• New executor-e2e scenario follows the existing MutationIssueState/requireGraphqlFeature pattern used by sibling scenarios (e.g. hideGithubIssueComment) with proper cleanup/teardown.

Note: the ts-critic sub-agent produced no parseable JSONL output after being prompted twice, so its findings could not be incorporated — this review reflects only my own pass over the diff.

No merge-blocking issues.

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

[github-actions]

Solid, well-targeted test rewrite: the removed authenticated_comment_actor//installation discovery path is cleanly replaced by explicit ctx.github_actor_login handling, and both the mint-derived-actor success path and the empty-actor-login rejection are directly tested (hide_older_uses_mint_derived_app_actor_without_discovery, hide_older_rejects_empty_app_actor_before_comment_writes), including an assertion that no /user or /installation calls are made. The TS resolveInstallation rename and actorOutputVar/app_slug-missing fail-closed behavior are also directly tested.

Two untested edges left inline: the PAT fallback's error arm (client.authenticated_user() failing, e.g. 403) isn't exercised for the hide-older branch, and an empty-string app_slug (vs. absent) isn't covered in resolveInstallation/main. Neither is merge-blocking, but both are cheap to add and close real gaps around this fail-closed identity logic.

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

[github-actions]

Compiler contract review — no blocking issues

Reviewed the actor-identity handoff for hide-older-comments under safe-outputs.github-app:

• File-list drift check: pr-meta.json.files includes no gate/fact IR files (filter_ir.rs, Fact enum untouched) and no .gen.ts/.gen.json files — no codegen drift expected. No .github/workflows/*.md changed without its .lock.yml. No committed .lock.yml added under tests/safe-outputs/. scripts/ado-script/*.js bundles correctly absent (gitignored build artifacts).
• Contract wiring verified end-to-end: SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN_VAR / SAFE_OUTPUTS_GITHUB_ACTOR_LOGIN_ENV constants in src/compile/types.rs are threaded through github_app_token_step_typed_for → generate_executor_ado_env → ExecutionContext.github_actor_login → comment_on_github_issue.rs, with the github_actor_required flag correctly scoped per SafeOutputsVariant (confirmed test_compile_github_app_auth_is_scoped_to_reviewed_variant and test_compile_github_app_hide_older_wires_actor_identity pass, and the non-hide-older App fixture asserts absence of --actor-output-var/ADO_AW_GITHUB_ACTOR_LOGIN).
• Fail-closed behavior: empty/whitespace ADO_AW_GITHUB_ACTOR_LOGIN is rejected before any comment write (hide_older_rejects_empty_app_actor_before_comment_writes), and the removed authenticated_comment_actor//installation fallback is fully replaced rather than left as dead code.
• Docs: both docs/ado-script.md and docs/safe-outputs.md updated to describe the new actor-derivation path.
• Ran cargo build --bin ado-aw and the new/adjacent compiler_tests (test_compile_github_app_hide_older_wires_actor_identity, test_compile_github_issue_app_fixture_scopes_tokens_by_stage, test_compile_github_app_auth_is_scoped_to_reviewed_variant) locally — all pass.

No raw-String identifier fields introduced (the new actor_output_var/github_actor_login are plain bot-login strings, not paths/refs/SHAs, so src/secure.rs newtyping doesn't apply here). No missing extension registration, no IR/summary contract changes. Nothing merge-blocking found from a compiler-contract perspective.

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