From 26f73c15054098e1dec02eeff4d9683eb5b8d5b3 Mon Sep 17 00:00:00 2001 From: James Devine Date: Wed, 2 Sep 2026 10:14:45 +0100 Subject: [PATCH] fix(safe-outputs): support GitHub App comment replacement Capture the App bot identity during token minting so hide-older-comments does not call endpoints forbidden to installation tokens. Add compiler, executor, and E2E coverage for the actor handoff. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: aa416c50-4e54-4a23-b35a-a6f5be7795a6 --- docs/ado-script.md | 12 +- docs/safe-outputs.md | 13 ++- .../__tests__/github-issue-scenarios.test.ts | 3 +- .../src/executor-e2e/__tests__/index.test.ts | 1 + .../executor-e2e/scenarios/github-issue.ts | 103 ++++++++++++++++++ .../github-app-token/__tests__/index.test.ts | 93 ++++++++++++++-- .../ado-script/src/github-app-token/index.ts | 68 +++++++++--- src/compile/agentic_pipeline.rs | 14 +++ src/compile/common.rs | 34 +++++- src/compile/extensions/ado_script.rs | 8 ++ src/compile/types.rs | 10 ++ src/safe_outputs/comment_on_github_issue.rs | 77 +++++++++---- src/safe_outputs/create_pull_request.rs | 1 + src/safe_outputs/github_api.rs | 68 +----------- src/safe_outputs/result.rs | 19 ++++ src/safe_outputs/upload_build_attachment.rs | 1 + tests/compiler_tests.rs | 35 ++++++ tests/executor-e2e/README.md | 14 ++- 18 files changed, 452 insertions(+), 122 deletions(-) diff --git a/docs/ado-script.md b/docs/ado-script.md index cbad4b71..a633f1fd 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -404,9 +404,15 @@ its own App JWT / minted installation token, so it carries no `SYSTEM_ACCESSTOKEN` and no ADO predefined mirrors. Its only env var is the masked private-key secret (`GH_APP_PRIVATE_KEY`, and `GH_APP_TOKEN` for revoke); every other, non-secret input (App ID, owner, repositories, output-variable -name, api-url) is a single-quoted **argv flag** rather than an env var, so a -pipeline variable can never shadow it (ADO injects pipeline variables into a -step's env, but argv comes only from the compiler-authored script). +name, optional actor-output-variable name, api-url) is a single-quoted **argv +flag** rather than an env var, so a pipeline variable can never shadow it (ADO +injects pipeline variables into a step's env, but argv comes only from the +compiler-authored script). When GitHub App-backed SafeOutputs enable +`hide-older-comments`, the bundle also captures `app_slug` from the +JWT-authenticated installation lookup and emits the derived `[bot]` +login as a non-secret same-job variable. The Stage 3 executor uses that +identity for exact matching instead of sending the installation access token +to actor-discovery endpoints it cannot call. ## End-to-end data flow diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index dc9c3a1e..a5ab9f35 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -752,9 +752,16 @@ JSON uses the snake_case parameter names below. marker and, by default, a trace footer (`footer: false` disables the visible footer). `hide-older-comments: true` first minimizes older comments carrying the same pipeline marker **and** authored by the authenticated actor. It never -trusts agent-authored marker text. `allowed-reasons` restricts minimization -reasons; supported reasons are `SPAM`, `ABUSE`, `OFF_TOPIC`, `OUTDATED` -(default), `RESOLVED`, and `LOW_QUALITY`. +trusts agent-authored marker text. With PAT authentication, Stage 3 resolves +the actor through GitHub's `GET /user` endpoint. With +`safe-outputs.github-app`, the JWT-authenticated token-mint step captures the +installation's App slug and passes the derived `[bot]` login to Stage 3; +installation access tokens are not sent to `/user` or `/installation` for +actor discovery. Actor resolution, older-comment discovery, and minimization +remain fail-closed and complete before the replacement comment is posted. +`allowed-reasons` restricts minimization reasons; supported reasons are +`SPAM`, `ABUSE`, `OFF_TOPIC`, `OUTDATED` (default), `RESOLVED`, and +`LOW_QUALITY`. `hide-github-issue-comment` accepts either a numeric REST comment ID or a GraphQL node ID. It resolves the owning issue, PR, or discussion and applies diff --git a/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts index 5296b667..c68e7233 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/github-issue-scenarios.test.ts @@ -174,7 +174,7 @@ describe("resolveGithubIssueEnv", () => { describe("registry", () => { it("registers the complete GitHub issue scenario family with unique ids", () => { const ids = githubIssueScenarios.map((s) => s.id ?? s.tool); - expect(new Set(ids).size).toBe(20); + expect(new Set(ids).size).toBe(21); expect(ids).toEqual([ "create-github-issue", "create-github-issue-label-denied", @@ -182,6 +182,7 @@ describe("registry", () => { "set-github-issue-type-clear", "create-github-issue-temporary-id-handoff", "comment-on-github-issue", + "comment-on-github-issue-hide-older", "hide-github-issue-comment", "add-github-issue-labels", "remove-github-issue-labels", diff --git a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts index 5ada7cf8..93ae55a1 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts @@ -37,6 +37,7 @@ describe("scenario registry", () => { "set-github-issue-type-clear", "create-github-issue-temporary-id-handoff", "comment-on-github-issue", + "comment-on-github-issue-hide-older", "hide-github-issue-comment", "add-github-issue-labels", "remove-github-issue-labels", diff --git a/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts b/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts index a4d712ce..4bf22515 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/github-issue.ts @@ -674,6 +674,108 @@ export const commentOnGithubIssue: Scenario = { .run(), }; +interface HideOlderCommentState extends MutationIssueState { + olderCommentId: number; + olderCommentNodeId: string; + definitionId: string; +} + +export const commentOnGithubIssueHideOlder: Scenario = { + id: "comment-on-github-issue-hide-older", + tool: "comment-on-github-issue", + config: (_ctx, state) => + mutationConfig(state, { + "hide-older-comments": true, + "allowed-reasons": ["OUTDATED"], + }), + setup: async (ctx) => { + const id = "comment-on-github-issue-hide-older"; + const env = resolveGithubIssueEnv(id); + await requireIssueWrite(env, id); + await requireGraphqlFeature(env, id, [["Mutation", "minimizeComment"]]); + const state = await seedMutationIssue(ctx, env, id); + const definitionId = "2079"; + try { + const older = await createIssueComment( + env.gh, + state.issueNumber, + [ + detBody(ctx, "comment-on-github-issue-hide-older-previous"), + ``, + ].join("\n\n"), + ); + return { + ...state, + olderCommentId: older.id, + olderCommentNodeId: older.nodeId, + definitionId, + }; + } catch (err) { + await closeMutationIssue(state).catch(() => {}); + throw err; + } + }, + ndjson: async (ctx, state) => ({ + issue_number: state.issueNumber, + body: detBody(ctx, "comment-on-github-issue-hide-older-replacement"), + }), + env: async (_ctx, state) => ({ + ...executeEnv(state), + SYSTEM_DEFINITIONID: state.definitionId, + }), + assert: async (ctx, state, record) => { + const minimized = await getCommentMinimization( + state.gh, + state.olderCommentNodeId, + ); + if (!minimized.isMinimized) { + throw new Error(`older comment ${state.olderCommentId} was not minimized`); + } + if (minimized.reason?.toUpperCase() !== "OUTDATED") { + throw new Error( + `older comment ${state.olderCommentId} has unexpected minimized reason '${minimized.reason ?? ""}'`, + ); + } + + const expected = detBody( + ctx, + "comment-on-github-issue-hide-older-replacement", + ); + const replacement = ( + await listIssueComments(state.gh, state.issueNumber) + ).find((comment) => comment.body.includes(expected)); + if (!replacement) { + throw new Error( + `issue #${state.issueNumber} has no replacement executor comment`, + ); + } + state.commentId = replacement.id; + const expectedMarker = ``; + if (!replacement.body.includes(expectedMarker)) { + throw new Error("replacement comment is missing the pipeline marker"); + } + if (record.result?.hidden_older_comments !== 1) { + throw new Error( + `executor reported hidden_older_comments=${String(record.result?.hidden_older_comments)}`, + ); + } + }, + cleanup: async (ctx, state) => + new Teardown() + .add("delete replacement comment", () => + deleteMatchingComments( + ctx, + state, + "comment-on-github-issue-hide-older-replacement", + ), + ) + .add("delete older comment", () => + deleteIssueComment(state.gh, state.olderCommentId), + ) + .add("close scratch issue", () => closeMutationIssue(state)) + .run(), +}; + interface HiddenCommentState extends MutationIssueState { commentNodeId: string; } @@ -1240,6 +1342,7 @@ export const githubIssueScenarios: Scenario[] = [ setGithubIssueTypeClear, createGithubIssueTemporaryIdHandoff, commentOnGithubIssue, + commentOnGithubIssueHideOlder, hideGithubIssueComment, addGithubIssueLabels, removeGithubIssueLabels, diff --git a/scripts/ado-script/src/github-app-token/__tests__/index.test.ts b/scripts/ado-script/src/github-app-token/__tests__/index.test.ts index 99401b79..5ffb33df 100644 --- a/scripts/ado-script/src/github-app-token/__tests__/index.test.ts +++ b/scripts/ado-script/src/github-app-token/__tests__/index.test.ts @@ -6,7 +6,7 @@ import { parseArgs, parsePermissions, parseRepositories, - resolveInstallationId, + resolveInstallation, mintInstallationToken, revoke, main, @@ -161,16 +161,20 @@ describe("parseRepositories", () => { }); }); -describe("resolveInstallationId", () => { - it("returns the id from the org endpoint when it succeeds", async () => { - const fetchFn = vi.fn().mockResolvedValueOnce(jsonResponse(200, { id: 42 })); - const id = await resolveInstallationId( +describe("resolveInstallation", () => { + it("returns installation metadata from the org endpoint", async () => { + const fetchFn = vi + .fn() + .mockResolvedValueOnce( + jsonResponse(200, { id: 42, app_slug: "ado-aw-app" }), + ); + const installation = await resolveInstallation( fetchFn as never, "https://api.github.com", "jwt", "octo-org", ); - expect(id).toBe(42); + expect(installation).toEqual({ id: 42, appSlug: "ado-aw-app" }); expect(fetchFn).toHaveBeenCalledTimes(1); expect(fetchFn.mock.calls[0]![0]).toContain("/orgs/octo-org/installation"); }); @@ -180,13 +184,13 @@ describe("resolveInstallationId", () => { .fn() .mockResolvedValueOnce(jsonResponse(404, "not found")) .mockResolvedValueOnce(jsonResponse(200, { id: 7 })); - const id = await resolveInstallationId( + const installation = await resolveInstallation( fetchFn as never, "https://api.github.com", "jwt", "octo-user", ); - expect(id).toBe(7); + expect(installation).toEqual({ id: 7, appSlug: undefined }); expect(fetchFn).toHaveBeenCalledTimes(2); expect(fetchFn.mock.calls[1]![0]).toContain("/users/octo-user/installation"); }); @@ -196,7 +200,7 @@ describe("resolveInstallationId", () => { .fn() .mockResolvedValue(jsonResponse(404, "nope")); await expect( - resolveInstallationId( + resolveInstallation( fetchFn as never, "https://api.github.com", "jwt", @@ -284,6 +288,8 @@ describe("parseArgs", () => { "octo-org", "--output-var", "GITHUB_APP_TOKEN", + "--actor-output-var", + "GITHUB_APP_ACTOR_LOGIN", "--repositories", "repo-a repo-b", "--permissions-json", @@ -295,6 +301,7 @@ describe("parseArgs", () => { appId: "1234567", owner: "octo-org", outputVar: "GITHUB_APP_TOKEN", + actorOutputVar: "GITHUB_APP_ACTOR_LOGIN", repositories: "repo-a repo-b", permissionsJson: '{"issues":"write"}', apiUrl: "https://ghe.example.com/api/v3", @@ -371,6 +378,74 @@ describe("main", () => { }); }); + it("emits the App bot login when --actor-output-var is set", async () => { + const { privateKey } = makeKeyPair(); + const fetchFn = vi + .fn() + .mockResolvedValueOnce( + jsonResponse(200, { id: 55, app_slug: "ado-aw-app" }), + ) + .mockResolvedValueOnce(jsonResponse(201, { token: "ghs_minted" })); + const writes: string[] = []; + const spy = vi + .spyOn(process.stdout, "write") + .mockImplementation((chunk: string | Uint8Array): boolean => { + writes.push(chunk.toString()); + return true; + }); + + const rc = await main( + { + appId: "123", + owner: "octo-org", + outputVar: "GITHUB_APP_TOKEN", + actorOutputVar: "GITHUB_APP_ACTOR_LOGIN", + }, + { GH_APP_PRIVATE_KEY: privateKey } as NodeJS.ProcessEnv, + fetchFn as never, + ); + spy.mockRestore(); + + expect(rc).toBe(0); + const out = writes.join(""); + expect(out).toContain( + "##vso[task.setvariable variable=GITHUB_APP_TOKEN;issecret=true]ghs_minted", + ); + expect(out).toContain( + "##vso[task.setvariable variable=GITHUB_APP_ACTOR_LOGIN]ado-aw-app[bot]", + ); + }); + + it("fails before minting when actor output is requested without app_slug", async () => { + const { privateKey } = makeKeyPair(); + const fetchFn = vi + .fn() + .mockResolvedValueOnce(jsonResponse(200, { id: 55 })); + const writes: string[] = []; + const spy = vi + .spyOn(process.stdout, "write") + .mockImplementation((chunk: string | Uint8Array): boolean => { + writes.push(chunk.toString()); + return true; + }); + + const rc = await main( + { + appId: "123", + owner: "octo-org", + actorOutputVar: "GITHUB_APP_ACTOR_LOGIN", + }, + { GH_APP_PRIVATE_KEY: privateKey } as NodeJS.ProcessEnv, + fetchFn as never, + ); + spy.mockRestore(); + + expect(rc).toBe(1); + expect(fetchFn).toHaveBeenCalledTimes(1); + expect(writes.join("")).toContain("returned no app_slug"); + expect(writes.join("")).not.toContain("issecret=true"); + }); + it("honours --api-url and --output-var", async () => { const { privateKey } = makeKeyPair(); const fetchFn = vi diff --git a/scripts/ado-script/src/github-app-token/index.ts b/scripts/ado-script/src/github-app-token/index.ts index 2a7bdd59..95c5a6c5 100644 --- a/scripts/ado-script/src/github-app-token/index.ts +++ b/scripts/ado-script/src/github-app-token/index.ts @@ -9,7 +9,7 @@ * Flow: * 1. Build a short-lived RS256 JWT signed with the App private key * (`node:crypto` — no `openssl`, no npm dep). - * 2. Resolve the installation ID for the configured owner + * 2. Resolve the installation metadata for the configured owner * (`GET /orgs/{owner}/installation`, falling back to * `GET /users/{owner}/installation`). * 3. Exchange the JWT for an installation access token @@ -33,6 +33,7 @@ * * Mint: node github-app-token.js \ * --app-id --owner --output-var \ + * [--actor-output-var ] \ * [--repositories "a b"] [--permissions-json '{"issues":"write"}'] \ * [--api-url https://host/api/v3] * env: GH_APP_PRIVATE_KEY (required, secret) @@ -42,13 +43,20 @@ * * Flags: `--app-id` the GitHub App ID; `--owner` installation owner (org/user); * `--output-var` the masked variable name to set (compiler-pinned, defaults to - * `GITHUB_APP_TOKEN`); `--repositories` space/comma-separated repo names to - * scope the token to; `--api-url` API base URL (default `https://api.github.com`, + * `GITHUB_APP_TOKEN`); `--actor-output-var` the optional non-secret App bot + * login variable; `--repositories` space/comma-separated repo names to scope + * the token to; `--api-url` API base URL (default `https://api.github.com`, * GHES uses `https:///api/v3`). */ import { createSign } from "node:crypto"; -import { logError, logInfo, logWarning, setSecretVar } from "../shared/vso-logger.js"; +import { + logError, + logInfo, + logWarning, + setSecretVar, + setVar, +} from "../shared/vso-logger.js"; const DEFAULT_API_URL = "https://api.github.com"; const DEFAULT_OUTPUT_VAR = "GITHUB_APP_TOKEN"; @@ -201,16 +209,21 @@ function ghHeaders(bearer: string): Record { } /** - * Resolve the installation ID for `owner`. Tries the org endpoint first, then - * the user endpoint (GitHub App installations exist on either an org or a + * Resolve the installation metadata for `owner`. Tries the org endpoint first, + * then the user endpoint (GitHub App installations exist on either an org or a * user account). */ -export async function resolveInstallationId( +export interface GithubInstallation { + id: number; + appSlug?: string; +} + +export async function resolveInstallation( fetchFn: FetchLike, apiUrl: string, jwt: string, owner: string, -): Promise { +): Promise { const candidates = [ `${apiUrl}/orgs/${encodeURIComponent(owner)}/installation`, `${apiUrl}/users/${encodeURIComponent(owner)}/installation`, @@ -223,9 +236,16 @@ export async function resolveInstallationId( headers: ghHeaders(jwt), }); if (resp.ok) { - const data = (await resp.json()) as { id?: number }; + const data = (await resp.json()) as { + id?: number; + app_slug?: string; + }; if (typeof data.id === "number") { - return data.id; + return { + id: data.id, + appSlug: + typeof data.app_slug === "string" ? data.app_slug.trim() : undefined, + }; } throw new Error( `installation lookup for '${owner}' returned no numeric id`, @@ -309,6 +329,7 @@ export interface CliArgs { appId?: string; owner?: string; outputVar?: string; + actorOutputVar?: string; repositories?: string; permissionsJson?: string; apiUrl?: string; @@ -343,6 +364,9 @@ export function parseArgs(argv: string[]): CliArgs { case "--output-var": if (value !== undefined) out.outputVar = value; break; + case "--actor-output-var": + if (value !== undefined) out.actorOutputVar = value; + break; case "--repositories": if (value !== undefined) out.repositories = value; break; @@ -427,19 +451,34 @@ export async function main( args.outputVar && args.outputVar.length > 0 ? args.outputVar : DEFAULT_OUTPUT_VAR; + const actorOutputVar = + args.actorOutputVar === undefined + ? undefined + : requireArg(args.actorOutputVar, "--actor-output-var"); const jwt = buildAppJwt(appId, privateKey); - const installationId = await resolveInstallationId( + const installation = await resolveInstallation( fetchFn, apiUrl, jwt, owner, ); + const actorLogin = + actorOutputVar === undefined + ? undefined + : (() => { + if (!installation.appSlug) { + throw new Error( + `installation lookup for '${owner}' returned no app_slug required by --actor-output-var`, + ); + } + return `${installation.appSlug}[bot]`; + })(); const token = await mintInstallationToken( fetchFn, apiUrl, jwt, - installationId, + installation.id, repositories, permissions, ); @@ -447,9 +486,12 @@ export async function main( // Mask + expose to the same-job consumer. Emitting the secret BEFORE // any log line that could contain it keeps ADO's scrubber ahead of leaks. setSecretVar(outputVar, token); + if (actorOutputVar && actorLogin) { + setVar(actorOutputVar, actorLogin); + } logInfo( `[github-app-token] minted installation token for owner '${owner}' ` + - `(installation ${installationId}, ${ + `(installation ${installation.id}, ${ repositories.length > 0 ? `${repositories.length} repo(s)` : "all repos" diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 79a99fb7..f3a32b18 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -2492,13 +2492,26 @@ fn build_safeoutputs_job( &repos, )); } + let github_actor_required = variant + .github_issue_tools + .iter() + .any(|tool| tool == "comment-on-github-issue") + && front_matter + .comment_on_github_issue_config()? + .is_some_and(|config| config.hide_older_comments); if let Some(app) = github_app { let permissions = front_matter.github_app_permissions_for_tools(&variant.github_issue_tools)?; + let actor_output_var = if github_actor_required { + Some(crate::compile::types::SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN_VAR) + } else { + None + }; steps.push( super::extensions::ado_script::github_app_token_step_typed_for( app, crate::compile::types::SAFE_OUTPUTS_GITHUB_APP_TOKEN_VAR, + actor_output_var, "Mint GitHub App token (SafeOutputs)", &permissions, )?, @@ -2510,6 +2523,7 @@ fn build_safeoutputs_job( .as_ref() .and_then(|permissions| permissions.write.as_deref()), github_auth.as_ref(), + github_actor_required, ); let resolved_config_path = "$(Agent.TempDirectory)/ado-aw-resolved-config.json"; steps.push(Step::Bash(write_custom_runtime_config_step( diff --git a/src/compile/common.rs b/src/compile/common.rs index 9a1f51ab..481a6f1f 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -2540,6 +2540,7 @@ pub fn generate_acquire_ado_token(service_connection: Option<&str>, variable_nam pub fn generate_executor_ado_env( write_service_connection: Option<&str>, github_auth: Option<&crate::compile::types::GithubSafeOutputsAuth>, + github_actor_required: bool, ) -> String { let mut lines: Vec = Vec::new(); // Select the ADO bearer via the shared `token_source_for` helper so the @@ -2551,6 +2552,15 @@ pub fn generate_executor_ado_env( "ADO_AW_GITHUB_TOKEN: $({})", github_auth.executor_token_var() )); + if github_actor_required + && let Some(actor_var) = github_auth.executor_actor_var() + { + lines.push(format!( + "{}: $({})", + crate::compile::types::SAFE_OUTPUTS_GITHUB_ACTOR_LOGIN_ENV, + actor_var + )); + } let api_url = serde_json::to_string(github_auth.api_url()) .expect("serializing a validated GitHub API URL cannot fail"); lines.push(format!("ADO_AW_GITHUB_API_URL: {api_url}")); @@ -6231,7 +6241,7 @@ safe-outputs: #[test] fn test_generate_executor_ado_env_with_connection() { - let result = generate_executor_ado_env(Some("my-sc"), None); + let result = generate_executor_ado_env(Some("my-sc"), None, false); assert!( result.contains("env:"), "Executor env block should include the 'env:' key" @@ -6257,7 +6267,7 @@ safe-outputs: #[test] fn test_generate_executor_ado_env_none_uses_system_access_token() { - let result = generate_executor_ado_env(None, None); + let result = generate_executor_ado_env(None, None, false); assert!( result.starts_with("env:\n"), "Should always emit env: block (executor needs SYSTEM_ACCESSTOKEN)" @@ -6282,7 +6292,7 @@ safe-outputs: variable: "MY_GITHUB_WRITE_TOKEN".to_string(), api_url: "https://api.github.com".to_string(), }; - let result = generate_executor_ado_env(None, Some(&auth)); + let result = generate_executor_ado_env(None, Some(&auth), false); assert!(result.starts_with("env:\n"), "Should emit env: block"); assert!( result.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), @@ -6308,7 +6318,7 @@ safe-outputs: variable: "ADO_AW_GITHUB_TOKEN".to_string(), api_url: "https://api.github.com".to_string(), }; - let result = generate_executor_ado_env(Some("write-sc"), Some(&auth)); + let result = generate_executor_ado_env(Some("write-sc"), Some(&auth), false); assert!(result.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)")); assert!(result.contains("ADO_AW_GITHUB_TOKEN: $(ADO_AW_GITHUB_TOKEN)")); assert!( @@ -6317,6 +6327,22 @@ safe-outputs: ); } + #[test] + fn test_generate_executor_ado_env_with_github_app_actor() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-app:\n app-id: 123\n owner: octo\n create-github-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let auth = fm.github_safe_outputs_auth().unwrap().unwrap(); + let result = generate_executor_ado_env(None, Some(&auth), true); + assert!(result.contains("ADO_AW_GITHUB_TOKEN: $(ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN)")); + assert!( + result.contains( + "ADO_AW_GITHUB_ACTOR_LOGIN: $(ADO_AW_SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN)" + ) + ); + } + // ─── Security validation tests ──────────────────────────────────────────── #[test] diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index 34846faf..a559b6f8 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -712,6 +712,7 @@ pub fn github_app_token_step_typed( github_app_token_step_typed_for( cfg, crate::engine::GITHUB_APP_TOKEN_VAR, + None, "Mint GitHub App token (Copilot engine auth)", &cfg.permissions, ) @@ -720,6 +721,7 @@ pub fn github_app_token_step_typed( pub fn github_app_token_step_typed_for( cfg: &crate::compile::types::GithubAppTokenConfig, output_var: &str, + actor_output_var: Option<&str>, display_name: &str, permissions: &std::collections::BTreeMap< String, @@ -739,6 +741,12 @@ pub fn github_app_token_step_typed_for( sh_single_quote(output_var) ), ]; + if let Some(actor_output_var) = actor_output_var { + args.push(format!( + "--actor-output-var {}", + sh_single_quote(actor_output_var) + )); + } if !cfg.repositories.is_empty() { args.push(format!( "--repositories {}", diff --git a/src/compile/types.rs b/src/compile/types.rs index 00b81388..4bceb19f 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -660,6 +660,9 @@ pub struct GithubAppTokenConfig { pub const DEFAULT_SAFE_OUTPUTS_GITHUB_TOKEN_VAR: &str = "ADO_AW_GITHUB_TOKEN"; pub const SAFE_OUTPUTS_GITHUB_APP_TOKEN_VAR: &str = "ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN"; +pub const SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN_VAR: &str = + "ADO_AW_SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN"; +pub const SAFE_OUTPUTS_GITHUB_ACTOR_LOGIN_ENV: &str = "ADO_AW_GITHUB_ACTOR_LOGIN"; /// Canonical GitHub issue-family safe-output names. /// @@ -738,6 +741,13 @@ impl GithubSafeOutputsAuth { } } + pub fn executor_actor_var(&self) -> Option<&str> { + match self { + Self::Token { .. } => None, + Self::App { .. } => Some(SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN_VAR), + } + } + pub fn api_url(&self) -> &str { match self { Self::Token { api_url, .. } => api_url, diff --git a/src/safe_outputs/comment_on_github_issue.rs b/src/safe_outputs/comment_on_github_issue.rs index cdf02a67..4c7b8eca 100644 --- a/src/safe_outputs/comment_on_github_issue.rs +++ b/src/safe_outputs/comment_on_github_issue.rs @@ -239,9 +239,21 @@ impl Executor for CommentOnGithubIssueResult { let mut older_node_ids = Vec::new(); if config.hide_older_comments { - let authenticated = match client.authenticated_comment_actor().await? { - Ok(user) => user, - Err(error) => return Ok(ExecutionResult::failure(error.to_string())), + let authenticated = match ctx.github_actor_login.as_deref() { + Some(login) if !login.trim().is_empty() => GithubUser { + login: login.trim().to_string(), + id: None, + node_id: None, + }, + Some(_) => { + return Ok(ExecutionResult::failure( + "ADO_AW_GITHUB_ACTOR_LOGIN is empty; cannot safely identify older GitHub App comments", + )); + } + None => match client.authenticated_user().await? { + Ok(user) => user, + Err(error) => return Ok(ExecutionResult::failure(error.to_string())), + }, }; let comments = match client .list_issue_comments(&target.repository, target.number) @@ -725,7 +737,7 @@ max: 2 } #[tokio::test] - async fn hide_older_derives_app_bot_identity_without_user_endpoint() { + async fn hide_older_uses_mint_derived_app_actor_without_discovery() { let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/repos/octo/repo/issues/7")) @@ -733,22 +745,6 @@ max: 2 .expect(1) .mount(&server) .await; - Mock::given(method("GET")) - .and(path("/user")) - .respond_with(ResponseTemplate::new(403).set_body_json(serde_json::json!({ - "message": "Resource not accessible by integration" - }))) - .expect(1) - .mount(&server) - .await; - Mock::given(method("GET")) - .and(path("/installation")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "app_slug": "ado-aw-app" - }))) - .expect(1) - .mount(&server) - .await; Mock::given(method("GET")) .and(path("/repos/octo/repo/issues/7/comments")) .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!([ @@ -793,13 +789,14 @@ max: 2 .mount(&server) .await; - let ctx = context( + let mut ctx = context( &server, serde_json::json!({ "target-repo": "octo/repo", "hide-older-comments": true }), ); + ctx.github_actor_login = Some("ado-aw-app[bot]".to_string()); let mut result = make_result(GithubIssueNumber::Number(7)); let execution = result.execute_sanitized(&ctx).await.unwrap(); assert!(execution.success, "{}", execution.message); @@ -807,6 +804,44 @@ max: 2 execution.data.as_ref().unwrap()["hidden_older_comments"], serde_json::json!(1) ); + assert!( + server + .received_requests() + .await + .unwrap() + .iter() + .all(|request| { + request.url.path() != "/user" && request.url.path() != "/installation" + }) + ); + } + + #[tokio::test] + async fn hide_older_rejects_empty_app_actor_before_comment_writes() { + let server = MockServer::start().await; + Mock::given(method("GET")) + .and(path("/repos/octo/repo/issues/7")) + .respond_with(ResponseTemplate::new(200).set_body_json(issue_json(7, false))) + .expect(1) + .mount(&server) + .await; + let mut ctx = context( + &server, + serde_json::json!({ + "target-repo": "octo/repo", + "hide-older-comments": true + }), + ); + ctx.github_actor_login = Some(" ".to_string()); + let mut result = make_result(GithubIssueNumber::Number(7)); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!execution.success); + assert!( + execution + .message + .contains("ADO_AW_GITHUB_ACTOR_LOGIN is empty") + ); + assert_eq!(server.received_requests().await.unwrap().len(), 1); } #[tokio::test] diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 5836d411..6f34f98e 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -3303,6 +3303,7 @@ index 0000000..abcdefg ado_project_id: None, access_token: Some("fake-token".to_string()), github_token: None, + github_actor_login: None, source_directory: dir.path().to_path_buf(), self_repository_directory: dir.path().to_path_buf(), working_directory: dir.path().to_path_buf(), diff --git a/src/safe_outputs/github_api.rs b/src/safe_outputs/github_api.rs index 3d3c2ce1..0d30c2d5 100644 --- a/src/safe_outputs/github_api.rs +++ b/src/safe_outputs/github_api.rs @@ -128,11 +128,6 @@ pub struct GithubUser { pub node_id: Option, } -#[derive(Debug, Deserialize)] -struct GithubInstallation { - app_slug: String, -} - /// Minimal milestone metadata needed by milestone assignment. #[derive(Debug, Clone, PartialEq, Eq, Deserialize)] pub struct GithubMilestone { @@ -281,52 +276,6 @@ impl GithubClient { Ok(response.json("Failed to parse authenticated GitHub user")) } - /// Resolve the actor identity used for issue comments. - /// - /// User/PAT tokens expose `GET /user`. Installation tokens do not, so on - /// the installation-token 403 path derive the bot login from - /// `GET /installation` instead. The caller can then compare the exact actor - /// login and avoid minimizing comments written by a different actor. - pub async fn authenticated_comment_actor( - &self, - ) -> anyhow::Result> { - let response = self.send(Method::GET, self.route(&["user"])?, None).await?; - if response.is_success() { - return Ok(response.json("Failed to parse authenticated GitHub user")); - } - if response.status != StatusCode::FORBIDDEN { - return Ok(Err(GithubApiError::from_response( - "Failed to fetch authenticated GitHub user", - response, - ))); - } - - let response = self - .send(Method::GET, self.route(&["installation"])?, None) - .await?; - let response = match response.require_success("Failed to fetch GitHub App installation") { - Ok(response) => response, - Err(error) => return Ok(Err(error)), - }; - let installation: GithubInstallation = - match response.json("Failed to parse GitHub App installation") { - Ok(installation) => installation, - Err(error) => return Ok(Err(error)), - }; - if installation.app_slug.trim().is_empty() { - return Ok(Err(GithubApiError { - operation: "Failed to parse GitHub App installation".to_string(), - status: Some(response.status), - message: "GitHub App installation contained no app_slug".to_string(), - })); - } - Ok(Ok(GithubUser { - login: format!("{}[bot]", installation.app_slug), - id: None, - node_id: None, - })) - } - pub async fn graphql( &self, operation: &str, @@ -751,7 +700,7 @@ mod tests { } #[tokio::test] - async fn derives_comment_actor_from_app_installation() { + async fn authenticated_user_rejects_installation_token_without_fallback() { let server = MockServer::start().await; Mock::given(method("GET")) .and(path("/user")) @@ -761,19 +710,10 @@ mod tests { .expect(1) .mount(&server) .await; - Mock::given(method("GET")) - .and(path("/installation")) - .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({ - "app_slug": "ado-aw-app" - }))) - .expect(1) - .mount(&server) - .await; let client = GithubClient::new(&server.uri(), "installation-token").unwrap(); - let actor = client.authenticated_comment_actor().await.unwrap().unwrap(); - assert_eq!(actor.login, "ado-aw-app[bot]"); - assert_eq!(actor.id, None); - assert_eq!(actor.node_id, None); + let error = client.authenticated_user().await.unwrap().unwrap_err(); + assert_eq!(error.status, Some(StatusCode::FORBIDDEN)); + assert_eq!(server.received_requests().await.unwrap().len(), 1); } #[tokio::test] diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 14dee953..e4721b11 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -93,6 +93,10 @@ pub struct ExecutionContext { pub access_token: Option, /// GitHub credential used by GitHub safe outputs in Stage 3. pub github_token: Option, + /// Authenticated GitHub App bot login captured while minting the Stage 3 + /// installation token. PAT-backed execution leaves this unset and resolves + /// the authenticated user through `GET /user`. + pub github_actor_login: Option, /// GitHub REST API base URL for Stage 3 issue calls. pub github_api_url: String, /// Working directory for file operations (safe outputs directory) @@ -406,6 +410,7 @@ impl ExecutionContext { ado_project_id: env("SYSTEM_TEAMPROJECTID"), access_token: env("SYSTEM_ACCESSTOKEN").or_else(|| env("AZURE_DEVOPS_EXT_PAT")), github_token: env("ADO_AW_GITHUB_TOKEN"), + github_actor_login: env("ADO_AW_GITHUB_ACTOR_LOGIN"), github_api_url: env("ADO_AW_GITHUB_API_URL") .unwrap_or_else(|| "https://api.github.com".to_string()), working_directory: std::env::current_dir().unwrap_or_default(), @@ -1120,6 +1125,20 @@ mod tests { assert_eq!(ctx.source_version.as_deref(), Some("abc1234")); } + #[test] + fn test_from_env_lookup_populates_github_actor_login() { + let ctx = ExecutionContext::from_env_lookup(env_from(&[( + "ADO_AW_GITHUB_ACTOR_LOGIN", + "ado-aw-app[bot]", + )])); + assert_eq!(ctx.github_actor_login.as_deref(), Some("ado-aw-app[bot]")); + assert!( + ExecutionContext::from_env_lookup(env_from(&[])) + .github_actor_login + .is_none() + ); + } + #[test] fn test_from_env_lookup_populates_checkout_directories() { let ctx = ExecutionContext::from_env_lookup(env_from(&[ diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index 7da41b0f..cfcfd4fb 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -915,6 +915,7 @@ attachment-type: "agent-artifact" ado_project_id: None, access_token: None, github_token: None, + github_actor_login: None, source_directory: working_directory.clone(), self_repository_directory: working_directory.clone(), working_directory, diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 7f3c3f8e..a20e7728 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6130,8 +6130,10 @@ fn test_compile_github_issue_app_fixture_scopes_tokens_by_stage() { )); assert!(compiled.contains("Mint GitHub App token (SafeOutputs)")); assert!(compiled.contains("--output-var 'ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN'")); + assert!(!compiled.contains("--actor-output-var")); assert!(compiled.contains("--permissions-json '{\"issues\":\"write\"}'")); assert!(compiled.contains("ADO_AW_GITHUB_TOKEN: $(ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN)")); + assert!(!compiled.contains("ADO_AW_GITHUB_ACTOR_LOGIN")); let agent_start = compiled.find("- job: Agent").expect("Agent job"); let detection_start = compiled.find("- job: Detection").expect("Detection job"); @@ -6144,6 +6146,8 @@ fn test_compile_github_issue_app_fixture_scopes_tokens_by_stage() { ] { assert!(block.contains("--permissions-json '{\"contents\":\"read\",\"issues\":\"read\"}'")); assert!(!block.contains("ADO_AW_GITHUB_TOKEN")); + assert!(!block.contains("ADO_AW_GITHUB_ACTOR_LOGIN")); + assert!(!block.contains("--actor-output-var")); } } @@ -6183,12 +6187,43 @@ Create a reviewed GitHub issue. assert!(!automatic.contains("Mint GitHub App token (SafeOutputs)")); assert!(!automatic.contains("ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN")); + assert!(!automatic.contains("ADO_AW_SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN")); assert!(reviewed.contains("Mint GitHub App token (SafeOutputs)")); + assert!(!reviewed.contains("--actor-output-var")); + assert!(!reviewed.contains("ADO_AW_GITHUB_ACTOR_LOGIN")); assert!(reviewed.contains("--repositories 'reviewed-repo'")); assert!(reviewed.contains("--permissions-json '{\"issues\":\"write\"}'")); assert!(reviewed.contains("Revoke GitHub App token (SafeOutputs)")); } +#[test] +fn test_compile_github_app_hide_older_wires_actor_identity() { + let compiled = compile_inline_agent( + "github-app-hide-older", + r#"--- +name: "GitHub App Hide Older" +description: "GitHub App actor identity is scoped to comment replacement" +engine: copilot +safe-outputs: + github-app: + app-id: 1234567 + owner: octo-org + comment-on-github-issue: + target-repo: octo-org/octo-repo + hide-older-comments: true +--- + +Replace the managed issue comment. +"#, + ); + assert!( + compiled.contains("--actor-output-var 'ADO_AW_SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN'") + ); + assert!(compiled.contains( + "ADO_AW_GITHUB_ACTOR_LOGIN: $(ADO_AW_SAFE_OUTPUTS_GITHUB_APP_ACTOR_LOGIN)" + )); +} + /// The example file in `examples/dogfood-failure-reporter.md` must compile /// cleanly. Mirror of the structural smoke test for `examples/sample-agent.md`. #[test] diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index e10c0950..b5d8427f 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -65,10 +65,16 @@ All deterministically-assertable ADO-write safe outputs plus the flagship scenario supplies only `ADO_AW_SELF_REPOSITORY_NAME` — matching what the compiler emits — so it also proves the executor resolves a repository from its name alone. -- **GitHub issues:** `create-github-issue`, `set-github-issue-type`, and the - same-run `temporary_id` handoff between them. These are the only scenarios - that assert against **GitHub** rather than ADO — see - [GitHub issue scenarios](#github-issue-scenarios) below. +- **GitHub issues:** the complete GitHub issue safe-output family, including + ordinary comment creation, `hide-github-issue-comment`, and a combined + `comment-on-github-issue` scenario with `hide-older-comments: true`. The + combined scenario uses the configured PAT to prove actor matching, + minimization, replacement creation, and result reporting. GitHub App + mint-to-executor identity wiring is covered by deterministic bundle, + compiler, and Rust integration tests rather than new live App credentials in + this pipeline. These are the only scenarios that assert against **GitHub** + rather than ADO — see [GitHub issue scenarios](#github-issue-scenarios) + below. Excluded (out of scope): none of the currently shipped safe outputs.