From db0f215015a9e42ecce88b68fe56fd6f7386b3b2 Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 25 Jul 2026 15:16:08 +0100 Subject: [PATCH 1/3] feat(safeoutputs): add GitHub issue outputs Promote GitHub issue creation to a public safe output, add native issue type updates with temporary-ID linkage, and isolate PAT/App credentials to Stage 3. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af --- AGENTS.md | 7 +- README.md | 4 +- docs/ado-aw-debug.md | 202 +------ docs/ado-script.md | 12 +- docs/codemods.md | 16 +- docs/engine.md | 29 +- docs/front-matter.md | 8 +- docs/safe-outputs.md | 185 +++++- examples/dogfood-failure-reporter.md | 4 +- .../github-app-token/__tests__/index.test.ts | 54 ++ .../ado-script/src/github-app-token/index.ts | 57 +- src/compile/agentic_pipeline.rs | 103 +++- .../0006_promote_debug_create_issue.rs | 172 ++++++ src/compile/codemods/mod.rs | 3 + src/compile/common.rs | 315 +++++----- src/compile/extensions/ado_script.rs | 86 ++- src/compile/mod.rs | 10 + src/compile/types.rs | 544 ++++++++++++++++-- src/execute.rs | 12 +- src/inspect/catalog.rs | 1 + src/main.rs | 20 - src/mcp.rs | 68 ++- src/safe_outputs/create_issue.rs | 272 +++++---- src/safe_outputs/create_pull_request.rs | 6 +- src/safe_outputs/mod.rs | 34 +- src/safe_outputs/result.rs | 91 ++- src/safe_outputs/set_issue_type.rs | 431 ++++++++++++++ src/safe_outputs/upload_build_attachment.rs | 6 +- src/secure.rs | 36 ++ tests/codemod_tests.rs | 21 + tests/compiler-smoke-e2e/README.md | 2 +- tests/compiler-smoke-e2e/REGISTERED.md | 2 +- tests/compiler_tests.rs | 42 +- tests/fixtures/ado-aw-debug-agent.md | 3 +- tests/fixtures/github-issue-app-agent.md | 29 + tests/safe-outputs/README.md | 2 +- tests/safe-outputs/REGISTERED.md | 6 +- tests/safe-outputs/smoke-failure-reporter.md | 4 +- 38 files changed, 2283 insertions(+), 616 deletions(-) create mode 100644 src/compile/codemods/0006_promote_debug_create_issue.rs create mode 100644 src/safe_outputs/set_issue_type.rs create mode 100644 tests/fixtures/github-issue-app-agent.md diff --git a/AGENTS.md b/AGENTS.md index 7ece836a..28c3b806 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -100,6 +100,7 @@ fail-closed and only pauses when the agent actually proposed a reviewed output. │ │ │ ├── 0003_flatten_work_item_config.rs # Legacy work-item config flatten codemod │ │ │ ├── 0004_legacy_path_markers.rs # Migrate {{ workspace }}/{{ working_directory }}/{{ trigger_repo_directory }} markers → explicit ADO path exprs (resolved from workspace:/repos:) │ │ │ ├── 0005_drop_build_attachment_allowed_build_ids.rs # Remove inert safe-outputs.upload-build-attachment.allowed-build-ids key (build attachments are current-run only) +│ │ │ ├── 0006_promote_debug_create_issue.rs # Move legacy ado-aw-debug.create-issue into public safe-outputs.create-issue with auth bridge │ │ │ └── helpers.rs # take_key, insert_no_overwrite, rename_key, ConflictPolicy │ │ ├── codemod_integration_test.rs # White-box rewrite-path tests (stub registry injection) │ │ ├── types.rs # Front matter grammar and types @@ -337,9 +338,9 @@ index to jump to the right page. `$(System.AccessToken)` semantics, the "Limit job authorization scope to current project" toggle, permission-bitmask decoder, REST recipe for inspecting ACEs, and the three fix paths. -- [`docs/ado-aw-debug.md`](docs/ado-aw-debug.md) — debug-only `ado-aw-debug:` - front-matter section (`skip-integrity`, `create-issue` for filing GitHub - issues from dogfood pipelines). NOT a regular safe-output. +- [`docs/ado-aw-debug.md`](docs/ado-aw-debug.md) — debug-only + `ado-aw-debug.skip-integrity` front-matter control. GitHub issue filing is a + regular safe output. - [`docs/supply-chain.md`](docs/supply-chain.md) — optional `supply-chain:` front-matter section that mirrors the compiler, AWF binary, ado-script bundle, and AWF/MCPG images from an internal Azure DevOps Artifacts feed, diff --git a/README.md b/README.md index b00bfe42..b09d0ab2 100644 --- a/README.md +++ b/README.md @@ -731,8 +731,8 @@ index to jump to the right page. - [`docs/supply-chain.md`](docs/supply-chain.md) — optional `supply-chain:` front-matter section for internal feed/registry mirrors and exact provenance-checked pipeline artifacts. -- [`docs/ado-aw-debug.md`](docs/ado-aw-debug.md) — debug-only `ado-aw-debug:` - front-matter section (`skip-integrity`, `create-issue`). +- [`docs/ado-aw-debug.md`](docs/ado-aw-debug.md) - debug-only + `ado-aw-debug.skip-integrity` front-matter control. **Compiler internals & operations** diff --git a/docs/ado-aw-debug.md b/docs/ado-aw-debug.md index 617dd8ad..a39c8bff 100644 --- a/docs/ado-aw-debug.md +++ b/docs/ado-aw-debug.md @@ -1,195 +1,49 @@ -# `ado-aw-debug:` — Debug-only front-matter section +# `ado-aw-debug:` - Debug-only front-matter section _Part of the [ado-aw documentation](../AGENTS.md)._ -> ⚠️ **This section is for dogfood pipelines only.** -> Anything declared under `ado-aw-debug:` is **not** part of the regular -> agent surface and is not recommended for general use. Knobs here exist so -> the team can validate `githubnext/ado-aw` changes against real Azure -> DevOps pipelines and file failures back to GitHub for triage. Each knob -> bypasses or weakens a normal safety control. +> This section is for compiler dogfood and local diagnostics. It is not a +> regular safe-output surface. -The compiler accepts a top-level `ado-aw-debug:` block in agent front -matter. Currently exposed knobs: - -| Knob | Purpose | Default | -| --- | --- | --- | -| `skip-integrity` | Omit the "Verify pipeline integrity" step from the generated YAML. OR-ed with the `--skip-integrity` CLI flag. | `false` | -| `create-issue` | Enable the [debug-only `create-issue`](#create-issue) safe output. | absent (disabled) | - -Unrecognised keys under `ado-aw-debug:` cause a compile-time error -(`#[serde(deny_unknown_fields)]`). - -## `create-issue` - -Files a GitHub issue against an operator-configured target repository. -Used to surface failures from ADO-hosted dogfood pipelines back to -`githubnext/ado-aw` for triage. - -### Why it's gated - -`create-issue` is **default-deny** at three layers: - -1. **MCP layer.** The SafeOutputs MCP server lists `create-issue` in - [`DEBUG_ONLY_TOOLS`][debug-only-tools], so the route is removed from - the tool router unless the compiler explicitly opts in via - `--enabled-tools`. -2. **Compiler layer.** `--enabled-tools create-issue` is only emitted - when `ado-aw-debug.create-issue:` is present in front matter. The - compiler also rejects `safe-outputs.create-issue:` outright, so the - tool can't be smuggled in via the regular safe-outputs surface. -3. **Executor layer.** Stage 3 maintains a separate - `ExecutionContext.debug_enabled_tools` set populated only from - `ado-aw-debug:`. The executor refuses any NDJSON `create-issue` - entry that isn't in that set, so a forged or smuggled NDJSON entry - fails closed before any token is read. - -[debug-only-tools]: ../src/safe_outputs/mod.rs - -### Front-matter schema +The compiler accepts: ```yaml ado-aw-debug: - create-issue: - target-repo: githubnext/ado-aw # REQUIRED. Operator-only; agent has no override. - title-prefix: "[pipeline-failure] " # Optional; prepended to every agent title. - labels: # Optional; static labels always applied. - - pipeline-failure - - automated - allowed-labels: # Optional; default-deny — see below. - - "agent-*" - - "pipeline-failure" - assignees: # Optional; static assignees always applied. - - "jamesdevine" - max: 3 # Optional; per-run budget. Default 1. + skip-integrity: true ``` -* **`target-repo`** is required. Format `owner/repo`. The agent has no - parameter to override it; you cannot redirect issues to a different - repository at runtime. -* **`title-prefix`** is appended at execution time. The final title length - (prefix + agent title) must be ≤ 256 characters; longer titles fail at - Stage 3. -* **`labels`** are applied unconditionally to every issue, on top of any - agent-supplied labels that pass `allowed-labels`. -* **`allowed-labels`** is **default-deny**: an empty or absent list means - **no agent-supplied labels are accepted**. To accept any agent label, - set `allowed-labels: ["*"]` explicitly. Patterns may include `*` - wildcards (e.g. `"agent-*"`). -* **Allowed-label matching is case-insensitive.** It uses the same - `tag_matches_pattern` helper as ADO tag allow-lists. GitHub labels are - case-sensitive, so `allowed-labels: ["safe"]` will also admit - `SAFE` and `Safe` — keep that in mind when modelling policy. -* **`assignees`** are merged with agent-supplied assignees. There is - intentionally no `allowed-assignees` allowlist in v1; if you need - one, configure assignees only via the static `assignees:` list and - skip the agent parameter. -* **`max`** controls per-run budget the same way it does for other - safe-output tools. +Unrecognized keys fail compilation (`#[serde(deny_unknown_fields)]`). -### Agent-supplied parameters +## `skip-integrity` -The agent calls the `create-issue` MCP tool with: +Equivalent to passing `--skip-integrity` to `ado-aw compile`. Setting either +omits the generated **Verify pipeline integrity** step. -```jsonc -{ - "title": "Pipeline failure on main", - "body": "", - "labels": ["pipeline-failure"], // optional - "assignees": ["copilot"] // optional -} -``` +The integrity step downloads the same ado-aw version used at compile time and +runs `ado-aw check` against the committed pipeline. Without it, a modified +compiled YAML file is not detected at runtime. Use this option only for +short-lived dogfood pipelines. -The MCP-side `Validate` impl rejects ADO pipeline-command sequences in -labels and assignees (see [src/safe_outputs/create_issue.rs](../src/safe_outputs/create_issue.rs)). -Stage 3 also neutralises `##vso[…]` in any error messages it produces, so -agent-supplied content cannot escape the executor's stdout. +## Retired: `ado-aw-debug.create-issue` -### Pipeline variable: `ADO_AW_DEBUG_GITHUB_TOKEN` - -Stage 3 authenticates against GitHub using the -**`ADO_AW_DEBUG_GITHUB_TOKEN`** ADO pipeline variable. The compiler emits +GitHub issue filing is now a regular safe output: ```yaml -env: - SYSTEM_ACCESSTOKEN: $(System.AccessToken) # default executor token (or $(SC_WRITE_TOKEN) if permissions.write is set) - ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN) # only when ado-aw-debug.create-issue is set +safe-outputs: + create-issue: + target-repo: githubnext/ado-aw ``` -into the executor step's `env:` block. The token is **not** exposed to -the agent in Stage 1 — the read-only `GITHUB_TOKEN` the agent sees is a -separate variable wired through `engine.env` and used only for GitHub -MCP read access. - -### Setting up the PAT +The front-matter codemod moves legacy `ado-aw-debug.create-issue` configuration +to `safe-outputs.create-issue`. When no public GitHub auth is already +configured, it also adds: -1. **Generate a fine-grained PAT** scoped to **only** `target-repo` (e.g. - `githubnext/ado-aw`). Required permissions: - * Repository access: only the target repo. - * Permissions: **Issues** = Read and write. Nothing else. -2. **Store as a secret pipeline variable** named exactly - `ADO_AW_DEBUG_GITHUB_TOKEN`. Mark it secret. Do **not** copy it into - `engine.env` or any non-secret variable. -3. **Confirm the operator-configured target-repo matches the PAT scope.** - The compiler validator only checks shape (`owner/repo`); it cannot - verify the PAT has access. If the PAT lacks Issues:write, the Stage 3 - call fails with the GitHub API error and Stage 3 reports - `succeeded with issues`. -4. `ado-aw configure` does **not** automate this variable today — set it - manually in the ADO pipeline definition. - -### Auto-footer - -Every issue gets an auto-appended traceability footer that looks like: - -```markdown - ---- -Pipeline: `dogfood-failure-reporter` -Run: -Trigger: `Manual` +```yaml +safe-outputs: + github-token: "$(ADO_AW_DEBUG_GITHUB_TOKEN)" ``` -The `` marker is stable so that future tooling can locate -the generated content without parsing prose. The footer is built from -`BUILD_BUILDID`, `BUILD_DEFINITIONNAME`, `BUILD_REASON`, -`SYSTEM_TEAMFOUNDATIONCOLLECTIONURI` and `SYSTEM_TEAMPROJECT` — these are -present whenever Stage 3 runs inside an ADO pipeline. - -If your pipeline / org / project names are sensitive, do not enable -`create-issue` against a public repo. - -### Security checklist - -- [ ] Target repo's GitHub PAT is scoped to that repo only and only has - Issues:write. -- [ ] `ADO_AW_DEBUG_GITHUB_TOKEN` is stored as a secret pipeline - variable, never hard-coded or printed. -- [ ] `allowed-labels` is set explicitly. Empty means default-deny; - `["*"]` accepts any agent label — pick deliberately. -- [ ] `target-repo` is private if the agent's prompts or pipeline - metadata are sensitive (the auto-footer publishes ADO run URLs and - pipeline names). -- [ ] `skip-integrity` is **not** enabled in pipelines triggered by - untrusted PRs. - -## `skip-integrity` - -Equivalent to passing `--skip-integrity` on the `ado-aw compile` CLI. -Setting either OR setting both omits the `Verify pipeline integrity` -step from the generated YAML. - -The integrity step downloads the same `ado-aw` binary the pipeline was -compiled with and runs `ado-aw check` against the committed pipeline -file. Without it, a tampered `*.yml` won't be caught at run time. - -Use this only for short-lived dogfood pipelines where you're iterating -on the compiler and re-compiling frequently. - -## See also - -- [`docs/safe-outputs.md`](safe-outputs.md) — regular safe-outputs - surface (`create-issue` is **not** in it). -- [`docs/cli.md`](cli.md) — `--skip-integrity` CLI flag. -- [`docs/ir.md`](ir.md) — typed pipeline IR and how debug-only choices such as - integrity-check omission are represented in generated steps. +That explicit token reference preserves the existing ADO secret during +migration; it is not a debug runtime path. New workflows should use the +`ADO_AW_GITHUB_TOKEN` default or GitHub App auth. See +[`docs/safe-outputs.md`](safe-outputs.md#github-issue-safe-outputs). diff --git a/docs/ado-script.md b/docs/ado-script.md index dbcb9021..ea728baf 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -56,11 +56,11 @@ pipeline** as runtime helpers. Today it produces thirteen bundles: review is configured), and attaches it to the build's `ado-aw-safe-outputs` summary tab via `##vso[task.uploadsummary]`. See [`safe-outputs.md`](safe-outputs.md). -- `github-app-token.js` — GitHub App token minter that runs immediately - before the Copilot invocation in the **Agent and Detection jobs** when - `engine.github-app-token` is configured. Builds an RS256 JWT from the App ID - (argv) + private key (masked env secret), resolves the installation for the - owner, exchanges it for an installation access token, and exposes it as a +- `github-app-token.js` — GitHub App token minter used by Agent/Detection and + GitHub issue SafeOutputs. Builds an RS256 JWT from the App ID (argv) + + private key (masked env secret), resolves the installation for the owner, + exchanges it for a repository/permission-scoped installation access token, + and exposes it as a masked same-job `GITHUB_APP_TOKEN`. Invoked again with a `revoke` argument after the Copilot run (best-effort) to delete the token via `DELETE /installation/token`. Compiler-owned, non-secret inputs (`--app-id`, @@ -555,7 +555,7 @@ scripts/ado-script/ │ │ ├── index.ts # main(): inspect upstream results + safe-outputs manifest → file/append work items │ │ └── __tests__/ # unit tests for signal detection and work-item filing behaviour │ ├── github-app-token/ # github-app-token.js entry point + GitHub App token minter -│ │ ├── index.ts # main(): RS256 JWT → resolve installation → mint installation token → masked GITHUB_APP_TOKEN +│ │ ├── index.ts # main(): RS256 JWT → resolve installation → mint scoped installation token → masked same-job variable │ │ └── __tests__/ # unit tests for JWT signing / installation resolution / token minting │ ├── prepare-pr-base/ # prepare-pr-base.js entry point + create-pull-request base-ref fetch/deepen │ │ ├── index.ts # main(): fetch/deepen target branch + set origin/HEAD so mcp.rs finds a diff base diff --git a/docs/codemods.md b/docs/codemods.md index e75281fa..d8fafc20 100644 --- a/docs/codemods.md +++ b/docs/codemods.md @@ -112,7 +112,8 @@ src/compile/codemods/ ├── 0002_pool_object_form.rs # Legacy scalar pool → explicit object form codemod ├── 0003_flatten_work_item_config.rs # Legacy work-item config flatten codemod ├── 0004_legacy_path_markers.rs # {{ workspace }} / {{ working_directory }} / {{ trigger_repo_directory }} → explicit ADO path exprs -└── 0005_drop_build_attachment_allowed_build_ids.rs # remove no-op upload-build-attachment.allowed-build-ids +├── 0005_drop_build_attachment_allowed_build_ids.rs # remove no-op upload-build-attachment.allowed-build-ids +└── 0006_promote_debug_create_issue.rs # move debug GitHub issue filing to regular safe-outputs ``` (New codemods are appended as `_.rs` files.) @@ -408,6 +409,19 @@ standard codemod compile warning tells the author it was auto-removed. It does stays meaningful (pipeline artifacts use the Build Artifacts `Create` API, which can target an arbitrary build). +## GitHub `create-issue` promotion (`0006_promote_debug_create_issue`) + +The dogfood-only `ado-aw-debug.create-issue` surface was retired when GitHub +issue filing became a regular safe output. The codemod moves the configuration +to `safe-outputs.create-issue`, preserves other debug fields such as +`skip-integrity`, and removes an empty `ado-aw-debug` mapping. + +If no public GitHub auth is already present, it adds +`safe-outputs.github-token: "$(ADO_AW_DEBUG_GITHUB_TOKEN)"` so an existing ADO +secret continues to work through the normal Stage 3 auth path. When both old +and new `create-issue` keys exist, the codemod fails with a manual-migration +error rather than overwriting either value. + ## Tests The codemod framework is covered by three layers of tests: diff --git a/docs/engine.md b/docs/engine.md index e8f8491f..9136705d 100644 --- a/docs/engine.md +++ b/docs/engine.md @@ -60,6 +60,9 @@ engine: app-id: 1234567 # literal App ID or client ID (required) owner: octo-org # installation owner (org or user login) repositories: [octo-repo] # optional; scopes the token to these repos + permissions: # optional normally; required for safe-output reuse + contents: read + issues: read # api-url: https://ghe.example.com/api/v3 # optional; GHES base URL # skip-token-revocation: false # optional; revoke by default # private-key: MY_SECRET_VAR # optional override; see below @@ -79,6 +82,7 @@ ado-aw secrets set GITHUB_APP_PRIVATE_KEY "$(cat app-private-key.pem)" | `api-url` | no | GitHub API base URL. Defaults to `https://api.github.com` (GHEC). For GitHub Enterprise Server, set the `/api/v3` base URL (e.g. `https://ghe.example.com/api/v3`). Must be an `https://` URL. | | `skip-token-revocation` | no | When `true`, do not revoke the minted token after the Copilot run. Defaults to `false` (the token is revoked — see below). | | `private-key` | no | Name of the ADO **secret** pipeline variable holding the private key (PEM). **Defaults to `GITHUB_APP_PRIVATE_KEY`** — the compiler owns the name, exactly like `GITHUB_TOKEN`, so the common case sets no field and just runs `ado-aw secrets set GITHUB_APP_PRIVATE_KEY …`. The value is an ADO variable macro target, so names with hyphens are accepted (for example Key Vault-backed variable groups commonly expose secrets as `AGENTIC-WORKFLOWS-GITHUB-APP-PRIVATE-KEY`). The mint bundle normalizes common ADO PEM representations (raw multiline PEM, escaped-newline text like `\\n`/`\\r\\n`, and whitespace-collapsed PEM bodies). Set this only to point at a differently-named secret (e.g. reusing an existing variable). | +| `permissions` | no | Repository-permission subset requested for each Agent/Detection token. Values are `read` or `write`; hyphenated names are normalized for the GitHub API. When GitHub issue safe outputs inherit these App credentials, this map is required and every repository permission must be read-only. | #### Setup @@ -107,7 +111,7 @@ ado-aw secrets set GITHUB_APP_PRIVATE_KEY "$(cat app-private-key.pem)" immediately before the Copilot invocation in **both** the Agent and Detection jobs. It builds a short-lived RS256 JWT from the App ID + private key, resolves the installation for `owner`, exchanges it for an installation - access token (optionally scoped to `repositories`), and exposes it as a + access token (optionally scoped to `repositories` and `permissions`), and exposes it as a **masked, same-job** `GITHUB_APP_TOKEN` variable. - The Copilot engine's `GITHUB_TOKEN` is then sourced from `$(GITHUB_APP_TOKEN)` instead of the operator-provided `$(GITHUB_TOKEN)` variable. @@ -116,8 +120,10 @@ ado-aw secrets set GITHUB_APP_PRIVATE_KEY "$(cat app-private-key.pem)" (`DELETE /installation/token`) so it does not remain valid for its full ~1h lifetime — matching `actions/create-github-app-token`'s default. Revocation is best-effort (`always()` + `continueOnError`) and never fails the build. -- The token is **never** provided to SafeOutputs, user-authored `steps:`, - ManualReview, Teardown, or Conclusion. +- The Agent/Detection token is **never** provided to SafeOutputs, + user-authored `steps:`, ManualReview, Teardown, or Conclusion. GitHub issue + safe outputs may reuse the App credentials to mint a separate + `issues: write` token in Stage 3; they never reuse the engine token itself. The mint/revoke steps run **outside** the AWF network sandbox (like the ADO-token and execution-context steps), reaching the GitHub API over the build @@ -129,13 +135,16 @@ agent pool's normal network — no AWF `network.allowed` entry is required. entirely separate: they describe Azure DevOps OAuth/service-connection tokens used by the pipeline and Stage 3 executor. The GitHub App token has no effect on them. -- The GitHub App token is for **Copilot engine authentication only**. It does - **not** authenticate the `gh` CLI, grant GitHub MCP permissions, or give the - agent sandbox GitHub write access. -- There is **no `permissions:` sub-field** to scope the token. Copilot access - rides on the App's own granted capability (configured App/org-side); narrowing - the installation token's permissions at mint time cannot grant Copilot access - and risks stripping the capability it needs. +- The engine token authenticates Copilot and the read-only GitHub access + available through that engine path. It does not authenticate user-authored + `gh` CLI steps. +- `permissions:` can only narrow permissions already granted to the App + installation. It cannot grant Copilot capability; that remains configured on + the App/org side. +- When the same App also backs GitHub issue safe outputs, ado-aw requires an + explicitly read-only engine permission map and mints a distinct Stage 3 token + with `issues: write`. See + [`docs/safe-outputs.md`](safe-outputs.md#github-issue-safe-outputs). #### Notes and limitations diff --git a/docs/front-matter.md b/docs/front-matter.md index ee7bb27c..521a91e0 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -228,7 +228,6 @@ supply-chain: # optional internal supply-chain mirror (see docs service-connection: shared-conn # optional feed/registry fallback; never applies to pipeline-artifact # ado-aw-debug: # debug-only knobs; see docs/ado-aw-debug.md # skip-integrity: false # omit generated pipeline integrity verification -# create-issue: false # dogfood-only GitHub issue filing for debug reports parameters: # optional ADO runtime parameters (surfaced in UI when queuing a run) - name: clearMemory displayName: "Clear agent memory" @@ -281,10 +280,13 @@ quiet. So adding validation coverage can only ever *surface* authoring mistakes `ado-aw-debug:` is accepted in front matter for repository dogfooding and local diagnostics. It is **not** a regular safe-output tool. Use -`skip-integrity` to omit generated pipeline integrity verification, or -`create-issue` to file a GitHub issue from debug pipelines; see +`skip-integrity` to omit generated pipeline integrity verification; see [`ado-aw-debug.md`](ado-aw-debug.md) for the full reference. +GitHub issue filing now uses regular `safe-outputs.create-issue` and +`safe-outputs.set-issue-type`; see +[`safe-outputs.md`](safe-outputs.md#github-issue-safe-outputs). + ## Per-Job Pool Overrides (`pool.overrides:`) By default `pool:` is applied to every generated job — Setup, Agent, Detection, diff --git a/docs/safe-outputs.md b/docs/safe-outputs.md index 1f2589e1..3ca44366 100644 --- a/docs/safe-outputs.md +++ b/docs/safe-outputs.md @@ -2,12 +2,6 @@ _Part of the [ado-aw documentation](../AGENTS.md)._ -> ℹ️ The debug-only `create-issue` tool (used by dogfood pipelines to file -> failure reports back to GitHub) is **not** a safe output and is not -> configurable here. It is gated by a separate `ado-aw-debug:` front-matter -> section and stripped from the SafeOutputs MCP server unless explicitly -> enabled. See [`docs/ado-aw-debug.md`](ado-aw-debug.md). - ## Safe Outputs Configuration The front matter supports a `safe-outputs:` field for configuring specific tool behaviors: @@ -175,8 +169,187 @@ ARM-minted token, e.g. for cross-org writes or named-identity attribution. See [`docs/network.md`](network.md) and [`docs/ir.md`](ir.md) for the typed SafeOutputs job wiring. +## GitHub issue safe outputs + +`create-issue` and `set-issue-type` call GitHub only from Stage 3, after threat +detection. The GitHub write credential is never exposed to Agent or Detection. +The MCP routes are configured-only: GitHub auth by itself does not expose them +to the agent; each tool appears only when its own front-matter key is present. + +### Authentication + +With no explicit auth, Stage 3 uses the secret ADO pipeline variable +`ADO_AW_GITHUB_TOKEN`: + +```yaml +safe-outputs: + create-issue: + target-repo: octo-org/octo-repo +``` + +Set it with: + +```text +ado-aw secrets set ADO_AW_GITHUB_TOKEN +``` + +The token needs **Issues: read and write** on the target repository. To use a +differently named secret, provide exactly one ADO macro: + +```yaml +safe-outputs: + github-token: "$(MY_GITHUB_ISSUES_TOKEN)" + github-api-url: https://ghe.example.com/api/v3 # optional PAT API base + create-issue: + target-repo: octo-org/octo-repo +``` + +Literal tokens and compound expressions are rejected at compile time. +`$(GITHUB_TOKEN)` is intentionally not the default because that variable is +used by the read-only Agent/Detection path, and it is rejected if supplied +explicitly. `github-api-url` defaults to `https://api.github.com`, accepts only +an `https://` URL, and applies only to PAT auth; App auth uses its nested +`api-url`. + +#### Shared GitHub App + +When `engine.github-app-token` is configured and neither SafeOutputs auth key is +present, the App credentials are reused but the tokens are not: + +```yaml +engine: + id: copilot + github-app-token: + app-id: 1234567 + private-key: GITHUB_APP_PRIVATE_KEY + owner: octo-org + repositories: [octo-repo] + permissions: + contents: read + issues: read + pull-requests: read + +safe-outputs: + create-issue: + target-repo: octo-org/octo-repo +``` + +Agent and Detection each mint their own explicitly read-only token. SafeOutputs +mints a separate token scoped to the configured target repository with only +`issues: write`, then revokes it after execution. Shared credentials are +rejected when the engine permission map is absent or contains a repository +`write` permission. + +#### Separate SafeOutputs GitHub App + +Use `safe-outputs.github-app` to isolate the write App: + +```yaml +safe-outputs: + github-app: + client-id: Iv23liSafeOutputsApp + private-key: SAFE_OUTPUTS_GITHUB_APP_PRIVATE_KEY + owner: octo-org + repositories: [octo-repo] + api-url: https://api.github.com + skip-token-revocation: false + create-issue: + target-repo: octo-org/octo-repo +``` + +`client-id` and `app-id` are aliases. `private-key` names an ADO secret +variable; it is not the PEM value or a `$(...)` macro. SafeOutputs derives the +minimum permission request, so arbitrary `github-app.permissions` overrides are +not accepted here. `github-app` and `github-token` are mutually exclusive. + +### Target repository + +`target-repo` is operator-controlled. It may be omitted only when the ADO build +source provider is GitHub and `BUILD_REPOSITORY_NAME` is an `owner/repo` slug. +Azure Repos workflows must set it explicitly. + +### Temporary IDs and approval + +Use a gh-aw-compatible temporary ID to refer to a newly created issue before +its real number exists: + +```json +{"title":"Build failure","body":"Detailed failure report long enough for validation.","temporary_id":"#aw_bug1"} +{"issue_number":"#aw_bug1","issue_type":"Bug"} +``` + +The ID format is `#aw_` plus 3-12 ASCII alphanumeric/underscore characters; +the leading `#` is optional. `create-issue` must run first and succeed. +Duplicate, unresolved, cross-repository, or reversed references fail before an +API call. + +When both tools are configured, they must have the same effective +`require-approval` setting so they execute in the same SafeOutputs job. A +section-level gate is the simplest form: + +```yaml +safe-outputs: + require-approval: true + create-issue: + target-repo: octo-org/octo-repo + require-temporary-id: true + set-issue-type: + target-repo: octo-org/octo-repo +``` + ## Available Safe Output Tools +### create-issue + +Creates a GitHub issue. + +```yaml +safe-outputs: + create-issue: + target-repo: octo-org/octo-repo + title-prefix: "[agent] " + labels: [automation] + allowed-labels: ["agent-*", bug] + assignees: [octocat] + require-temporary-id: true + max: 1 +``` + +- `target-repo` *(optional only for GitHub-backed builds)* - fixed + `owner/repo` target. +- `title-prefix` *(optional)* - prepended in Stage 3. +- `labels` *(optional)* - static labels always applied. +- `allowed-labels` *(optional)* - allowlist for agent labels. Empty/absent is + default-deny; `["*"]` permits any label. +- `assignees` *(optional)* - static assignees merged with agent input. +- `require-temporary-id` *(optional, default `false`)* - reject proposals that + omit `temporary_id`. +- `max` *(optional, default `1`)* - per-run creation budget. + +Agent parameters are `title`, `body`, optional `labels`, optional `assignees`, +and optional `temporary_id`. + +### set-issue-type + +Sets or clears a native GitHub Issue Type: + +```yaml +safe-outputs: + set-issue-type: + target-repo: octo-org/octo-repo + allowed: [Bug, Feature, Task] + max: 5 +``` + +- `target-repo` *(optional only for GitHub-backed builds)* - target for numeric + issue numbers; temporary IDs carry their created repository. +- `allowed` *(optional)* - case-insensitive type allowlist. Empty/absent allows + any configured repository type. Clearing is always allowed. +- `max` *(optional, default `5`)* - per-run update budget. + +Agent parameters are required `issue_number` (positive number or temporary ID) +and required `issue_type`. Pass `""` to clear the type. + ### comment-on-work-item Adds a comment to an existing Azure DevOps work item. This is the ADO equivalent of gh-aw's `add-comment` tool. diff --git a/examples/dogfood-failure-reporter.md b/examples/dogfood-failure-reporter.md index f615874c..ea1f2a02 100644 --- a/examples/dogfood-failure-reporter.md +++ b/examples/dogfood-failure-reporter.md @@ -5,7 +5,7 @@ on: schedule: daily permissions: read: my-read-arm-connection -ado-aw-debug: +safe-outputs: create-issue: target-repo: githubnext/ado-aw title-prefix: "[pipeline-failure] " @@ -44,7 +44,7 @@ in Azure DevOps inside an AWF-isolated sandbox. - Do not attempt to redirect issues to a different repository — the agent has no `target_repo` parameter and the target is fixed by the operator. -- The `ADO_AW_DEBUG_GITHUB_TOKEN` PAT is **not** visible to you; it is +- The `ADO_AW_GITHUB_TOKEN` PAT is **not** visible to you; it is used only by Stage 3 to authenticate against GitHub. - Issues are reviewed for prompt injection by Stage 2 before they are filed, so do not include text that looks like ADO pipeline commands 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 887b9c22..99401b79 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 @@ -4,6 +4,7 @@ import { generateKeyPairSync, createVerify } from "node:crypto"; import { buildAppJwt, parseArgs, + parsePermissions, parseRepositories, resolveInstallationId, mintInstallationToken, @@ -131,6 +132,33 @@ describe("parseRepositories", () => { expect(parseRepositories("")).toEqual([]); expect(parseRepositories(" single ")).toEqual(["single"]); }); + + describe("parsePermissions", () => { + it("parses read/write permission JSON", () => { + expect( + parsePermissions('{"contents":"read","issues":"write"}'), + ).toEqual({ + contents: "read", + issues: "write", + }); + expect(parsePermissions(undefined)).toEqual({}); + }); + + it("rejects invalid permission names and levels", () => { + expect(() => parsePermissions('{"pull-requests":"read"}')).toThrow( + /permission name/, + ); + expect(() => parsePermissions('{"issues":"admin"}')).toThrow( + /read.*write/, + ); + expect(() => parsePermissions('{"__proto__":"read"}')).toThrow( + /reserved/, + ); + expect(() => parsePermissions('{"constructor":"read"}')).toThrow( + /reserved/, + ); + }); + }); }); describe("resolveInstallationId", () => { @@ -213,6 +241,24 @@ describe("mintInstallationToken", () => { expect(JSON.parse(fetchFn.mock.calls[0]![1].body)).toEqual({}); }); + it("includes an explicit permission subset", async () => { + const fetchFn = vi + .fn() + .mockResolvedValueOnce(jsonResponse(201, { token: "ghs_scoped" })); + await mintInstallationToken( + fetchFn as never, + "https://api.github.com", + "jwt", + 1, + ["repo-a"], + { issues: "write" }, + ); + expect(JSON.parse(fetchFn.mock.calls[0]![1].body)).toEqual({ + repositories: ["repo-a"], + permissions: { issues: "write" }, + }); + }); + it("throws on a non-2xx response", async () => { const fetchFn = vi .fn() @@ -240,6 +286,8 @@ describe("parseArgs", () => { "GITHUB_APP_TOKEN", "--repositories", "repo-a repo-b", + "--permissions-json", + '{"issues":"write"}', "--api-url", "https://ghe.example.com/api/v3", ]); @@ -248,6 +296,7 @@ describe("parseArgs", () => { owner: "octo-org", outputVar: "GITHUB_APP_TOKEN", repositories: "repo-a repo-b", + permissionsJson: '{"issues":"write"}', apiUrl: "https://ghe.example.com/api/v3", }); }); @@ -304,6 +353,7 @@ describe("main", () => { owner: "octo-org", outputVar: "GITHUB_APP_TOKEN", repositories: "repo-a repo-b", + permissionsJson: '{"issues":"write"}', }, { GH_APP_PRIVATE_KEY: privateKey } as NodeJS.ProcessEnv, fetchFn as never, @@ -315,6 +365,10 @@ describe("main", () => { expect(out).toContain( "##vso[task.setvariable variable=GITHUB_APP_TOKEN;issecret=true]ghs_minted", ); + expect(JSON.parse(fetchFn.mock.calls[1]![1].body)).toEqual({ + repositories: ["repo-a", "repo-b"], + permissions: { issues: "write" }, + }); }); it("honours --api-url and --output-var", async () => { diff --git a/scripts/ado-script/src/github-app-token/index.ts b/scripts/ado-script/src/github-app-token/index.ts index 6dee0f44..2a7bdd59 100644 --- a/scripts/ado-script/src/github-app-token/index.ts +++ b/scripts/ado-script/src/github-app-token/index.ts @@ -1,6 +1,5 @@ /** - * github-app-token — mint a GitHub App installation access token for the - * Copilot engine (issue #1316). + * github-app-token — mint a scoped GitHub App installation access token. * * Mirrors gh-aw's `create-github-app-token` model, adapted to Azure DevOps: * the GitHub App **App ID** and **private key** are supplied via env vars @@ -18,7 +17,7 @@ * set of repositories. * 4. Emit the token as a **masked, same-job** pipeline variable * (`##vso[task.setvariable …;issecret=true]`) so a downstream step in the - * same job (the Copilot invocation) can read it via `$(GITHUB_APP_TOKEN)` + * same job can read it through a compiler-selected `$(...)` variable * without it leaking into the log. * * Trust boundary: runs OUTSIDE the AWF sandbox (a normal ADO script step, like @@ -34,7 +33,8 @@ * * Mint: node github-app-token.js \ * --app-id --owner --output-var \ - * [--repositories "a b"] [--api-url https://host/api/v3] + * [--repositories "a b"] [--permissions-json '{"issues":"write"}'] \ + * [--api-url https://host/api/v3] * env: GH_APP_PRIVATE_KEY (required, secret) * * Revoke: node github-app-token.js revoke [--api-url https://host/api/v3] @@ -138,6 +138,43 @@ export function parseRepositories(raw: string | undefined): string[] { .filter((s) => s.length > 0); } +export function parsePermissions( + raw: string | undefined, +): Record { + if (!raw) return {}; + const parsed: unknown = JSON.parse(raw); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error("--permissions-json must be a JSON object"); + } + const permissions = Object.create(null) as Record< + string, + "read" | "write" + >; + for (const [name, level] of Object.entries(parsed)) { + if ( + name === "__proto__" || + name === "prototype" || + name === "constructor" + ) { + throw new Error( + `reserved GitHub App permission name ${JSON.stringify(name)}`, + ); + } + if (!/^[a-z0-9_]+$/.test(name)) { + throw new Error( + `invalid GitHub App permission name ${JSON.stringify(name)}`, + ); + } + if (level !== "read" && level !== "write") { + throw new Error( + `GitHub App permission ${JSON.stringify(name)} must be "read" or "write"`, + ); + } + permissions[name] = level; + } + return permissions; +} + interface FetchLike { ( url: string, @@ -213,11 +250,15 @@ export async function mintInstallationToken( jwt: string, installationId: number, repositories: string[], + permissions: Record = {}, ): Promise { const body: Record = {}; if (repositories.length > 0) { body.repositories = repositories; } + if (Object.keys(permissions).length > 0) { + body.permissions = permissions; + } const resp = await fetchFn( `${apiUrl}/app/installations/${installationId}/access_tokens`, { @@ -269,6 +310,7 @@ export interface CliArgs { owner?: string; outputVar?: string; repositories?: string; + permissionsJson?: string; apiUrl?: string; } @@ -304,6 +346,9 @@ export function parseArgs(argv: string[]): CliArgs { case "--repositories": if (value !== undefined) out.repositories = value; break; + case "--permissions-json": + if (value !== undefined) out.permissionsJson = value; + break; case "--api-url": if (value !== undefined) out.apiUrl = value; break; @@ -376,6 +421,7 @@ export async function main( const owner = requireArg(args.owner, "--owner"); const privateKey = requireEnv(env, "GH_APP_PRIVATE_KEY"); const repositories = parseRepositories(args.repositories); + const permissions = parsePermissions(args.permissionsJson); const apiUrl = normalizeApiUrl(args.apiUrl); const outputVar = args.outputVar && args.outputVar.length > 0 @@ -395,9 +441,10 @@ export async function main( jwt, installationId, repositories, + permissions, ); - // Mask + expose to the same-job Copilot step. Emitting the secret BEFORE + // 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); logInfo( diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 0a978f35..cbcebbc9 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -98,6 +98,7 @@ use super::types::{ /// The `safe-outputs:` key for the create-pull-request tool. Matches the kebab /// name `FrontMatter::create_pr_config`/`partition_safe_outputs_by_approval` use. const CREATE_PULL_REQUEST_TOOL: &str = "create-pull-request"; +const GITHUB_ISSUE_TOOLS: &[&str] = &["create-issue", "set-issue-type"]; /// Built pipeline context — the result of running every validation, /// scalar computation, extension declaration fanout, and canonical- @@ -139,6 +140,7 @@ pub(crate) fn build_pipeline_context( )?; common::validate_safe_outputs_keys(front_matter)?; front_matter.validate_require_approval()?; + common::validate_github_issue_outputs_config(front_matter)?; common::validate_comment_target(front_matter)?; common::validate_update_work_item_target(front_matter)?; common::validate_submit_pr_review_events(front_matter)?; @@ -281,14 +283,6 @@ pub(crate) fn build_pipeline_context( .and_then(|p| p.write.as_deref()), "SC_WRITE_TOKEN", ); - let executor_ado_env = common::generate_executor_ado_env( - front_matter - .permissions - .as_ref() - .and_then(|p| p.write.as_deref()), - common::debug_create_issue_enabled(front_matter), - ); - // Skip integrity check resolution let skip_integrity = skip_integrity || front_matter @@ -359,7 +353,6 @@ pub(crate) fn build_pipeline_context( pipeline_path: pipeline_path.clone(), acquire_read_token, acquire_write_token, - executor_ado_env, integrity_check_yaml, agent_content_value, debug_pipeline, @@ -441,25 +434,40 @@ pub(crate) fn build_canonical_jobs( // variant should pay for the bundle download + prepare step. let create_pr_configured = front_matter.create_pr_config().is_some(); let create_pr_reviewed = reviewed.iter().any(|t| t == CREATE_PULL_REQUEST_TOOL); + let github_issue_tools_configured = front_matter.has_github_issue_outputs(); + let github_issue_tools_reviewed = reviewed + .iter() + .any(|tool| GITHUB_ISSUE_TOOLS.contains(&tool.as_str())); if reviewed.is_empty() || auto.is_empty() { jobs.push(build_safeoutputs_job( front_matter, cfg, &p, - &SafeOutputsVariant::default_single(create_pr_configured), + &SafeOutputsVariant::default_single( + create_pr_configured, + github_issue_tools_configured, + ), )?); } else { jobs.push(build_safeoutputs_job( front_matter, cfg, &p, - &SafeOutputsVariant::automatic(&reviewed, create_pr_configured && !create_pr_reviewed), + &SafeOutputsVariant::automatic( + &reviewed, + create_pr_configured && !create_pr_reviewed, + github_issue_tools_configured && !github_issue_tools_reviewed, + ), )?); jobs.push(build_safeoutputs_job( front_matter, cfg, &p, - &SafeOutputsVariant::reviewed(&reviewed, create_pr_configured && create_pr_reviewed), + &SafeOutputsVariant::reviewed( + &reviewed, + create_pr_configured && create_pr_reviewed, + github_issue_tools_configured && github_issue_tools_reviewed, + ), )?); } if let Some(teardown) = build_teardown_job(front_matter, cfg, &p)? { @@ -536,9 +544,6 @@ pub(crate) struct StandaloneCtx { /// `AzureCLI@2` task YAML body (or empty when no read service connection). pub(crate) acquire_read_token: String, pub(crate) acquire_write_token: String, - /// `env:` block for executor step (always non-empty — has - /// SYSTEM_ACCESSTOKEN at minimum). - pub(crate) executor_ado_env: String, /// `Verify pipeline integrity` step YAML (or empty when skipped). pub(crate) integrity_check_yaml: String, /// Agent prompt body (either inlined imports or @@ -1290,6 +1295,7 @@ struct SafeOutputsVariant { /// (issue #1453 review). Avoids a wasted Node install + bundle fetch + /// prepare step in the variant that will never open a PR. runs_create_pull_request: bool, + runs_github_issue_tools: bool, /// Whether this is the manual-review-gated `SafeOutputs_Reviewed` variant. /// Used to select the correct pool override without relying on the job name. is_reviewed: bool, @@ -1298,39 +1304,53 @@ struct SafeOutputsVariant { impl SafeOutputsVariant { /// The default single-job variant: no filter, canonical names. Runs every /// configured tool, so it executes `create-pull-request` iff configured. - fn default_single(runs_create_pull_request: bool) -> Self { + fn default_single( + runs_create_pull_request: bool, + runs_github_issue_tools: bool, + ) -> Self { Self { base: "SafeOutputs", display: "SafeOutputs", artifact: "safe_outputs", filter_args: String::new(), runs_create_pull_request, + runs_github_issue_tools, is_reviewed: false, } } /// The automatic variant in a split: excludes every reviewed tool. Runs /// `create-pull-request` only when it is configured and NOT review-gated. - fn automatic(reviewed: &[String], runs_create_pull_request: bool) -> Self { + fn automatic( + reviewed: &[String], + runs_create_pull_request: bool, + runs_github_issue_tools: bool, + ) -> Self { Self { base: "SafeOutputs", display: "SafeOutputs", artifact: "safe_outputs", filter_args: filter_flags("--exclude", reviewed), runs_create_pull_request, + runs_github_issue_tools, is_reviewed: false, } } /// The reviewed variant in a split: runs only the reviewed tools. Runs /// `create-pull-request` only when it is configured and review-gated. - fn reviewed(reviewed: &[String], runs_create_pull_request: bool) -> Self { + fn reviewed( + reviewed: &[String], + runs_create_pull_request: bool, + runs_github_issue_tools: bool, + ) -> Self { Self { base: "SafeOutputs_Reviewed", display: "SafeOutputs (reviewed)", artifact: "safe_outputs_reviewed", filter_args: filter_flags("--only", reviewed), runs_create_pull_request, + runs_github_issue_tools, is_reviewed: true, } } @@ -1432,6 +1452,14 @@ fn build_safeoutputs_job( prefix: &JobPrefix<'_>, variant: &SafeOutputsVariant, ) -> Result { + let github_auth = if variant.runs_github_issue_tools { + front_matter.github_safe_outputs_auth()? + } else { + None + }; + let github_app = github_auth + .as_ref() + .and_then(crate::compile::types::GithubSafeOutputsAuth::app_config); let mut steps: Vec = Vec::new(); steps.push(checkout_self_step(&cfg.self_checkout_fetch)); // Acquire write token (when configured) @@ -1474,25 +1502,59 @@ fn build_safeoutputs_job( // then emit the same `prepare-pr-base` step. The bundle auth projects // `System.AccessToken` (the build identity the checkout persists credentials // for), so the git fetch is authenticated regardless of the write token. - if variant.runs_create_pull_request { + if variant.runs_create_pull_request || github_app.is_some() { steps.extend( super::extensions::ado_script::install_and_download_steps_typed( front_matter.supply_chain(), ), ); + } + if variant.runs_create_pull_request { let repos = create_pr_prepare_repos(front_matter, &cfg.working_directory); steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( super::extensions::ado_script::PreparePrBaseMode::TargetWorktree, &repos, )); } + if let Some(app) = github_app { + let permissions = std::collections::BTreeMap::from([( + "issues".to_string(), + crate::compile::types::GithubAppPermissionLevel::Write, + )]); + steps.push( + super::extensions::ado_script::github_app_token_step_typed_for( + app, + crate::compile::types::SAFE_OUTPUTS_GITHUB_APP_TOKEN_VAR, + "Mint GitHub App token (SafeOutputs)", + &permissions, + )?, + ); + } + let executor_ado_env = common::generate_executor_ado_env( + front_matter + .permissions + .as_ref() + .and_then(|permissions| permissions.write.as_deref()), + github_auth.as_ref(), + ); // Execute safe outputs (Stage 3) — typed BashStep with typed env block steps.push(Step::Bash(execute_safe_outputs_step( &cfg.source_path, &cfg.working_directory, - &cfg.executor_ado_env, + &executor_ado_env, &variant.filter_args, )?)); + if let Some(app) = github_app + && !app.skip_token_revocation + { + steps.push( + super::extensions::ado_script::github_app_token_revoke_step_typed_for( + app, + crate::compile::types::SAFE_OUTPUTS_GITHUB_APP_TOKEN_VAR, + "Revoke GitHub App token (SafeOutputs)", + )?, + ); + } // Copy logs steps.push(Step::Bash(copy_logs_safeoutputs_step(&cfg.engine_log_dir))); // Publish @@ -4003,7 +4065,6 @@ mod tests { pipeline_path: "source.lock.yml".to_string(), acquire_read_token: String::new(), acquire_write_token: String::new(), - executor_ado_env: "env:\n SYSTEM_ACCESSTOKEN: $(System.AccessToken)\n".to_string(), integrity_check_yaml: String::new(), agent_content_value: String::new(), debug_pipeline: false, diff --git a/src/compile/codemods/0006_promote_debug_create_issue.rs b/src/compile/codemods/0006_promote_debug_create_issue.rs new file mode 100644 index 00000000..a30676e5 --- /dev/null +++ b/src/compile/codemods/0006_promote_debug_create_issue.rs @@ -0,0 +1,172 @@ +//! `ado-aw-debug.create-issue` -> `safe-outputs.create-issue` +//! +//! The GitHub issue tool graduated from the dogfood-only debug surface to a +//! regular safe output. This codemod performs a one-way migration and removes +//! the legacy key entirely; there is no deprecated parser/runtime alias. +//! +//! When the workflow has no explicit SafeOutputs GitHub authentication, the +//! codemod preserves the existing pipeline secret by adding: +//! +//! ```yaml +//! safe-outputs: +//! github-token: "$(ADO_AW_DEBUG_GITHUB_TOKEN)" +//! ``` +//! +//! That value is handled by the regular Stage 3 token path and can be renamed +//! later to the new `ADO_AW_GITHUB_TOKEN` default. + +use anyhow::{Result, bail}; +use serde_yaml::{Mapping, Value}; + +use super::{Codemod, CodemodContext}; + +const INTRODUCED_IN: &str = "0.46.0"; + +pub static CODEMOD: Codemod = Codemod { + id: "promote_debug_create_issue", + summary: "ado-aw-debug.create-issue moved to safe-outputs.create-issue", + introduced_in: INTRODUCED_IN, + apply: apply_codemod, +}; + +fn key(name: &str) -> Value { + Value::String(name.to_string()) +} + +fn apply_codemod(fm: &mut Mapping, _ctx: &CodemodContext) -> Result { + let Some(debug) = fm.get(key("ado-aw-debug")) else { + return Ok(false); + }; + let Some(debug_map) = debug.as_mapping() else { + return Ok(false); + }; + let Some(create_issue) = debug_map.get(key("create-issue")).cloned() else { + return Ok(false); + }; + + let mut migrated_debug = debug_map.clone(); + migrated_debug.remove(key("create-issue")); + + let mut safe_outputs = match fm.get(key("safe-outputs")) { + Some(value) => value.as_mapping().cloned().ok_or_else(|| { + anyhow::anyhow!( + "manual migration required: `safe-outputs` must be a mapping before \ + moving `ado-aw-debug.create-issue`" + ) + })?, + None => Mapping::new(), + }; + + if safe_outputs.contains_key(key("create-issue")) { + bail!( + "manual migration required: both `ado-aw-debug.create-issue` and \ + `safe-outputs.create-issue` are configured" + ); + } + + if !safe_outputs.contains_key(key("github-token")) + && !safe_outputs.contains_key(key("github-app")) + { + safe_outputs.insert( + key("github-token"), + Value::String("$(ADO_AW_DEBUG_GITHUB_TOKEN)".to_string()), + ); + } + safe_outputs.insert(key("create-issue"), create_issue); + + if migrated_debug.is_empty() { + fm.remove(key("ado-aw-debug")); + } else { + fm.insert(key("ado-aw-debug"), Value::Mapping(migrated_debug)); + } + fm.insert(key("safe-outputs"), Value::Mapping(safe_outputs)); + Ok(true) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn ctx() -> CodemodContext { + CodemodContext { + compiler_version: INTRODUCED_IN, + } + } + + fn map(yaml: &str) -> Mapping { + serde_yaml::from_str(yaml).unwrap() + } + + #[test] + fn moves_create_issue_and_preserves_other_debug_fields() { + let mut fm = map( + "ado-aw-debug:\n skip-integrity: true\n create-issue:\n target-repo: octo/repo\n", + ); + assert!(apply_codemod(&mut fm, &ctx()).unwrap()); + + let debug = fm + .get(key("ado-aw-debug")) + .and_then(Value::as_mapping) + .unwrap(); + assert_eq!(debug.get(key("skip-integrity")), Some(&Value::Bool(true))); + assert!(!debug.contains_key(key("create-issue"))); + + let safe_outputs = fm + .get(key("safe-outputs")) + .and_then(Value::as_mapping) + .unwrap(); + assert!(safe_outputs.contains_key(key("create-issue"))); + assert_eq!( + safe_outputs + .get(key("github-token")) + .and_then(Value::as_str), + Some("$(ADO_AW_DEBUG_GITHUB_TOKEN)") + ); + } + + #[test] + fn removes_empty_debug_section() { + let mut fm = map("ado-aw-debug:\n create-issue:\n target-repo: octo/repo\n"); + assert!(apply_codemod(&mut fm, &ctx()).unwrap()); + assert!(!fm.contains_key(key("ado-aw-debug"))); + } + + #[test] + fn preserves_existing_safe_outputs_auth() { + let mut fm = map( + "ado-aw-debug:\n create-issue:\n target-repo: octo/repo\nsafe-outputs:\n github-token: $(NEW_TOKEN)\n noop: {}\n", + ); + assert!(apply_codemod(&mut fm, &ctx()).unwrap()); + let safe_outputs = fm + .get(key("safe-outputs")) + .and_then(Value::as_mapping) + .unwrap(); + assert_eq!( + safe_outputs + .get(key("github-token")) + .and_then(Value::as_str), + Some("$(NEW_TOKEN)") + ); + assert!(safe_outputs.contains_key(key("noop"))); + } + + #[test] + fn errors_without_mutation_when_new_key_already_exists() { + let mut fm = map( + "ado-aw-debug:\n create-issue:\n target-repo: old/repo\nsafe-outputs:\n create-issue:\n target-repo: new/repo\n", + ); + let snapshot = fm.clone(); + let err = apply_codemod(&mut fm, &ctx()).unwrap_err().to_string(); + assert!(err.contains("both `ado-aw-debug.create-issue`")); + assert_eq!(fm, snapshot); + } + + #[test] + fn is_idempotent() { + let mut fm = map("ado-aw-debug:\n create-issue:\n target-repo: octo/repo\n"); + assert!(apply_codemod(&mut fm, &ctx()).unwrap()); + let snapshot = fm.clone(); + assert!(!apply_codemod(&mut fm, &ctx()).unwrap()); + assert_eq!(fm, snapshot); + } +} diff --git a/src/compile/codemods/mod.rs b/src/compile/codemods/mod.rs index 3da531c1..3c6795fc 100644 --- a/src/compile/codemods/mod.rs +++ b/src/compile/codemods/mod.rs @@ -43,6 +43,8 @@ mod m0003_flatten_work_item_config; mod m0004_legacy_path_markers; #[path = "0005_drop_build_attachment_allowed_build_ids.rs"] mod m0005_drop_build_attachment_allowed_build_ids; +#[path = "0006_promote_debug_create_issue.rs"] +mod m0006_promote_debug_create_issue; #[allow(unused_imports)] // Re-exported for future codemods; only `take_key` is in-tree use. pub use helpers::{ConflictPolicy, insert_no_overwrite, rename_key, take_key}; @@ -111,6 +113,7 @@ pub static CODEMODS: &[&Codemod] = &[ &m0003_flatten_work_item_config::CODEMOD, &m0004_legacy_path_markers::CODEMOD, &m0005_drop_build_attachment_allowed_build_ids::CODEMOD, + &m0006_promote_debug_create_issue::CODEMOD, ]; /// Result of running the codemod registry on a single front-matter diff --git a/src/compile/common.rs b/src/compile/common.rs index 0902a89b..87376d85 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1592,17 +1592,6 @@ pub fn generate_integrity_check(skip: bool) -> String { .to_string() } -/// Returns `true` when the agent's front matter sets -/// `ado-aw-debug.create-issue:` — the gate that activates the debug-only -/// `create-issue` safe output. -pub(crate) fn debug_create_issue_enabled(front_matter: &FrontMatter) -> bool { - front_matter - .ado_aw_debug - .as_ref() - .and_then(|d| d.create_issue.as_ref()) - .is_some() -} - /// Validate the `ado-aw-debug:` section. /// /// When `create-issue:` is present: @@ -1636,40 +1625,79 @@ pub fn validate_ado_aw_debug_config(front_matter: &FrontMatter) -> Result<()> { } } - let Some(debug) = front_matter.ado_aw_debug.as_ref() else { - return Ok(()); - }; - let Some(ci) = debug.create_issue.as_ref() else { - return Ok(()); - }; + Ok(()) +} - crate::safe_outputs::validate_target_repo(&ci.target_repo)?; +pub fn validate_github_issue_outputs_config(front_matter: &FrontMatter) -> Result<()> { + if let Some(config) = front_matter.create_issue_config()? { + if let Some(target_repo) = config.target_repo.as_deref() { + crate::safe_outputs::validate_target_repo(target_repo)?; + crate::validate::reject_pipeline_injection( + target_repo, + "safe-outputs.create-issue.target-repo", + )?; + } + if let Some(prefix) = config.title_prefix.as_deref() { + crate::validate::reject_pipeline_injection( + prefix, + "safe-outputs.create-issue.title-prefix", + )?; + } + for label in &config.labels { + crate::validate::reject_pipeline_injection( + label, + "safe-outputs.create-issue.labels", + )?; + } + for label in &config.allowed_labels { + crate::validate::reject_pipeline_injection( + label, + "safe-outputs.create-issue.allowed-labels", + )?; + } + for assignee in &config.assignees { + crate::validate::reject_pipeline_injection( + assignee, + "safe-outputs.create-issue.assignees", + )?; + } + } - crate::validate::reject_pipeline_injection( - &ci.target_repo, - "ado-aw-debug.create-issue.target-repo", - )?; - if let Some(prefix) = ci.title_prefix.as_deref() { - crate::validate::reject_pipeline_injection( - prefix, - "ado-aw-debug.create-issue.title-prefix", - )?; - } - for label in &ci.labels { - crate::validate::reject_pipeline_injection(label, "ado-aw-debug.create-issue.labels")?; - } - for label in &ci.allowed_labels { - crate::validate::reject_pipeline_injection( - label, - "ado-aw-debug.create-issue.allowed-labels", - )?; - } - for assignee in &ci.assignees { - crate::validate::reject_pipeline_injection( - assignee, - "ado-aw-debug.create-issue.assignees", - )?; + if let Some(config) = front_matter.set_issue_type_config()? { + if let Some(target_repo) = config.target_repo.as_deref() { + crate::safe_outputs::validate_target_repo(target_repo)?; + crate::validate::reject_pipeline_injection( + target_repo, + "safe-outputs.set-issue-type.target-repo", + )?; + } + for issue_type in &config.allowed { + crate::validate::reject_pipeline_injection( + issue_type, + "safe-outputs.set-issue-type.allowed", + )?; + } + } + + if front_matter.safe_outputs.contains_key("create-issue") + && front_matter.safe_outputs.contains_key("set-issue-type") + { + let create_reviewed = front_matter + .tool_requires_approval("create-issue") + .is_some(); + let type_reviewed = front_matter + .tool_requires_approval("set-issue-type") + .is_some(); + if create_reviewed != type_reviewed { + anyhow::bail!( + "safe-outputs.create-issue and safe-outputs.set-issue-type must have the \ + same effective require-approval setting so temporary issue IDs remain in \ + one SafeOutputs job" + ); + } } + + let _ = front_matter.github_safe_outputs_auth()?; Ok(()) } @@ -1818,22 +1846,27 @@ pub fn generate_acquire_ado_token(service_connection: Option<&str>, variable_nam /// scope" settings. Avoids the operational overhead of an ARM service /// connection. The agent (Stage 1) never maps this variable, so the /// token remains executor-only. -/// * `ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN)` when -/// `debug_create_issue_enabled` is `true` — GitHub PAT used by the -/// `ado-aw-debug.create-issue` safe output. Sourced from a dedicated -/// pipeline variable so it stays separate from the read-only `GITHUB_TOKEN` -/// the agent (Stage 1) sees. +/// * `ADO_AW_GITHUB_TOKEN` and `ADO_AW_GITHUB_API_URL` when GitHub issue +/// outputs are configured. The token source is a Stage 3 PAT or separately +/// minted App token and remains isolated from Agent/Detection. pub fn generate_executor_ado_env( write_service_connection: Option<&str>, - debug_create_issue_enabled: bool, + github_auth: Option<&crate::compile::types::GithubSafeOutputsAuth>, ) -> String { let mut lines: Vec = Vec::new(); // Select the ADO bearer via the shared `token_source_for` helper so the // executor and the Conclusion job cannot disagree on the token source. let token = crate::compile::ado_bundle::token_source_for(write_service_connection); lines.push(format!("SYSTEM_ACCESSTOKEN: $({})", token.variable())); - if debug_create_issue_enabled { - lines.push("ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN)".to_string()); + if let Some(github_auth) = github_auth { + lines.push(format!( + "ADO_AW_GITHUB_TOKEN: $({})", + github_auth.executor_token_var() + )); + lines.push(format!( + "ADO_AW_GITHUB_API_URL: {}", + github_auth.api_url() + )); } // The two-space indent on each value line is the YAML relative indent for // a key nested under `env:`. replace_with_indent prepends the base @@ -1865,9 +1898,7 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { use crate::safe_outputs::{ALL_KNOWN_SAFE_OUTPUTS, ALWAYS_ON_TOOLS, NON_MCP_SAFE_OUTPUT_KEYS}; use std::collections::HashSet; - let debug_create_issue = debug_create_issue_enabled(front_matter); - - if front_matter.safe_outputs.is_empty() && !debug_create_issue { + if front_matter.safe_outputs.is_empty() { return String::new(); } @@ -1908,13 +1939,6 @@ pub fn generate_enabled_tools_args(front_matter: &FrontMatter) -> String { } } - // Debug-only tools must be added explicitly — they're stripped from the - // MCP layer by default and only become reachable when listed here. - if debug_create_issue && seen.insert("create-issue".to_string()) { - tools.push("create-issue".to_string()); - effective_mcp_tool_count += 1; - } - if effective_mcp_tool_count == 0 { // Every user-specified key was either a non-MCP key or a guard path // from the defensive check above. Return empty to keep all tools @@ -4339,37 +4363,6 @@ mod tests { ); } - #[test] - fn test_debug_create_issue_enabled_helper() { - let yaml_off = "---\nname: test\ndescription: test\n---\n"; - let (fm_off, _) = parse_markdown(yaml_off).unwrap(); - assert!(!debug_create_issue_enabled(&fm_off)); - - let yaml_on = r#"--- -name: test -description: test -ado-aw-debug: - create-issue: - target-repo: githubnext/ado-aw ---- -"#; - let (fm_on, _) = parse_markdown(yaml_on).unwrap(); - assert!(debug_create_issue_enabled(&fm_on)); - - let yaml_section_only = r#"--- -name: test -description: test -ado-aw-debug: - skip-integrity: true ---- -"#; - let (fm_section, _) = parse_markdown(yaml_section_only).unwrap(); - assert!( - !debug_create_issue_enabled(&fm_section), - "ado-aw-debug.skip-integrity alone must NOT enable create-issue" - ); - } - // ─── generate_debug_pipeline_replacements ──────────────────────────────── // ─── validate_submit_pr_review_events ──────────────────────────────────── @@ -4645,14 +4638,23 @@ ado-aw-debug: ); } - // ─── ado-aw-debug wiring ──────────────────────────────────────────────── + #[test] + fn test_github_auth_without_issue_tools_does_not_enable_them() { + let (fm, _) = parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-token: $(TOKEN)\n---\n", + ) + .unwrap(); + assert!(generate_enabled_tools_args(&fm).is_empty()); + } + + // ─── GitHub issue output wiring ───────────────────────────────────────── #[test] - fn test_generate_enabled_tools_args_debug_create_issue_alone() { + fn test_generate_enabled_tools_args_create_issue_alone() { let yaml = r#"--- name: test description: test -ado-aw-debug: +safe-outputs: create-issue: target-repo: githubnext/ado-aw --- @@ -4661,7 +4663,7 @@ ado-aw-debug: let args = generate_enabled_tools_args(&fm); assert!( args.contains("--enabled-tools create-issue"), - "ado-aw-debug.create-issue should add create-issue to --enabled-tools, got: {}", + "safe-outputs.create-issue should add create-issue to --enabled-tools, got: {}", args ); // Always-on tools should also be present so the filter activates. @@ -4669,14 +4671,13 @@ ado-aw-debug: } #[test] - fn test_generate_enabled_tools_args_debug_plus_safe_outputs() { + fn test_generate_enabled_tools_args_create_issue_plus_other_output() { let yaml = r#"--- name: test description: test safe-outputs: create-pull-request: target-branch: main -ado-aw-debug: create-issue: target-repo: githubnext/ado-aw --- @@ -4690,7 +4691,7 @@ ado-aw-debug: } #[test] - fn test_generate_enabled_tools_args_no_debug_does_not_emit_create_issue() { + fn test_generate_enabled_tools_args_without_create_issue_does_not_emit_it() { let yaml = r#"--- name: test description: test @@ -4703,16 +4704,16 @@ safe-outputs: let args = generate_enabled_tools_args(&fm); assert!( !args.contains("create-issue"), - "create-issue must not appear without ado-aw-debug.create-issue" + "create-issue must not appear unless configured" ); } #[test] - fn test_validate_ado_aw_debug_config_accepts_valid_config() { + fn test_validate_github_issue_outputs_accepts_valid_config() { let yaml = r#"--- name: test description: test -ado-aw-debug: +safe-outputs: create-issue: target-repo: githubnext/ado-aw title-prefix: "[bug] " @@ -4722,7 +4723,7 @@ ado-aw-debug: --- "#; let (fm, _) = parse_markdown(yaml).unwrap(); - assert!(validate_ado_aw_debug_config(&fm).is_ok()); + assert!(validate_github_issue_outputs_config(&fm).is_ok()); } #[test] @@ -4732,7 +4733,7 @@ ado-aw-debug: } #[test] - fn test_validate_ado_aw_debug_config_rejects_missing_target_repo() { + fn test_validate_github_issue_outputs_rejects_empty_target_repo() { let yaml = r#"--- name: test description: test @@ -4742,14 +4743,14 @@ ado-aw-debug: --- "#; let (fm, _) = parse_markdown(yaml).unwrap(); - let result = validate_ado_aw_debug_config(&fm); + let result = validate_github_issue_outputs_config(&fm); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!(msg.contains("target-repo"), "msg: {}", msg); } #[test] - fn test_validate_ado_aw_debug_config_rejects_invalid_target_repo() { + fn test_validate_github_issue_outputs_rejects_invalid_target_repo() { let yaml = r#"--- name: test description: test @@ -4759,14 +4760,14 @@ ado-aw-debug: --- "#; let (fm, _) = parse_markdown(yaml).unwrap(); - let result = validate_ado_aw_debug_config(&fm); + let result = validate_github_issue_outputs_config(&fm); assert!(result.is_err()); let msg = result.unwrap_err().to_string(); assert!(msg.contains("owner/repo"), "msg: {}", msg); } #[test] - fn test_validate_ado_aw_debug_config_rejects_pipeline_injection_in_label() { + fn test_validate_github_issue_outputs_rejects_pipeline_injection_in_label() { let yaml = r###"--- name: test description: test @@ -4778,12 +4779,12 @@ ado-aw-debug: --- "###; let (fm, _) = parse_markdown(yaml).unwrap(); - let result = validate_ado_aw_debug_config(&fm); + let result = validate_github_issue_outputs_config(&fm); assert!(result.is_err()); } #[test] - fn test_validate_ado_aw_debug_config_rejects_pipeline_injection_in_title_prefix() { + fn test_validate_github_issue_outputs_rejects_pipeline_injection_in_title_prefix() { let yaml = r###"--- name: test description: test @@ -4794,7 +4795,7 @@ ado-aw-debug: --- "###; let (fm, _) = parse_markdown(yaml).unwrap(); - let result = validate_ado_aw_debug_config(&fm); + let result = validate_github_issue_outputs_config(&fm); assert!(result.is_err()); } @@ -4886,10 +4887,7 @@ safe-outputs: } #[test] - fn test_validate_safe_outputs_keys_does_not_double_report_debug_only_tool() { - // create-issue is in DEBUG_ONLY_TOOLS — validate_ado_aw_debug_config - // gives a better error for it. This validator should skip rather - // than redundantly flag it as "unknown". + fn test_validate_safe_outputs_keys_accepts_create_issue() { let yaml = r#"--- name: test description: test @@ -4970,11 +4968,7 @@ safe-outputs: } #[test] - fn test_validate_rejects_create_issue_under_safe_outputs() { - // Defence-in-depth: `create-issue` MUST NOT appear under - // `safe-outputs:` even when `ado-aw-debug:` isn't set. Allowing it - // there would let a forged config flow into ctx.tool_configs and - // sidestep the executor-side gate. + fn test_validate_accepts_create_issue_under_safe_outputs() { let yaml = r#"--- name: test description: test @@ -4984,14 +4978,51 @@ safe-outputs: --- "#; let (fm, _) = parse_markdown(yaml).unwrap(); - let result = validate_ado_aw_debug_config(&fm); - assert!(result.is_err()); - let msg = result.unwrap_err().to_string(); - assert!( - msg.contains("debug-only") && msg.contains("ado-aw-debug"), - "expected debug-only redirection error, got: {}", - msg - ); + assert!(validate_ado_aw_debug_config(&fm).is_ok()); + assert!(validate_github_issue_outputs_config(&fm).is_ok()); + } + + #[test] + fn test_validate_rejects_mixed_approval_lanes_for_linked_issue_tools() { + let yaml = r#"--- +name: test +description: test +safe-outputs: + create-issue: + target-repo: githubnext/ado-aw + require-approval: true + set-issue-type: + target-repo: githubnext/ado-aw + require-approval: false +--- +"#; + let (fm, _) = parse_markdown(yaml).unwrap(); + let error = validate_github_issue_outputs_config(&fm) + .unwrap_err() + .to_string(); + assert!(error.contains("same effective require-approval")); + } + + #[test] + fn test_validate_rejects_pat_and_app_auth_together() { + let yaml = r#"--- +name: test +description: test +safe-outputs: + github-token: $(TOKEN) + github-app: + client-id: Iv23liExample + owner: githubnext + repositories: [ado-aw] + create-issue: + target-repo: githubnext/ado-aw +--- +"#; + let (fm, _) = parse_markdown(yaml).unwrap(); + let error = validate_github_issue_outputs_config(&fm) + .unwrap_err() + .to_string(); + assert!(error.contains("mutually exclusive")); } // ─── parameter name validation ────────────────────────────────────────── @@ -5194,7 +5225,7 @@ safe-outputs: #[test] fn test_generate_executor_ado_env_with_connection() { - let result = generate_executor_ado_env(Some("my-sc"), false); + let result = generate_executor_ado_env(Some("my-sc"), None); assert!( result.contains("env:"), "Executor env block should include the 'env:' key" @@ -5213,14 +5244,14 @@ safe-outputs: "When write SC is configured, fall back to ARM-minted token, not System.AccessToken" ); assert!( - !result.contains("ADO_AW_DEBUG_GITHUB_TOKEN"), - "Without debug flag, GitHub token must not be exposed to executor" + !result.contains("ADO_AW_GITHUB_TOKEN"), + "Without GitHub issue outputs, GitHub token must not be exposed" ); } #[test] fn test_generate_executor_ado_env_none_uses_system_access_token() { - let result = generate_executor_ado_env(None, false); + let result = generate_executor_ado_env(None, None); assert!( result.starts_with("env:\n"), "Should always emit env: block (executor needs SYSTEM_ACCESSTOKEN)" @@ -5234,22 +5265,26 @@ safe-outputs: "Without write SC, must not reference SC_WRITE_TOKEN" ); assert!( - !result.contains("ADO_AW_DEBUG_GITHUB_TOKEN"), - "Without debug flag, GitHub token must not appear" + !result.contains("ADO_AW_GITHUB_TOKEN"), + "Without GitHub issue outputs, GitHub token must not appear" ); } #[test] fn test_generate_executor_ado_env_with_create_issue_only() { - let result = generate_executor_ado_env(None, true); + let auth = crate::compile::types::GithubSafeOutputsAuth::Token { + variable: "MY_GITHUB_WRITE_TOKEN".to_string(), + api_url: "https://api.github.com".to_string(), + }; + let result = generate_executor_ado_env(None, Some(&auth)); assert!(result.starts_with("env:\n"), "Should emit env: block"); assert!( result.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), "Default executor token is $(System.AccessToken) even with debug enabled" ); assert!( - result.contains("ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN)"), - "Debug flag should expose the GitHub PAT pipeline variable" + result.contains("ADO_AW_GITHUB_TOKEN: $(MY_GITHUB_WRITE_TOKEN)"), + "Executor should map the configured Stage 3 GitHub token" ); assert!( !result.contains("SC_WRITE_TOKEN"), @@ -5259,9 +5294,13 @@ safe-outputs: #[test] fn test_generate_executor_ado_env_with_both_tokens() { - let result = generate_executor_ado_env(Some("write-sc"), true); + let auth = crate::compile::types::GithubSafeOutputsAuth::Token { + 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)); assert!(result.contains("SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN)")); - assert!(result.contains("ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN)")); + assert!(result.contains("ADO_AW_GITHUB_TOKEN: $(ADO_AW_GITHUB_TOKEN)")); assert!( !result.contains("$(System.AccessToken)"), "Write SC overrides System.AccessToken default" diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index a3539325..e11aa8a5 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -17,7 +17,7 @@ //! in **both** Setup and Agent. That's correct architecture given ADO's //! topology, not waste. -use anyhow::Result; +use anyhow::{Context, Result}; use super::{CompileContext, CompilerExtension, Declarations, ExtensionPhase}; use crate::compile::agentic_pipeline::{ @@ -560,6 +560,23 @@ fn resolver_step_typed() -> Step { /// entry is required. pub fn github_app_token_step_typed( cfg: &crate::compile::types::GithubAppTokenConfig, +) -> Result { + github_app_token_step_typed_for( + cfg, + crate::engine::GITHUB_APP_TOKEN_VAR, + "Mint GitHub App token (Copilot engine auth)", + &cfg.permissions, + ) +} + +pub fn github_app_token_step_typed_for( + cfg: &crate::compile::types::GithubAppTokenConfig, + output_var: &str, + display_name: &str, + permissions: &std::collections::BTreeMap< + String, + crate::compile::types::GithubAppPermissionLevel, + >, ) -> Result { cfg.validate()?; // The App ID is a non-secret literal (numeric App ID or alphanumeric client @@ -571,7 +588,7 @@ pub fn github_app_token_step_typed( // pipeline variable can redirect the minted token. format!( "--output-var {}", - sh_single_quote(crate::engine::GITHUB_APP_TOKEN_VAR) + sh_single_quote(output_var) ), ]; if !cfg.repositories.is_empty() { @@ -583,11 +600,20 @@ pub fn github_app_token_step_typed( if let Some(api_url) = &cfg.api_url { args.push(format!("--api-url {}", sh_single_quote(api_url))); } + if !permissions.is_empty() { + let normalized: std::collections::BTreeMap = permissions + .iter() + .map(|(name, level)| (name.replace('-', "_"), level.as_str().to_string())) + .collect(); + let json = serde_json::to_string(&normalized) + .context("serialize GitHub App token permissions")?; + args.push(format!("--permissions-json {}", sh_single_quote(&json))); + } let script = format!( "set -eo pipefail\nnode '{GITHUB_APP_TOKEN_PATH}' {}\n", args.join(" ") ); - let step = BashStep::new("Mint GitHub App token (Copilot engine auth)", script) + let step = BashStep::new(display_name, script) .with_condition(Condition::Succeeded) // Only the secret rides in env — masked, never on the command line. Its // variable name is the `private-key` override or the default @@ -684,6 +710,18 @@ pub fn prepare_pr_base_step_typed(mode: PreparePrBaseMode, repos: &[PreparePrBas /// non-secret inputs are argv, not env). pub fn github_app_token_revoke_step_typed( cfg: &crate::compile::types::GithubAppTokenConfig, +) -> Result { + github_app_token_revoke_step_typed_for( + cfg, + crate::engine::GITHUB_APP_TOKEN_VAR, + "Revoke GitHub App token", + ) +} + +pub fn github_app_token_revoke_step_typed_for( + cfg: &crate::compile::types::GithubAppTokenConfig, + token_var: &str, + display_name: &str, ) -> Result { // Validate for symmetry with the mint step, so neither function silently // assumes the other ran first (e.g. if a future caller emits revoke alone). @@ -699,12 +737,12 @@ pub fn github_app_token_revoke_step_typed( None => String::new(), }; let script = format!("node '{GITHUB_APP_TOKEN_PATH}' revoke{api_url_arg}\n"); - let step = BashStep::new("Revoke GitHub App token", script) + let step = BashStep::new(display_name, script) .with_condition(Condition::Always) .with_continue_on_error(true) .with_env( "GH_APP_TOKEN", - EnvValue::secret(crate::engine::GITHUB_APP_TOKEN_VAR), + EnvValue::secret(token_var), ); Ok(Step::Bash(step)) } @@ -1300,6 +1338,7 @@ mod tests { repositories: vec!["octo-repo".to_string(), "other-repo".to_string()], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { panic!("expected a bash step"); @@ -1366,6 +1405,7 @@ mod tests { repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { panic!("expected a bash step"); @@ -1376,6 +1416,36 @@ mod tests { )); } + #[test] + fn github_app_token_step_emits_permission_subset() { + use crate::compile::types::{GithubAppPermissionLevel, GithubAppTokenConfig}; + let cfg = GithubAppTokenConfig { + app_id: "1234567".to_string(), + private_key: None, + owner: "octo-org".to_string(), + repositories: vec!["octo-repo".to_string()], + api_url: None, + skip_token_revocation: false, + permissions: std::collections::BTreeMap::from([ + ("issues".to_string(), GithubAppPermissionLevel::Read), + ( + "pull-requests".to_string(), + GithubAppPermissionLevel::Read, + ), + ]), + }; + let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { + panic!("expected bash step"); + }; + assert!( + step.script.contains( + "--permissions-json '{\"issues\":\"read\",\"pull_requests\":\"read\"}'" + ), + "permissions must be deterministic normalized JSON:\n{}", + step.script + ); + } + #[test] fn github_app_token_step_accepts_client_id_app_id() { use crate::compile::types::GithubAppTokenConfig; @@ -1388,6 +1458,7 @@ mod tests { repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { panic!("expected a bash step"); @@ -1409,6 +1480,7 @@ mod tests { repositories: vec![], api_url: Some("https://ghe.example.com/api/v3".to_string()), skip_token_revocation: false, + permissions: Default::default(), }; let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { panic!("expected a bash step"); @@ -1438,6 +1510,7 @@ mod tests { repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let Step::Bash(step) = github_app_token_step_typed(&cfg).unwrap() else { panic!("expected a bash step"); @@ -1455,6 +1528,7 @@ mod tests { repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let err = github_app_token_step_typed(&cfg).unwrap_err(); assert!( @@ -1474,6 +1548,7 @@ mod tests { repositories: vec![], api_url: Some("https://ghe.example.com/api/v3".to_string()), skip_token_revocation: false, + permissions: Default::default(), }; let Step::Bash(step) = github_app_token_revoke_step_typed(&cfg).unwrap() else { panic!("expected a bash step"); @@ -1513,6 +1588,7 @@ mod tests { repositories: vec![], api_url: Some("http://insecure.example.com/api/v3".to_string()), skip_token_revocation: false, + permissions: Default::default(), }; let err = github_app_token_revoke_step_typed(&cfg).unwrap_err(); assert!( diff --git a/src/compile/mod.rs b/src/compile/mod.rs index bc522ee8..89fc2ff4 100644 --- a/src/compile/mod.rs +++ b/src/compile/mod.rs @@ -192,6 +192,16 @@ async fn compile_pipeline_inner( if let Some(advisory) = crate::engine::github_app_token_secrecy_advisory(&front_matter.engine) { eprintln!("Warning: {advisory}"); } + if front_matter.safe_outputs.contains_key("github-app") + && let Some(crate::compile::types::GithubSafeOutputsAuth::App { config }) = + front_matter.github_safe_outputs_auth()? + { + eprintln!( + "Warning: safe-outputs.github-app uses pipeline variable '{}' for the GitHub App \ + private key. Ensure it is stored as a SECRET.", + config.private_key_var() + ); + } // Determine output path. By default use `.lock.yml` to match // gh-aw's convention for compiled-pipeline files (so they can be diff --git a/src/compile/types.rs b/src/compile/types.rs index 1b38b280..faa19326 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -382,8 +382,9 @@ pub struct EngineOptions { /// `github-app-token` ado-script bundle) immediately before the Copilot /// invocation in the Agent and Detection jobs. The minted GitHub App /// installation token is wired into `GITHUB_TOKEN` for the Copilot engine - /// env only — never SafeOutputs, user steps, ManualReview, Teardown, or - /// Conclusion. Absent ⇒ `GITHUB_TOKEN` is sourced from the + /// env only. GitHub issue SafeOutputs may reuse the App credentials to mint + /// a separate scoped Stage 3 token; they never receive the engine token. + /// Absent ⇒ `GITHUB_TOKEN` is sourced from the /// `$(GITHUB_TOKEN)` pipeline variable as before. #[serde(default, rename = "github-app-token")] #[sanitize_config(skip)] @@ -420,14 +421,34 @@ pub struct EngineOptions { /// repositories: [octo-repo] # optional; scopes the installation token /// # private-key: MY_SECRET # optional; defaults to GITHUB_APP_PRIVATE_KEY /// ``` -#[derive(Debug, Deserialize, Clone, SanitizeConfig)] +#[derive(Debug, Deserialize, Serialize, Clone, Copy, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +pub enum GithubAppPermissionLevel { + Read, + Write, +} + +impl GithubAppPermissionLevel { + pub fn as_str(self) -> &'static str { + match self { + Self::Read => "read", + Self::Write => "write", + } + } +} + +#[derive(Debug, Deserialize, Serialize, Clone, SanitizeConfig)] pub struct GithubAppTokenConfig { /// The GitHub App ID — a **literal** value, either a numeric App ID /// (e.g. `1234567`, quoted or unquoted) or an alphanumeric client ID /// (e.g. `Iv23liABC…`). The App ID is not secret (it is visible in the /// App's settings and is the JWT `iss`), so it is plain per-app config like /// `owner` — it is emitted verbatim, never indirected through a variable. - #[serde(rename = "app-id", deserialize_with = "de_string_or_number")] + #[serde( + rename = "app-id", + alias = "client-id", + deserialize_with = "de_string_or_number" + )] pub app_id: String, /// Optional name of the ADO **secret** pipeline variable holding the GitHub /// App private key (PEM). Defaults to @@ -458,6 +479,45 @@ pub struct GithubAppTokenConfig { /// valid for its full ~1h lifetime. #[serde(default, rename = "skip-token-revocation")] pub skip_token_revocation: bool, + /// Optional repository-permission subset requested at token mint time. + #[serde(default)] + #[sanitize_config(skip)] + pub permissions: std::collections::BTreeMap, +} + +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"; + +#[derive(Debug, Clone)] +pub enum GithubSafeOutputsAuth { + Token { variable: String, api_url: String }, + App { config: GithubAppTokenConfig }, +} + +impl GithubSafeOutputsAuth { + pub fn executor_token_var(&self) -> &str { + match self { + Self::Token { variable, .. } => variable, + Self::App { .. } => SAFE_OUTPUTS_GITHUB_APP_TOKEN_VAR, + } + } + + pub fn api_url(&self) -> &str { + match self { + Self::Token { api_url, .. } => api_url, + Self::App { config } => config + .api_url + .as_deref() + .unwrap_or("https://api.github.com"), + } + } + + pub fn app_config(&self) -> Option<&GithubAppTokenConfig> { + match self { + Self::App { config } => Some(config), + Self::Token { .. } => None, + } + } } /// Default name of the ADO secret pipeline variable holding the GitHub App @@ -513,6 +573,10 @@ impl GithubAppTokenConfig { /// each `repositories` entry must be a single safe path segment; `api-url` /// (when set) must be an `https://` URL with a host. pub fn validate(&self) -> anyhow::Result<()> { + self.validate_for("engine.github-app-token") + } + + pub fn validate_for(&self, path: &str) -> anyhow::Result<()> { use crate::validate::{is_safe_path_segment, is_valid_ado_variable_name}; if self.app_id.is_empty() || !self.app_id.starts_with(|c: char| c.is_ascii_alphanumeric()) @@ -522,7 +586,7 @@ impl GithubAppTokenConfig { .all(|c| c.is_ascii_alphanumeric() || matches!(c, '.' | '_' | '-')) { anyhow::bail!( - "engine.github-app-token.app-id '{}' must be a non-empty GitHub App ID \ + "{path}.app-id '{}' must be a non-empty GitHub App ID \ (numeric, e.g. 1234567) or client ID (e.g. Iv23liABC): it must start with \ an alphanumeric character and contain only [A-Za-z0-9._-]. It is a literal \ value, not a variable name (a leading '-', e.g. a negative number, is invalid).", @@ -533,7 +597,7 @@ impl GithubAppTokenConfig { && !is_valid_ado_variable_name(private_key) { anyhow::bail!( - "engine.github-app-token.private-key '{}' must be an ADO variable name \ + "{path}.private-key '{}' must be an ADO variable name \ starting with a letter, digit, or '_' and containing only letters, digits, \ '.', '_', or '-' (it names the secret variable holding the PEM; omit it to \ use the default '{}').", @@ -543,7 +607,7 @@ impl GithubAppTokenConfig { } if !is_safe_path_segment(&self.owner) { anyhow::bail!( - "engine.github-app-token.owner '{}' is not a valid GitHub owner name \ + "{path}.owner '{}' is not a valid GitHub owner name \ (allowed: [A-Za-z0-9._-], no '/', no leading '.').", self.owner ); @@ -551,7 +615,7 @@ impl GithubAppTokenConfig { for repo in &self.repositories { if !is_safe_path_segment(repo) { anyhow::bail!( - "engine.github-app-token.repositories entry '{}' is not a valid \ + "{path}.repositories entry '{}' is not a valid \ GitHub repository name (allowed: [A-Za-z0-9._-], no '/', no \ leading '.').", repo @@ -559,21 +623,47 @@ impl GithubAppTokenConfig { } } if let Some(api_url) = &self.api_url { + crate::validate::reject_pipeline_injection( + api_url, + &format!("{path}.api-url"), + )?; let parsed = url::Url::parse(api_url).map_err(|e| { anyhow::anyhow!( - "engine.github-app-token.api-url '{}' is not a valid URL: {}", + "{path}.api-url '{}' is not a valid URL: {}", api_url, e ) })?; if parsed.scheme() != "https" || parsed.host_str().is_none() { anyhow::bail!( - "engine.github-app-token.api-url '{}' must be an https:// URL with a host \ + "{path}.api-url '{}' must be an https:// URL with a host \ (e.g. https://ghe.example.com/api/v3).", api_url ); } } + for permission in self.permissions.keys() { + if matches!( + permission.as_str(), + "__proto__" | "prototype" | "constructor" + ) { + anyhow::bail!( + "{path}.permissions key '{}' is reserved and cannot be used", + permission + ); + } + if permission.is_empty() + || !permission + .chars() + .all(|c| c.is_ascii_lowercase() || c.is_ascii_digit() || matches!(c, '-' | '_')) + { + anyhow::bail!( + "{path}.permissions key '{}' must contain only lowercase ASCII letters, \ + digits, '-' or '_'", + permission + ); + } + } Ok(()) } } @@ -1094,11 +1184,8 @@ pub struct FrontMatter { /// Per-tool configuration for safe outputs #[serde(default, rename = "safe-outputs")] pub safe_outputs: HashMap, - /// Debug-only configuration. Top-level section that gates features only - /// intended for ado-aw dogfood/debug pipelines (e.g., `create-issue` for - /// filing failure reports back to GitHub during local testing). NOT a - /// regular safe-output section — anything declared here is omitted from - /// the regular safe-outputs documentation surface. + /// Debug-only configuration. Top-level section for compiler dogfood knobs + /// that are not part of the regular safe-output surface. #[serde(default, rename = "ado-aw-debug")] pub ado_aw_debug: Option, /// Unified trigger configuration: schedule, pipeline, PR triggers and filters @@ -1194,7 +1281,13 @@ impl FrontMatter { /// itself rather than naming a safe-output tool. These must never be treated /// as tool names (e.g. in `--enabled-tools`, Stage-3 budgets, or unknown-key /// validation). -pub const SAFE_OUTPUT_RESERVED_KEYS: &[&str] = &["require-approval"]; +pub const SAFE_OUTPUT_RESERVED_KEYS: &[&str] = + &[ + "require-approval", + "github-token", + "github-api-url", + "github-app", + ]; /// Automatic action a manual-validation gate takes when its pending period /// elapses with no human response. Mirrors `ManualValidation@1`'s `onTimeout`. @@ -1286,6 +1379,260 @@ impl FrontMatter { self.safe_output_tool_names().next().is_some() } + pub fn has_github_issue_outputs(&self) -> bool { + self.safe_outputs.contains_key("create-issue") + || self.safe_outputs.contains_key("set-issue-type") + } + + fn typed_safe_output_config(&self, key: &str) -> anyhow::Result> + where + T: serde::de::DeserializeOwned + Default + SanitizeConfigTrait, + { + let Some(raw) = self.safe_outputs.get(key) else { + return Ok(None); + }; + if raw.is_null() { + let mut config = T::default(); + config.sanitize_config_fields(); + return Ok(Some(config)); + } + let mut raw = raw.clone(); + if let Some(object) = raw.as_object_mut() { + object.remove("require-approval"); + } + let mut config: T = serde_json::from_value(raw) + .map_err(|e| anyhow::anyhow!("safe-outputs.{key} has invalid configuration: {e}"))?; + config.sanitize_config_fields(); + Ok(Some(config)) + } + + pub fn create_issue_config( + &self, + ) -> anyhow::Result> { + self.typed_safe_output_config("create-issue") + } + + pub fn set_issue_type_config( + &self, + ) -> anyhow::Result> { + self.typed_safe_output_config("set-issue-type") + } + + fn parse_safe_outputs_github_token(raw: &str) -> anyhow::Result { + let variable = raw + .strip_prefix("$(") + .and_then(|value| value.strip_suffix(')')) + .filter(|value| !value.is_empty()) + .ok_or_else(|| { + anyhow::anyhow!( + "safe-outputs.github-token must be exactly one ADO secret-variable macro \ + such as `$(ADO_AW_GITHUB_TOKEN)`" + ) + })?; + if !crate::validate::is_valid_ado_variable_name(variable) { + anyhow::bail!( + "safe-outputs.github-token references invalid ADO variable name '{}'", + variable + ); + } + if variable.eq_ignore_ascii_case("GITHUB_TOKEN") + || variable.eq_ignore_ascii_case(crate::engine::GITHUB_APP_TOKEN_VAR) + { + anyhow::bail!( + "safe-outputs.github-token must reference a dedicated Stage 3 secret, not \ + the Agent/Detection credential '{}'", + variable + ); + } + Ok(variable.to_string()) + } + + fn parse_safe_outputs_github_api_url(raw: Option<&serde_json::Value>) -> anyhow::Result { + let Some(raw) = raw else { + return Ok("https://api.github.com".to_string()); + }; + let raw = raw.as_str().ok_or_else(|| { + anyhow::anyhow!("safe-outputs.github-api-url must be an https:// URL string") + })?; + crate::validate::reject_pipeline_injection(raw, "safe-outputs.github-api-url")?; + let parsed = url::Url::parse(raw).map_err(|e| { + anyhow::anyhow!("safe-outputs.github-api-url '{}' is invalid: {}", raw, e) + })?; + if parsed.scheme() != "https" || parsed.host_str().is_none() { + anyhow::bail!( + "safe-outputs.github-api-url '{}' must be an https:// URL with a host", + raw + ); + } + Ok(parsed.to_string().trim_end_matches('/').to_string()) + } + + fn scope_github_app_to_issue_targets( + &self, + mut config: GithubAppTokenConfig, + path: &str, + ) -> anyhow::Result { + if let Some(api_url) = config.api_url.take() { + let parsed = url::Url::parse(&api_url) + .map_err(|e| anyhow::anyhow!("{path}.api-url is invalid: {e}"))?; + config.api_url = Some(parsed.to_string().trim_end_matches('/').to_string()); + } + let mut targets = Vec::new(); + let mut has_implicit_target = false; + if self.safe_outputs.contains_key("create-issue") { + match self + .create_issue_config()? + .and_then(|config| config.target_repo) + { + Some(target) => targets.push(target), + None => has_implicit_target = true, + } + } + if self.safe_outputs.contains_key("set-issue-type") { + match self + .set_issue_type_config()? + .and_then(|config| config.target_repo) + { + Some(target) + if !targets + .iter() + .any(|existing| existing.eq_ignore_ascii_case(&target)) => + { + targets.push(target); + } + Some(_) => {} + None => has_implicit_target = true, + } + } + if has_implicit_target && !targets.is_empty() { + anyhow::bail!( + "GitHub App-backed create-issue and set-issue-type cannot mix implicit current \ + repository targets with explicit target-repo values; configure target-repo \ + consistently for both tools" + ); + } + + if targets.is_empty() { + if config.repositories.is_empty() { + anyhow::bail!( + "{path}.repositories must be set when GitHub issue outputs omit target-repo; \ + the compiler cannot safely scope the App token to a runtime repository" + ); + } + return Ok(config); + } + + let mut repositories = Vec::new(); + for target in targets { + crate::safe_outputs::validate_target_repo(&target) + .map_err(|e| anyhow::anyhow!("safe-outputs target-repo: {e}"))?; + let (owner, repository) = target + .split_once('/') + .expect("validated target-repo contains slash"); + if !owner.eq_ignore_ascii_case(&config.owner) { + anyhow::bail!( + "{path}.owner '{}' does not match target repository owner '{}'", + config.owner, + owner + ); + } + if !repositories + .iter() + .any(|existing: &String| existing.eq_ignore_ascii_case(repository)) + { + repositories.push(repository.to_string()); + } + } + config.repositories = repositories; + Ok(config) + } + + pub fn github_safe_outputs_auth( + &self, + ) -> anyhow::Result> { + if !self.has_github_issue_outputs() { + return Ok(None); + } + + let explicit_token = self.safe_outputs.get("github-token"); + let explicit_api_url = self.safe_outputs.get("github-api-url"); + let explicit_app = self.safe_outputs.get("github-app"); + if explicit_token.is_some() && explicit_app.is_some() { + anyhow::bail!( + "safe-outputs.github-token and safe-outputs.github-app are mutually exclusive" + ); + } + + if let Some(raw) = explicit_app { + if explicit_api_url.is_some() { + anyhow::bail!( + "safe-outputs.github-api-url applies only to PAT auth; set \ + safe-outputs.github-app.api-url for GitHub App auth" + ); + } + let config: GithubAppTokenConfig = serde_json::from_value(raw.clone()).map_err(|e| { + anyhow::anyhow!("safe-outputs.github-app has invalid configuration: {e}") + })?; + config.validate_for("safe-outputs.github-app")?; + if !config.permissions.is_empty() { + anyhow::bail!( + "safe-outputs.github-app.permissions is not supported for GitHub issue \ + outputs; ado-aw derives the minimum `issues: write` permission" + ); + } + let config = + self.scope_github_app_to_issue_targets(config, "safe-outputs.github-app")?; + return Ok(Some(GithubSafeOutputsAuth::App { config })); + } + + if let Some(raw) = explicit_token { + let raw = raw.as_str().ok_or_else(|| { + anyhow::anyhow!( + "safe-outputs.github-token must be a string containing an ADO macro" + ) + })?; + return Ok(Some(GithubSafeOutputsAuth::Token { + variable: Self::parse_safe_outputs_github_token(raw)?, + api_url: Self::parse_safe_outputs_github_api_url(explicit_api_url)?, + })); + } + + if let Some(engine_app) = self.engine.github_app_token() { + if explicit_api_url.is_some() { + anyhow::bail!( + "safe-outputs.github-api-url applies only to PAT auth; set \ + engine.github-app-token.api-url for inherited App auth" + ); + } + engine_app.validate()?; + if engine_app.permissions.is_empty() { + anyhow::bail!( + "GitHub issue safe outputs inherit engine.github-app-token credentials, \ + but engine.github-app-token.permissions is empty. Configure explicit \ + read-only repository permissions or separate SafeOutputs auth." + ); + } + if let Some((name, _)) = engine_app + .permissions + .iter() + .find(|(_, level)| **level == GithubAppPermissionLevel::Write) + { + anyhow::bail!( + "engine.github-app-token.permissions.{name} is `write`; shared App \ + credentials require an explicitly read-only Agent/Detection token" + ); + } + let config = self + .scope_github_app_to_issue_targets(engine_app.clone(), "engine.github-app-token")?; + return Ok(Some(GithubSafeOutputsAuth::App { config })); + } + + Ok(Some(GithubSafeOutputsAuth::Token { + variable: DEFAULT_SAFE_OUTPUTS_GITHUB_TOKEN_VAR.to_string(), + api_url: Self::parse_safe_outputs_github_api_url(explicit_api_url)?, + })) + } + /// The parsed, sanitized `create-pull-request` config, or `None` when the /// tool is not configured. Mirrors Stage 3's `ExecutionContext::get_tool_config` /// (deserialize + `sanitize_config_fields`) so the compiler resolves per-repo @@ -1604,14 +1951,9 @@ pub struct PermissionsConfig { /// Debug-only configuration block. /// /// Lives under the `ado-aw-debug:` top-level front-matter key. Holds knobs -/// that only make sense for pipelines we're actively dogfooding from -/// `githubnext/ado-aw` and that we explicitly do **not** want to advertise -/// as part of the regular agent surface. -/// -/// Adding a new field: pair the front-matter knob with a corresponding -/// compile-side hook (e.g., a debug-only safe output should also be added -/// to `crate::safe_outputs::DEBUG_ONLY_TOOLS` so the MCP layer enforces a -/// matching default-deny gate). +/// that only make sense for pipelines we're actively dogfooding and that we +/// explicitly do **not** want to advertise as part of the regular agent +/// surface. #[derive(Debug, Deserialize, Clone, Default)] #[serde(deny_unknown_fields)] pub struct AdoAwDebugConfig { @@ -1620,20 +1962,11 @@ pub struct AdoAwDebugConfig { /// CLI flag. #[serde(default, rename = "skip-integrity")] pub skip_integrity: bool, - - /// Configuration for the debug-only `create-issue` safe output. - /// Presence of this field is what enables the tool — when omitted - /// the SafeOutputs MCP layer hides it via `DEBUG_ONLY_TOOLS`. - #[serde(default, rename = "create-issue")] - pub create_issue: Option, } impl SanitizeConfigTrait for AdoAwDebugConfig { fn sanitize_config_fields(&mut self) { // skip_integrity: bool — nothing to sanitize - if let Some(ref mut ci) = self.create_issue { - ci.sanitize_config_fields(); - } } } @@ -3241,6 +3574,110 @@ github-app-token: gat.validate().unwrap(); } + #[test] + fn github_issue_outputs_default_to_separate_stage3_pat() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let auth = fm.github_safe_outputs_auth().unwrap().unwrap(); + assert!(matches!( + auth, + GithubSafeOutputsAuth::Token { variable, .. } + if variable == DEFAULT_SAFE_OUTPUTS_GITHUB_TOKEN_VAR + )); + } + + #[test] + fn github_issue_outputs_accept_explicit_pat_macro() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-token: $(MY_ISSUES_TOKEN)\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let auth = fm.github_safe_outputs_auth().unwrap().unwrap(); + assert!(matches!( + auth, + GithubSafeOutputsAuth::Token { variable, .. } if variable == "MY_ISSUES_TOKEN" + )); + } + + #[test] + fn github_issue_outputs_reject_literal_pat() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-token: literal-secret\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let error = fm.github_safe_outputs_auth().unwrap_err().to_string(); + assert!(error.contains("exactly one ADO secret-variable macro")); + } + + #[test] + fn github_issue_outputs_reject_agent_github_token_variable() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-token: $(GITHUB_TOKEN)\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let error = fm.github_safe_outputs_auth().unwrap_err().to_string(); + assert!(error.contains("dedicated Stage 3 secret")); + } + + #[test] + fn github_issue_pat_accepts_explicit_api_url() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-api-url: https://ghe.example.com/api/v3/\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let auth = fm.github_safe_outputs_auth().unwrap().unwrap(); + assert_eq!(auth.api_url(), "https://ghe.example.com/api/v3"); + } + + #[test] + fn github_issue_pat_rejects_api_url_injection() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-api-url: \"https://api.github.com/\\n BASH_ENV: $(EVIL)\"\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let error = fm.github_safe_outputs_auth().unwrap_err().to_string(); + assert!( + error.contains("ADO expression") || error.contains("newlines"), + "unexpected error: {error}" + ); + } + + #[test] + fn github_issue_outputs_require_read_only_engine_app_permissions() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nengine:\n id: copilot\n github-app-token:\n app-id: 123\n owner: octo\n repositories: [repo]\nsafe-outputs:\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let error = fm.github_safe_outputs_auth().unwrap_err().to_string(); + assert!(error.contains("permissions is empty")); + } + + #[test] + fn safe_outputs_github_app_accepts_client_id_alias_and_scopes_repo() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-app:\n client-id: Iv23liExample\n owner: octo\n repositories: [broader-repo]\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let auth = fm.github_safe_outputs_auth().unwrap().unwrap(); + let GithubSafeOutputsAuth::App { config } = auth else { + panic!("expected app auth"); + }; + assert_eq!(config.app_id, "Iv23liExample"); + assert_eq!(config.repositories, vec!["repo".to_string()]); + } + + #[test] + fn github_app_rejects_mixed_implicit_and_explicit_issue_targets() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-app:\n client-id: Iv23liExample\n owner: octo\n repositories: [repo]\n create-issue: {}\n set-issue-type:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let error = fm.github_safe_outputs_auth().unwrap_err().to_string(); + assert!(error.contains("cannot mix implicit current repository targets")); + } + #[test] fn test_github_app_token_validate_rejects_bad_app_id() { let gat = GithubAppTokenConfig { @@ -3250,6 +3687,7 @@ github-app-token: repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let err = gat.validate().unwrap_err().to_string(); assert!(err.contains("app-id"), "unexpected error: {err}"); @@ -3285,6 +3723,7 @@ github-app-token: repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let err = gat.validate().unwrap_err().to_string(); assert!(err.contains("private-key"), "unexpected error: {err}"); @@ -3299,6 +3738,7 @@ github-app-token: repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; gat.validate() .expect("hyphenated ADO variable names are valid macro targets"); @@ -3326,6 +3766,7 @@ github-app-token: repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let err = gat.validate().unwrap_err().to_string(); assert!( @@ -3344,6 +3785,7 @@ github-app-token: repositories: vec![], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let err = gat.validate().unwrap_err().to_string(); assert!(err.contains("owner"), "unexpected error: {err}"); @@ -3358,11 +3800,30 @@ github-app-token: repositories: vec!["ok-repo".to_string(), "bad;repo".to_string()], api_url: None, skip_token_revocation: false, + permissions: Default::default(), }; let err = gat.validate().unwrap_err().to_string(); assert!(err.contains("repositories"), "unexpected error: {err}"); } + #[test] + fn test_github_app_token_validate_rejects_reserved_permission_key() { + let gat = GithubAppTokenConfig { + app_id: "1234567".to_string(), + private_key: None, + owner: "octo-org".to_string(), + repositories: vec!["repo".to_string()], + api_url: None, + skip_token_revocation: false, + permissions: std::collections::BTreeMap::from([( + "__proto__".to_string(), + GithubAppPermissionLevel::Read, + )]), + }; + let error = gat.validate().unwrap_err().to_string(); + assert!(error.contains("reserved")); + } + // ─── PermissionsConfig deserialization ─────────────────────────────── #[test] @@ -4036,12 +4497,24 @@ Body let (fm, _) = super::super::common::parse_markdown(content).unwrap(); let debug = fm.ado_aw_debug.expect("ado-aw-debug should parse"); assert!(debug.skip_integrity); - let ci = debug.create_issue.expect("create-issue should parse"); - assert_eq!(ci.target_repo, "githubnext/ado-aw"); + let ci: crate::safe_outputs::CreateIssueConfig = serde_json::from_value( + fm.safe_outputs + .get("create-issue") + .expect("codemod should move create-issue") + .clone(), + ) + .unwrap(); + assert_eq!(ci.target_repo.as_deref(), Some("githubnext/ado-aw")); assert_eq!(ci.title_prefix.as_deref(), Some("[bug] ")); assert_eq!(ci.labels, vec!["pipeline-failure".to_string()]); assert_eq!(ci.allowed_labels, vec!["agent-*".to_string()]); assert_eq!(ci.assignees, vec!["jamesdevine".to_string()]); + assert_eq!( + fm.safe_outputs + .get("github-token") + .and_then(|v| v.as_str()), + Some("$(ADO_AW_DEBUG_GITHUB_TOKEN)") + ); } #[test] @@ -4057,7 +4530,6 @@ Body let (fm, _) = super::super::common::parse_markdown(content).unwrap(); let debug = fm.ado_aw_debug.unwrap(); assert!(!debug.skip_integrity); - assert!(debug.create_issue.is_none()); } #[test] diff --git a/src/execute.rs b/src/execute.rs index 11f9dde4..84c489b6 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -19,7 +19,8 @@ use crate::safe_outputs::{ CreateGitTagResult, CreateIssueResult, CreatePrResult, CreateWikiPageResult, CreateWorkItemResult, ExecutionContext, ExecutionResult, Executor, LinkWorkItemsResult, MissingDataResult, MissingToolResult, NoopResult, QueueBuildResult, ReplyToPrCommentResult, - ReportIncompleteResult, ResolvePrThreadResult, SubmitPrReviewResult, ToolResult, + ReportIncompleteResult, ResolvePrThreadResult, SetIssueTypeResult, SubmitPrReviewResult, + ToolResult, UpdatePrResult, UpdateWikiPageResult, UpdateWorkItemResult, UploadBuildAttachmentResult, UploadPipelineArtifactResult, UploadWorkitemAttachmentResult, }; @@ -109,6 +110,7 @@ pub async fn execute_safe_outputs( ReplyToPrCommentResult, ResolvePrThreadResult, CreateIssueResult, + SetIssueTypeResult, ); let mut results = Vec::new(); @@ -513,7 +515,7 @@ async fn find_tool_executor( if let Some(r) = dispatch_resource_tools(tool_name, entry, ctx).await? { return Ok(Some(r)); } - if let Some(r) = dispatch_debug_tools(tool_name, entry, ctx).await? { + if let Some(r) = dispatch_github_tools(tool_name, entry, ctx).await? { return Ok(Some(r)); } Ok(None) @@ -582,15 +584,15 @@ async fn dispatch_resource_tools( }) } -/// Dispatch debug-only tools (gated by `ado-aw-debug:` front-matter section -/// at compile time and `DEBUG_ONLY_TOOLS` at the MCP layer at runtime). -async fn dispatch_debug_tools( +/// Dispatch GitHub issue tools. +async fn dispatch_github_tools( tool_name: &str, entry: &Value, ctx: &ExecutionContext, ) -> Result> { dispatch_executor_tools!(tool_name, entry, ctx, { "create-issue" => CreateIssueResult, + "set-issue-type" => SetIssueTypeResult, }) } diff --git a/src/inspect/catalog.rs b/src/inspect/catalog.rs index 5a34c866..291c9d78 100644 --- a/src/inspect/catalog.rs +++ b/src/inspect/catalog.rs @@ -251,6 +251,7 @@ fn safe_output_description(name: &str) -> &'static str { } "report-incomplete" => "Parameters for reporting that a task could not be completed", "resolve-pr-thread" => "Parameters for resolving or reactivating a PR review thread", + "set-issue-type" => "Sets or clears the native type on a GitHub issue", "submit-pr-review" => "Parameters for submitting a pull request review", "update-pr" => "Parameters for updating a pull request", "update-wiki-page" => "Parameters for editing a wiki page (agent-provided)", diff --git a/src/main.rs b/src/main.rs index 6c41e657..b356e541 100644 --- a/src/main.rs +++ b/src/main.rs @@ -844,26 +844,6 @@ async fn build_execution_context( }) .map(|(k, v)| (k.clone(), v.clone())) .collect(); - // Merge ado-aw-debug.create-issue config under the same tool_configs map - // so Stage 3's `ctx.get_tool_config::("create-issue")` - // works exactly like every other safe-output. Without this merge the - // executor would only ever see Default::default(). - // - // Crucially, also record `create-issue` in `debug_enabled_tools` so the - // Stage 3 executor can independently enforce the `ado-aw-debug` gate - // — without this, a forged NDJSON entry whose tool name is `create-issue` - // could bypass the MCP-layer default-deny. - if let Some(d) = front_matter.ado_aw_debug.as_ref() - && let Some(ci) = d.create_issue.as_ref() - { - match serde_json::to_value(ci) { - Ok(v) => { - ctx.tool_configs.insert("create-issue".to_string(), v); - ctx.debug_enabled_tools.insert("create-issue".to_string()); - } - Err(e) => log::warn!("Failed to serialize ado-aw-debug.create-issue config: {e}"), - } - } ctx.allowed_repositories = allowed_repositories; // Per-checkout-alias git refs, so Stage 3 can resolve a per-repo // create-pull-request target branch (infer-target-from-checkout-ref). Uses diff --git a/src/mcp.rs b/src/mcp.rs index 221c76ac..baabcf34 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -18,9 +18,10 @@ use crate::safe_outputs::{ MissingDataParams, MissingDataResult, MissingToolParams, MissingToolResult, NoopParams, NoopResult, PIPELINE_ARTIFACT_DEFAULT_MAX_FILE_SIZE, QueueBuildParams, QueueBuildResult, ReplyToPrCommentParams, ReplyToPrCommentResult, ReportIncompleteParams, ReportIncompleteResult, - ResolvePrThreadParams, ResolvePrThreadResult, SubmitPrReviewParams, SubmitPrReviewResult, - ToolResult, UpdatePrParams, UpdatePrResult, UpdateWikiPageParams, UpdateWikiPageResult, - UpdateWorkItemParams, UpdateWorkItemResult, UploadBuildAttachmentParams, + ResolvePrThreadParams, ResolvePrThreadResult, SetIssueTypeParams, SetIssueTypeResult, + SubmitPrReviewParams, SubmitPrReviewResult, ToolResult, UpdatePrParams, UpdatePrResult, + UpdateWikiPageParams, UpdateWikiPageResult, UpdateWorkItemParams, UpdateWorkItemResult, + UploadBuildAttachmentParams, UploadBuildAttachmentResult, UploadPipelineArtifactParams, UploadPipelineArtifactResult, UploadWorkitemAttachmentParams, UploadWorkitemAttachmentResult, anyhow_to_mcp_error, }; @@ -55,7 +56,7 @@ fn generate_short_id() -> String { } // Re-export from tools module -use crate::safe_outputs::{ALWAYS_ON_TOOLS, DEBUG_ONLY_TOOLS}; +use crate::safe_outputs::{ALWAYS_ON_TOOLS, CONFIGURED_ONLY_TOOLS, DEBUG_ONLY_TOOLS}; // ============================================================================ // Git merge-base helpers (used by find_merge_base) @@ -375,12 +376,13 @@ async fn undo_synthetic_commit(git_dir: &std::path::Path) -> Result<(), McpError /// Decide whether a single tool should remain in the router after filtering. /// /// Three categories, evaluated in priority order: -/// - `DEBUG_ONLY_TOOLS` — kept only when the caller explicitly lists the tool. +/// - `DEBUG_ONLY_TOOLS` / `CONFIGURED_ONLY_TOOLS` — kept only when explicitly +/// listed. /// - `ALWAYS_ON_TOOLS` — always kept regardless of the enabled list. /// - Everything else — permissive default when no list is provided; otherwise /// filtered to what the caller requested. fn should_keep_tool(tool_name: &str, enabled_tools: Option<&[String]>) -> bool { - if DEBUG_ONLY_TOOLS.contains(&tool_name) { + if DEBUG_ONLY_TOOLS.contains(&tool_name) || CONFIGURED_ONLY_TOOLS.contains(&tool_name) { enabled_tools .map(|list| list.iter().any(|e| e.as_str() == tool_name)) .unwrap_or(false) @@ -438,7 +440,7 @@ fn apply_tool_filter( ); } else { info!( - "Default tool exposure: {} of {} tools served (debug-only stripped)", + "Default tool exposure: {} of {} tools served", remaining.len(), total ); @@ -705,19 +707,12 @@ impl SafeOutputs { Ok(CallToolResult::success(vec![])) } - /// Debug-only: file a GitHub issue. Default-deny gated via - /// `DEBUG_ONLY_TOOLS` so this route is only reachable when the compiler - /// explicitly lists `create-issue` in `--enabled-tools` (which it does - /// only when the agent's front matter sets `ado-aw-debug.create-issue`). - /// Stage 3 authenticates against GitHub using the - /// `ADO_AW_DEBUG_GITHUB_TOKEN` pipeline variable. + /// File a GitHub issue through the Stage 3 safe-output executor. #[tool( name = "create-issue", - description = "Debug-only: file a GitHub issue against the operator-configured \ -target repository. Provide a concise title and a markdown body. \ -Optional `labels` and `assignees` are subject to operator-controlled allowlists. \ -This tool is gated by the agent's `ado-aw-debug.create-issue` front-matter section \ -and is not available in regular pipelines." + description = "Create a GitHub issue against the operator-configured target repository. \ +Provide the final title and markdown body. Optional labels are subject to the configured \ +allowlist. Set temporary_id when a later safe output must refer to the newly-created issue." )] async fn create_issue( &self, @@ -734,6 +729,22 @@ and is not available in regular pipelines." Ok(CallToolResult::success(vec![])) } + #[tool( + name = "set-issue-type", + description = "Set the native GitHub issue type. issue_number may be a positive issue \ +number or a temporary_id from an earlier create_issue call in the same run. Pass an empty \ +issue_type to clear the current type." + )] + async fn set_issue_type( + &self, + params: Parameters, + ) -> Result { + let result: SetIssueTypeResult = params.0.try_into()?; + let _ = self.write_safe_output_file(&result).await; + info!("Issue type update queued"); + Ok(CallToolResult::success(vec![])) + } + #[tool( name = "comment-on-work-item", description = "Add a comment to an existing Azure DevOps work item. \ @@ -2065,9 +2076,11 @@ mod tests { use crate::safe_outputs::ALL_KNOWN_SAFE_OUTPUTS; let temp_dir = tempfile::tempdir().unwrap(); - // Pass an enable list that includes every debug-only tool so they - // remain in the router for this introspection check. - let enabled: Vec = DEBUG_ONLY_TOOLS.iter().map(|s| s.to_string()).collect(); + let enabled: Vec = DEBUG_ONLY_TOOLS + .iter() + .chain(CONFIGURED_ONLY_TOOLS.iter()) + .map(|s| s.to_string()) + .collect(); let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled)) .await .unwrap(); @@ -2092,7 +2105,7 @@ mod tests { } #[tokio::test] - async fn test_filter_strips_debug_only_when_no_enabled_list() { + async fn test_default_filter_hides_configured_only_github_tools() { let temp_dir = tempfile::tempdir().unwrap(); let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), None) .await @@ -2112,10 +2125,12 @@ mod tests { } // Spot check a regular tool is present in the permissive default. assert!(tool_names.contains(&"create-work-item".to_string())); + assert!(!tool_names.contains(&"create-issue".to_string())); + assert!(!tool_names.contains(&"set-issue-type".to_string())); } #[tokio::test] - async fn test_filter_keeps_debug_only_when_explicitly_enabled() { + async fn test_filter_keeps_create_issue_when_explicitly_enabled() { let temp_dir = tempfile::tempdir().unwrap(); let enabled = vec!["create-issue".to_string()]; let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled)) @@ -2129,13 +2144,14 @@ mod tests { .collect(); assert!( tool_names.contains(&"create-issue".to_string()), - "Explicitly enabled debug-only tool should be present, got: {:?}", + "Explicitly enabled create-issue tool should be present, got: {:?}", tool_names ); + assert!(!tool_names.contains(&"set-issue-type".to_string())); } #[tokio::test] - async fn test_filter_strips_debug_only_when_other_tool_enabled() { + async fn test_filter_strips_create_issue_when_other_tool_enabled() { let temp_dir = tempfile::tempdir().unwrap(); let enabled = vec!["create-work-item".to_string()]; let so = SafeOutputs::new(temp_dir.path(), temp_dir.path(), Some(&enabled)) @@ -2149,7 +2165,7 @@ mod tests { .collect(); assert!( !tool_names.contains(&"create-issue".to_string()), - "Debug-only tool must remain stripped when not in the explicit enable list" + "create-issue must remain filtered when not explicitly enabled" ); assert!(tool_names.contains(&"create-work-item".to_string())); } diff --git a/src/safe_outputs/create_issue.rs b/src/safe_outputs/create_issue.rs index 94a558b1..8ca48a2b 100644 --- a/src/safe_outputs/create_issue.rs +++ b/src/safe_outputs/create_issue.rs @@ -1,15 +1,8 @@ -//! Debug-only `create-issue` safe output. +//! `create-issue` safe output. //! //! Files a GitHub issue against an operator-configured target repository. -//! This is **not** a regular safe output — it is gated entirely by the -//! `ado-aw-debug.create-issue` front-matter section and stripped from the -//! SafeOutputs MCP server unless explicitly enabled (see -//! [`crate::safe_outputs::DEBUG_ONLY_TOOLS`]). -//! -//! Intended use: dogfood pipelines compiled from `githubnext/ado-aw` that need -//! to file failure reports back to GitHub for triage. Stage 3 authenticates -//! with a dedicated PAT exposed via the `ADO_AW_DEBUG_GITHUB_TOKEN` pipeline -//! variable and surfaced through [`ExecutionContext::github_token`]. +//! Stage 3 authenticates with the credential exposed through +//! [`ExecutionContext::github_token`]; Agent and Detection never see it. //! //! Notable design points: //! * `target-repo` is operator-only — the agent never supplies it and cannot @@ -31,6 +24,7 @@ use std::sync::OnceLock; use super::PATH_SEGMENT; use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; use crate::sanitize::{SanitizeContent, sanitize as sanitize_text}; +use crate::secure::GithubTemporaryId; use crate::tool_result; use crate::validate::reject_pipeline_injection; use ado_aw_derive::SanitizeConfig; @@ -52,6 +46,10 @@ pub struct CreateIssueParams { /// GitHub usernames to assign to the issue. #[serde(default)] pub assignees: Vec, + + /// Temporary identifier used by later safe outputs in the same run. + #[serde(default)] + pub temporary_id: Option, } impl Validate for CreateIssueParams { @@ -88,6 +86,8 @@ tool_result! { labels: Vec, #[serde(default)] assignees: Vec, + #[serde(default)] + temporary_id: Option, } } @@ -104,16 +104,14 @@ impl SanitizeContent for CreateIssueResult { } } -/// Operator-side configuration for `ado-aw-debug.create-issue`. -/// -/// Lives under `ado-aw-debug:` rather than `safe-outputs:` to keep the tool -/// out of the regular safe-output surface. +/// Operator-side configuration for `safe-outputs.create-issue`. #[derive(Debug, Clone, Default, SanitizeConfig, Serialize, Deserialize)] #[serde(deny_unknown_fields)] pub struct CreateIssueConfig { - /// Required: target GitHub repository in `owner/repo` form. - #[serde(rename = "target-repo")] - pub target_repo: String, + /// Target GitHub repository in `owner/repo` form. When omitted, Stage 3 + /// resolves the current repository only for GitHub-backed ADO builds. + #[serde(default, rename = "target-repo")] + pub target_repo: Option, /// Optional prefix prepended to every agent-supplied title (e.g. /// `"[pipeline-failure] "`). @@ -137,9 +135,13 @@ pub struct CreateIssueConfig { #[serde(default)] pub assignees: Vec, + /// Require every proposal to include a temporary ID. + #[serde(default, rename = "require-temporary-id")] + #[sanitize_config(skip)] + pub require_temporary_id: bool, + /// Per-run budget (max number of issues filed). Read by the generic - /// budget machinery in `crate::execute`. Stored here so - /// `deny_unknown_fields` accepts it under `ado-aw-debug.create-issue`. + /// budget machinery in `crate::execute`. #[serde(default, skip_serializing_if = "Option::is_none")] #[sanitize_config(skip)] pub max: Option, @@ -165,23 +167,59 @@ fn target_repo_regex() -> &'static Regex { pub(crate) fn validate_target_repo(target_repo: &str) -> anyhow::Result<()> { ensure!( !target_repo.is_empty(), - "ado-aw-debug.create-issue.target-repo is required (expected 'owner/repo')" + "target-repo is required (expected 'owner/repo')" ); ensure!( target_repo_regex().is_match(target_repo), - "ado-aw-debug.create-issue.target-repo '{}' is not in 'owner/repo' format \ + "target-repo '{}' is not in 'owner/repo' format \ (owner: alphanumerics/hyphens; repo: alphanumerics/dots/hyphens/underscores)", target_repo ); if let Some((_owner, repo)) = target_repo.split_once('/') { ensure!( repo != "." && repo != "..", - "ado-aw-debug.create-issue.target-repo repo segment must not be '.' or '..'" + "target-repo repo segment must not be '.' or '..'" ); } Ok(()) } +pub(crate) fn resolve_target_repo( + configured: Option<&str>, + ctx: &ExecutionContext, +) -> Result { + let target = if let Some(target) = configured { + target.to_string() + } else { + let provider = ctx.repository_provider.as_deref().unwrap_or_default(); + if !provider.eq_ignore_ascii_case("github") + && !provider.eq_ignore_ascii_case("githubenterprise") + { + return Err(ExecutionResult::failure( + "target-repo is required when the Azure DevOps pipeline source is not GitHub", + )); + } + if provider.eq_ignore_ascii_case("githubenterprise") + && ctx.github_api_url.eq_ignore_ascii_case("https://api.github.com") + { + return Err(ExecutionResult::failure( + "safe-outputs.github-api-url or GitHub App api-url is required for a \ + GitHub Enterprise source", + )); + } + match ctx.repository_name.clone() { + Some(name) => name, + None => { + return Err(ExecutionResult::failure( + "BUILD_REPOSITORY_NAME is not set; configure target-repo explicitly", + )); + } + } + }; + validate_target_repo(&target).map_err(|e| ExecutionResult::failure(e.to_string()))?; + Ok(target) +} + /// Build the auto-appended traceability footer. /// /// Embeds a stable `` marker so future tooling can locate @@ -250,20 +288,9 @@ impl Executor for CreateIssueResult { self.body.len() ); - // SECURITY GATE: independently of the SafeOutputs MCP filter, refuse - // to act on a `create-issue` NDJSON entry unless the operator - // authorised it via the `ado-aw-debug.create-issue` front-matter - // section. This closes the gap where a forged NDJSON entry (or a - // mis-placed `safe-outputs.create-issue` config) could otherwise - // bypass the MCP-layer default-deny. - if !ctx - .debug_enabled_tools - .contains(::NAME) - { + if !ctx.tool_configs.contains_key("create-issue") { return Ok(ExecutionResult::failure( - "create-issue is a debug-only tool and is not enabled for this \ - pipeline. Configure `ado-aw-debug.create-issue` in front matter \ - to authorise it.", + "create-issue is not configured for this workflow", )); } @@ -271,17 +298,32 @@ impl Executor for CreateIssueResult { Some(t) => t, None => { return Ok(ExecutionResult::failure( - "ADO_AW_DEBUG_GITHUB_TOKEN not set; required by ado-aw-debug.create-issue", + "ADO_AW_GITHUB_TOKEN is not set; configure safe-outputs.github-token \ + or safe-outputs.github-app", )); } }; let config: CreateIssueConfig = ctx.get_tool_config("create-issue"); - debug!("create-issue: target-repo={}", config.target_repo); - - if let Err(e) = validate_target_repo(&config.target_repo) { - return Ok(ExecutionResult::failure(e.to_string())); + if config.require_temporary_id && self.temporary_id.is_none() { + return Ok(ExecutionResult::failure( + "create-issue requires temporary_id because \ + safe-outputs.create-issue.require-temporary-id is true", + )); + } + if let Some(temporary_id) = &self.temporary_id + && ctx.has_resolved_github_issue(temporary_id)? + { + return Ok(ExecutionResult::failure(format!( + "temporary_id '{}' was already used in this run", + temporary_id.canonical() + ))); } + let target_repo = match resolve_target_repo(config.target_repo.as_deref(), ctx) { + Ok(target) => target, + Err(result) => return Ok(result), + }; + debug!("create-issue: target-repo={target_repo}"); // Validate agent-supplied labels against allowed-labels. // Default-deny semantics: an empty list means NO agent labels are @@ -345,13 +387,13 @@ impl Executor for CreateIssueResult { let all_assignees = merge_dedup_strings(&config.assignees, &self.assignees); // Split target-repo only after validation. - let (owner, repo) = config - .target_repo + let (owner, repo) = target_repo .split_once('/') .context("target-repo must be 'owner/repo'")?; let url = format!( - "https://api.github.com/repos/{}/{}/issues", + "{}/repos/{}/{}/issues", + ctx.github_api_url.trim_end_matches('/'), utf8_percent_encode(owner, PATH_SEGMENT), utf8_percent_encode(repo, PATH_SEGMENT), ); @@ -383,7 +425,15 @@ impl Executor for CreateIssueResult { .json() .await .context("Failed to parse GitHub API response")?; - let number = body.get("number").and_then(|v| v.as_i64()).unwrap_or(0); + let Some(number) = body + .get("number") + .and_then(|v| v.as_u64()) + .filter(|number| *number > 0) + else { + return Ok(ExecutionResult::failure( + "GitHub create-issue response contained no positive issue number", + )); + }; let html_url = body .get("html_url") .and_then(|v| v.as_str()) @@ -391,17 +441,25 @@ impl Executor for CreateIssueResult { .to_string(); info!( "Filed GitHub issue {}#{}: {}", - config.target_repo, number, html_url + target_repo, number, html_url ); + if let Some(temporary_id) = &self.temporary_id { + ctx.register_resolved_github_issue( + temporary_id, + crate::safe_outputs::ResolvedGithubIssue { + repository: target_repo.clone(), + number, + url: html_url.clone(), + }, + )?; + } Ok(ExecutionResult::success_with_data( - format!( - "Filed issue {}#{}: {}", - config.target_repo, number, html_url - ), + format!("Filed issue {}#{}: {}", target_repo, number, html_url), serde_json::json!({ "number": number, "url": html_url, - "target_repo": config.target_repo, + "target_repo": target_repo, + "temporary_id": self.temporary_id.as_ref().map(GithubTemporaryId::canonical), }), )) } else { @@ -411,7 +469,8 @@ impl Executor for CreateIssueResult { .unwrap_or_else(|_| "".to_string()); Ok(ExecutionResult::failure(format!( "Failed to file GitHub issue (HTTP {}): {}", - status, body_text + status, + crate::sanitize::neutralize_pipeline_commands(&body_text) ))) } } @@ -427,34 +486,12 @@ mod tests { fn ctx_with_config( config: serde_json::Value, github_token: Option, - ) -> ExecutionContext { - let mut tool_configs: HashMap = HashMap::new(); - tool_configs.insert("create-issue".to_string(), config); - let mut debug_enabled_tools = std::collections::HashSet::new(); - debug_enabled_tools.insert("create-issue".to_string()); - ExecutionContext { - github_token, - tool_configs, - debug_enabled_tools, - working_directory: PathBuf::from("."), - source_directory: PathBuf::from("."), - ..Default::default() - } - } - - /// Build a context that mirrors a forged-NDJSON scenario: tool config is - /// present, but the operator never authorised the debug tool via - /// `ado-aw-debug.create-issue`, so `debug_enabled_tools` is empty. - fn ctx_unauthorized( - config: serde_json::Value, - github_token: Option, ) -> ExecutionContext { let mut tool_configs: HashMap = HashMap::new(); tool_configs.insert("create-issue".to_string(), config); ExecutionContext { github_token, tool_configs, - debug_enabled_tools: std::collections::HashSet::new(), working_directory: PathBuf::from("."), source_directory: PathBuf::from("."), ..Default::default() @@ -467,6 +504,7 @@ mod tests { body: "The agent step failed during stage 1 with a network timeout.".to_string(), labels: vec![], assignees: vec![], + temporary_id: None, } } @@ -519,6 +557,7 @@ mod tests { body: "body\u{0008}with\u{0001}ctl chars (more than 30 characters total)".to_string(), labels: vec!["la\u{0007}bel".to_string()], assignees: vec!["jo\u{0008}hn".to_string()], + temporary_id: None, }; result.sanitize_content_fields(); assert!(!result.title.contains('\u{0007}')); @@ -536,6 +575,7 @@ mod tests { body: "anything".to_string(), labels: vec![], assignees: vec![], + temporary_id: None, }; assert_eq!( result.dry_run_summary(), @@ -570,6 +610,27 @@ mod tests { assert!(validate_target_repo("owner/..").is_err()); } + #[test] + fn resolve_target_repo_uses_current_github_source() { + let ctx = ExecutionContext { + repository_provider: Some("GitHub".to_string()), + repository_name: Some("octo/repo".to_string()), + ..Default::default() + }; + assert_eq!(resolve_target_repo(None, &ctx).unwrap(), "octo/repo"); + } + + #[test] + fn resolve_target_repo_rejects_azure_repos_default() { + let ctx = ExecutionContext { + repository_provider: Some("TfsGit".to_string()), + repository_name: Some("repo".to_string()), + ..Default::default() + }; + let error = resolve_target_repo(None, &ctx).unwrap_err(); + assert!(error.message.contains("target-repo is required")); + } + #[test] fn test_merge_dedup_strings_dedupes_case_insensitively() { let merged = merge_dedup_strings( @@ -593,12 +654,25 @@ mod tests { let exec = result.execute_sanitized(&ctx).await.unwrap(); assert!(!exec.success); assert!( - exec.message.contains("ADO_AW_DEBUG_GITHUB_TOKEN"), - "expected ADO_AW_DEBUG_GITHUB_TOKEN message, got: {}", + exec.message.contains("ADO_AW_GITHUB_TOKEN"), + "expected ADO_AW_GITHUB_TOKEN message, got: {}", exec.message ); } + #[tokio::test] + async fn test_execute_rejects_when_tool_not_configured() { + let params = valid_params(); + let mut result: CreateIssueResult = params.try_into().unwrap(); + let ctx = ExecutionContext { + github_token: Some("token-that-must-not-be-used".to_string()), + ..Default::default() + }; + let exec = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!exec.success); + assert!(exec.message.contains("not configured")); + } + #[tokio::test] async fn test_execute_fails_when_target_repo_invalid() { let params = valid_params(); @@ -616,6 +690,22 @@ mod tests { ); } + #[tokio::test] + async fn test_execute_requires_temporary_id_when_configured() { + let params = valid_params(); + let mut result: CreateIssueResult = params.try_into().unwrap(); + let ctx = ctx_with_config( + serde_json::json!({ + "target-repo": "githubnext/ado-aw", + "require-temporary-id": true + }), + Some("fake-pat".to_string()), + ); + let exec = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!exec.success); + assert!(exec.message.contains("requires temporary_id")); + } + #[tokio::test] async fn test_execute_rejects_disallowed_label() { let params = CreateIssueParams { @@ -666,35 +756,6 @@ mod tests { } } - #[tokio::test] - async fn test_execute_rejects_when_debug_tool_not_authorized() { - // Forged-NDJSON scenario: payload contains a `create-issue` entry - // and ctx.tool_configs has a config under "create-issue" — but the - // operator never set `ado-aw-debug.create-issue`, so - // `debug_enabled_tools` is empty. The executor MUST refuse before - // touching the token. - let params = valid_params(); - let mut result: CreateIssueResult = params.try_into().unwrap(); - let ctx = ctx_unauthorized( - serde_json::json!({"target-repo": "githubnext/ado-aw"}), - Some("token-that-must-not-be-used".to_string()), - ); - let exec = result.execute_sanitized(&ctx).await.unwrap(); - assert!(!exec.success); - assert!( - exec.message.contains("debug-only tool"), - "expected debug-only refusal, got: {}", - exec.message - ); - // Also confirm the message does NOT mention the token — a token - // mention would imply we made it past the gate. - assert!( - !exec.message.contains("GITHUB_TOKEN"), - "executor must refuse before checking token: {}", - exec.message - ); - } - #[tokio::test] async fn test_execute_rejects_agent_label_when_allowed_labels_empty() { // Default-deny: empty allowed-labels means no agent labels allowed. @@ -779,6 +840,7 @@ mod tests { body: "This is a sufficiently long body for the issue parameters.".to_string(), labels: vec!["##vso[task.complete]".to_string()], assignees: vec![], + temporary_id: None, }; let ctx = ctx_with_config( serde_json::json!({ @@ -821,7 +883,7 @@ allowed-labels: ["agent-*"] assignees: [u1] "#; let cfg: CreateIssueConfig = serde_yaml::from_str(yaml).unwrap(); - assert_eq!(cfg.target_repo, "githubnext/ado-aw"); + assert_eq!(cfg.target_repo.as_deref(), Some("githubnext/ado-aw")); assert_eq!(cfg.title_prefix.as_deref(), Some("[bug] ")); assert_eq!(cfg.labels, vec!["a".to_string()]); assert_eq!(cfg.allowed_labels, vec!["agent-*".to_string()]); diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 44f6812e..a84bbc09 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -3156,9 +3156,10 @@ index 0000000..abcdefg source_directory: dir.path().to_path_buf(), working_directory: dir.path().to_path_buf(), tool_configs: std::collections::HashMap::new(), - debug_enabled_tools: std::collections::HashSet::new(), repository_id: Some("repo-id".to_string()), repository_name: Some("test-repo".to_string()), + repository_provider: Some("TfsGit".to_string()), + github_api_url: "https://api.github.com".to_string(), allowed_repositories: std::collections::HashMap::new(), repo_refs: std::collections::HashMap::new(), agent_stats: None, @@ -3170,6 +3171,9 @@ index 0000000..abcdefg source_branch: None, source_branch_name: None, source_version: None, + resolved_github_issues: std::sync::Arc::new(std::sync::Mutex::new( + std::collections::HashMap::new(), + )), triggered_by_build_id: None, triggered_by_definition_name: None, triggered_by_build_number: None, diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index 34318db1..593c0fe5 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -33,18 +33,19 @@ pub const NON_MCP_SAFE_OUTPUT_KEYS: &[&str] = &[]; /// registering a tool. Unlike [`NON_MCP_SAFE_OUTPUT_KEYS`], these are /// deliberately absent from [`ALL_KNOWN_SAFE_OUTPUTS`] (they have no tool type) /// and must be explicitly allowed in `validate_safe_outputs_keys`. -pub const SAFE_OUTPUT_CONFIG_KEYS: &[&str] = &["report-failure-as-work-item"]; +pub const SAFE_OUTPUT_CONFIG_KEYS: &[&str] = &[ + "report-failure-as-work-item", + "github-token", + "github-api-url", + "github-app", +]; -/// Tools that are gated behind `ado-aw-debug:` front-matter sections and must -/// NOT be exposed to a regular pipeline. The SafeOutputs MCP filter strips -/// these even when `enabled_tools` is `None`, so they only become reachable -/// when the compiler explicitly lists them in `--enabled-tools`. -/// -/// Adding a new debug-only tool: register its result type with -/// `tool_result! { write = true, ... }`, add it here, and gate the -/// compiler-side `--enabled-tools` injection on its corresponding -/// `ado-aw-debug.` front-matter section. -pub const DEBUG_ONLY_TOOLS: &[&str] = tool_names![CreateIssueResult]; +/// Future tools gated behind `ado-aw-debug:` front matter. +pub const DEBUG_ONLY_TOOLS: &[&str] = &[]; + +/// Public tools exposed only when explicitly configured in `safe-outputs:`. +pub const CONFIGURED_ONLY_TOOLS: &[&str] = + tool_names![CreateIssueResult, SetIssueTypeResult]; /// All recognised safe-output keys accepted in front matter `safe-outputs:`. /// This is the union of write-requiring tool types and diagnostic tool types. @@ -74,6 +75,8 @@ pub const ALL_KNOWN_SAFE_OUTPUTS: &[&str] = all_safe_output_names![ SubmitPrReviewResult, ReplyToPrCommentResult, ResolvePrThreadResult, + CreateIssueResult, + SetIssueTypeResult, // Always-on diagnostics NoopResult, MissingDataResult, @@ -365,6 +368,7 @@ mod reply_to_pr_comment; mod report_incomplete; mod resolve_pr_thread; mod result; +mod set_issue_type; mod submit_pr_review; mod update_pr; mod update_wiki_page; @@ -392,9 +396,10 @@ pub use reply_to_pr_comment::*; pub use report_incomplete::*; pub use resolve_pr_thread::*; pub use result::{ - ExecutionContext, ExecutionResult, Executor, ToolResult, Validate, anyhow_to_mcp_error, - org_from_url, + ExecutionContext, ExecutionResult, Executor, ResolvedGithubIssue, ToolResult, Validate, + anyhow_to_mcp_error, org_from_url, }; +pub use set_issue_type::*; pub use submit_pr_review::*; pub use update_pr::*; pub use update_wiki_page::*; @@ -437,6 +442,9 @@ mod tests { const { assert!(CreateIssueResult::REQUIRES_WRITE); } + const { + assert!(SetIssueTypeResult::REQUIRES_WRITE); + } const { assert!(CreateWorkItemResult::REQUIRES_WRITE); } diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 12057eea..f2bd577b 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -7,6 +7,7 @@ use std::collections::{HashMap, HashSet}; use std::sync::{Arc, Mutex}; use crate::sanitize::{SanitizeConfig, SanitizeContent}; +use crate::secure::GithubTemporaryId; /// Trait for tool results that include a name field pub trait ToolResult: Serialize { @@ -17,13 +18,14 @@ pub trait ToolResult: Serialize { /// Each tool can override this; the operator can further override via `max` in front matter. const DEFAULT_MAX: u32 = 1; - /// Whether this tool performs write operations against ADO. + /// Whether this tool performs an external write operation. /// - /// The Stage 3 executor always receives a write-capable token via + /// ADO-backed tools receive a write-capable token via /// `SYSTEM_ACCESSTOKEN`: by default the pipeline's built-in /// `$(System.AccessToken)` (scoped by pipeline settings), or /// `$(SC_WRITE_TOKEN)` minted from an ARM service connection when - /// `permissions.write` is configured. + /// `permissions.write` is configured. GitHub-backed tools use the separate + /// Stage 3 GitHub credential. /// /// This flag is informational — used by audit and (historically) by /// the compiler's permission validator. It is NOT a gate. Diagnostic / @@ -42,6 +44,14 @@ pub trait Validate { } } +/// A GitHub issue created earlier in the same Stage 3 execution. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ResolvedGithubIssue { + pub repository: String, + pub number: u64, + pub url: String, +} + /// Context provided to executors during Stage 3 execution #[derive(Debug, Clone)] pub struct ExecutionContext { @@ -58,27 +68,22 @@ pub struct ExecutionContext { /// `$(System.AccessToken)` by default or `$(SC_WRITE_TOKEN)` (ARM-minted) /// when `permissions.write` is configured. pub access_token: Option, - /// GitHub PAT used by debug-only safe outputs (e.g. `ado-aw-debug.create-issue`). - /// Sourced from the `ADO_AW_DEBUG_GITHUB_TOKEN` pipeline variable. Intentionally - /// **separate** from `access_token` (ADO) and from the read-only `GITHUB_TOKEN` - /// the agent sees in Stage 1 — only Stage 3 ever sees this token. + /// GitHub credential used by GitHub safe outputs in Stage 3. pub github_token: Option, + /// GitHub REST API base URL for Stage 3 issue calls. + pub github_api_url: String, /// Working directory for file operations (safe outputs directory) pub working_directory: std::path::PathBuf, /// Source checkout directory (BUILD_SOURCESDIRECTORY) where git repos are checked out pub source_directory: std::path::PathBuf, /// Per-tool configuration, keyed by tool name pub tool_configs: HashMap, - /// Debug-only tools (e.g. `create-issue`) that the operator authorized - /// via the `ado-aw-debug:` front-matter section. Stage 3 executors for - /// `crate::safe_outputs::DEBUG_ONLY_TOOLS` MUST reject NDJSON entries - /// whose tool name is absent from this set — otherwise a forged entry - /// could bypass the MCP-layer default-deny gate. Empty by default. - pub debug_enabled_tools: HashSet, /// Repository ID (from BUILD_REPOSITORY_ID) pub repository_id: Option, /// Repository name (from BUILD_REPOSITORY_NAME) pub repository_name: Option, + /// Repository provider (from BUILD_REPOSITORY_PROVIDER). + pub repository_provider: Option, /// Allowed repositories for PRs: "self" + checkout list aliases /// Maps alias to ADO repo name (e.g., "other-repo" -> "org/other-repo") pub allowed_repositories: HashMap, @@ -181,6 +186,8 @@ pub struct ExecutionContext { /// the `Clone` semantics need to share state. Each `Default` instance /// gets its own fresh empty set, which is correct for tests. pub uploaded_pipeline_artifact_keys: Arc>>, + /// Temporary GitHub issue IDs resolved by successful `create-issue` calls. + pub resolved_github_issues: Arc>>, } impl ExecutionContext { @@ -194,14 +201,61 @@ impl ExecutionContext { &self, tool_name: &str, ) -> T { - let mut config: T = self + let value = self .tool_configs .get(tool_name) - .and_then(|v| serde_json::from_value(v.clone()).ok()) + .cloned() + .map(|mut value| { + if let Some(object) = value.as_object_mut() { + object.remove("require-approval"); + } + value + }); + let mut config: T = value + .and_then(|v| serde_json::from_value(v).ok()) .unwrap_or_default(); config.sanitize_config_fields(); config } + + pub fn has_resolved_github_issue( + &self, + temporary_id: &GithubTemporaryId, + ) -> anyhow::Result { + let issues = self + .resolved_github_issues + .lock() + .map_err(|_| anyhow::anyhow!("temporary GitHub issue map lock poisoned"))?; + Ok(issues.contains_key(&temporary_id.canonical())) + } + + pub fn register_resolved_github_issue( + &self, + temporary_id: &GithubTemporaryId, + issue: ResolvedGithubIssue, + ) -> anyhow::Result<()> { + let id = temporary_id.canonical(); + let mut issues = self + .resolved_github_issues + .lock() + .map_err(|_| anyhow::anyhow!("temporary GitHub issue map lock poisoned"))?; + if issues.contains_key(&id) { + anyhow::bail!("temporary_id '{id}' was already used in this run"); + } + issues.insert(id, issue); + Ok(()) + } + + pub fn resolve_github_issue( + &self, + temporary_id: &GithubTemporaryId, + ) -> anyhow::Result> { + let issues = self + .resolved_github_issues + .lock() + .map_err(|_| anyhow::anyhow!("temporary GitHub issue map lock poisoned"))?; + Ok(issues.get(&temporary_id.canonical()).cloned()) + } } /// Extract the organization name from an Azure DevOps org URL. @@ -246,13 +300,15 @@ impl ExecutionContext { ado_project: env("SYSTEM_TEAMPROJECT"), ado_project_id: env("SYSTEM_TEAMPROJECTID"), access_token: env("SYSTEM_ACCESSTOKEN").or_else(|| env("AZURE_DEVOPS_EXT_PAT")), - github_token: env("ADO_AW_DEBUG_GITHUB_TOKEN"), + github_token: env("ADO_AW_GITHUB_TOKEN"), + 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(), source_directory, tool_configs: HashMap::new(), - debug_enabled_tools: HashSet::new(), repository_id: env("BUILD_REPOSITORY_ID"), repository_name: env("BUILD_REPOSITORY_NAME"), + repository_provider: env("BUILD_REPOSITORY_PROVIDER"), allowed_repositories: HashMap::new(), repo_refs: HashMap::new(), agent_stats: None, @@ -287,6 +343,7 @@ impl ExecutionContext { // Per-run state for upload-pipeline-artifact dedupe. uploaded_pipeline_artifact_keys: Arc::new(Mutex::new(HashSet::new())), + resolved_github_issues: Arc::new(Mutex::new(HashMap::new())), } } } diff --git a/src/safe_outputs/set_issue_type.rs b/src/safe_outputs/set_issue_type.rs new file mode 100644 index 00000000..a16b3bd9 --- /dev/null +++ b/src/safe_outputs/set_issue_type.rs @@ -0,0 +1,431 @@ +//! `set-issue-type` safe output. + +use anyhow::Context; +use log::{debug, info}; +use percent_encoding::utf8_percent_encode; +use schemars::JsonSchema; +use serde::{Deserialize, Serialize}; + +use super::PATH_SEGMENT; +use super::create_issue::{resolve_target_repo, validate_target_repo}; +use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; +use crate::sanitize::{SanitizeContent, sanitize_config}; +use crate::secure::GithubTemporaryId; +use crate::tool_result; +use crate::validate::reject_pipeline_injection; +use ado_aw_derive::SanitizeConfig; + +#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[serde(untagged)] +pub enum GithubIssueNumber { + Number(u64), + Temporary(GithubTemporaryId), +} + +#[derive(Deserialize, JsonSchema)] +pub struct SetIssueTypeParams { + /// Positive GitHub issue number or a temporary ID from create_issue. + pub issue_number: GithubIssueNumber, + /// Native issue type name. An empty string clears the type. + pub issue_type: String, +} + +impl Validate for SetIssueTypeParams { + fn validate(&self) -> anyhow::Result<()> { + if let GithubIssueNumber::Number(number) = self.issue_number { + anyhow::ensure!(number > 0, "issue_number must be positive"); + } + anyhow::ensure!( + self.issue_type.len() <= 128, + "issue_type must be 128 characters or fewer" + ); + reject_pipeline_injection(&self.issue_type, "set-issue-type.issue_type")?; + Ok(()) + } +} + +tool_result! { + name = "set-issue-type", + write = true, + params = SetIssueTypeParams, + default_max = 5, + /// Result of setting or clearing a GitHub issue's native type. + pub struct SetIssueTypeResult { + issue_number: GithubIssueNumber, + issue_type: String, + } +} + +impl SanitizeContent for SetIssueTypeResult { + fn sanitize_content_fields(&mut self) { + self.issue_type = sanitize_config(&self.issue_type); + } +} + +#[derive(Debug, Clone, Default, SanitizeConfig, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct SetIssueTypeConfig { + #[serde(default, rename = "target-repo")] + pub target_repo: Option, + #[serde(default)] + pub allowed: Vec, + #[serde(default, skip_serializing_if = "Option::is_none")] + #[sanitize_config(skip)] + pub max: Option, +} + +#[async_trait::async_trait] +impl Executor for SetIssueTypeResult { + fn dry_run_summary(&self) -> String { + let target = match &self.issue_number { + GithubIssueNumber::Number(number) => format!("#{number}"), + GithubIssueNumber::Temporary(id) => id.canonical(), + }; + if self.issue_type.is_empty() { + format!("clear GitHub issue type on {target}") + } else { + format!("set GitHub issue type on {target} to '{}'", self.issue_type) + } + } + + async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { + if !ctx.tool_configs.contains_key("set-issue-type") { + return Ok(ExecutionResult::failure( + "set-issue-type is not configured for this workflow", + )); + } + + let token = match ctx.github_token.as_ref() { + Some(token) => token, + None => { + return Ok(ExecutionResult::failure( + "ADO_AW_GITHUB_TOKEN is not set; configure safe-outputs.github-token \ + or safe-outputs.github-app", + )); + } + }; + let config: SetIssueTypeConfig = ctx.get_tool_config("set-issue-type"); + + let resolved_type = if self.issue_type.is_empty() { + String::new() + } else if config.allowed.is_empty() { + self.issue_type.clone() + } else if let Some(allowed) = config + .allowed + .iter() + .find(|allowed| allowed.eq_ignore_ascii_case(&self.issue_type)) + { + allowed.clone() + } else { + return Ok(ExecutionResult::failure(format!( + "Issue type '{}' is not in the allowed list: {}", + crate::sanitize::neutralize_pipeline_commands(&self.issue_type), + config.allowed.join(", ") + ))); + }; + + let (target_repo, issue_number) = match &self.issue_number { + GithubIssueNumber::Number(number) => { + let target = match resolve_target_repo(config.target_repo.as_deref(), ctx) { + Ok(target) => target, + Err(result) => return Ok(result), + }; + (target, *number) + } + GithubIssueNumber::Temporary(temporary_id) => { + let Some(issue) = ctx.resolve_github_issue(temporary_id)? else { + return Ok(ExecutionResult::failure(format!( + "temporary issue ID '{}' has not been resolved; create-issue must \ + succeed earlier in the same SafeOutputs job", + temporary_id.canonical() + ))); + }; + if let Some(configured) = config.target_repo.as_deref() { + if let Err(error) = validate_target_repo(configured) { + return Ok(ExecutionResult::failure(error.to_string())); + } + if !configured.eq_ignore_ascii_case(&issue.repository) { + return Ok(ExecutionResult::failure(format!( + "temporary issue ID '{}' resolved to repository '{}', which does \ + not match set-issue-type.target-repo '{}'", + temporary_id.canonical(), + issue.repository, + configured + ))); + } + } + (issue.repository, issue.number) + } + }; + + let (owner, repo) = target_repo + .split_once('/') + .context("target-repo must be 'owner/repo'")?; + let url = format!( + "{}/repos/{}/{}/issues/{}", + ctx.github_api_url.trim_end_matches('/'), + utf8_percent_encode(owner, PATH_SEGMENT), + utf8_percent_encode(repo, PATH_SEGMENT), + issue_number + ); + debug!("PATCHing GitHub issue type at {url}"); + + let response = reqwest::Client::new() + .patch(&url) + .header("Accept", "application/vnd.github+json") + .header("X-GitHub-Api-Version", "2022-11-28") + .header( + "User-Agent", + format!("ado-aw/{}", env!("CARGO_PKG_VERSION")), + ) + .bearer_auth(token) + .json(&serde_json::json!({ "type": resolved_type })) + .send() + .await + .context("Failed to send request to GitHub API")?; + + let status = response.status(); + if !status.is_success() { + let body = response + .text() + .await + .unwrap_or_else(|_| "".to_string()); + return Ok(ExecutionResult::failure(format!( + "Failed to set GitHub issue type (HTTP {}): {}", + status, + crate::sanitize::neutralize_pipeline_commands(&body) + ))); + } + + let action = if resolved_type.is_empty() { + "Cleared".to_string() + } else { + format!("Set to '{resolved_type}'") + }; + info!( + "{} native type for GitHub issue {}#{}", + action, target_repo, issue_number + ); + Ok(ExecutionResult::success_with_data( + format!("{} issue type for {}#{}", action, target_repo, issue_number), + serde_json::json!({ + "number": issue_number, + "target_repo": target_repo, + "issue_type": resolved_type, + }), + )) + } +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::safe_outputs::{CreateIssueParams, ToolResult}; + use std::collections::HashMap; + + #[test] + fn result_name_and_default_budget_match_contract() { + assert_eq!(SetIssueTypeResult::NAME, "set-issue-type"); + assert_eq!(SetIssueTypeResult::DEFAULT_MAX, 5); + } + + #[test] + fn validates_numeric_and_temporary_targets() { + assert!( + SetIssueTypeParams { + issue_number: GithubIssueNumber::Number(1), + issue_type: "Bug".to_string(), + } + .validate() + .is_ok() + ); + assert!( + SetIssueTypeParams { + issue_number: GithubIssueNumber::Temporary( + GithubTemporaryId::parse("#aw_bug1").unwrap() + ), + issue_type: String::new(), + } + .validate() + .is_ok() + ); + } + + #[test] + fn rejects_zero_issue_number() { + let result = SetIssueTypeParams { + issue_number: GithubIssueNumber::Number(0), + issue_type: "Bug".to_string(), + } + .validate(); + assert!(result.is_err()); + } + + #[tokio::test] + async fn create_then_set_type_resolves_temporary_id() { + use wiremock::matchers::{body_json, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/repos/octo/repo/issues")) + .respond_with(ResponseTemplate::new(201).set_body_json(serde_json::json!({ + "number": 42, + "html_url": "https://github.example/octo/repo/issues/42" + }))) + .expect(1) + .mount(&server) + .await; + Mock::given(method("PATCH")) + .and(path("/repos/octo/repo/issues/42")) + .and(body_json(serde_json::json!({ "type": "Bug" }))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .expect(1) + .mount(&server) + .await; + + let mut tool_configs = HashMap::new(); + tool_configs.insert( + "create-issue".to_string(), + serde_json::json!({ + "target-repo": "octo/repo", + "require-temporary-id": true + }), + ); + tool_configs.insert( + "set-issue-type".to_string(), + serde_json::json!({ + "target-repo": "octo/repo", + "allowed": ["Bug", "Feature"] + }), + ); + let ctx = ExecutionContext { + github_token: Some("token".to_string()), + github_api_url: server.uri(), + tool_configs, + ..Default::default() + }; + + let mut create: crate::safe_outputs::CreateIssueResult = CreateIssueParams { + title: "A real issue title".to_string(), + body: "A detailed issue body that is long enough for validation.".to_string(), + labels: vec![], + assignees: vec![], + temporary_id: Some(GithubTemporaryId::parse("#aw_bug1").unwrap()), + } + .try_into() + .unwrap(); + let created = create.execute_sanitized(&ctx).await.unwrap(); + assert!(created.success, "create failed: {}", created.message); + + let mut duplicate: crate::safe_outputs::CreateIssueResult = CreateIssueParams { + title: "A second real issue title".to_string(), + body: "Another detailed issue body that is long enough for validation.".to_string(), + labels: vec![], + assignees: vec![], + temporary_id: Some(GithubTemporaryId::parse("#aw_bug1").unwrap()), + } + .try_into() + .unwrap(); + let duplicate_result = duplicate.execute_sanitized(&ctx).await.unwrap(); + assert!(!duplicate_result.success); + assert!(duplicate_result.message.contains("already used")); + + let mut set_type: SetIssueTypeResult = SetIssueTypeParams { + issue_number: GithubIssueNumber::Temporary( + GithubTemporaryId::parse("aw_bug1").unwrap(), + ), + issue_type: "bug".to_string(), + } + .try_into() + .unwrap(); + let updated = set_type.execute_sanitized(&ctx).await.unwrap(); + assert!(updated.success, "set type failed: {}", updated.message); + assert_eq!( + updated + .data + .as_ref() + .and_then(|data| data["number"].as_u64()), + Some(42) + ); + } + + #[tokio::test] + async fn unresolved_temporary_id_fails_before_http() { + let mut tool_configs = HashMap::new(); + tool_configs.insert( + "set-issue-type".to_string(), + serde_json::json!({"target-repo": "octo/repo"}), + ); + let ctx = ExecutionContext { + github_token: Some("token".to_string()), + tool_configs, + ..Default::default() + }; + let mut result: SetIssueTypeResult = SetIssueTypeParams { + issue_number: GithubIssueNumber::Temporary( + GithubTemporaryId::parse("#aw_missing").unwrap(), + ), + issue_type: "Bug".to_string(), + } + .try_into() + .unwrap(); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!execution.success); + assert!(execution.message.contains("has not been resolved")); + } + + #[tokio::test] + async fn rejects_when_tool_not_configured() { + let ctx = ExecutionContext { + github_token: Some("token-that-must-not-be-used".to_string()), + ..Default::default() + }; + let mut result: SetIssueTypeResult = SetIssueTypeParams { + issue_number: GithubIssueNumber::Number(1), + issue_type: "Bug".to_string(), + } + .try_into() + .unwrap(); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!execution.success); + assert!(execution.message.contains("not configured")); + } + + #[tokio::test] + async fn empty_issue_type_clears_with_rest_patch() { + use wiremock::matchers::{body_json, method, path}; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let server = MockServer::start().await; + Mock::given(method("PATCH")) + .and(path("/repos/octo/repo/issues/7")) + .and(body_json(serde_json::json!({ "type": "" }))) + .respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({}))) + .expect(1) + .mount(&server) + .await; + let mut tool_configs = HashMap::new(); + tool_configs.insert( + "set-issue-type".to_string(), + serde_json::json!({ + "target-repo": "octo/repo", + "allowed": ["Bug"] + }), + ); + let ctx = ExecutionContext { + github_token: Some("token".to_string()), + github_api_url: server.uri(), + tool_configs, + ..Default::default() + }; + let mut result: SetIssueTypeResult = SetIssueTypeParams { + issue_number: GithubIssueNumber::Number(7), + issue_type: String::new(), + } + .try_into() + .unwrap(); + let execution = result.execute_sanitized(&ctx).await.unwrap(); + assert!(execution.success, "clear failed: {}", execution.message); + } +} diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index d343d261..d7ecdb00 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -915,9 +915,10 @@ attachment-type: "agent-artifact" source_directory: working_directory.clone(), working_directory, tool_configs: std::collections::HashMap::new(), - debug_enabled_tools: std::collections::HashSet::new(), repository_id: None, repository_name: None, + repository_provider: None, + github_api_url: "https://api.github.com".to_string(), allowed_repositories: std::collections::HashMap::new(), repo_refs: std::collections::HashMap::new(), agent_stats: None, @@ -929,6 +930,9 @@ attachment-type: "agent-artifact" source_branch: None, source_branch_name: None, source_version: None, + resolved_github_issues: std::sync::Arc::new(std::sync::Mutex::new( + std::collections::HashMap::new(), + )), triggered_by_build_id: None, triggered_by_definition_name: None, triggered_by_build_number: None, diff --git a/src/secure.rs b/src/secure.rs index 0490962c..08be29b9 100644 --- a/src/secure.rs +++ b/src/secure.rs @@ -259,6 +259,42 @@ validated_string! { } } +validated_string! { + /// A temporary GitHub issue identifier used to link safe outputs in one run. + /// + /// Accepts gh-aw's canonical `#aw_` form and the bare `aw_` alias, + /// where `` is 3-12 ASCII alphanumeric/underscore characters. + GithubTemporaryId, "temporary_id", |value: &str, label: &str| { + let bare = value.strip_prefix('#').unwrap_or(value); + let Some(suffix) = bare.strip_prefix("aw_") else { + anyhow::bail!( + "{label} must use `#aw_` followed by 3-12 ASCII alphanumeric/underscore characters" + ); + }; + if !(3..=12).contains(&suffix.len()) + || !suffix + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '_') + { + anyhow::bail!( + "{label} must use `#aw_` followed by 3-12 ASCII alphanumeric/underscore characters" + ); + } + Ok(()) + } +} + +impl GithubTemporaryId { + /// Canonical map/reference form with the leading `#`. + pub fn canonical(&self) -> String { + if self.as_str().starts_with('#') { + self.as_str().to_string() + } else { + format!("#{}", self.as_str()) + } + } +} + validated_string! { /// A DNS-style hostname. HostName, "hostname", |value: &str, label: &str| { diff --git a/tests/codemod_tests.rs b/tests/codemod_tests.rs index 813c0124..890aaabb 100644 --- a/tests/codemod_tests.rs +++ b/tests/codemod_tests.rs @@ -132,6 +132,27 @@ fn compile_migrates_legacy_workspace_marker_in_steps() { ); } +#[test] +fn compile_migrates_debug_create_issue_to_public_safe_output() { + let dir = fresh_temp_dir(); + let original = "---\nname: issue-migration\ndescription: d\nado-aw-debug:\n skip-integrity: true\n create-issue:\n target-repo: octo/repo\n---\nbody\n"; + let source = write_source(dir.path(), original); + let output = run_compile(&source); + assert!( + output.status.success(), + "compile should succeed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let after = fs::read_to_string(&source).expect("re-read source"); + assert!(after.contains("ado-aw-debug:\n skip-integrity: true")); + assert!(after.contains("safe-outputs:")); + assert!(after.contains("github-token: $(ADO_AW_DEBUG_GITHUB_TOKEN)")); + assert_eq!(after.matches("create-issue:").count(), 1); + assert!( + String::from_utf8_lossy(&output.stderr).contains("promote_debug_create_issue") + ); +} + // ─── Healthy compile (no codemods needed) ────────────────────────────────── #[test] diff --git a/tests/compiler-smoke-e2e/README.md b/tests/compiler-smoke-e2e/README.md index 7585db42..ced7cf3a 100644 --- a/tests/compiler-smoke-e2e/README.md +++ b/tests/compiler-smoke-e2e/README.md @@ -104,7 +104,7 @@ them. Every child receives its own secret `GITHUB_TOKEN` for Copilot CLI authentication, using the same credential policy as the release-backed smoke definitions. The candidate failure-reporter child alone additionally receives -`ADO_AW_DEBUG_GITHUB_TOKEN`, with Issues read/write limited to +`ADO_AW_GITHUB_TOKEN`, with Issues read/write limited to `jamesadevine/ado-aw-issues`. Do not put either token in a variable group or on the orchestrator. ADO's server-side definition clone operation does not copy secret values; provision them explicitly. diff --git a/tests/compiler-smoke-e2e/REGISTERED.md b/tests/compiler-smoke-e2e/REGISTERED.md index 2182894c..d0c59f8f 100644 --- a/tests/compiler-smoke-e2e/REGISTERED.md +++ b/tests/compiler-smoke-e2e/REGISTERED.md @@ -47,7 +47,7 @@ and stores the five child definition IDs as non-secret definition variables. Every child definition needs its own secret `GITHUB_TOKEN` for Copilot CLI authentication. Definition `2558` additionally needs -`ADO_AW_DEBUG_GITHUB_TOKEN`; server-side definition cloning does not copy +`ADO_AW_GITHUB_TOKEN`; server-side definition cloning does not copy secret values. No secret values belong in this file. diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 89872480..df4a8780 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -5758,8 +5758,7 @@ fn test_default_pipeline_mounts_az_and_allows_azure_hosts() { /// Compile the `ado-aw-debug-agent.md` fixture and assert the /// front-matter section's compile-time effects: /// 1. The integrity check step is omitted (`skip-integrity: true`). -/// 2. The Stage 3 executor `env:` block exposes -/// `ADO_AW_DEBUG_GITHUB_TOKEN`. +/// 2. The Stage 3 executor `env:` block exposes `ADO_AW_GITHUB_TOKEN`. /// 3. `--enabled-tools create-issue` is wired into the SafeOutputs MCP /// invocation. /// 4. The output is otherwise valid YAML. @@ -5780,17 +5779,46 @@ fn test_compile_ado_aw_debug_fixture() { .expect("Should have executor step"); let after_execute = &compiled[execute_block_start..]; assert!( - after_execute.contains("ADO_AW_DEBUG_GITHUB_TOKEN: $(ADO_AW_DEBUG_GITHUB_TOKEN)"), - "Executor step must expose ADO_AW_DEBUG_GITHUB_TOKEN when ado-aw-debug.create-issue is set: {after_execute}" + after_execute.contains("ADO_AW_GITHUB_TOKEN: $(ADO_AW_GITHUB_TOKEN)"), + "Executor step must expose ADO_AW_GITHUB_TOKEN when safe-outputs.create-issue is set: {after_execute}" ); // --enabled-tools includes create-issue assert!( compiled_has_enabled_tool(&compiled, "create-issue"), - "Compiler must add --enabled-tools create-issue when ado-aw-debug.create-issue is set" + "Compiler must add --enabled-tools create-issue when safe-outputs.create-issue is set" ); } +#[test] +fn test_compile_github_issue_app_fixture_scopes_tokens_by_stage() { + let compiled = compile_fixture("github-issue-app-agent.md"); + assert_valid_yaml(&compiled, "github-issue-app-agent.md"); + assert!(compiled_has_enabled_tool(&compiled, "create-issue")); + assert!(compiled_has_enabled_tool(&compiled, "set-issue-type")); + assert!(compiled.contains("Mint GitHub App token (SafeOutputs)")); + assert!(compiled.contains("--output-var 'ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN'")); + assert!(compiled.contains("--permissions-json '{\"issues\":\"write\"}'")); + assert!(compiled.contains( + "ADO_AW_GITHUB_TOKEN: $(ADO_AW_SAFE_OUTPUTS_GITHUB_APP_TOKEN)" + )); + + let agent_start = compiled.find("- job: Agent").expect("Agent job"); + let detection_start = compiled.find("- job: Detection").expect("Detection job"); + let safe_outputs_start = compiled + .find("- job: SafeOutputs") + .expect("SafeOutputs job"); + for block in [ + &compiled[agent_start..detection_start], + &compiled[detection_start..safe_outputs_start], + ] { + assert!(block.contains( + "--permissions-json '{\"contents\":\"read\",\"issues\":\"read\"}'" + )); + assert!(!block.contains("ADO_AW_GITHUB_TOKEN")); + } +} + /// The example file in `examples/dogfood-failure-reporter.md` must compile /// cleanly. Mirror of the structural smoke test for `examples/sample-agent.md`. #[test] @@ -5808,8 +5836,8 @@ fn test_example_dogfood_failure_reporter_structure() { "Example should start with front matter" ); assert!( - content.contains("ado-aw-debug:"), - "Example should declare ado-aw-debug section" + content.contains("safe-outputs:"), + "Example should declare safe-outputs section" ); assert!( content.contains("create-issue:"), diff --git a/tests/fixtures/ado-aw-debug-agent.md b/tests/fixtures/ado-aw-debug-agent.md index 331caaa3..a67bd190 100644 --- a/tests/fixtures/ado-aw-debug-agent.md +++ b/tests/fixtures/ado-aw-debug-agent.md @@ -1,8 +1,9 @@ --- name: "Ado AW Debug Test Agent" -description: "Fixture exercising the ado-aw-debug.create-issue front matter section" +description: "Fixture exercising skip-integrity plus public GitHub issue output" ado-aw-debug: skip-integrity: true +safe-outputs: create-issue: target-repo: githubnext/ado-aw title-prefix: "[pipeline-failure] " diff --git a/tests/fixtures/github-issue-app-agent.md b/tests/fixtures/github-issue-app-agent.md new file mode 100644 index 00000000..45b4e2b9 --- /dev/null +++ b/tests/fixtures/github-issue-app-agent.md @@ -0,0 +1,29 @@ +--- +name: "GitHub Issue App Test Agent" +description: "Fixture exercising shared GitHub App issue safe outputs" +engine: + id: copilot + github-app-token: + app-id: 1234567 + private-key: GITHUB_APP_PRIVATE_KEY + owner: octo-org + repositories: + - octo-repo + permissions: + contents: read + issues: read +safe-outputs: + create-issue: + target-repo: octo-org/octo-repo + require-temporary-id: true + max: 1 + set-issue-type: + target-repo: octo-org/octo-repo + allowed: + - Bug + - Feature + - Task + max: 5 +--- + +Create an issue and assign its native type. diff --git a/tests/safe-outputs/README.md b/tests/safe-outputs/README.md index c7bea030..011a3c36 100644 --- a/tests/safe-outputs/README.md +++ b/tests/safe-outputs/README.md @@ -134,7 +134,7 @@ In `https://dev.azure.com/msazuresphere/AgentPlayground`: validated by `tests/compiler-smoke-e2e/`. 3. Capture each Pipeline ID and update `REGISTERED.md`. -4. Provision the `ADO_AW_DEBUG_GITHUB_TOKEN` secret (fine-grained PAT, +4. Provision the `ADO_AW_GITHUB_TOKEN` secret (fine-grained PAT, Issues: read/write on `jamesadevine/ado-aw-issues`) on the `smoke-failure-reporter` pipeline **only**. Confirm the staging repository has the `pipeline-failure` and `ado-aw-smoke` labels. diff --git a/tests/safe-outputs/REGISTERED.md b/tests/safe-outputs/REGISTERED.md index 9fad2c47..04a71e58 100644 --- a/tests/safe-outputs/REGISTERED.md +++ b/tests/safe-outputs/REGISTERED.md @@ -12,7 +12,7 @@ cutover. | `azure-cli.md` | `daily around 03:00` | `2546` | Verifies AWF az CLI mount + ADO auth via `AZURE_DEVOPS_EXT_PAT`. | | `noop-target.md` | _no schedule_ | `2547` | Target of the `queue-build` executor-e2e scenario. `E2E_QUEUE_PIPELINE_ID=2547` on executor definition `2550`. | | `janitor.md` | `weekly on monday around 02:00` | `2548` | Prunes `ado-aw-smoke-*` artifacts older than 30 days. | -| `smoke-failure-reporter.md` | `daily around 04:30` | `2549` | Files `[smoke-failure] …` issues on `jamesadevine/ado-aw-issues`. Requires the `ADO_AW_DEBUG_GITHUB_TOKEN` secret pipeline variable, **only on this pipeline**. | +| `smoke-failure-reporter.md` | `daily around 04:30` | `2549` | Files `[smoke-failure] …` issues on `jamesadevine/ado-aw-issues`. Requires the `ADO_AW_GITHUB_TOKEN` secret pipeline variable, **only on this pipeline**. | ## Deterministic E2E definitions @@ -72,14 +72,14 @@ the following one-time setup in (`tests/executor-e2e/azure-pipelines.yml`) with the `noop-target` Pipeline ID from step 3. This enables the `queue-build` deterministic scenario. -5. Provision pipeline variable `ADO_AW_DEBUG_GITHUB_TOKEN` (secret) on +5. Provision pipeline variable `ADO_AW_GITHUB_TOKEN` (secret) on the `smoke-failure-reporter` pipeline **only**. Use a GitHub fine-grained PAT scoped to `Issues: Read and write` on `jamesadevine/ado-aw-issues` only. Confirm the target repository has the `pipeline-failure` and `ado-aw-smoke` labels. ```powershell - ado-aw secrets set ADO_AW_DEBUG_GITHUB_TOKEN ` + ado-aw secrets set ADO_AW_GITHUB_TOKEN ` --org msazuresphere --project AgentPlayground ` --definition-ids ` --value diff --git a/tests/safe-outputs/smoke-failure-reporter.md b/tests/safe-outputs/smoke-failure-reporter.md index b3a94f9c..cccff6c9 100644 --- a/tests/safe-outputs/smoke-failure-reporter.md +++ b/tests/safe-outputs/smoke-failure-reporter.md @@ -13,7 +13,7 @@ engine: permissions: read: agent-playground-read write: agent-playground-write -ado-aw-debug: +safe-outputs: create-issue: target-repo: jamesadevine/ado-aw-issues title-prefix: "[smoke-failure] " @@ -81,7 +81,7 @@ front-matter `name:` values from their source Markdown. - Do **not** call `create-issue` with a `target_repo` parameter. The agent has no override; the target is fixed by the operator at `jamesadevine/ado-aw-issues`. -- The `ADO_AW_DEBUG_GITHUB_TOKEN` PAT is not visible to you. Stage 3 +- The `ADO_AW_GITHUB_TOKEN` PAT is not visible to you. Stage 3 uses it to authenticate against GitHub. After the appropriate `create-issue` calls (or one `report-incomplete` From 218ba879b16260def5b7f96d7b32d8b9541d373f Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 25 Jul 2026 17:00:00 +0100 Subject: [PATCH 2/3] fix(safeoutputs): address PR review feedback Accept quoted numeric GitHub issue numbers with clear deserialization and report target configuration errors before duplicate temporary-ID errors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af --- src/safe_outputs/create_issue.rs | 36 ++++++++++++++--- src/safe_outputs/set_issue_type.rs | 62 +++++++++++++++++++++++++++++- 2 files changed, 92 insertions(+), 6 deletions(-) diff --git a/src/safe_outputs/create_issue.rs b/src/safe_outputs/create_issue.rs index 8ca48a2b..7a5ddf23 100644 --- a/src/safe_outputs/create_issue.rs +++ b/src/safe_outputs/create_issue.rs @@ -311,6 +311,11 @@ impl Executor for CreateIssueResult { safe-outputs.create-issue.require-temporary-id is true", )); } + let target_repo = match resolve_target_repo(config.target_repo.as_deref(), ctx) { + Ok(target) => target, + Err(result) => return Ok(result), + }; + debug!("create-issue: target-repo={target_repo}"); if let Some(temporary_id) = &self.temporary_id && ctx.has_resolved_github_issue(temporary_id)? { @@ -319,11 +324,6 @@ impl Executor for CreateIssueResult { temporary_id.canonical() ))); } - let target_repo = match resolve_target_repo(config.target_repo.as_deref(), ctx) { - Ok(target) => target, - Err(result) => return Ok(result), - }; - debug!("create-issue: target-repo={target_repo}"); // Validate agent-supplied labels against allowed-labels. // Default-deny semantics: an empty list means NO agent labels are @@ -706,6 +706,32 @@ mod tests { assert!(exec.message.contains("requires temporary_id")); } + #[tokio::test] + async fn target_configuration_error_precedes_duplicate_temporary_id() { + let temporary_id = GithubTemporaryId::parse("#aw_dup1").unwrap(); + let mut params = valid_params(); + params.temporary_id = Some(temporary_id.clone()); + let mut result: CreateIssueResult = params.try_into().unwrap(); + let ctx = ctx_with_config( + serde_json::json!({"require-temporary-id": true}), + Some("token".to_string()), + ); + ctx.register_resolved_github_issue( + &temporary_id, + crate::safe_outputs::ResolvedGithubIssue { + repository: "octo/repo".to_string(), + number: 1, + url: "https://github.com/octo/repo/issues/1".to_string(), + }, + ) + .unwrap(); + + let exec = result.execute_sanitized(&ctx).await.unwrap(); + assert!(!exec.success); + assert!(exec.message.contains("target-repo is required")); + assert!(!exec.message.contains("already used")); + } + #[tokio::test] async fn test_execute_rejects_disallowed_label() { let params = CreateIssueParams { diff --git a/src/safe_outputs/set_issue_type.rs b/src/safe_outputs/set_issue_type.rs index a16b3bd9..d85e3bd0 100644 --- a/src/safe_outputs/set_issue_type.rs +++ b/src/safe_outputs/set_issue_type.rs @@ -15,13 +15,60 @@ use crate::tool_result; use crate::validate::reject_pipeline_injection; use ado_aw_derive::SanitizeConfig; -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] +#[derive(Debug, Clone, Serialize, JsonSchema)] #[serde(untagged)] pub enum GithubIssueNumber { Number(u64), Temporary(GithubTemporaryId), } +impl<'de> Deserialize<'de> for GithubIssueNumber { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + struct IssueNumberVisitor; + + impl serde::de::Visitor<'_> for IssueNumberVisitor { + type Value = GithubIssueNumber; + + fn expecting(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + formatter.write_str("a positive issue number or #aw_ temporary issue ID") + } + + fn visit_u64(self, value: u64) -> Result { + Ok(GithubIssueNumber::Number(value)) + } + + fn visit_i64(self, value: i64) -> Result + where + E: serde::de::Error, + { + u64::try_from(value) + .map(GithubIssueNumber::Number) + .map_err(|_| E::custom("issue_number must be positive")) + } + + fn visit_str(self, value: &str) -> Result + where + E: serde::de::Error, + { + if value.chars().all(|c| c.is_ascii_digit()) { + return value + .parse::() + .map(GithubIssueNumber::Number) + .map_err(|_| E::custom("quoted issue_number is outside the u64 range")); + } + GithubTemporaryId::parse(value) + .map(GithubIssueNumber::Temporary) + .map_err(E::custom) + } + } + + deserializer.deserialize_any(IssueNumberVisitor) + } +} + #[derive(Deserialize, JsonSchema)] pub struct SetIssueTypeParams { /// Positive GitHub issue number or a temporary ID from create_issue. @@ -261,6 +308,19 @@ mod tests { assert!(result.is_err()); } + #[test] + fn quoted_numeric_issue_number_deserializes_as_number() { + let params: SetIssueTypeParams = serde_json::from_value(serde_json::json!({ + "issue_number": "42", + "issue_type": "Bug" + })) + .unwrap(); + assert!(matches!( + params.issue_number, + GithubIssueNumber::Number(42) + )); + } + #[tokio::test] async fn create_then_set_type_resolves_temporary_id() { use wiremock::matchers::{body_json, method, path}; From 99a8ae4ac0e4149446a67c5fd2e87c1e9f42bb7a Mon Sep 17 00:00:00 2001 From: James Devine Date: Sat, 25 Jul 2026 17:17:37 +0100 Subject: [PATCH 3/3] fix(safeoutputs): address follow-up review feedback Quote GitHub API URLs in generated YAML, reject URL fragments, make temporary-ID registration failures operator-facing, and document intentional clear semantics. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 48973d5e-a00b-43bc-9ef8-433f66ca12af --- src/compile/common.rs | 11 +++++++---- src/compile/types.rs | 22 ++++++++++++++++++++++ src/safe_outputs/create_issue.rs | 22 +++++++++++++++++++--- src/safe_outputs/result.rs | 3 +++ src/safe_outputs/set_issue_type.rs | 2 ++ 5 files changed, 53 insertions(+), 7 deletions(-) diff --git a/src/compile/common.rs b/src/compile/common.rs index 87376d85..3e63b16a 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -1863,10 +1863,9 @@ pub fn generate_executor_ado_env( "ADO_AW_GITHUB_TOKEN: $({})", github_auth.executor_token_var() )); - lines.push(format!( - "ADO_AW_GITHUB_API_URL: {}", - github_auth.api_url() - )); + 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}")); } // The two-space indent on each value line is the YAML relative indent for // a key nested under `env:`. replace_with_indent prepends the base @@ -5286,6 +5285,10 @@ safe-outputs: result.contains("ADO_AW_GITHUB_TOKEN: $(MY_GITHUB_WRITE_TOKEN)"), "Executor should map the configured Stage 3 GitHub token" ); + assert!( + result.contains("ADO_AW_GITHUB_API_URL: \"https://api.github.com\""), + "GitHub API URL must be emitted as a quoted YAML scalar" + ); assert!( !result.contains("SC_WRITE_TOKEN"), "No write SC means no SC_WRITE_TOKEN" diff --git a/src/compile/types.rs b/src/compile/types.rs index faa19326..8e1added 100644 --- a/src/compile/types.rs +++ b/src/compile/types.rs @@ -641,6 +641,12 @@ impl GithubAppTokenConfig { api_url ); } + if parsed.query().is_some() || parsed.fragment().is_some() { + anyhow::bail!( + "{path}.api-url '{}' must not contain a query string or fragment", + api_url + ); + } } for permission in self.permissions.keys() { if matches!( @@ -1464,6 +1470,12 @@ impl FrontMatter { raw ); } + if parsed.query().is_some() || parsed.fragment().is_some() { + anyhow::bail!( + "safe-outputs.github-api-url '{}' must not contain a query string or fragment", + raw + ); + } Ok(parsed.to_string().trim_end_matches('/').to_string()) } @@ -3644,6 +3656,16 @@ github-app-token: ); } + #[test] + fn github_issue_pat_rejects_api_url_fragment() { + let (fm, _) = super::super::common::parse_markdown( + "---\nname: test\ndescription: test\nsafe-outputs:\n github-api-url: https://api.github.com/api/v3#fragment\n create-issue:\n target-repo: octo/repo\n---\n", + ) + .unwrap(); + let error = fm.github_safe_outputs_auth().unwrap_err().to_string(); + assert!(error.contains("query string or fragment")); + } + #[test] fn github_issue_outputs_require_read_only_engine_app_permissions() { let (fm, _) = super::super::common::parse_markdown( diff --git a/src/safe_outputs/create_issue.rs b/src/safe_outputs/create_issue.rs index 7a5ddf23..107f949c 100644 --- a/src/safe_outputs/create_issue.rs +++ b/src/safe_outputs/create_issue.rs @@ -443,15 +443,31 @@ impl Executor for CreateIssueResult { "Filed GitHub issue {}#{}: {}", target_repo, number, html_url ); - if let Some(temporary_id) = &self.temporary_id { - ctx.register_resolved_github_issue( + if let Some(temporary_id) = &self.temporary_id + && let Err(error) = ctx.register_resolved_github_issue( temporary_id, crate::safe_outputs::ResolvedGithubIssue { repository: target_repo.clone(), number, url: html_url.clone(), }, - )?; + ) + { + return Ok(ExecutionResult::failure_with_data( + format!( + "Filed issue {}#{} but failed to register temporary_id '{}': {}", + target_repo, + number, + temporary_id.canonical(), + crate::sanitize::neutralize_pipeline_commands(&error.to_string()) + ), + serde_json::json!({ + "number": number, + "url": html_url, + "target_repo": target_repo, + "temporary_id": temporary_id.canonical(), + }), + )); } Ok(ExecutionResult::success_with_data( format!("Filed issue {}#{}: {}", target_repo, number, html_url), diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index f2bd577b..4dc2365a 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -206,6 +206,9 @@ impl ExecutionContext { .get(tool_name) .cloned() .map(|mut value| { + // Approval is compiler orchestration metadata, not executor + // configuration. Strip it before typed configs with + // deny_unknown_fields are deserialized. if let Some(object) = value.as_object_mut() { object.remove("require-approval"); } diff --git a/src/safe_outputs/set_issue_type.rs b/src/safe_outputs/set_issue_type.rs index d85e3bd0..0d1efc30 100644 --- a/src/safe_outputs/set_issue_type.rs +++ b/src/safe_outputs/set_issue_type.rs @@ -217,6 +217,8 @@ impl Executor for SetIssueTypeResult { ); debug!("PATCHing GitHub issue type at {url}"); + // gh-aw's set-issue-type contract uses an empty string to clear the + // native type; preserve that wire behavior for front-matter parity. let response = reqwest::Client::new() .patch(&url) .header("Accept", "application/vnd.github+json")