diff --git a/.github/workflows/prompt-contracts.yml b/.github/workflows/prompt-contracts.yml index 83c9e61ac..dd95ccc75 100644 --- a/.github/workflows/prompt-contracts.yml +++ b/.github/workflows/prompt-contracts.yml @@ -32,9 +32,9 @@ jobs: run: cargo test --test prompt_contract_tests --test prompt_eval_contract_tests - name: Install gh-aw - uses: github/gh-aw-actions/setup-cli@ba6380cc6e5be5d21677bebe04d52fb48e3abec7 # v0.81.6 + uses: github/gh-aw-actions/setup-cli@6aab9e5b5c91c615506061f09bedd81a23babe3c # v0.86.2 with: - version: v0.81.6 + version: v0.86.2 - name: Compile prompt evaluator strictly run: | diff --git a/AGENTS.md b/AGENTS.md index 9f231c073..2cd957f00 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -96,6 +96,7 @@ fail-closed and only pauses when the agent actually proposed a reviewed output. │ │ │ └── integration_tests.rs # Import resolution + merge integration tests │ │ ├── extensions/ # CompilerExtension trait and infrastructure extensions │ │ │ ├── mod.rs # Trait, Extension enum, collect_extensions(), re-exports +│ │ │ ├── container_runtime.rs # Typed Docker runtime config shared by MCPG stdio servers (Mount/Network/Tmpfs/AddHost/ContainerUser, ContainerRuntimeConfig) │ │ │ ├── ado_aw_marker.rs # Always-on metadata marker extension (emits # ado-aw-metadata JSON) │ │ │ ├── github.rs # Always-on GitHub MCP extension │ │ │ ├── safe_outputs.rs # Always-on SafeOutputs MCP extension @@ -623,7 +624,7 @@ the directive and it's inert at runtime. To review the generated shell as ordinary files: ```bash -cargo run -- export-bash-scripts --out /tmp/ado-aw-shell +cargo run -- export-bash-scripts --output /tmp/ado-aw-shell ``` ### Markdown-only smoke suite diff --git a/docs/ado-script.md b/docs/ado-script.md index 6d9522038..fa1c726e5 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -82,7 +82,7 @@ pipeline** as runtime helpers. Today it produces the following shipped bundles: the target tip at depth 1. Cross-org repositories are partitioned into a separate trusted credential scope and passed with validated organization/project/repository coordinates. The checkout remote must match - those coordinates exactly before the Bearer reaches REST or git; mismatch or + those coordinates exactly before attempting REST or git; mismatch or preparation failure stops the trusted task before Agent/executor execution. Same-org per-dir failures remain isolated warnings. The bearer remains shell-local or in masked `SYSTEM_ACCESSTOKEN` env and spawned-git @@ -207,7 +207,7 @@ Resolution is single-pass: nested markers inside an inlined body are not re-expanded. The bundle lives at `import.js` and ships in the same -`ado-script.zip` release asset as `gate.js` and the ten +`ado-script.zip` release asset as `gate.js` and the nine `exec-context-*.js` bundles listed in the workspace layout, so pipelines download it through the same Agent-job asset flow. `import.js` uses only the Node standard library, so the ncc bundle is @@ -654,6 +654,18 @@ scripts/ado-script/ │ ├── 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 │ │ └── __tests__/ # unit tests for fetch/deepen + origin/HEAD + benign-failure paths +│ ├── ado-proxy/ # ado-proxy.js entry point + credential-isolated ADO policy proxy +│ │ ├── index.ts # main(): starts the trusted HTTP proxy server +│ │ ├── server.ts # HTTP server, request routing +│ │ ├── policy.ts # scope/capability policy evaluation +│ │ ├── scope.ts # organization-relative current/additional scope index +│ │ ├── catalog.ts # versioned deny-by-default read-operation catalog +│ │ ├── catalog.gen.json # generated by `cargo run -- export-ado-proxy-catalog` +│ │ ├── route.ts / upstream.ts / response.ts / headers.ts / token.ts / config.ts / api-version.ts / ca.ts / log.ts +│ │ └── *.test.ts # per-module unit tests + `proxy.e2e.test.ts` +│ ├── azure-wif-refresh/ # azure-wif-refresh.js entry point + renewable WIF assertion sidecar +│ │ ├── index.ts # main(): rotate a private token file for user-defined stdio MCP servers +│ │ └── __tests__/ # unit tests for rotation and isolation behaviour │ ├── trigger-e2e/ # test-only: FACT_META gate-spec table + trigger-evaluation E2E scenarios (not a bundle) │ │ ├── gate-spec.ts # FACT_META mirror of Rust Fact::ALL; drift-guarded by export-fact-catalog + fact-catalog.gen.json │ │ ├── fact-catalog.gen.json # generated by `cargo run -- export-fact-catalog`; deep-compared by gate-spec.test.ts @@ -675,7 +687,9 @@ scripts/ado-script/ ├── conclusion.js # ncc bundle output (gitignored) ├── approval-summary.js # ncc bundle output (gitignored) ├── github-app-token.js # ncc bundle output (gitignored) -└── prepare-pr-base.js # ncc bundle output (gitignored) +├── prepare-pr-base.js # ncc bundle output (gitignored) +├── ado-proxy.js # ncc bundle output (gitignored) +└── azure-wif-refresh.js # ncc bundle output (gitignored) ``` The release workflow (`.github/workflows/release.yml`) runs @@ -685,8 +699,9 @@ captures every bundle, including `gate.js`, `import.js`, `exec-context-manual.js`, `exec-context-pipeline.js`, `exec-context-ci-push.js`, `exec-context-workitem.js`, `exec-context-schedule.js`, `exec-context-pr-checks.js`, -`exec-context-repo.js`, `conclusion.js`, `approval-summary.js`, and -`github-app-token.js` — into the +`exec-context-repo.js`, `conclusion.js`, `approval-summary.js`, +`github-app-token.js`, `prepare-pr-base.js`, `ado-proxy.js`, and +`azure-wif-refresh.js` — into the `ado-script.zip` release asset. Pipelines download that asset at runtime by URL pinned to the compiler's `CARGO_PKG_VERSION`, verify its SHA-256 against the `checksums.txt` asset, then extract. diff --git a/docs/conclusion.md b/docs/conclusion.md index a89db81bd..a3968a0cf 100644 --- a/docs/conclusion.md +++ b/docs/conclusion.md @@ -103,6 +103,20 @@ Conclusion reports deduplicate by rendered work-item title. The job searches for an existing open work item with the same title; if it finds one, it appends a comment. Otherwise it creates a new work item. +## Testing + +Unit coverage lives in +`scripts/ado-script/src/conclusion/__tests__/index.test.ts` (manifest parsing, +signal rendering, per-tool config). + +End-to-end coverage lives in the deterministic executor suite +([`tests/executor-e2e/`](../tests/executor-e2e/README.md)): the `conclusion-*` +scenarios run `ado-aw execute` for a `noop` / `missing-tool` / `missing-data` +signal, then run the compiled `conclusion.js` over the resulting +`safe-outputs-executed.ndjson`, and assert the filed Azure DevOps work item +(title, type, tags, body), the append-on-duplicate-title path, and the +`report-as-work-item: false` opt-out. + ## Relationship to gh-aw This mirrors gh-aw's conclusion-job pattern: a single always-running diff --git a/docs/extending.md b/docs/extending.md index 7a84e95d5..af573ae07 100644 --- a/docs/extending.md +++ b/docs/extending.md @@ -457,8 +457,8 @@ ShellScript::new(&START_CONTAINER) ### Reviewing the scripts as files ```bash -cargo run -- export-bash-scripts --out /tmp/ado-aw-shell -cargo run -- export-bash-scripts --out /tmp/ado-aw-shell --format json +cargo run -- export-bash-scripts --output /tmp/ado-aw-shell +cargo run -- export-bash-scripts --output /tmp/ado-aw-shell --format json ``` Writes one `.sh` per registered script with a provenance header naming the diff --git a/docs/ir.md b/docs/ir.md index 1c9263de4..9c9dd6b02 100644 --- a/docs/ir.md +++ b/docs/ir.md @@ -4,7 +4,7 @@ _Part of the [ado-aw documentation](../AGENTS.md)._ ado-aw no longer compiles pipelines by substituting strings into YAML template files. Every production target builds a typed Azure DevOps pipeline IR, resolves graph-level facts, lowers that IR to `serde_yaml::Value`, and serializes once with `serde_yaml::to_string`. -The implementation lives under `src/compile/ir/`. The canonical agentic-pipeline shape (Setup → Agent → Detection → SafeOutputs → Teardown, plus an optional always-running Conclusion job when `conclusion:` is configured) lives in `src/compile/agentic_pipeline.rs` and is shared by every target. Per-target wrappers handle only the envelope: +The implementation lives under `src/compile/ir/`. The canonical agentic-pipeline shape (Setup → Agent → Detection → (ManualReview?) → Custom_\* → SafeOutputs(+SafeOutputs_Reviewed?) → Teardown → Conclusion) lives in `src/compile/agentic_pipeline.rs` and is shared by every target. `ManualReview` is inserted only when a safe output is configured with `require-approval`; the `SafeOutputs`/`SafeOutputs_Reviewed` split occurs only when both gated and non-gated outputs are configured; Conclusion is emitted whenever `safe-outputs:` is configured (there is no separate `conclusion:` front-matter field). Per-target wrappers handle only the envelope: - `src/compile/standalone_ir.rs` - `src/compile/onees_ir.rs` @@ -224,7 +224,11 @@ The extension trait lives in `src/compile/extensions/mod.rs` and now has exactly pub trait CompilerExtension { fn name(&self) -> &str; fn phase(&self) -> ExtensionPhase; - fn declarations(&self, ctx: &CompileContext) -> Result; + /// Default returns `Ok(Declarations::default())` — override when the + /// extension contributes steps, hosts, tools, or other signals. + fn declarations(&self, ctx: &CompileContext) -> Result { + Ok(Declarations::default()) + } } ``` @@ -287,7 +291,7 @@ The production target wrappers are: - `job_ir.rs` — wraps the canonical shape as a target-job template with external `dependsOn` / `condition` template parameters. - `stage_ir.rs` — wraps the canonical shape as a target-stage template with the stage-level external-parameter wrapper. -The canonical Setup → Agent → Detection → SafeOutputs → Teardown shape, plus the optional Conclusion job, lives in `agentic_pipeline.rs` and is reused unchanged by every wrapper above; extensions plug into it via `Declarations` (steps, env, hosts, MCPG entries, and Agent-job condition clauses — see `Declarations::agent_conditions`). +The canonical Setup → Agent → Detection → (ManualReview?) → Custom_\* → SafeOutputs(+SafeOutputs_Reviewed?) → Teardown → Conclusion shape lives in `agentic_pipeline.rs` and is reused unchanged by every wrapper above; extensions plug into it via `Declarations` (steps, env, hosts, MCPG entries, and Agent-job condition clauses — see `Declarations::agent_conditions`). When adding a target, follow the same pattern: parse and validate front matter, collect extension `Declarations`, build typed jobs/stages/steps, set the correct `PipelineShape`, and call the shared emit path. diff --git a/docs/network.md b/docs/network.md index 6d8ff7417..19d503927 100644 --- a/docs/network.md +++ b/docs/network.md @@ -6,7 +6,7 @@ _Part of the [ado-aw documentation](../AGENTS.md)._ Network isolation is provided by AWF (Agentic Workflow Firewall), which provides L7 (HTTP/HTTPS) egress control using Squid proxy and Docker containers. AWF restricts network access to an allowlist of approved domains. -Generated pipelines run AWF v0.27.32+ in **strict topology mode**: both the Agent and Detection jobs invoke AWF rootlessly with an explicit `--network-isolation` flag — there is no `sudo`, `--enable-host-access`, or `--legacy-security` fallback, and no author-facing knob to opt back into the legacy topology. The Agent additionally passes `--topology-attach awmg-mcpg` so the trusted MCPG container is attached to AWF's internal `awf-net`, and appends that hostname to `NO_PROXY`/`no_proxy` so MCP traffic bypasses Squid; Detection has no MCPG attachment. See [`docs/mcpg.md`](mcpg.md) for the MCPG topology and [`docs/mcp.md`](mcp.md) for MCP server configuration. +Generated pipelines run AWF v0.27.32+ in **strict topology mode**: both the Agent and Detection jobs invoke AWF rootlessly with an explicit `--network-isolation` flag — there is no `sudo`, `--enable-host-access`, or `--legacy-security` fallback, and no author-facing knob to opt back into the legacy topology. The Agent additionally passes `--topology-attach awmg-mcpg` so the trusted MCPG container is attached to AWF's internal `awf-net`, and appends that hostname to `NO_PROXY`/`no_proxy` so MCP traffic bypasses Squid; when the credential-isolated `ado-proxy` sidecar is enabled (`permissions.read` is configured), the Agent passes a second `--topology-attach awmg-ado-proxy` and adds that hostname to `NO_PROXY`/`no_proxy` too. Detection has no MCPG/ado-proxy attachment. See [`docs/mcpg.md`](mcpg.md) for the MCPG topology, [`docs/mcp.md`](mcp.md) for MCP server configuration, and [`docs/ado-proxy-design.md`](ado-proxy-design.md) for the ado-proxy sidecar. The `ado-aw` compiler binary is distributed via [GitHub Releases](https://github.com/githubnext/ado-aw/releases) with SHA256 checksum verification. The AWF binary is distributed via [GitHub Releases](https://github.com/github/gh-aw-firewall/releases) with SHA256 checksum verification. Docker is sourced via the `DockerInstaller@0` ADO task. @@ -205,6 +205,7 @@ Available ecosystem identifiers include: | `swift` | Swift.org, CocoaPods | | `terraform` | HashiCorp releases, Terraform registry | | `threat-detection` | Copilot API and telemetry domains used by the Detection stage | +| `copilot-vendor` | Copilot vendor API/telemetry domains (`api.business.githubcopilot.com`, `api.enterprise.githubcopilot.com`, `api.individual.githubcopilot.com`, `telemetry.enterprise.githubcopilot.com`) | **Compound identifier** (expands to a union of component identifiers): @@ -212,7 +213,7 @@ Available ecosystem identifiers include: |------------|------------| | `default-safe-outputs` | `defaults` + `dev-tools` + `github` + `local` — the standard set of domains needed for most safe-output execution scenarios | -Additional ecosystems: `bazel`, `chrome`, `clojure`, `dart`, `deno`, `elixir`, `fonts`, `github-actions`, `haskell`, `julia`, `kotlin`, `latex`, `lean`, `lua`, `node-cdns`, `ocaml`, `perl`, `php`, `playwright`, `powershell`, `python-native`, `r`, `scala`, `zig`. +Additional ecosystems: `bazel`, `chrome`, `clojure`, `copilot-vendor`, `dart`, `deno`, `elixir`, `fonts`, `github-actions`, `haskell`, `julia`, `kotlin`, `latex`, `lean`, `lua`, `node-cdns`, `ocaml`, `perl`, `php`, `playwright`, `powershell`, `python-native`, `r`, `scala`, `zig`. The full domain lists for direct identifiers are defined in `src/data/ecosystem_domains.json`. Compound identifiers are defined in `src/ecosystem_domains.rs`. diff --git a/docs/tools.md b/docs/tools.md index cc3b607d3..a2c350991 100644 --- a/docs/tools.md +++ b/docs/tools.md @@ -161,10 +161,10 @@ subcommands. When configured, use `tools.azure-devops` for authenticated ADO reads. Do not run `az login` or inject Azure credentials into the Agent sandbox; use SafeOutputs or request a supported tool instead. -A daily smoke pipeline at -[`tests/safe-outputs/azure-cli.md`](../tests/safe-outputs/azure-cli.md) -exercises binary/subcommand availability without claiming authenticated direct -ADO access. +Detection, mounting, and prompt-advisory gating are covered by unit tests in +`src/compile/extensions/azure_cli.rs` and `tests/compiler_tests.rs`; there is +no dedicated agentic smoke pipeline exercising binary/subcommand availability +end-to-end. ### GitHub CLI (`gh`) diff --git a/prompts/create-ado-agentic-workflow.md b/prompts/create-ado-agentic-workflow.md index cd988c0d6..5b72cd19b 100644 --- a/prompts/create-ado-agentic-workflow.md +++ b/prompts/create-ado-agentic-workflow.md @@ -33,7 +33,7 @@ If interactive, ask only missing essentials first. ### 2. Build Front Matter Use only required keys plus task-required options: - `name`, `description` -- optional: `target`, `engine`, `workspace`, `pool`, `repos`, `imports`, `tools`, `runtimes`, `mcp-servers`, `safe-outputs`, `on`, `steps`, `post-steps`, `setup`, `teardown`, `permissions`, `parameters`, `env`, `variable-groups`, `network`, `execution-context`, `inlined-imports`, `supply-chain` +- optional: `target`, `engine`, `workspace`, `pool`, `repos`, `imports`, `tools`, `runtimes`, `mcp-servers`, `safe-outputs`, `on`, `steps`, `post-steps`, `setup`, `teardown`, `permissions`, `permissions-required`, `parameters`, `env`, `variable-groups`, `network`, `execution-context`, `inlined-imports`, `supply-chain` Rules: - Omit fields that equal defaults. diff --git a/scripts/ado-script/src/conclusion/__tests__/index.test.ts b/scripts/ado-script/src/conclusion/__tests__/index.test.ts index 72c34accb..f94fab0d9 100644 --- a/scripts/ado-script/src/conclusion/__tests__/index.test.ts +++ b/scripts/ado-script/src/conclusion/__tests__/index.test.ts @@ -329,38 +329,56 @@ describe("conclusion/main", () => { }); it("files a missing-tool work item when the manifest contains missing_tool", async () => { - setManifestEntries([{ name: "missing_tool", tool_name: "gh", context: "tool_name: gh" }]); + setManifestEntries([ + { + name: "missing_tool", + status: "succeeded", + result: { tool_name: "gh", context: "needed for repository inspection" }, + }, + ]); await main(); expect(fileOrAppendWorkItem).toHaveBeenCalledTimes(1); + const body = (fileOrAppendWorkItem as ReturnType).mock + .calls[0]?.[3] as string; expect(fileOrAppendWorkItem).toHaveBeenCalledWith( "MyProject", expect.objectContaining({ enabled: true }), "[ado-aw] Agent encountered missing tool: feature reporter", - expect.stringContaining("- gh"), + body, ); + expect(body).toContain("- gh"); + expect(body).toContain("- needed for repository inspection"); }); it("files a missing-data work item when the manifest contains missing_data", async () => { setManifestEntries([ { name: "missing_data", - data_type: "pull_request", - reason: "PR metadata not available", - context: "data_type: pull_request", + status: "succeeded", + result: { + data_type: "pull_request", + reason: "PR metadata not available", + context: "needed for review", + }, }, ]); await main(); expect(fileOrAppendWorkItem).toHaveBeenCalledTimes(1); + const body = (fileOrAppendWorkItem as ReturnType).mock + .calls[0]?.[3] as string; expect(fileOrAppendWorkItem).toHaveBeenCalledWith( "MyProject", expect.objectContaining({ enabled: true }), "[ado-aw] Agent reported missing data: feature reporter", - expect.stringContaining("PR metadata not available"), + body, ); + expect(body).toContain("- pull_request"); + expect(body).toContain("- PR metadata not available"); + expect(body).toContain("- needed for review"); }); it("appends a comment to an existing work item instead of creating a duplicate", async () => { diff --git a/scripts/ado-script/src/conclusion/index.ts b/scripts/ado-script/src/conclusion/index.ts index 6c37bc32f..bd9c3f9fb 100644 --- a/scripts/ado-script/src/conclusion/index.ts +++ b/scripts/ado-script/src/conclusion/index.ts @@ -281,7 +281,7 @@ function extractNamedValue( fieldName: "tool_name" | "data_type", pattern: RegExp, ): string | undefined { - const direct = entry[fieldName]; + const direct = entry[fieldName] ?? entry.result?.[fieldName]; if (typeof direct === "string" && direct.trim().length > 0) { return direct.trim(); } @@ -319,7 +319,9 @@ function buildPipelineFailureReport(config: RuntimeConfig): SignalReport | null function buildNoopReport(config: RuntimeConfig, entries: readonly ManifestEntry[]): SignalReport | null { if (entries.length === 0) return null; - const contexts = unique(entries.map((entry) => entry.context ?? undefined)); + const contexts = unique( + entries.map((entry) => entry.context ?? toOptionalString(entry.result?.context)), + ); const lines = [ "The conclusion job detected one or more `noop` diagnostic signals.", "", @@ -347,7 +349,9 @@ function buildMissingToolReport( extractNamedValue(entry, "tool_name", /tool[_ -]?name\s*:\s*([^\r\n,;]+)/i) ), ); - const contexts = unique(entries.map((entry) => entry.context ?? undefined)); + const contexts = unique( + entries.map((entry) => entry.context ?? toOptionalString(entry.result?.context)), + ); const lines = [ "The conclusion job detected one or more `missing_tool` diagnostic signals.", "", @@ -378,8 +382,12 @@ function buildMissingDataReport( extractNamedValue(entry, "data_type", /data[_ -]?type\s*:\s*([^\r\n,;]+)/i) ), ); - const contexts = unique(entries.map((entry) => entry.context ?? undefined)); - const reasons = unique(entries.map((entry) => entry.reason ?? undefined)); + const contexts = unique( + entries.map((entry) => entry.context ?? toOptionalString(entry.result?.context)), + ); + const reasons = unique( + entries.map((entry) => entry.reason ?? toOptionalString(entry.result?.reason)), + ); const lines = [ "The conclusion job detected one or more `missing_data` diagnostic signals.", "", diff --git a/scripts/ado-script/src/executor-e2e/__tests__/conclusion-cli.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/conclusion-cli.test.ts new file mode 100644 index 000000000..b3a181786 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/__tests__/conclusion-cli.test.ts @@ -0,0 +1,95 @@ +import { mkdtemp, rm, writeFile } from "node:fs/promises"; +import { tmpdir } from "node:os"; +import { join } from "node:path"; + +import { afterEach, describe, expect, it } from "vitest"; + +import { CONCLUSION_BUNDLE_ENV, resolveConclusionBundle, runConclusion } from "../conclusion-cli.js"; +import { SkipError } from "../scenario.js"; + +const originalBundle = process.env[CONCLUSION_BUNDLE_ENV]; + +afterEach(() => { + if (originalBundle === undefined) delete process.env[CONCLUSION_BUNDLE_ENV]; + else process.env[CONCLUSION_BUNDLE_ENV] = originalBundle; +}); + +describe("resolveConclusionBundle", () => { + it("skips the scenario when the bundle env var is unset", () => { + delete process.env[CONCLUSION_BUNDLE_ENV]; + expect(() => resolveConclusionBundle()).toThrow(SkipError); + }); + + it("skips the scenario when the configured bundle does not exist", () => { + process.env[CONCLUSION_BUNDLE_ENV] = join(tmpdir(), "definitely-missing-conclusion.js"); + expect(() => resolveConclusionBundle()).toThrow(SkipError); + }); +}); + +describe("runConclusion", () => { + it("passes the safe-output dir, pipeline name and per-tool config to the bundle", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-conclusion-cli-")); + try { + // Fake bundle: echo the env the harness handed it, so the test pins the + // env-var contract shared with the compiler-generated Conclusion job. + const bundle = join(dir, "fake-conclusion.js"); + await writeFile( + bundle, + `const keys = ["AW_SAFE_OUTPUT_DIR","AW_PIPELINE_NAME","AW_AGENT_RESULT",` + + `"AW_NOOP_TITLE_PREFIX","SYSTEM_TEAMPROJECT","SYSTEM_COLLECTIONURI",` + + `"ADO_AW_ACCESS_TOKEN_KIND","BUILD_BUILDID"];\n` + + `console.log(JSON.stringify(Object.fromEntries(keys.map((k) => [k, process.env[k]]))));\n`, + "utf8", + ); + process.env[CONCLUSION_BUNDLE_ENV] = bundle; + + const result = await runConclusion({ + safeOutputDir: join(dir, "out"), + pipelineName: "ado-aw-det-1-conclusion-noop", + orgUrl: "https://dev.azure.com/org/", + project: "P", + token: "t", + buildId: "1", + config: { AW_NOOP_TITLE_PREFIX: "[prefix]" }, + log: () => {}, + }); + + expect(JSON.parse(result.stdout.trim())).toEqual({ + AW_SAFE_OUTPUT_DIR: join(dir, "out"), + AW_PIPELINE_NAME: "ado-aw-det-1-conclusion-noop", + AW_AGENT_RESULT: "Succeeded", + AW_NOOP_TITLE_PREFIX: "[prefix]", + SYSTEM_TEAMPROJECT: "P", + SYSTEM_COLLECTIONURI: "https://dev.azure.com/org/", + ADO_AW_ACCESS_TOKEN_KIND: "bearer", + BUILD_BUILDID: "1", + }); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it("fails when the bundle exits non-zero", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-conclusion-cli-")); + try { + const bundle = join(dir, "crashing-conclusion.js"); + await writeFile(bundle, `console.error("boom");\nprocess.exit(3);\n`, "utf8"); + process.env[CONCLUSION_BUNDLE_ENV] = bundle; + + await expect( + runConclusion({ + safeOutputDir: dir, + pipelineName: "p", + orgUrl: "https://dev.azure.com/org/", + project: "P", + token: "t", + buildId: "1", + config: {}, + log: () => {}, + }), + ).rejects.toThrow(/conclusion\.js exited 3/); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts index 665e0ea04..0d5207790 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/runner.test.ts @@ -315,3 +315,116 @@ fs.writeFileSync(path.join(out, "safe-outputs-executed.ndjson"), [ } }); }); + +/** + * `postExecute` runs a post-Stage-3 consumer (the Conclusion reporter) against + * the manifest the executor just wrote, before `assert`. These tests pin the + * ordering, the safe-output dir it is handed, and the failure/skip handling. + */ +describe("runScenario post-execute phase", () => { + /** Fake `ado-aw` that reports the primary tool as succeeded. */ + async function writeOkBin(dir: string): Promise { + const bin = join(dir, "ok-ado-aw.js"); + await writeFile( + bin, + `#!/usr/bin/env node +const fs = require("node:fs"); +const path = require("node:path"); +const out = process.argv[process.argv.indexOf("--safe-output-dir") + 1]; +fs.writeFileSync( + path.join(out, "safe-outputs-executed.ndjson"), + JSON.stringify({ name: "noop", status: "succeeded", result: {} }) + "\\n", +); +`, + { encoding: "utf8", mode: 0o755 }, + ); + return bin; + } + + function postExecuteScenario( + postExecute: Scenario["postExecute"], + order: string[], + ): Scenario { + return { + id: "post-execute", + tool: "noop", + config: () => ({}), + setup: async () => ({}), + ndjson: async () => ({}), + postExecute, + assert: async () => { + order.push("assert"); + }, + cleanup: async () => { + order.push("cleanup"); + }, + }; + } + + it("runs before assert and receives the executor's safe-output dir and records", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-post-")); + try { + const bin = await writeOkBin(dir); + const order: string[] = []; + let seenDir = ""; + let seenRecords: ExecutedRecord[] = []; + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + postExecuteScenario(async (_ctx, _state, run) => { + order.push("post-execute"); + seenDir = run.safeOutputDir; + seenRecords = run.records; + // The executed manifest must be readable from the handed-over dir. + await readFile(join(run.safeOutputDir, "safe-outputs-executed.ndjson"), "utf8"); + }, order), + ); + + expect(res.ok).toBe(true); + expect(order).toEqual(["post-execute", "assert", "cleanup"]); + expect(seenDir).toBe(join(dir, "post-execute", "out")); + expect(seenRecords.map((r) => r.name)).toEqual(["noop"]); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it("records a post-execute failure without running assert, but still cleans up", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-post-")); + try { + const bin = await writeOkBin(dir); + const order: string[] = []; + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + postExecuteScenario(async () => { + throw new Error("conclusion.js exited 3"); + }, order), + ); + + expect(res.ok).toBe(false); + expect(res.phase).toBe("post-execute"); + expect(res.message).toBe("conclusion.js exited 3"); + expect(order).toEqual(["cleanup"]); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); + + it("treats a SkipError from post-execute as a skip", async () => { + const dir = await mkdtemp(join(tmpdir(), "ado-aw-runner-post-")); + try { + const bin = await writeOkBin(dir); + const order: string[] = []; + const res = await runScenario( + { ...fakeCtx(), adoAwBin: bin, workDir: dir }, + postExecuteScenario(async () => { + throw new SkipError("conclusion bundle not built"); + }, order), + ); + + expect(res).toMatchObject({ ok: true, skipped: true, phase: "skipped" }); + expect(order).toEqual(["cleanup"]); + } finally { + await rm(dir, { recursive: true, force: true }); + } + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/conclusion-cli.ts b/scripts/ado-script/src/executor-e2e/conclusion-cli.ts new file mode 100644 index 000000000..758f31468 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/conclusion-cli.ts @@ -0,0 +1,113 @@ +/** + * Wrapper around the compiled `conclusion.js` bundle for the deterministic E2E + * harness. + * + * Production shape: the Conclusion job runs `node conclusion.js` after the + * SafeOutputs job, reading `safe-outputs-executed.ndjson` from the downloaded + * `safe_outputs` artifact and filing/appending Azure DevOps work items for the + * diagnostic signals it finds (`noop`, `missing-tool`, `missing-data`) and for + * upstream job failures. The compiler passes its configuration as flat env vars + * (`AW__TITLE_PREFIX`, `AW__TAGS`, …) — see + * `src/compile/agentic_pipeline.rs` and `docs/conclusion.md`. + * + * This module reproduces exactly that invocation against the manifest a real + * `ado-aw execute` run just wrote, so the harness covers the whole + * signal → manifest → work-item path rather than stopping at Stage 3. + * + * Test-harness module; not shipped in `ado-script.zip`. + */ +import { existsSync } from "node:fs"; + +import { partialOutput, spawnCollect } from "./execute-cli.js"; +import { SkipError } from "./scenario.js"; + +/** Env var carrying the path to the compiled `conclusion.js` bundle. */ +export const CONCLUSION_BUNDLE_ENV = "EXECUTOR_E2E_CONCLUSION_BUNDLE"; + +export interface RunConclusionOptions { + /** Directory holding `safe-outputs-executed.ndjson`. */ + safeOutputDir: string; + /** Pipeline name the reporter renders into titles and the stats block. */ + pipelineName: string; + orgUrl: string; + project: string; + token: string; + buildId: string; + /** Conclusion-specific `AW_*` config vars (title prefix, tags, opt-outs). */ + config: Record; + log: (msg: string) => void; +} + +export interface RunConclusionResult { + exitCode: number; + stdout: string; + stderr: string; +} + +/** + * Resolve the compiled bundle path, or skip the scenario when it is absent. + * + * The bundle is a build artifact (`npm run build:conclusion`), not a checked-in + * file, so a harness run that was not given one must skip rather than fail — + * the same contract the optional-precondition scenarios use. + */ +export function resolveConclusionBundle(): string { + const configured = process.env[CONCLUSION_BUNDLE_ENV]?.trim(); + if (!configured) { + throw new SkipError( + `${CONCLUSION_BUNDLE_ENV} is not set; run 'npm run build:conclusion' and point it at conclusion.js`, + ); + } + if (!existsSync(configured)) { + throw new SkipError(`${CONCLUSION_BUNDLE_ENV}='${configured}' does not exist`); + } + return configured; +} + +/** + * Run the conclusion reporter once over `safeOutputDir`. + * + * `conclusion.js` is deliberately fail-open (it exits 0 even when work-item + * filing fails, so post-pipeline housekeeping can never fail an otherwise green + * build). A non-zero exit therefore means the bundle itself crashed, which we + * surface as an error; a filing failure is caught by the scenario's assertion + * against the ADO REST API instead of by the exit code. + */ +export async function runConclusion( + opts: RunConclusionOptions, +): Promise { + const bundle = resolveConclusionBundle(); + const env: NodeJS.ProcessEnv = { + ...process.env, + SYSTEM_ACCESSTOKEN: opts.token, + ADO_AW_ACCESS_TOKEN_KIND: "bearer", + SYSTEM_COLLECTIONURI: opts.orgUrl, + SYSTEM_TEAMPROJECT: opts.project, + BUILD_BUILDID: opts.buildId, + AW_SAFE_OUTPUT_DIR: opts.safeOutputDir, + AW_PIPELINE_NAME: opts.pipelineName, + // The upstream job results the compiler wires in. All succeeded, so the + // pipeline-failure signal stays silent and only the diagnostic signals in + // the manifest are reported. + AW_AGENT_RESULT: "Succeeded", + AW_DETECTION_RESULT: "Succeeded", + AW_SAFEOUTPUTS_RESULT: "Succeeded", + ...opts.config, + }; + + opts.log(`[conclusion] running: node ${bundle} (AW_SAFE_OUTPUT_DIR=${opts.safeOutputDir})`); + const { exitCode, stdout, stderr } = await spawnCollect( + process.execPath, + [bundle], + env, + "conclusion.js", + ); + if (stdout.trim()) opts.log(`[conclusion] stdout:\n${stdout.trim()}`); + if (stderr.trim()) opts.log(`[conclusion] stderr:\n${stderr.trim()}`); + if (exitCode !== 0) { + throw new Error( + `conclusion.js exited ${exitCode}${partialOutput(stdout, stderr)}`, + ); + } + return { exitCode, stdout, stderr }; +} diff --git a/scripts/ado-script/src/executor-e2e/execute-cli.ts b/scripts/ado-script/src/executor-e2e/execute-cli.ts index 7e1fa0210..f4c58dafa 100644 --- a/scripts/ado-script/src/executor-e2e/execute-cli.ts +++ b/scripts/ado-script/src/executor-e2e/execute-cli.ts @@ -120,6 +120,13 @@ export interface RunExecuteResult { records: ExecutedRecord[]; /** The record matching `tool` (dashes -> underscores), if any. */ record?: ExecutedRecord; + /** + * Directory holding `safe_outputs.ndjson` and the executor-written + * `safe-outputs-executed.ndjson`. Exposed so a post-execute phase (e.g. the + * conclusion reporter, which consumes the executed manifest) can run against + * exactly the files this invocation produced. + */ + safeOutputDir: string; } /** Parse `safe-outputs-executed.ndjson` content into typed records. */ @@ -233,7 +240,7 @@ export async function runExecute(opts: RunExecuteOptions): Promise r.name === snake); - return { exitCode, stdout, stderr, records, record }; + return { exitCode, stdout, stderr, records, record, safeOutputDir }; } /** Append a truncated snapshot of a subprocess's output to a timeout message. */ @@ -244,12 +251,18 @@ export function partialOutput(stdout: string, stderr: string): string { return parts.join(""); } -function spawnCollect( +/** + * Spawn a child process, collect stdout/stderr, and reject when it exceeds the + * harness timeout. Shared with the conclusion-bundle runner so both child + * processes get identical hang protection and output capture. + */ +export function spawnCollect( cmd: string, args: string[], env: NodeJS.ProcessEnv, + label = "ado-aw execute", ): Promise<{ exitCode: number; stdout: string; stderr: string }> { - // Guard against a hung `ado-aw execute` blocking the whole suite: kill the + // Guard against a hung child blocking the whole suite: kill the // child after a bounded timeout and surface a meaningful error instead of // waiting for the ADO job-level timeout. const timeoutMs = Number(process.env.EXECUTOR_E2E_EXECUTE_TIMEOUT_MS) || 600_000; @@ -273,7 +286,7 @@ function spawnCollect( if (timedOut) { // Include any accumulated output so a hung run is diagnosable from the // error/issue body rather than only from the raw ADO logs. - reject(new Error(`ado-aw execute timed out after ${timeoutMs}ms${partialOutput(stdout, stderr)}`)); + reject(new Error(`${label} timed out after ${timeoutMs}ms${partialOutput(stdout, stderr)}`)); return; } resolve({ exitCode: code ?? -1, stdout, stderr }); diff --git a/scripts/ado-script/src/executor-e2e/runner.ts b/scripts/ado-script/src/executor-e2e/runner.ts index 725415ab8..c6da281f3 100644 --- a/scripts/ado-script/src/executor-e2e/runner.ts +++ b/scripts/ado-script/src/executor-e2e/runner.ts @@ -154,6 +154,23 @@ export async function runScenario( }); } + // ---- post-execute (optional; e.g. the Conclusion reporter) ---- + if (scenario.postExecute) { + ctx.log(`[${scenarioId}] post-execute`); + try { + await scenario.postExecute(ctx, state, { + safeOutputDir: result.safeOutputDir, + records: result.records, + }); + } catch (err) { + if (err instanceof SkipError) { + ctx.log(`[${scenarioId}] SKIPPED: ${err.message}`); + return finish({ ok: true, skipped: true, phase: "skipped", message: err.message }); + } + return finish({ ok: false, phase: "post-execute", message: errMessage(err) }); + } + } + // ---- assert ---- try { await scenario.assert(ctx, state, result.record, result.records); diff --git a/scripts/ado-script/src/executor-e2e/scenario.ts b/scripts/ado-script/src/executor-e2e/scenario.ts index be8b3315b..d57eb43cf 100644 --- a/scripts/ado-script/src/executor-e2e/scenario.ts +++ b/scripts/ado-script/src/executor-e2e/scenario.ts @@ -94,6 +94,14 @@ export interface ScenarioSource { readonly prefix: (tool: string) => string; } +/** Files and records produced by one `ado-aw execute` run, handed to `postExecute`. */ +export interface PostExecuteRun { + /** Directory holding `safe_outputs.ndjson` + `safe-outputs-executed.ndjson`. */ + readonly safeOutputDir: string; + /** Every parsed record from the executed manifest. */ + readonly records: ExecutedRecord[]; +} + /** * A single deterministic executor scenario. * @@ -161,6 +169,20 @@ export interface Scenario { * child process (e.g. BUILD_SOURCESDIRECTORY pointing at a git checkout). */ env?(ctx: ScenarioContext, state: State): Promise>; + /** + * Optional phase that runs **after** a successful `ado-aw execute` and + * before `assert`. + * + * This exists for post-Stage-3 consumers of the executor's output — the + * Conclusion job reads `safe-outputs-executed.ndjson` from the same + * safe-output directory and files diagnostic work items from it. Running it + * here reproduces the production ordering (SafeOutputs → Conclusion) against + * a real manifest instead of a fixture. + * + * A throw records the scenario as failed in the `post-execute` phase; + * `cleanup()` still runs. + */ + postExecute?(ctx: ScenarioContext, state: State, run: PostExecuteRun): Promise; /** * Some scenarios intentionally submit invalid staged output and should pass * only when the executor rejects it with the expected failure. @@ -194,7 +216,7 @@ export interface Scenario { export interface ScenarioResult { tool: string; ok: boolean; - /** "setup" | "execute" | "assert" | "cleanup" | "skipped". */ + /** "setup" | "execute" | "post-execute" | "assert" | "cleanup" | "skipped". */ phase?: string; message?: string; durationMs: number; diff --git a/scripts/ado-script/src/executor-e2e/scenarios/conclusion.ts b/scripts/ado-script/src/executor-e2e/scenarios/conclusion.ts new file mode 100644 index 000000000..86a231c35 --- /dev/null +++ b/scripts/ado-script/src/executor-e2e/scenarios/conclusion.ts @@ -0,0 +1,313 @@ +/** + * Conclusion-job scenarios: work-item filing for the diagnostic signals. + * + * The signal safe-outputs (`noop`, `missing-tool`, `missing-data`) have no ADO + * write path of their own — the executor only records them in + * `safe-outputs-executed.ndjson`. Their *observable* effect is produced one job + * later by the Conclusion job (`conclusion.js`), which reads that manifest and + * files (or appends to) an Azure DevOps work item per signal — see + * `docs/conclusion.md`. + * + * The scenarios in `signals.ts` stop at the executor record, so nothing covered + * the signal → manifest → work-item path end to end. These scenarios close that + * gap: each runs the real executor, then the real conclusion bundle over the + * manifest it just wrote, and asserts the resulting work item via the ADO REST + * API. + * + * Coverage per scenario: + * - `conclusion-noop` — a work item is created for a `noop` signal, carrying + * the configured title, type, tags and rendered body. + * - `conclusion-missing-tool` — same for `missing-tool`, and the second + * conclusion run appends a comment instead of creating a duplicate + * (title deduplication). + * - `conclusion-missing-data` — same for `missing-data`, including the + * reported data type and reason. + * - `conclusion-report-as-work-item-false` — the per-tool opt-out files + * nothing at all. + * + * Test-harness module; not shipped in `ado-script.zip`. + */ +import { runConclusion } from "../conclusion-cli.js"; +import type { PostExecuteRun, Scenario, ScenarioContext } from "../scenario.js"; + +/** Work item type used for every conclusion scenario (the reporter's default). */ +const WORK_ITEM_TYPE = "Task"; + +/** Title prefix handed to the reporter; the rendered title appends the pipeline name. */ +const TITLE_PREFIX = "[ado-aw-e2e conclusion]"; + +interface ConclusionState { + /** Value of `AW_PIPELINE_NAME`; unique per build and scenario. */ + pipelineName: string; + /** The title the reporter is expected to render: ` `. */ + title: string; + /** Tag applied to created work items (also used for cleanup diagnostics). */ + tag: string; + /** Populated in `postExecute` once the work item is observed. */ + workItemId?: number; + /** stdout of the last conclusion run, asserted by the opt-out scenario. */ + stdout?: string; +} + +function conclusionState(ctx: ScenarioContext, scenarioId: string): ConclusionState { + const pipelineName = ctx.prefix(scenarioId); + return { + pipelineName, + title: `${TITLE_PREFIX} ${pipelineName}`, + tag: `ado-aw-e2e-${ctx.buildId}`, + }; +} + +/** Per-tool conclusion env, mirroring the flat `AW__*` vars the compiler emits. */ +function toolConfig( + envPrefix: string, + state: ConclusionState, + extra: Record = {}, +): Record { + return { + [`${envPrefix}_TITLE_PREFIX`]: TITLE_PREFIX, + [`${envPrefix}_WORK_ITEM_TYPE`]: WORK_ITEM_TYPE, + [`${envPrefix}_TAGS`]: JSON.stringify([state.tag]), + ...extra, + }; +} + +/** + * Wait for the work item to become visible to WIQL. + * + * `findWorkItemByTitle` goes through the WIQL endpoint, whose index lags work + * item creation by a second or two. Polling here (rather than reading once) + * keeps the assertion deterministic, and it also guarantees the *reporter's* + * own dedup query can see the item before a second run is asked to append. + */ +async function waitForWorkItem( + ctx: ScenarioContext, + title: string, + timeoutMs = 90_000, +): Promise { + const deadline = Date.now() + timeoutMs; + for (;;) { + const id = await ctx.rest.findWorkItemByTitle(title); + if (id !== undefined) return id; + if (Date.now() >= deadline) { + throw new Error( + `no work item titled '${title}' became visible within ${timeoutMs}ms`, + ); + } + await new Promise((resolve) => setTimeout(resolve, 3_000)); + } +} + +/** Read a work item field as a string (missing/non-string fields fail loudly). */ +function fieldText(fields: Record, name: string): string { + const value = fields[name]; + if (typeof value !== "string") { + throw new Error(`work item field ${name} is not a string (got ${JSON.stringify(value)})`); + } + return value; +} + +/** + * Assert the shared shape of a conclusion-filed work item: title, type, tag and + * the substrings the reporter is expected to render into the description. + * Returns the asserted work item id so callers can make further checks against + * it without re-deriving (and re-guarding) it. + * + * Substrings are chosen to be free of `<`, `>` and `&` so the check holds + * whether or not Azure DevOps stores the body as Markdown or re-encodes it. + */ +async function assertFiledWorkItem( + ctx: ScenarioContext, + state: ConclusionState, + expectedBodySubstrings: readonly string[], +): Promise { + const workItemId = state.workItemId; + if (workItemId === undefined) { + throw new Error("postExecute did not record a work item id"); + } + const item = await ctx.rest.getWorkItem(workItemId); + const title = fieldText(item.fields, "System.Title"); + if (title !== state.title) { + throw new Error(`work item title is '${title}', expected '${state.title}'`); + } + const type = fieldText(item.fields, "System.WorkItemType"); + if (type !== WORK_ITEM_TYPE) { + throw new Error(`work item type is '${type}', expected '${WORK_ITEM_TYPE}'`); + } + const tags = fieldText(item.fields, "System.Tags"); + if (!tags.split(";").map((t) => t.trim()).includes(state.tag)) { + throw new Error(`work item tags '${tags}' do not include '${state.tag}'`); + } + const description = fieldText(item.fields, "System.Description"); + for (const expected of expectedBodySubstrings) { + if (!description.includes(expected)) { + throw new Error( + `work item description does not contain '${expected}': ${description.slice(0, 800)}`, + ); + } + } + return workItemId; +} + +/** Best-effort teardown: delete the filed work item (resolving it by title if needed). */ +async function cleanupWorkItem(ctx: ScenarioContext, state: ConclusionState): Promise { + const id = state.workItemId ?? (await ctx.rest.findWorkItemByTitle(state.title)); + if (id === undefined) return; + await ctx.rest.deleteWorkItem(id); +} + +/** Run the reporter once against the manifest the executor just wrote. */ +async function reportOnce( + ctx: ScenarioContext, + state: ConclusionState, + run: PostExecuteRun, + config: Record, +): Promise { + const result = await runConclusion({ + safeOutputDir: run.safeOutputDir, + pipelineName: state.pipelineName, + orgUrl: ctx.orgUrl, + project: ctx.project, + token: ctx.token, + buildId: ctx.buildId, + config, + log: ctx.log, + }); + state.stdout = result.stdout; + return result.stdout; +} + +export const conclusionNoop: Scenario = { + id: "conclusion-noop", + tool: "noop", + config: () => ({}), + setup: async (ctx) => conclusionState(ctx, "conclusion-noop"), + ndjson: async (ctx) => ({ + context: `deterministic conclusion e2e noop for build ${ctx.buildId}`, + }), + postExecute: async (ctx, state, run) => { + await reportOnce(ctx, state, run, toolConfig("AW_NOOP", state)); + state.workItemId = await waitForWorkItem(ctx, state.title); + ctx.log(`[conclusion-noop] filed work item #${state.workItemId}`); + }, + assert: async (ctx, state) => { + await assertFiledWorkItem(ctx, state, [ + "noop", + "Occurrences: 1", + `deterministic conclusion e2e noop for build ${ctx.buildId}`, + `Build ID: ${ctx.buildId}`, + ]); + }, + cleanup: cleanupWorkItem, +}; + +export const conclusionMissingTool: Scenario = { + id: "conclusion-missing-tool", + tool: "missing-tool", + config: () => ({}), + setup: async (ctx) => conclusionState(ctx, "conclusion-missing-tool"), + ndjson: async (ctx) => ({ + tool_name: `ado-aw-det-${ctx.buildId}-bash`, + context: `deterministic conclusion e2e missing-tool for build ${ctx.buildId}`, + }), + postExecute: async (ctx, state, run) => { + const config = toolConfig("AW_MISSING_TOOL", state); + await reportOnce(ctx, state, run, config); + state.workItemId = await waitForWorkItem(ctx, state.title); + ctx.log(`[conclusion-missing-tool] filed work item #${state.workItemId}`); + // Second run over the same manifest: the reporter must dedup on the + // rendered title and append a comment rather than file a duplicate. + await reportOnce(ctx, state, run, config); + }, + assert: async (ctx, state) => { + const workItemId = await assertFiledWorkItem(ctx, state, [ + "missing_tool", + `ado-aw-det-${ctx.buildId}-bash`, + `deterministic conclusion e2e missing-tool for build ${ctx.buildId}`, + ]); + // Exactly one: the title is unique to this build and scenario, so the work + // item is always freshly created by the first conclusion run (which files, + // and does not comment). A second comment would mean the reporter appended + // twice; zero would mean it filed a duplicate work item instead. + const comments = await ctx.rest.getWorkItemComments(workItemId); + if (comments.length !== 1) { + throw new Error( + `expected exactly one appended comment after the second conclusion run, got ${comments.length}`, + ); + } + const commentText = comments[0]?.text ?? ""; + if (!commentText.includes("missing_tool")) { + throw new Error( + `appended comment does not describe the missing_tool signal: ${commentText.slice(0, 400)}`, + ); + } + }, + cleanup: cleanupWorkItem, +}; + +export const conclusionMissingData: Scenario = { + id: "conclusion-missing-data", + tool: "missing-data", + config: () => ({}), + setup: async (ctx) => conclusionState(ctx, "conclusion-missing-data"), + ndjson: async (ctx) => ({ + data_type: "deterministic-conclusion-e2e-data-type", + reason: `deterministic conclusion e2e missing-data for build ${ctx.buildId}`, + }), + postExecute: async (ctx, state, run) => { + await reportOnce(ctx, state, run, toolConfig("AW_MISSING_DATA", state)); + state.workItemId = await waitForWorkItem(ctx, state.title); + ctx.log(`[conclusion-missing-data] filed work item #${state.workItemId}`); + }, + assert: async (ctx, state) => { + await assertFiledWorkItem(ctx, state, [ + "missing_data", + "deterministic-conclusion-e2e-data-type", + `deterministic conclusion e2e missing-data for build ${ctx.buildId}`, + ]); + }, + cleanup: cleanupWorkItem, +}; + +export const conclusionOptOut: Scenario = { + id: "conclusion-report-as-work-item-false", + tool: "noop", + config: () => ({}), + setup: async (ctx) => conclusionState(ctx, "conclusion-report-as-work-item-false"), + ndjson: async (ctx) => ({ + context: `deterministic conclusion e2e report-as-work-item-false for build ${ctx.buildId}`, + }), + postExecute: async (ctx, state, run) => { + await reportOnce( + ctx, + state, + run, + toolConfig("AW_NOOP", state, { AW_NOOP_REPORT_AS_WORK_ITEM: "false" }), + ); + }, + assert: async (ctx, state) => { + const stdout = state.stdout ?? ""; + if (!stdout.includes("report-as-work-item is false")) { + throw new Error( + `conclusion did not log the per-tool opt-out: ${stdout.slice(0, 800)}`, + ); + } + // The absence check is secondary to the log assertion above: WIQL lags + // creation, so a filed item might not be visible yet. It still catches a + // regression where the opt-out is ignored on a later run of the suite. + const id = await ctx.rest.findWorkItemByTitle(state.title); + if (id !== undefined) { + throw new Error( + `work item #${id} was filed despite report-as-work-item: false`, + ); + } + }, + cleanup: cleanupWorkItem, +}; + +export const conclusionScenarios: Scenario[] = [ + conclusionNoop, + conclusionMissingTool, + conclusionMissingData, + conclusionOptOut, +]; diff --git a/scripts/ado-script/src/executor-e2e/scenarios/index.ts b/scripts/ado-script/src/executor-e2e/scenarios/index.ts index e2d277abb..35dd16ca4 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/index.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/index.ts @@ -4,6 +4,7 @@ */ import type { Scenario } from "../scenario.js"; import { buildScenarios } from "./build.js"; +import { conclusionScenarios } from "./conclusion.js"; import { createPullRequestScenarios } from "./create-pull-request.js"; import { crossOrgScenarios } from "./cross-org.js"; import { gitScenarios } from "./git.js"; @@ -16,6 +17,7 @@ import { workItemScenarios } from "./work-item.js"; /** Every scenario, in a deterministic run order. */ export const allScenarios: Scenario[] = [ ...signalScenarios, + ...conclusionScenarios, ...workItemScenarios, ...wikiScenarios, ...prScenarios, diff --git a/scripts/ado-script/src/shared/__tests__/wit.test.ts b/scripts/ado-script/src/shared/__tests__/wit.test.ts index d1db485d0..2ce872d90 100644 --- a/scripts/ado-script/src/shared/__tests__/wit.test.ts +++ b/scripts/ado-script/src/shared/__tests__/wit.test.ts @@ -154,7 +154,7 @@ describe("shared/wit", () => { await expect(findWorkItemByTitle("p", "title")).resolves.toBeNull(); }); - it("createWorkItem builds a JsonPatch document and prefixes the type with $", async () => { + it("createWorkItem builds a JsonPatch document with the SDK type name", async () => { mockWitApi.createWorkItem.mockResolvedValue({ id: 99, _links: { html: { href: "https://example.test/wit/99" } }, @@ -179,7 +179,7 @@ describe("shared/wit", () => { }, ], "MyProject", - "$Task", + "Task", ); expect(result).toEqual({ id: 99, url: "https://example.test/wit/99" }); }); @@ -291,7 +291,7 @@ describe("shared/wit", () => { }, ], "MyProject", - "$Bug", + "Bug", ); expect(result).toEqual({ action: "created", diff --git a/scripts/ado-script/src/shared/wit.ts b/scripts/ado-script/src/shared/wit.ts index 7a584641a..769ce0554 100644 --- a/scripts/ado-script/src/shared/wit.ts +++ b/scripts/ado-script/src/shared/wit.ts @@ -202,9 +202,9 @@ export async function createWorkItem( { "Content-Type": "application/json-patch+json" }, patch, project, - `$${type}`, + type, ); - if (typeof created.id !== "number") { + if (!created || typeof created.id !== "number") { throw new Error("createWorkItem returned a work item without a numeric id"); } const url = diff --git a/src/ado/mod.rs b/src/ado/mod.rs index 2e060be5c..2de29d4e8 100644 --- a/src/ado/mod.rs +++ b/src/ado/mod.rs @@ -2016,6 +2016,30 @@ pub async fn download_build_artifact( ) })?; + let artifact_dir = prepare_artifact_extraction_dir(dest_dir, &artifact.name)?; + + debug!( + "Downloading build artifact '{}' from {}", + artifact.name, download_url + ); + + let resp = auth + .apply(client.get(download_url)) + .send() + .await + .with_context(|| format!("Failed to download build artifact '{}'", artifact.name))?; + + let resp = check_artifact_download_status(resp, artifact, dest_dir).await?; + + let temp_zip = stream_artifact_to_temp_zip(resp, artifact, dest_dir).await?; + extract_artifact_zip(temp_zip, artifact, &artifact_dir) +} + +/// Create (or reset) the directory an artifact will be extracted into. +fn prepare_artifact_extraction_dir( + dest_dir: &std::path::Path, + artifact_name: &str, +) -> Result { std::fs::create_dir_all(dest_dir).with_context(|| { format!( "Failed to create artifact destination directory '{}'", @@ -2023,7 +2047,7 @@ pub async fn download_build_artifact( ) })?; - let artifact_dir = dest_dir.join(&artifact.name); + let artifact_dir = dest_dir.join(artifact_name); if artifact_dir.exists() { std::fs::remove_dir_all(&artifact_dir).with_context(|| { format!( @@ -2039,40 +2063,48 @@ pub async fn download_build_artifact( ) })?; - debug!( - "Downloading build artifact '{}' from {}", - artifact.name, download_url - ); - - let mut resp = auth - .apply(client.get(download_url)) - .send() - .await - .with_context(|| format!("Failed to download build artifact '{}'", artifact.name))?; + Ok(artifact_dir) +} +/// Validate the HTTP status of the artifact download response, returning a +/// structured error (with PAT-scope guidance on 401/403) on failure. +async fn check_artifact_download_status( + resp: reqwest::Response, + artifact: &BuildArtifact, + dest_dir: &std::path::Path, +) -> Result { let status = resp.status(); - if !status.is_success() { - let body = resp.text().await.unwrap_or_default(); - if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::FORBIDDEN { - let run_id_hint = artifact.source.as_deref().unwrap_or(""); - return Err(anyhow::anyhow!( - "ADO API returned {} when downloading build artifact '{}': {}. This call requires PAT scopes Build (Read) and Build Artifacts (Read). As a manual alternative, try `az pipelines runs artifact download --run-id {} --artifact-name {} --path {}`.", - status, - artifact.name, - body, - run_id_hint, - artifact.name, - dest_dir.display() - )); - } - anyhow::bail!( - "ADO API returned {} when downloading build artifact '{}': {}", + if status.is_success() { + return Ok(resp); + } + + let body = resp.text().await.unwrap_or_default(); + if status == reqwest::StatusCode::UNAUTHORIZED || status == reqwest::StatusCode::FORBIDDEN { + let run_id_hint = artifact.source.as_deref().unwrap_or(""); + return Err(anyhow::anyhow!( + "ADO API returned {} when downloading build artifact '{}': {}. This call requires PAT scopes Build (Read) and Build Artifacts (Read). As a manual alternative, try `az pipelines runs artifact download --run-id {} --artifact-name {} --path {}`.", status, artifact.name, - body - ); + body, + run_id_hint, + artifact.name, + dest_dir.display() + )); } + anyhow::bail!( + "ADO API returned {} when downloading build artifact '{}': {}", + status, + artifact.name, + body + ); +} +/// Stream the artifact download response into a temp zip file under `dest_dir`. +async fn stream_artifact_to_temp_zip( + mut resp: reqwest::Response, + artifact: &BuildArtifact, + dest_dir: &std::path::Path, +) -> Result { let mut temp_zip = tempfile::Builder::new() .prefix(&format!(".tmp-{}-", artifact.id)) .suffix(".zip") @@ -2103,32 +2135,26 @@ pub async fn download_build_artifact( ) })?; - let archive_file = temp_zip.reopen().with_context(|| { - format!( - "Failed to reopen temp zip for build artifact '{}'", - artifact.name - ) - })?; - let mut archive = zip::ZipArchive::new(archive_file).with_context(|| { - format!( - "Failed to read downloaded zip for build artifact '{}'", - artifact.name - ) - })?; + Ok(temp_zip) +} - let repeated_root = (0..archive.len()).try_fold(true, |all_match, index| { +/// Determine whether every entry in the archive shares the artifact name as +/// its top-level path component (i.e. the zip has a single repeated root +/// directory that should be stripped on extraction). +fn artifact_zip_has_repeated_root( + archive: &mut zip::ZipArchive, + artifact_name: &str, +) -> Result { + (0..archive.len()).try_fold(true, |all_match, index| { let entry = archive.by_index(index).with_context(|| { - format!( - "Failed to read zip entry {} from build artifact '{}'", - index, artifact.name - ) + format!("Failed to read zip entry {index} from build artifact '{artifact_name}'") })?; let entry_name = entry.name().to_string(); let relative_path = entry.enclosed_name().ok_or_else(|| { anyhow::anyhow!( "Refusing to extract unsafe path '{}' from build artifact '{}'", entry_name, - artifact.name + artifact_name ) })?; Ok::<_, anyhow::Error>( @@ -2136,65 +2162,95 @@ pub async fn download_build_artifact( && relative_path .components() .next() - .is_some_and(|component| component.as_os_str() == artifact.name.as_str()), + .is_some_and(|component| component.as_os_str() == artifact_name), ) - })?; + }) +} - for index in 0..archive.len() { - let mut entry = archive.by_index(index).with_context(|| { - format!( - "Failed to read zip entry {} from build artifact '{}'", - index, artifact.name +/// Extract a single zip entry to `artifact_dir`, stripping the repeated root +/// component when `repeated_root` is true. +fn extract_zip_entry( + entry: &mut zip::read::ZipFile, + artifact_name: &str, + artifact_dir: &std::path::Path, + repeated_root: bool, +) -> Result<()> { + let entry_name = entry.name().to_string(); + let relative_path = entry + .enclosed_name() + .map(|path| path.to_owned()) + .ok_or_else(|| { + anyhow::anyhow!( + "Refusing to extract unsafe path '{}' from build artifact '{}'", + entry_name, + artifact_name ) })?; - let entry_name = entry.name().to_string(); - let relative_path = entry - .enclosed_name() - .map(|path| path.to_owned()) - .ok_or_else(|| { - anyhow::anyhow!( - "Refusing to extract unsafe path '{}' from build artifact '{}'", - entry_name, - artifact.name - ) - })?; - let relative_path = if repeated_root { - relative_path - .strip_prefix(&artifact.name) - .expect("repeated artifact root was validated") - } else { - relative_path.as_path() - }; - let output_path = artifact_dir.join(relative_path); - - if entry.is_dir() { - std::fs::create_dir_all(&output_path).with_context(|| { - format!( - "Failed to create extracted directory '{}'", - output_path.display() - ) - })?; - continue; - } - - if let Some(parent) = output_path.parent() { - std::fs::create_dir_all(parent).with_context(|| { - format!("Failed to create parent directory '{}'", parent.display()) - })?; - } + let relative_path = if repeated_root { + relative_path + .strip_prefix(artifact_name) + .expect("repeated artifact root was validated") + } else { + relative_path.as_path() + }; + let output_path = artifact_dir.join(relative_path); - let mut output = std::fs::File::create(&output_path).with_context(|| { + if entry.is_dir() { + std::fs::create_dir_all(&output_path).with_context(|| { format!( - "Failed to create extracted file '{}'", + "Failed to create extracted directory '{}'", output_path.display() ) })?; - std::io::copy(&mut entry, &mut output).with_context(|| { + return Ok(()); + } + + if let Some(parent) = output_path.parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("Failed to create parent directory '{}'", parent.display()))?; + } + + let mut output = std::fs::File::create(&output_path).with_context(|| { + format!( + "Failed to create extracted file '{}'", + output_path.display() + ) + })?; + std::io::copy(entry, &mut output).with_context(|| { + format!("Failed to extract '{entry_name}' from build artifact '{artifact_name}'") + })?; + Ok(()) +} + +/// Open the downloaded temp zip and extract its contents into `artifact_dir`. +fn extract_artifact_zip( + temp_zip: tempfile::NamedTempFile, + artifact: &BuildArtifact, + artifact_dir: &std::path::Path, +) -> Result<()> { + let archive_file = temp_zip.reopen().with_context(|| { + format!( + "Failed to reopen temp zip for build artifact '{}'", + artifact.name + ) + })?; + let mut archive = zip::ZipArchive::new(archive_file).with_context(|| { + format!( + "Failed to read downloaded zip for build artifact '{}'", + artifact.name + ) + })?; + + let repeated_root = artifact_zip_has_repeated_root(&mut archive, &artifact.name)?; + + for index in 0..archive.len() { + let mut entry = archive.by_index(index).with_context(|| { format!( - "Failed to extract '{}' from build artifact '{}'", - entry_name, artifact.name + "Failed to read zip entry {} from build artifact '{}'", + index, artifact.name ) })?; + extract_zip_entry(&mut entry, &artifact.name, artifact_dir, repeated_root)?; } Ok(()) diff --git a/src/ado_proxy/catalog.rs b/src/ado_proxy/catalog.rs index 576823dbd..ec59fab96 100644 --- a/src/ado_proxy/catalog.rs +++ b/src/ado_proxy/catalog.rs @@ -249,6 +249,18 @@ pub fn catalog() -> Catalog { } pub fn operations() -> Vec { + let mut ops = discovery_operations(); + ops.extend(core_operations()); + ops.extend(repos_operations()); + ops.extend(pipelines_operations()); + ops.extend(boards_operations()); + ops +} + +/// Discovery-capability operations: unauthenticated host/area/resource-area +/// probes plus the connection-data handshake that every ADO client issues +/// before its first data call. +fn discovery_operations() -> Vec { vec![ Operation { id: "discovery.host-options", @@ -325,6 +337,12 @@ pub fn operations() -> Vec { Json, &["connectOptions", "lastChangeId", "lastChangeId64"] ), + ] +} + +/// Core-capability operations: project lookup and validation probes. +fn core_operations() -> Vec { + vec![ get!( "core.project-get", Core, @@ -352,6 +370,13 @@ pub fn operations() -> Vec { "getDefaultTeamImageUrl", ] ), + ] +} + +/// Repos-capability operations: repository metadata, refs, items, commits, +/// and pull-request read paths. +fn repos_operations() -> Vec { + vec![ get!( "repos.repository-get", Repos, @@ -498,6 +523,13 @@ pub fn operations() -> Vec { Json, NO_QUERY ), + ] +} + +/// Pipelines-capability operations: build/release definitions, builds, +/// timelines, and pipeline runs. +fn pipelines_operations() -> Vec { + vec![ get!( "pipelines.definitions-list", Pipelines, @@ -601,6 +633,13 @@ pub fn operations() -> Vec { Json, NO_QUERY ), + ] +} + +/// Boards-capability operations: work item read, comments, updates, and +/// revision history. +fn boards_operations() -> Vec { + vec![ get!( "boards.work-item-get", Boards, diff --git a/src/audit/analyzers/custom_jobs.rs b/src/audit/analyzers/custom_jobs.rs index 74ce8b1fb..5480f20b8 100644 --- a/src/audit/analyzers/custom_jobs.rs +++ b/src/audit/analyzers/custom_jobs.rs @@ -298,9 +298,10 @@ fn correlate_tool_with_graph( } else { metadata_approval_path }; - if let (Some(component), Some(config_digest)) = - (entry.component.as_ref(), entry.config_schema_digest.as_ref()) - && !component.schema_digest.is_empty() + if let (Some(component), Some(config_digest)) = ( + entry.component.as_ref(), + entry.config_schema_digest.as_ref(), + ) && !component.schema_digest.is_empty() && component.schema_digest != *config_digest { findings.push(Finding { @@ -413,141 +414,191 @@ fn add_report_findings( findings: &mut Vec, ) { for report in reports { - if let Some(component) = &report.component_provenance { - let missing = [ - ("source", component.source.trim().is_empty()), - ("sha", component.sha.trim().is_empty()), - ( - "manifest_digest", - component.manifest_digest.trim().is_empty(), - ), - ("schema_digest", component.schema_digest.trim().is_empty()), - ] - .into_iter() - .filter_map(|(name, missing)| missing.then_some(name)) - .collect::>(); - if !missing.is_empty() { - findings.push(Finding { - category: String::from("safe_outputs"), - severity: Severity::High, - title: format!( - "Incomplete custom component provenance for {}", - report.tool - ), - description: format!( - "The compile-time aw_info marker is missing {} for custom tool '{}'.", - missing.join(", "), - report.tool - ), - impact: Some(String::from( - "The exact custom component revision or schema cannot be independently identified.", - )), - }); - } - } + let ran = report.ado_job.as_ref().is_some_and(custom_job_ran); - if let (Some(expected), Some(actual)) = - (report.expected_job_id.as_deref(), report.ado_job.as_ref()) - && stable_ado_job_identity_matches(actual, expected) == Some(false) - { - findings.push(Finding { - category: String::from("safe_outputs"), - severity: Severity::High, - title: format!("Custom job identity mismatch for {}", report.tool), - description: format!( - "Custom tool '{}' was compiled for ADO job '{}', but the correlated timeline job identifies as '{}'.", - report.tool, - expected, - best_ado_job_identity(actual) - ), - impact: Some(String::from( - "The observed job may not be the compiler-approved executor for these proposals.", - )), - }); - } + findings.extend(incomplete_provenance_finding(report)); + findings.extend(job_identity_mismatch_finding(report)); + findings.extend(ran_without_proposals_finding(report, ran)); + findings.extend(ran_after_unsafe_detection_finding(audit, report, ran)); + findings.extend(ran_without_approval_finding(audit, report, ran)); + findings.extend(expected_job_missing_finding(audit, report)); + } +} - let ran = report.ado_job.as_ref().is_some_and(custom_job_ran); - if ran && report.proposed_count == 0 { - findings.push(Finding { - category: String::from("safe_outputs"), - severity: Severity::High, - title: format!("Custom job ran without proposals for {}", report.tool), - description: format!( - "The custom ADO job for '{}' started even though the Agent artifact contains no proposals for that tool.", - report.tool - ), - impact: Some(String::from( - "The compiler-generated proposal gate and the observed runtime state are inconsistent.", - )), - }); - } +/// The compile-time `aw_info` marker for a custom component should record its +/// full provenance; flag any report missing part of that record. +fn incomplete_provenance_finding(report: &CustomSafeOutputJobAudit) -> Option { + let component = report.component_provenance.as_ref()?; + let missing = [ + ("source", component.source.trim().is_empty()), + ("sha", component.sha.trim().is_empty()), + ( + "manifest_digest", + component.manifest_digest.trim().is_empty(), + ), + ("schema_digest", component.schema_digest.trim().is_empty()), + ] + .into_iter() + .filter_map(|(name, missing)| missing.then_some(name)) + .collect::>(); + if missing.is_empty() { + return None; + } - if ran - && audit - .detection_analysis - .as_ref() - .is_some_and(|analysis| !analysis.safe_to_process) - { - findings.push(Finding { - category: String::from("safe_outputs"), - severity: Severity::High, - title: format!("Custom job ran after unsafe detection for {}", report.tool), - description: format!( - "The custom ADO job for '{}' started even though threat detection marked the safe-output batch unsafe.", - report.tool - ), - impact: Some(String::from( - "A custom write-capable job appears to have bypassed the aggregate detection gate.", - )), - }); - } + Some(Finding { + category: String::from("safe_outputs"), + severity: Severity::High, + title: format!("Incomplete custom component provenance for {}", report.tool), + description: format!( + "The compile-time aw_info marker is missing {} for custom tool '{}'.", + missing.join(", "), + report.tool + ), + impact: Some(String::from( + "The exact custom component revision or schema cannot be independently identified.", + )), + }) +} - if ran - && matches!( - report.approval_path.as_deref(), - Some("manual_review" | "post_review_dependency") - ) - && !manual_review_succeeded(&audit.jobs) - { - findings.push(Finding { - category: String::from("safe_outputs"), - severity: Severity::High, - title: format!("Custom reviewed job ran without approval for {}", report.tool), - description: format!( - "The custom ADO job for '{}' is on the '{}' path, but no successful ManualReview job is present.", - report.tool, - report.approval_path.as_deref().unwrap_or_default() - ), - impact: Some(String::from( - "The observed execution state is inconsistent with the compiler's manual-review gate.", - )), - }); - } +/// The compiled expected ADO job identity for a custom tool should match the +/// correlated timeline job; flag any mismatch. +fn job_identity_mismatch_finding(report: &CustomSafeOutputJobAudit) -> Option { + let expected = report.expected_job_id.as_deref()?; + let actual = report.ado_job.as_ref()?; + if stable_ado_job_identity_matches(actual, expected) != Some(false) { + return None; + } - if report.proposed_count > 0 - && report.ado_job.is_none() - && custom_job_should_have_appeared(audit, report) - { - findings.push(Finding { - category: String::from("safe_outputs"), - severity: Severity::High, - title: format!("Expected custom job missing for {}", report.tool), - description: format!( - "{} proposal(s) were recorded for custom tool '{}', detection allowed processing, but the expected custom ADO job{} is absent from the timeline.", - report.proposed_count, - report.tool, - report - .expected_job_id - .as_deref() - .map(|id| format!(" '{id}'")) - .unwrap_or_default() - ), - impact: Some(String::from( - "The custom proposals have no corresponding job-level execution outcome.", - )), - }); - } + Some(Finding { + category: String::from("safe_outputs"), + severity: Severity::High, + title: format!("Custom job identity mismatch for {}", report.tool), + description: format!( + "Custom tool '{}' was compiled for ADO job '{}', but the correlated timeline job identifies as '{}'.", + report.tool, + expected, + best_ado_job_identity(actual) + ), + impact: Some(String::from( + "The observed job may not be the compiler-approved executor for these proposals.", + )), + }) +} + +/// A custom job that ran without any recorded proposals for its tool +/// indicates the compiler-generated proposal gate did not hold. +fn ran_without_proposals_finding(report: &CustomSafeOutputJobAudit, ran: bool) -> Option { + if !ran || report.proposed_count != 0 { + return None; } + + Some(Finding { + category: String::from("safe_outputs"), + severity: Severity::High, + title: format!("Custom job ran without proposals for {}", report.tool), + description: format!( + "The custom ADO job for '{}' started even though the Agent artifact contains no proposals for that tool.", + report.tool + ), + impact: Some(String::from( + "The compiler-generated proposal gate and the observed runtime state are inconsistent.", + )), + }) +} + +/// A custom job that ran after threat detection marked the batch unsafe +/// indicates the aggregate detection gate was bypassed. +fn ran_after_unsafe_detection_finding( + audit: &AuditData, + report: &CustomSafeOutputJobAudit, + ran: bool, +) -> Option { + let unsafe_detected = audit + .detection_analysis + .as_ref() + .is_some_and(|analysis| !analysis.safe_to_process); + if !ran || !unsafe_detected { + return None; + } + + Some(Finding { + category: String::from("safe_outputs"), + severity: Severity::High, + title: format!("Custom job ran after unsafe detection for {}", report.tool), + description: format!( + "The custom ADO job for '{}' started even though threat detection marked the safe-output batch unsafe.", + report.tool + ), + impact: Some(String::from( + "A custom write-capable job appears to have bypassed the aggregate detection gate.", + )), + }) +} + +/// A reviewed custom job that ran without a successful `ManualReview` job +/// indicates the compiler's manual-review gate was bypassed. +fn ran_without_approval_finding( + audit: &AuditData, + report: &CustomSafeOutputJobAudit, + ran: bool, +) -> Option { + let is_reviewed_path = matches!( + report.approval_path.as_deref(), + Some("manual_review" | "post_review_dependency") + ); + if !ran || !is_reviewed_path || manual_review_succeeded(&audit.jobs) { + return None; + } + + Some(Finding { + category: String::from("safe_outputs"), + severity: Severity::High, + title: format!( + "Custom reviewed job ran without approval for {}", + report.tool + ), + description: format!( + "The custom ADO job for '{}' is on the '{}' path, but no successful ManualReview job is present.", + report.tool, + report.approval_path.as_deref().unwrap_or_default() + ), + impact: Some(String::from( + "The observed execution state is inconsistent with the compiler's manual-review gate.", + )), + }) +} + +/// Proposals with no corresponding job-level execution outcome indicate the +/// expected custom ADO job is missing from the timeline. +fn expected_job_missing_finding( + audit: &AuditData, + report: &CustomSafeOutputJobAudit, +) -> Option { + if report.proposed_count == 0 + || report.ado_job.is_some() + || !custom_job_should_have_appeared(audit, report) + { + return None; + } + + Some(Finding { + category: String::from("safe_outputs"), + severity: Severity::High, + title: format!("Expected custom job missing for {}", report.tool), + description: format!( + "{} proposal(s) were recorded for custom tool '{}', detection allowed processing, but the expected custom ADO job{} is absent from the timeline.", + report.proposed_count, + report.tool, + report + .expected_job_id + .as_deref() + .map(|id| format!(" '{id}'")) + .unwrap_or_default() + ), + impact: Some(String::from( + "The custom proposals have no corresponding job-level execution outcome.", + )), + }) } fn custom_job_should_have_appeared(audit: &AuditData, report: &CustomSafeOutputJobAudit) -> bool { @@ -673,8 +724,7 @@ fn matching_timeline_jobs<'a>( let id_name_matches = jobs .iter() .filter(|job| { - expected_job_id - .is_some_and(|expected| candidate_matches_job_id(&job.name, expected)) + expected_job_id.is_some_and(|expected| candidate_matches_job_id(&job.name, expected)) || candidate_matches_job_id(&job.name, &generated) }) .collect::>(); diff --git a/src/audit/findings.rs b/src/audit/findings.rs index bcbf7f617..4f58d4b47 100644 --- a/src/audit/findings.rs +++ b/src/audit/findings.rs @@ -37,129 +37,173 @@ fn add_ado_proxy_findings( return; }; + add_ado_proxy_unhealthy_lifecycle_finding(proxy, findings, recommendations); + add_ado_proxy_credential_unavailable_finding(proxy, findings, recommendations); + add_ado_proxy_upstream_failed_finding(proxy, findings, recommendations); + add_ado_proxy_out_of_scope_response_finding(proxy, findings, recommendations); + add_ado_proxy_prompt_conflict_finding(proxy, findings, recommendations); + add_ado_proxy_prohibited_request_finding(proxy, findings, recommendations); + add_ado_proxy_malformed_record_finding(proxy, findings, recommendations); +} + +fn add_ado_proxy_unhealthy_lifecycle_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { if proxy .lifecycle .as_ref() - .is_some_and(|lifecycle| !lifecycle.healthy_before_teardown) + .is_none_or(|lifecycle| lifecycle.healthy_before_teardown) { - push_finding( - findings, - Finding { - category: String::from("ado_proxy"), - severity: Severity::High, - title: String::from("ado-proxy was not healthy before teardown"), - description: String::from( - "The proxy did not reach or retain its expected running/listening state before teardown.", - ), - impact: Some(String::from( - "Azure DevOps reads through wrapped az or the Azure DevOps MCP may have failed.", - )), - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("high"), - action: String::from("Inspect ado-proxy lifecycle diagnostics"), - reason: String::from( - "Container state and startup logs identify topology, CA, configuration, or lifecycle failures.", - ), - example: Some(String::from( - "Inspect agent_outputs_/logs/ado-proxy/container.log and container-state.txt", - )), - }, - ); + return; } + push_finding( + findings, + Finding { + category: String::from("ado_proxy"), + severity: Severity::High, + title: String::from("ado-proxy was not healthy before teardown"), + description: String::from( + "The proxy did not reach or retain its expected running/listening state before teardown.", + ), + impact: Some(String::from( + "Azure DevOps reads through wrapped az or the Azure DevOps MCP may have failed.", + )), + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("high"), + action: String::from("Inspect ado-proxy lifecycle diagnostics"), + reason: String::from( + "Container state and startup logs identify topology, CA, configuration, or lifecycle failures.", + ), + example: Some(String::from( + "Inspect agent_outputs_/logs/ado-proxy/container.log and container-state.txt", + )), + }, + ); +} + +fn add_ado_proxy_credential_unavailable_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { let credential_unavailable = proxy_reason_count(proxy, &["credential-unavailable"]); - if credential_unavailable > 0 { - push_finding( - findings, - Finding { - category: String::from("ado_proxy"), - severity: Severity::High, - title: String::from("ado-proxy credential was unavailable"), - description: format!( - "The proxy could not acquire its Azure DevOps read credential for {credential_unavailable} request(s)." - ), - impact: Some(String::from( - "Authorized Azure DevOps reads could not be forwarded upstream.", - )), - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("high"), - action: String::from("Inspect the permissions.read service connection"), - reason: String::from( - "The trusted proxy token source failed; the credential must not be moved into the agent.", - ), - example: None, - }, - ); + if credential_unavailable == 0 { + return; } + push_finding( + findings, + Finding { + category: String::from("ado_proxy"), + severity: Severity::High, + title: String::from("ado-proxy credential was unavailable"), + description: format!( + "The proxy could not acquire its Azure DevOps read credential for {credential_unavailable} request(s)." + ), + impact: Some(String::from( + "Authorized Azure DevOps reads could not be forwarded upstream.", + )), + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("high"), + action: String::from("Inspect the permissions.read service connection"), + reason: String::from( + "The trusted proxy token source failed; the credential must not be moved into the agent.", + ), + example: None, + }, + ); +} + +fn add_ado_proxy_upstream_failed_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { let upstream_failed = proxy_reason_count(proxy, &["upstream-failed"]); - if upstream_failed > 0 { - push_finding( - findings, - Finding { - category: String::from("ado_proxy"), - severity: Severity::High, - title: String::from("ado-proxy could not reach Azure DevOps upstream"), - description: format!( - "{upstream_failed} authorized request(s) failed while reaching the upstream service." - ), - impact: Some(String::from( - "The agent's Azure DevOps reads may be incomplete even though policy allowed them.", - )), - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("high"), - action: String::from("Inspect ado-proxy upstream connectivity"), - reason: String::from( - "AWF/Squid egress, CA trust, or Azure DevOps availability prevented an allowed request.", - ), - example: None, - }, - ); + if upstream_failed == 0 { + return; } + push_finding( + findings, + Finding { + category: String::from("ado_proxy"), + severity: Severity::High, + title: String::from("ado-proxy could not reach Azure DevOps upstream"), + description: format!( + "{upstream_failed} authorized request(s) failed while reaching the upstream service." + ), + impact: Some(String::from( + "The agent's Azure DevOps reads may be incomplete even though policy allowed them.", + )), + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("high"), + action: String::from("Inspect ado-proxy upstream connectivity"), + reason: String::from( + "AWF/Squid egress, CA trust, or Azure DevOps availability prevented an allowed request.", + ), + example: None, + }, + ); +} + +fn add_ado_proxy_out_of_scope_response_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { let out_of_scope_response = proxy_reason_count(proxy, &["out-of-scope-response"]); - if out_of_scope_response > 0 { - push_finding( - findings, - Finding { - category: String::from("security"), - severity: Severity::High, - title: String::from("ado-proxy blocked an over-broad upstream response"), - description: format!( - "Response filtering rejected {out_of_scope_response} response(s) containing resources outside the configured scope." - ), - impact: Some(String::from( - "The proxy prevented out-of-scope Azure DevOps data from reaching the agent.", - )), - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("high"), - action: String::from( - "Inspect the affected ado-proxy operation and response filter", - ), - reason: String::from( - "The response shape may have changed or the operation may require a tighter catalog filter; do not bypass response filtering.", - ), - example: None, - }, - ); + if out_of_scope_response == 0 { + return; } + push_finding( + findings, + Finding { + category: String::from("security"), + severity: Severity::High, + title: String::from("ado-proxy blocked an over-broad upstream response"), + description: format!( + "Response filtering rejected {out_of_scope_response} response(s) containing resources outside the configured scope." + ), + impact: Some(String::from( + "The proxy prevented out-of-scope Azure DevOps data from reaching the agent.", + )), + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("high"), + action: String::from("Inspect the affected ado-proxy operation and response filter"), + reason: String::from( + "The response shape may have changed or the operation may require a tighter catalog filter; do not bypass response filtering.", + ), + example: None, + }, + ); +} + +fn add_ado_proxy_prompt_conflict_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { let prompt_conflict_reasons = [ "capability-disabled", "out-of-scope", @@ -167,35 +211,41 @@ fn add_ado_proxy_findings( "query-not-allowed", ]; let prompt_conflicts = proxy_reason_count(proxy, &prompt_conflict_reasons); - if prompt_conflicts > 0 { - push_finding( - findings, - Finding { - category: String::from("configuration"), - severity: Severity::Medium, - title: String::from("Agent requests conflicted with permissions.read"), - description: format!( - "{prompt_conflicts} request(s) were denied by configured capability, scope, API-version, or query limits: {}.", - format_proxy_reasons(proxy, &prompt_conflict_reasons) - ), - impact: None, - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("medium"), - action: String::from( - "Align the agent prompt with effective Azure DevOps permissions", - ), - reason: String::from( - "The prompt requested data outside the declared front-matter contract. Deliberately review front matter only when broader access is legitimate.", - ), - example: None, - }, - ); + if prompt_conflicts == 0 { + return; } + push_finding( + findings, + Finding { + category: String::from("configuration"), + severity: Severity::Medium, + title: String::from("Agent requests conflicted with permissions.read"), + description: format!( + "{prompt_conflicts} request(s) were denied by configured capability, scope, API-version, or query limits: {}.", + format_proxy_reasons(proxy, &prompt_conflict_reasons) + ), + impact: None, + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("medium"), + action: String::from("Align the agent prompt with effective Azure DevOps permissions"), + reason: String::from( + "The prompt requested data outside the declared front-matter contract. Deliberately review front matter only when broader access is legitimate.", + ), + example: None, + }, + ); +} + +fn add_ado_proxy_prohibited_request_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { let prohibited_reasons = [ "method-not-read", "denied-route-family", @@ -204,61 +254,71 @@ fn add_ado_proxy_findings( "malformed-target", ]; let prohibited = proxy_reason_count(proxy, &prohibited_reasons); - if prohibited > 0 { - push_finding( - findings, - Finding { - category: String::from("security"), - severity: Severity::Medium, - title: String::from("ado-proxy blocked prohibited request shapes"), - description: format!( - "{prohibited} direct write, denied-family, unknown, or malformed request(s) were blocked: {}.", - format_proxy_reasons(proxy, &prohibited_reasons) - ), - impact: None, - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("medium"), - action: String::from("Remove unsupported Azure DevOps requests from the prompt"), - reason: String::from( - "Direct writes and uncatalogued APIs must not be enabled by widening the proxy policy.", - ), - example: None, - }, - ); + if prohibited == 0 { + return; } - if proxy.malformed_record_count > 0 { - push_finding( - findings, - Finding { - category: String::from("ado_proxy"), - severity: Severity::Medium, - title: String::from("ado-proxy decision log contained malformed records"), - description: format!( - "{} decision record(s) did not match the declared v1 schema.", - proxy.malformed_record_count - ), - impact: Some(String::from( - "The audit summary may omit affected proxy decisions.", - )), - }, - ); - push_recommendation( - recommendations, - Recommendation { - priority: String::from("medium"), - action: String::from("Check ado-proxy bundle/compiler schema compatibility"), - reason: String::from( - "The analyzer rejected records rather than guessing at an unknown shape.", - ), - example: None, - }, - ); + push_finding( + findings, + Finding { + category: String::from("security"), + severity: Severity::Medium, + title: String::from("ado-proxy blocked prohibited request shapes"), + description: format!( + "{prohibited} direct write, denied-family, unknown, or malformed request(s) were blocked: {}.", + format_proxy_reasons(proxy, &prohibited_reasons) + ), + impact: None, + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("medium"), + action: String::from("Remove unsupported Azure DevOps requests from the prompt"), + reason: String::from( + "Direct writes and uncatalogued APIs must not be enabled by widening the proxy policy.", + ), + example: None, + }, + ); +} + +fn add_ado_proxy_malformed_record_finding( + proxy: &crate::audit::model::AdoProxyAnalysis, + findings: &mut Vec, + recommendations: &mut Vec, +) { + if proxy.malformed_record_count == 0 { + return; } + + push_finding( + findings, + Finding { + category: String::from("ado_proxy"), + severity: Severity::Medium, + title: String::from("ado-proxy decision log contained malformed records"), + description: format!( + "{} decision record(s) did not match the declared v1 schema.", + proxy.malformed_record_count + ), + impact: Some(String::from( + "The audit summary may omit affected proxy decisions.", + )), + }, + ); + push_recommendation( + recommendations, + Recommendation { + priority: String::from("medium"), + action: String::from("Check ado-proxy bundle/compiler schema compatibility"), + reason: String::from( + "The analyzer rejected records rather than guessing at an unknown shape.", + ), + example: None, + }, + ); } fn proxy_reason_count(proxy: &crate::audit::model::AdoProxyAnalysis, reasons: &[&str]) -> u64 { diff --git a/src/compile/ado_bundle.rs b/src/compile/ado_bundle.rs index 6ec933a5a..4bb45757f 100644 --- a/src/compile/ado_bundle.rs +++ b/src/compile/ado_bundle.rs @@ -13,8 +13,8 @@ //! * [`Bundle`] enumerates every bundle, with its on-disk [`Bundle::path`] and //! its [`Bundle::auth`] requirement. //! * [`apply_bundle_auth`] is the single chokepoint that projects -//! `SYSTEM_ACCESSTOKEN` into a step for every bearer-requiring bundle, so no -//! call site can forget it again. +//! `SYSTEM_ACCESSTOKEN` and marks it as bearer auth for every +//! bearer-requiring bundle, so no call site can forget either half again. //! * [`token_source_for`] unifies the `System.AccessToken` vs `SC_WRITE_TOKEN` //! selection that was previously duplicated between the Conclusion job and //! the Stage 3 executor. @@ -227,16 +227,14 @@ impl Bundle { /// Project the bundle's auth env contract onto a step. /// /// For [`BundleAuth::Bearer`] bundles this maps `SYSTEM_ACCESSTOKEN` from the -/// chosen [`TokenSource`]. For [`BundleAuth::None`] it is a no-op — the `token` -/// argument is ignored (a `None`-auth bundle needs no bearer, and today no -/// caller routes such a bundle through this function). This is the single -/// guarantee that every bearer-requiring bundle step carries a token — the -/// structural fix for the class of bug behind #1307. +/// chosen [`TokenSource`] and sets `ADO_AW_ACCESS_TOKEN_KIND=bearer`, which the +/// shared Node auth helper needs for both `System.AccessToken` and minted +/// `SC_WRITE_TOKEN` values. For [`BundleAuth::None`] it is a no-op. pub fn apply_bundle_auth(step: BashStep, bundle: Bundle, token: TokenSource) -> BashStep { match bundle.auth() { - BundleAuth::Bearer => { - step.with_env("SYSTEM_ACCESSTOKEN", EnvValue::secret(token.variable())) - } + BundleAuth::Bearer => step + .with_env("SYSTEM_ACCESSTOKEN", EnvValue::secret(token.variable())) + .with_env("ADO_AW_ACCESS_TOKEN_KIND", EnvValue::literal("bearer")), BundleAuth::None => step, } } @@ -292,15 +290,28 @@ mod tests { let step = BashStep::new("t", "node x\n"); let out = apply_bundle_auth(step, *b, TokenSource::SystemAccessToken); let has_token = out.env.contains_key("SYSTEM_ACCESSTOKEN"); + let has_token_kind = out.env.contains_key("ADO_AW_ACCESS_TOKEN_KIND"); match b.auth() { - BundleAuth::Bearer => assert!( - has_token, - "{b:?} requires a bearer and apply_bundle_auth must project it" - ), - BundleAuth::None => assert!( - !has_token, - "{b:?} is None and must not carry SYSTEM_ACCESSTOKEN" - ), + BundleAuth::Bearer => { + assert!( + has_token, + "{b:?} requires a bearer and apply_bundle_auth must project it" + ); + assert!( + has_token_kind, + "{b:?} requires a bearer and apply_bundle_auth must mark its kind" + ); + } + BundleAuth::None => { + assert!( + !has_token, + "{b:?} is None and must not carry SYSTEM_ACCESSTOKEN" + ); + assert!( + !has_token_kind, + "{b:?} is None and must not carry ADO_AW_ACCESS_TOKEN_KIND" + ); + } } } } diff --git a/src/safe_outputs/assign_github_issue_milestone.rs b/src/safe_outputs/assign_github_issue_milestone.rs index f42bf1df8..4b7a4b9a8 100644 --- a/src/safe_outputs/assign_github_issue_milestone.rs +++ b/src/safe_outputs/assign_github_issue_milestone.rs @@ -171,6 +171,111 @@ fn milestone_is_allowed( }) } +/// Result of resolving (or creating) the milestone to assign: `(number, +/// title, created)`. +type ResolvedMilestone = (u64, String, bool); + +/// Resolve an explicitly numbered milestone request against the already +/// fetched milestone list, checking the allow-list policy. +/// +/// Returns `Ok(Err(result))` for any policy/not-found failure so callers can +/// propagate it as an `ExecutionResult` without an early `return` of their +/// own. +fn resolve_milestone_by_number( + config: &AssignGithubIssueMilestoneConfig, + milestones: &[crate::safe_outputs::github_api::GithubMilestone], + target_repo: &str, + requested_number: u64, +) -> anyhow::Result> { + let Some(milestone) = milestones + .iter() + .find(|milestone| milestone.number == requested_number) + else { + return Ok(Err(ExecutionResult::failure(format!( + "GitHub milestone #{requested_number} does not exist in {target_repo}" + )))); + }; + if !milestone_is_allowed(config, Some(milestone.number), &milestone.title) { + return Ok(Err(ExecutionResult::failure(format!( + "GitHub milestone #{} ('{}') is not in the allowed list", + milestone.number, + crate::sanitize::neutralize_pipeline_commands(&milestone.title) + )))); + } + Ok(Ok((milestone.number, milestone.title.clone(), false))) +} + +/// Resolve a milestone request by title: matches an existing milestone, +/// disambiguates duplicates, and auto-creates a new milestone when +/// `auto-create` is enabled and no existing milestone matches. +async fn resolve_milestone_by_title( + client: &GithubClient, + config: &AssignGithubIssueMilestoneConfig, + milestones: &[crate::safe_outputs::github_api::GithubMilestone], + target_repo: &str, + requested_title: &str, +) -> anyhow::Result> { + let matches: Vec<_> = milestones + .iter() + .filter(|milestone| milestone.title == requested_title) + .collect(); + if matches.len() > 1 { + return Ok(Err(ExecutionResult::failure(format!( + "multiple GitHub milestones in {} have the exact title '{}'; use \ + milestone_number to disambiguate", + target_repo, + crate::sanitize::neutralize_pipeline_commands(requested_title) + )))); + } + if let Some(milestone) = matches.first() { + if !milestone_is_allowed(config, Some(milestone.number), &milestone.title) { + return Ok(Err(ExecutionResult::failure(format!( + "GitHub milestone '{}' (#{}) is not in the allowed list", + crate::sanitize::neutralize_pipeline_commands(&milestone.title), + milestone.number + )))); + } + return Ok(Ok((milestone.number, milestone.title.clone(), false))); + } + + if !milestone_is_allowed(config, None, requested_title) { + return Ok(Err(ExecutionResult::failure(format!( + "GitHub milestone '{}' is not in the allowed list", + crate::sanitize::neutralize_pipeline_commands(requested_title) + )))); + } + if !config.auto_create { + return Ok(Err(ExecutionResult::failure(format!( + "GitHub milestone '{}' does not exist in {} and auto-create is false", + crate::sanitize::neutralize_pipeline_commands(requested_title), + target_repo + )))); + } + + let response = client + .send( + Method::POST, + client.milestones_url(target_repo)?, + Some(&serde_json::json!({ "title": requested_title })), + ) + .await?; + let response = match response.require_success("Failed to create GitHub milestone") { + Ok(response) => response, + Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), + }; + let payload: serde_json::Value = response.json("Failed to parse created GitHub milestone")?; + let Some(number) = payload + .get("number") + .and_then(serde_json::Value::as_u64) + .filter(|number| *number > 0) + else { + return Ok(Err(ExecutionResult::failure( + "GitHub create milestone response contained no positive milestone number", + ))); + }; + Ok(Ok((number, requested_title.to_string(), true))) +} + #[async_trait::async_trait] impl Executor for AssignGithubIssueMilestoneResult { fn dry_run_summary(&self) -> String { @@ -238,92 +343,25 @@ impl Executor for AssignGithubIssueMilestoneResult { Err(error) => return Ok(ExecutionResult::failure(error.to_string())), }; - let (milestone_number, milestone_title, created) = if let Some(requested_number) = - self.milestone_number - { - let Some(milestone) = milestones - .iter() - .find(|milestone| milestone.number == requested_number) - else { - return Ok(ExecutionResult::failure(format!( - "GitHub milestone #{requested_number} does not exist in {}", - target.repository - ))); - }; - if !milestone_is_allowed(&config, Some(milestone.number), &milestone.title) { - return Ok(ExecutionResult::failure(format!( - "GitHub milestone #{} ('{}') is not in the allowed list", - milestone.number, - crate::sanitize::neutralize_pipeline_commands(&milestone.title) - ))); - } - (milestone.number, milestone.title.clone(), false) + let resolved = if let Some(requested_number) = self.milestone_number { + resolve_milestone_by_number(&config, &milestones, &target.repository, requested_number)? } else { let requested_title = self .milestone_title .as_deref() .expect("validated result has a milestone title"); - let matches: Vec<_> = milestones - .iter() - .filter(|milestone| milestone.title == requested_title) - .collect(); - if matches.len() > 1 { - return Ok(ExecutionResult::failure(format!( - "multiple GitHub milestones in {} have the exact title '{}'; use \ - milestone_number to disambiguate", - target.repository, - crate::sanitize::neutralize_pipeline_commands(requested_title) - ))); - } - if let Some(milestone) = matches.first() { - if !milestone_is_allowed(&config, Some(milestone.number), &milestone.title) { - return Ok(ExecutionResult::failure(format!( - "GitHub milestone '{}' (#{}) is not in the allowed list", - crate::sanitize::neutralize_pipeline_commands(&milestone.title), - milestone.number - ))); - } - (milestone.number, milestone.title.clone(), false) - } else { - if !milestone_is_allowed(&config, None, requested_title) { - return Ok(ExecutionResult::failure(format!( - "GitHub milestone '{}' is not in the allowed list", - crate::sanitize::neutralize_pipeline_commands(requested_title) - ))); - } - if !config.auto_create { - return Ok(ExecutionResult::failure(format!( - "GitHub milestone '{}' does not exist in {} and auto-create is false", - crate::sanitize::neutralize_pipeline_commands(requested_title), - target.repository - ))); - } - let response = client - .send( - Method::POST, - client.milestones_url(&target.repository)?, - Some(&serde_json::json!({ "title": requested_title })), - ) - .await?; - let response = match response.require_success("Failed to create GitHub milestone") { - Ok(response) => response, - Err(error) => { - return Ok(ExecutionResult::failure(error.to_string())); - } - }; - let payload: serde_json::Value = - response.json("Failed to parse created GitHub milestone")?; - let Some(number) = payload - .get("number") - .and_then(serde_json::Value::as_u64) - .filter(|number| *number > 0) - else { - return Ok(ExecutionResult::failure( - "GitHub create milestone response contained no positive milestone number", - )); - }; - (number, requested_title.to_string(), true) - } + resolve_milestone_by_title( + &client, + &config, + &milestones, + &target.repository, + requested_title, + ) + .await? + }; + let (milestone_number, milestone_title, created) = match resolved { + Ok(resolved) => resolved, + Err(result) => return Ok(result), }; let issue_url = client.issue_url(&target.repository, target.number)?; diff --git a/src/safe_outputs/create_github_issue.rs b/src/safe_outputs/create_github_issue.rs index b4e2fc9dc..fca8e913a 100644 --- a/src/safe_outputs/create_github_issue.rs +++ b/src/safe_outputs/create_github_issue.rs @@ -176,6 +176,80 @@ const ALLOWED_LABELS_ANY: &str = "*"; /// hitting the API with an over-long string. const MAX_FINAL_TITLE_LEN: usize = 256; +/// Validates agent-supplied `labels` against `config.allowed-labels`. +/// +/// Default-deny semantics: an empty list means NO agent labels are +/// accepted. Operators must opt in to unrestricted by setting +/// `allowed-labels: ["*"]`. Static labels under `labels:` are always +/// applied regardless and are not checked here. +/// +/// Returns `Err(message)` with a ready-to-use failure message when any +/// agent-supplied label is not covered by the allowlist. +fn validate_agent_labels( + labels: &[String], + config: &CreateGithubIssueConfig, +) -> Result<(), String> { + if labels.is_empty() { + return Ok(()); + } + let allow_any = config + .allowed_labels + .iter() + .any(|p| p == ALLOWED_LABELS_ANY); + if allow_any { + return Ok(()); + } + let disallowed: Vec = labels + .iter() + .filter(|label| { + !config + .allowed_labels + .iter() + .any(|pattern| super::tag_matches_pattern(label, pattern)) + }) + .map(|label| { + // Neutralise pipeline-command sequences before we echo + // agent-supplied content into our own log line and the + // failure message. + crate::sanitize::neutralize_pipeline_commands(label) + }) + .collect(); + if disallowed.is_empty() { + return Ok(()); + } + let msg = if config.allowed_labels.is_empty() { + format!( + "Agent-supplied labels rejected (no `allowed-labels` configured; \ + set `allowed-labels: [\"*\"]` to permit any): {}", + disallowed.join(", ") + ) + } else { + format!( + "Agent-supplied labels not in allowed-labels: {}", + disallowed.join(", ") + ) + }; + Err(msg) +} + +/// Applies `config.title-prefix` (if any) and enforces +/// [`MAX_FINAL_TITLE_LEN`] on the result. +fn build_final_title(title: &str, config: &CreateGithubIssueConfig) -> Result { + let final_title = match &config.title_prefix { + Some(prefix) => format!("{prefix}{title}"), + None => title.to_string(), + }; + if final_title.len() > MAX_FINAL_TITLE_LEN { + return Err(format!( + "Final issue title exceeds {MAX_FINAL_TITLE_LEN} characters \ + ({} chars after applying title-prefix). Shorten title-prefix \ + or the agent title.", + final_title.len() + )); + } + Ok(final_title) +} + #[async_trait::async_trait] impl Executor for CreateGithubIssueResult { fn dry_run_summary(&self) -> String { @@ -195,14 +269,11 @@ impl Executor for CreateGithubIssueResult { )); } - let token = match ctx.github_token.as_ref() { - Some(t) => t, - None => { - return Ok(ExecutionResult::failure( - "ADO_AW_GITHUB_TOKEN is not set; configure safe-outputs.github-token \ - or safe-outputs.github-app", - )); - } + let Some(token) = ctx.github_token.as_ref() else { + return Ok(ExecutionResult::failure( + "ADO_AW_GITHUB_TOKEN is not set; configure safe-outputs.github-token \ + or safe-outputs.github-app", + )); }; let config: CreateGithubIssueConfig = ctx.get_tool_config("create-github-issue")?; @@ -230,63 +301,14 @@ impl Executor for CreateGithubIssueResult { ))); } - // Validate agent-supplied labels against allowed-labels. - // Default-deny semantics: an empty list means NO agent labels are - // accepted. Operators must opt in to unrestricted by setting - // `allowed-labels: ["*"]`. Static labels under `labels:` are always - // applied regardless. - if !self.labels.is_empty() { - let allow_any = config - .allowed_labels - .iter() - .any(|p| p == ALLOWED_LABELS_ANY); - if !allow_any { - let disallowed: Vec = self - .labels - .iter() - .filter(|label| { - !config - .allowed_labels - .iter() - .any(|pattern| super::tag_matches_pattern(label, pattern)) - }) - .map(|label| { - // Neutralise pipeline-command sequences before we - // echo agent-supplied content into our own log line - // and the failure message. - crate::sanitize::neutralize_pipeline_commands(label) - }) - .collect(); - if !disallowed.is_empty() { - let msg = if config.allowed_labels.is_empty() { - format!( - "Agent-supplied labels rejected (no `allowed-labels` configured; \ - set `allowed-labels: [\"*\"]` to permit any): {}", - disallowed.join(", ") - ) - } else { - format!( - "Agent-supplied labels not in allowed-labels: {}", - disallowed.join(", ") - ) - }; - return Ok(ExecutionResult::failure(msg)); - } - } + if let Err(msg) = validate_agent_labels(&self.labels, &config) { + return Ok(ExecutionResult::failure(msg)); } - let final_title = match &config.title_prefix { - Some(prefix) => format!("{}{}", prefix, self.title), - None => self.title.clone(), + let final_title = match build_final_title(&self.title, &config) { + Ok(title) => title, + Err(msg) => return Ok(ExecutionResult::failure(msg)), }; - if final_title.len() > MAX_FINAL_TITLE_LEN { - return Ok(ExecutionResult::failure(format!( - "Final issue title exceeds {MAX_FINAL_TITLE_LEN} characters \ - ({} chars after applying title-prefix). Shorten title-prefix \ - or the agent title.", - final_title.len() - ))); - } let body_with_footer = format!("{}\n\n{}", self.body, build_github_trace_footer(ctx)); let all_labels = merge_github_values(&config.labels, &self.labels); let all_assignees = merge_github_values(&config.assignees, &self.assignees); @@ -303,71 +325,83 @@ impl Executor for CreateGithubIssueResult { }); let response = client.send(Method::POST, url, Some(&payload)).await?; + self.handle_issue_response(ctx, &target_repo, response) + .await + } +} - let status = response.status; - if status.is_success() { - let body: serde_json::Value = response - .json("Failed to parse GitHub API response") - .map_err(anyhow::Error::new)?; - let Some(number) = body - .get("number") - .and_then(|v| v.as_u64()) - .filter(|number| *number > 0) - else { - return Ok(ExecutionResult::failure( - "GitHub create-github-issue response contained no positive issue number", - )); - }; - let html_url = body - .get("html_url") - .and_then(|v| v.as_str()) - .unwrap_or("") - .to_string(); - info!( - "Filed GitHub issue {}#{}: {}", - target_repo, number, html_url - ); - 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), +impl CreateGithubIssueResult { + /// Interprets the GitHub API response for the create-issue request, + /// registering the `temporary_id` (if any) on success. + async fn handle_issue_response( + &self, + ctx: &ExecutionContext, + target_repo: &str, + response: crate::safe_outputs::GithubResponse, + ) -> anyhow::Result { + if !response.status.is_success() { + let error = response + .require_success("Failed to file GitHub issue") + .expect_err("non-success response must produce an API error"); + return Ok(ExecutionResult::failure(error.to_string())); + } + + let body: serde_json::Value = response + .json("Failed to parse GitHub API response") + .map_err(anyhow::Error::new)?; + let Some(number) = body + .get("number") + .and_then(|v| v.as_u64()) + .filter(|number| *number > 0) + else { + return Ok(ExecutionResult::failure( + "GitHub create-github-issue response contained no positive issue number", + )); + }; + let html_url = body + .get("html_url") + .and_then(|v| v.as_str()) + .unwrap_or("") + .to_string(); + info!( + "Filed GitHub issue {}#{}: {}", + target_repo, number, html_url + ); + 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.to_string(), + 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": self.temporary_id.as_ref().map(GithubTemporaryId::canonical), + "temporary_id": temporary_id.canonical(), }), - )) - } else { - let error = response - .require_success("Failed to file GitHub issue") - .expect_err("non-success response must produce an API error"); - Ok(ExecutionResult::failure(error.to_string())) + )); } + Ok(ExecutionResult::success_with_data( + format!("Filed issue {}#{}: {}", target_repo, number, html_url), + serde_json::json!({ + "number": number, + "url": html_url, + "target_repo": target_repo, + "temporary_id": self.temporary_id.as_ref().map(GithubTemporaryId::canonical), + }), + )) } } diff --git a/src/safe_outputs/create_work_item.rs b/src/safe_outputs/create_work_item.rs index 6677b4e20..c352112fd 100644 --- a/src/safe_outputs/create_work_item.rs +++ b/src/safe_outputs/create_work_item.rs @@ -30,7 +30,6 @@ pub struct CreateWorkItemParams { /// semicolon (ADO uses semicolons as tag separators). #[serde(default)] pub tags: Vec, - } impl Validate for CreateWorkItemParams { @@ -330,9 +329,7 @@ fn sorted_custom_fields( .iter() .map(|(field, value)| (field.as_str(), value.as_str())) .collect(); - custom_fields.sort_unstable_by(|(a, _), (b, _)| { - a.to_ascii_lowercase().cmp(&b.to_ascii_lowercase()) - }); + custom_fields.sort_unstable_by_key(|(field, _)| field.to_ascii_lowercase()); custom_fields } @@ -359,6 +356,184 @@ fn artifact_link_op(project: &str, repository_id: &str, branch: &str) -> serde_j }) } +/// Merge validated agent-provided tags with the operator-configured tags, +/// deduplicating case-insensitively and returning the merged list. +fn merge_tags(config_tags: &[String], agent_tags: &[String]) -> Vec { + let mut all_tags = config_tags.to_vec(); + for tag in agent_tags { + if !all_tags.iter().any(|t| t.eq_ignore_ascii_case(tag)) { + all_tags.push(tag.clone()); + } + } + all_tags +} + +/// Check agent-provided tags against the operator-configured allowlist (if any). +/// +/// Returns `Err(message)` describing the disallowed tags when the check fails. +fn check_allowed_tags(tags: &[String], allowed_tags: &[String]) -> Result<(), String> { + if tags.is_empty() || allowed_tags.is_empty() { + return Ok(()); + } + let disallowed: Vec<_> = tags + .iter() + .filter(|tag| { + !allowed_tags + .iter() + .any(|pattern| super::tag_matches_pattern(tag, pattern)) + }) + .collect(); + if disallowed.is_empty() { + return Ok(()); + } + Err(format!( + "Agent-provided tags not in allowed-tags: {}", + disallowed + .iter() + .map(|t| t.as_str()) + .collect::>() + .join(", ") + )) +} + +/// Build the JSON Patch document used to create the work item, applying the +/// title/description, optional configured fields, tags, and custom fields. +/// +/// Returns `Err(message)` when the resulting field set is invalid (e.g. +/// duplicate fields). +fn build_patch_document( + config: &CreateWorkItemConfig, + title: &str, + description_with_stats: &str, + all_tags: &[String], +) -> Result, String> { + let description_field = description_field_for(config); + validate_patch_fields(config, description_field, !all_tags.is_empty()) + .map_err(|error| error.to_string())?; + + let mut patch_doc = vec![ + field_op("System.Title", title), + field_op(description_field, description_with_stats), + // Tell Azure DevOps the description is markdown + serde_json::json!({ + "op": "add", + "path": format!("/multilineFieldsFormat/{description_field}"), + "value": "Markdown" + }), + ]; + + if let Some(area_path) = &config.area_path { + patch_doc.push(field_op("System.AreaPath", area_path)); + } + if let Some(iteration_path) = &config.iteration_path { + patch_doc.push(field_op("System.IterationPath", iteration_path)); + } + if let Some(assignee) = config.assignee.as_deref() { + let assignee = + super::normalize_work_item_assignee(assignee, "safe-outputs.create-work-item.assignee") + .map_err(|error| error.to_string())?; + patch_doc.push(field_op("System.AssignedTo", assignee)); + } + // Merge static config tags with validated agent-provided tags (dedup, case-insensitive) + if !all_tags.is_empty() { + patch_doc.push(field_op("System.Tags", all_tags.join("; "))); + } + + // Add any custom fields + for (field, value) in sorted_custom_fields(&config.custom_fields) { + patch_doc.push(field_op(field, value)); + } + + Ok(patch_doc) +} + +/// Handle a successful (HTTP 2xx) work item creation response: parse the +/// body, register the resolved temporary ID, and build the final result. +async fn handle_creation_success( + response: reqwest::Response, + ctx: &ExecutionContext, + temporary_id: &WorkItemTemporaryId, + title: &str, + project: &str, + work_item_type: &str, + artifact_link_included: Option, +) -> anyhow::Result { + let body: serde_json::Value = response + .json() + .await + .context("Failed to parse response JSON")?; + + let Some(work_item_id) = body.get("id").and_then(|v| v.as_u64()).filter(|id| *id > 0) else { + return Ok(ExecutionResult::failure( + "Azure DevOps create-work-item response contained no positive work-item ID", + )); + }; + let work_item_url = body + .get("_links") + .and_then(|l| l.get("html")) + .and_then(|h| h.get("href")) + .and_then(|h| h.as_str()) + .unwrap_or(""); + + info!("Work item created: #{} - {}", work_item_id, work_item_url); + + if let Err(error) = ctx.register_resolved_work_item( + temporary_id, + crate::safe_outputs::ResolvedWorkItem { + id: work_item_id, + url: work_item_url.to_string(), + }, + ) { + return Ok(ExecutionResult::failure_with_data( + format!( + "Created work item #{} but failed to register temporary_id '{}': {}", + work_item_id, + temporary_id.canonical(), + crate::sanitize::neutralize_pipeline_commands(&error.to_string()) + ), + serde_json::json!({ + "id": work_item_id, + "url": work_item_url, + "temporary_id": temporary_id.canonical(), + }), + )); + } + + let message = match &artifact_link_included { + Some(link_msg) => format!( + "Created work item #{}: {} (artifact link: {})", + work_item_id, title, link_msg + ), + None => format!("Created work item #{}: {}", work_item_id, title), + }; + + Ok(ExecutionResult::success_with_data( + message, + serde_json::json!({ + "id": work_item_id, + "url": work_item_url, + "project": project, + "type": work_item_type, + "artifact_link": artifact_link_included, + "temporary_id": temporary_id.canonical(), + }), + )) +} + +/// Handle a non-success HTTP response from the work item creation request. +async fn handle_creation_failure(response: reqwest::Response) -> ExecutionResult { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + + ExecutionResult::failure(format!( + "Failed to create work item (HTTP {}): {}", + status, error_body + )) +} + /// Resolve the artifact link patch op and a human-readable status message. /// /// Returns `Ok(None)` when artifact linking is disabled. @@ -503,27 +678,8 @@ impl Executor for CreateWorkItemResult { } // Validate agent-provided tags against allowed-tags (if configured) - if !self.tags.is_empty() && !config.allowed_tags.is_empty() { - let disallowed: Vec<_> = self - .tags - .iter() - .filter(|tag| { - !config - .allowed_tags - .iter() - .any(|pattern| super::tag_matches_pattern(tag, pattern)) - }) - .collect(); - if !disallowed.is_empty() { - return Ok(ExecutionResult::failure(format!( - "Agent-provided tags not in allowed-tags: {}", - disallowed - .iter() - .map(|t| t.as_str()) - .collect::>() - .join(", ") - ))); - } + if let Err(message) = check_allowed_tags(&self.tags, &config.allowed_tags) { + return Ok(ExecutionResult::failure(message)); } // Build the Azure DevOps REST API URL for creating work items @@ -537,58 +693,16 @@ impl Executor for CreateWorkItemResult { ); debug!("API URL: {}", url); - let description_field = description_field_for(&config); - let mut all_tags = config.tags.clone(); - for tag in &self.tags { - if !all_tags.iter().any(|t| t.eq_ignore_ascii_case(tag)) { - all_tags.push(tag.clone()); - } - } + let all_tags = merge_tags(&config.tags, &self.tags); // Build the patch document for work item creation let description_with_stats = crate::agent_stats::append_stats_to_body(&self.description, ctx, config.include_stats); - if let Err(error) = validate_patch_fields(&config, description_field, !all_tags.is_empty()) - { - return Ok(ExecutionResult::failure(error.to_string())); - } - let mut patch_doc = vec![ - field_op("System.Title", &self.title), - field_op(description_field, &description_with_stats), - // Tell Azure DevOps the description is markdown - serde_json::json!({ - "op": "add", - "path": format!("/multilineFieldsFormat/{description_field}"), - "value": "Markdown" - }), - ]; - - // Add optional configured fields - if let Some(area_path) = &config.area_path { - patch_doc.push(field_op("System.AreaPath", area_path)); - } - if let Some(iteration_path) = &config.iteration_path { - patch_doc.push(field_op("System.IterationPath", iteration_path)); - } - if let Some(assignee) = config.assignee.as_deref() { - let assignee = match super::normalize_work_item_assignee( - assignee, - "safe-outputs.create-work-item.assignee", - ) { - Ok(assignee) => assignee, - Err(error) => return Ok(ExecutionResult::failure(error.to_string())), + let mut patch_doc = + match build_patch_document(&config, &self.title, &description_with_stats, &all_tags) { + Ok(patch_doc) => patch_doc, + Err(message) => return Ok(ExecutionResult::failure(message)), }; - patch_doc.push(field_op("System.AssignedTo", assignee)); - } - // Merge static config tags with validated agent-provided tags (dedup, case-insensitive) - if !all_tags.is_empty() { - patch_doc.push(field_op("System.Tags", all_tags.join("; "))); - } - - // Add any custom fields - for (field, value) in sorted_custom_fields(&config.custom_fields) { - patch_doc.push(field_op(field, value)); - } // Create HTTP client (needed for both work item creation and optional repo lookup) let client = reqwest::Client::new(); @@ -626,81 +740,18 @@ impl Executor for CreateWorkItemResult { .context("Failed to send request to Azure DevOps")?; if response.status().is_success() { - let body: serde_json::Value = response - .json() - .await - .context("Failed to parse response JSON")?; - - let Some(work_item_id) = body - .get("id") - .and_then(|v| v.as_u64()) - .filter(|id| *id > 0) - else { - return Ok(ExecutionResult::failure( - "Azure DevOps create-work-item response contained no positive work-item ID", - )); - }; - let work_item_url = body - .get("_links") - .and_then(|l| l.get("html")) - .and_then(|h| h.get("href")) - .and_then(|h| h.as_str()) - .unwrap_or(""); - - info!("Work item created: #{} - {}", work_item_id, work_item_url); - - if let Err(error) = ctx.register_resolved_work_item( + handle_creation_success( + response, + ctx, &self.temporary_id, - crate::safe_outputs::ResolvedWorkItem { - id: work_item_id, - url: work_item_url.to_string(), - }, - ) { - return Ok(ExecutionResult::failure_with_data( - format!( - "Created work item #{} but failed to register temporary_id '{}': {}", - work_item_id, - self.temporary_id.canonical(), - crate::sanitize::neutralize_pipeline_commands(&error.to_string()) - ), - serde_json::json!({ - "id": work_item_id, - "url": work_item_url, - "temporary_id": self.temporary_id.canonical(), - }), - )); - } - - let message = match &artifact_link_included { - Some(link_msg) => format!( - "Created work item #{}: {} (artifact link: {})", - work_item_id, self.title, link_msg - ), - None => format!("Created work item #{}: {}", work_item_id, self.title), - }; - - Ok(ExecutionResult::success_with_data( - message, - serde_json::json!({ - "id": work_item_id, - "url": work_item_url, - "project": project, - "type": config.work_item_type, - "artifact_link": artifact_link_included, - "temporary_id": self.temporary_id.canonical(), - }), - )) + &self.title, + project, + &config.work_item_type, + artifact_link_included, + ) + .await } else { - let status = response.status(); - let error_body = response - .text() - .await - .unwrap_or_else(|_| "Unknown error".to_string()); - - Ok(ExecutionResult::failure(format!( - "Failed to create work item (HTTP {}): {}", - status, error_body - ))) + Ok(handle_creation_failure(response).await) } } } @@ -741,12 +792,10 @@ mod tests { description: "This is a sufficiently long description for the work item.".to_string(), tags: vec![], }; - let result: CreateWorkItemResult = ( - params, - WorkItemTemporaryId::parse("#aw_test1").unwrap(), - ) - .try_into() - .unwrap(); + let result: CreateWorkItemResult = + (params, WorkItemTemporaryId::parse("#aw_test1").unwrap()) + .try_into() + .unwrap(); assert_eq!(result.name, "create-work-item"); assert_eq!(result.title, "Implement feature"); assert!(result.description.contains("sufficiently long")); @@ -759,11 +808,8 @@ mod tests { description: "This is a sufficiently long description for the work item.".to_string(), tags: vec![], }; - let result: Result = ( - params, - WorkItemTemporaryId::parse("#aw_test1").unwrap(), - ) - .try_into(); + let result: Result = + (params, WorkItemTemporaryId::parse("#aw_test1").unwrap()).try_into(); let err = result.unwrap_err().to_string(); assert!( err.contains("title must be more than 5 characters"), @@ -778,11 +824,8 @@ mod tests { description: "Too short".to_string(), tags: vec![], }; - let result: Result = ( - params, - WorkItemTemporaryId::parse("#aw_test1").unwrap(), - ) - .try_into(); + let result: Result = + (params, WorkItemTemporaryId::parse("#aw_test1").unwrap()).try_into(); let err = result.unwrap_err().to_string(); assert!( err.contains("description must be more than 30 characters"), @@ -797,11 +840,8 @@ mod tests { description: "This is a sufficiently long description for the work item.".to_string(), tags: vec!["tag-one; tag-two".to_string()], }; - let result: Result = ( - params, - WorkItemTemporaryId::parse("#aw_test1").unwrap(), - ) - .try_into(); + let result: Result = + (params, WorkItemTemporaryId::parse("#aw_test1").unwrap()).try_into(); let err = result.unwrap_err().to_string(); assert!( err.contains("semicolon"), @@ -816,12 +856,10 @@ mod tests { description: "This is a sufficiently long description for the work item.".to_string(), tags: vec!["agent-created".to_string(), "automated".to_string()], }; - let result: CreateWorkItemResult = ( - params, - WorkItemTemporaryId::parse("#aw_test1").unwrap(), - ) - .try_into() - .unwrap(); + let result: CreateWorkItemResult = + (params, WorkItemTemporaryId::parse("#aw_test1").unwrap()) + .try_into() + .unwrap(); assert_eq!(result.tags, vec!["agent-created", "automated"]); } @@ -833,12 +871,10 @@ mod tests { .to_string(), tags: vec![], }; - let result: CreateWorkItemResult = ( - params, - WorkItemTemporaryId::parse("#aw_test1").unwrap(), - ) - .try_into() - .unwrap(); + let result: CreateWorkItemResult = + (params, WorkItemTemporaryId::parse("#aw_test1").unwrap()) + .try_into() + .unwrap(); let json = serde_json::to_string(&result).unwrap(); assert!(json.contains(r#""name":"create-work-item""#)); @@ -1004,12 +1040,10 @@ tags: #[test] fn test_patch_field_validation_rejects_custom_field_title_collision() { let mut config = CreateWorkItemConfig::default(); - config - .custom_fields - .insert( - AdoWorkItemFieldRef::parse("System.Title").unwrap(), - "overridden title".to_string(), - ); + config.custom_fields.insert( + AdoWorkItemFieldRef::parse("System.Title").unwrap(), + "overridden title".to_string(), + ); let error = validate_patch_fields(&config, description_field_for(&config), false) .unwrap_err() .to_string(); diff --git a/src/safe_outputs/link_github_sub_issue.rs b/src/safe_outputs/link_github_sub_issue.rs index 4a5e48ea3..5c0696d83 100644 --- a/src/safe_outputs/link_github_sub_issue.rs +++ b/src/safe_outputs/link_github_sub_issue.rs @@ -8,10 +8,10 @@ use serde_json::Value; use crate::safe_outputs::{ ExecutionContext, ExecutionResult, Executor, GithubClient, GithubIssueNumber, - GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, Validate, - resolve_github_issue_target, validate_github_mutation_filter_config, - validate_github_mutation_filters, validate_github_repository, - validate_github_target_capability, + GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, GithubTargetMetadata, + ResolvedGithubIssueTarget, Validate, resolve_github_issue_target, + validate_github_mutation_filter_config, validate_github_mutation_filters, + validate_github_repository, validate_github_target_capability, }; use crate::sanitize::{SanitizeContent, sanitize_config}; use crate::tool_result; @@ -143,6 +143,51 @@ impl Executor for LinkGithubSubIssueResult { if let Err(error) = validate_link_github_sub_issue_config(&config) { return Ok(ExecutionResult::failure(error.to_string())); } + + let (parent, sub_issue) = match self.resolve_targets(ctx, &config)? { + Ok(targets) => targets, + Err(result) => return Ok(result), + }; + + let client = GithubClient::new(&ctx.github_api_url, token)?; + let (parent_metadata, sub_metadata) = + match fetch_and_validate_metadata(&client, &parent, &sub_issue, &config).await? { + Ok(metadata) => metadata, + Err(result) => return Ok(result), + }; + let Some(parent_node_id) = parent_metadata.node_id.as_deref() else { + return Ok(ExecutionResult::failure(format!( + "GitHub parent issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", + parent.repository, parent.number + ))); + }; + let Some(sub_node_id) = sub_metadata.node_id.as_deref() else { + return Ok(ExecutionResult::failure(format!( + "GitHub sub-issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", + sub_issue.repository, sub_issue.number + ))); + }; + + if let Some(result) = + check_existing_parent(&client, sub_node_id, parent_node_id, &parent, &sub_issue).await? + { + return Ok(result); + } + + link_sub_issue(&client, parent_node_id, sub_node_id, &parent, &sub_issue).await + } +} + +impl LinkGithubSubIssueResult { + /// Resolves the parent and sub-issue targets and checks they refer to the + /// same repository and to two distinct issues. + fn resolve_targets( + &self, + ctx: &ExecutionContext, + config: &LinkGithubSubIssueConfig, + ) -> anyhow::Result< + Result<(ResolvedGithubIssueTarget, ResolvedGithubIssueTarget), ExecutionResult>, + > { let policy = GithubRepositoryPolicy::new(config.target_repo.as_deref(), &config.allowed_repos); let parent = match resolve_github_issue_target( @@ -152,7 +197,7 @@ impl Executor for LinkGithubSubIssueResult { ctx, )? { Ok(target) => target, - Err(result) => return Ok(result), + Err(result) => return Ok(Err(result)), }; let sub_issue = match resolve_github_issue_target( &self.sub_issue_number, @@ -161,165 +206,184 @@ impl Executor for LinkGithubSubIssueResult { ctx, )? { Ok(target) => target, - Err(result) => return Ok(result), + Err(result) => return Ok(Err(result)), }; if !parent .repository .eq_ignore_ascii_case(&sub_issue.repository) { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "parent issue repository '{}' and sub-issue repository '{}' must be the same", parent.repository, sub_issue.repository - ))); + )))); } if parent.number == sub_issue.number { - return Ok(ExecutionResult::failure( + return Ok(Err(ExecutionResult::failure( "parent_issue_number and sub_issue_number resolved to the same GitHub issue", - )); - } - - let client = GithubClient::new(&ctx.github_api_url, token)?; - let parent_metadata = match client.get_issue(&parent.repository, parent.number).await? { - Ok(metadata) => metadata, - Err(error) => return Ok(ExecutionResult::failure(error.to_string())), - }; - let sub_metadata = match client - .get_issue(&sub_issue.repository, sub_issue.number) - .await? - { - Ok(metadata) => metadata, - Err(error) => return Ok(ExecutionResult::failure(error.to_string())), - }; - for metadata in [&parent_metadata, &sub_metadata] { - if let Err(result) = - validate_github_target_capability(metadata, GithubTargetCapabilities::ISSUES_ONLY) - { - return Ok(result); - } - } - let parent_filters = GithubMutationFilters { - required_labels: &config.parent_required_labels, - required_title_prefix: config.parent_title_prefix.as_deref(), - }; - let sub_filters = GithubMutationFilters { - required_labels: &config.sub_required_labels, - required_title_prefix: config.sub_title_prefix.as_deref(), - }; - if let Err(result) = validate_github_mutation_filters(&parent_metadata, parent_filters) { - return Ok(result); - } - if let Err(result) = validate_github_mutation_filters(&sub_metadata, sub_filters) { - return Ok(result); - } - let Some(parent_node_id) = parent_metadata.node_id.as_deref() else { - return Ok(ExecutionResult::failure(format!( - "GitHub parent issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", - parent.repository, parent.number ))); - }; - let Some(sub_node_id) = sub_metadata.node_id.as_deref() else { - return Ok(ExecutionResult::failure(format!( - "GitHub sub-issue {}#{} has no GraphQL node ID; sub-issues are unsupported or unavailable", - sub_issue.repository, sub_issue.number - ))); - }; + } + Ok(Ok((parent, sub_issue))) + } +} - let preflight = match client - .graphql( - "Check GitHub sub-issue parent", - GET_SUB_ISSUE_PARENT, - serde_json::json!({ "id": sub_node_id }), - ) - .await? +/// Fetches parent and sub-issue metadata and validates capability and mutation filters. +async fn fetch_and_validate_metadata( + client: &GithubClient, + parent: &ResolvedGithubIssueTarget, + sub_issue: &ResolvedGithubIssueTarget, + config: &LinkGithubSubIssueConfig, +) -> anyhow::Result> { + let parent_metadata = match client.get_issue(&parent.repository, parent.number).await? { + Ok(metadata) => metadata, + Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), + }; + let sub_metadata = match client + .get_issue(&sub_issue.repository, sub_issue.number) + .await? + { + Ok(metadata) => metadata, + Err(error) => return Ok(Err(ExecutionResult::failure(error.to_string()))), + }; + for metadata in [&parent_metadata, &sub_metadata] { + if let Err(result) = + validate_github_target_capability(metadata, GithubTargetCapabilities::ISSUES_ONLY) { - Ok(data) => data, - Err(error) => { - return Ok(ExecutionResult::failure(format!( - "GitHub sub-issues are unsupported or unavailable: {error}" - ))); - } - }; - if let Some(existing) = match parse_existing_parent(&preflight) { - Ok(parent) => parent, - Err(message) => return Ok(ExecutionResult::failure(message)), - } { - let same_parent = existing.id == parent_node_id; - if same_parent { - info!( - "GitHub issue {}#{} is already a sub-issue of #{}", - parent.repository, sub_issue.number, parent.number - ); - return Ok(ExecutionResult::success_with_data( - format!( - "GitHub issue {}#{} is already a sub-issue of #{}", - parent.repository, sub_issue.number, parent.number - ), - serde_json::json!({ - "parent_issue_number": parent.number, - "sub_issue_number": sub_issue.number, - "target_repo": parent.repository, - "already_linked": true, - }), - )); - } - let existing_target = format!("{}#{}", existing.repository, existing.number); - return Ok(ExecutionResult::failure(format!( - "GitHub issue {}#{} is already linked to a different parent ({existing_target}); refusing to replace it", - sub_issue.repository, sub_issue.number - ))); + return Ok(Err(result)); } + } + let parent_filters = GithubMutationFilters { + required_labels: &config.parent_required_labels, + required_title_prefix: config.parent_title_prefix.as_deref(), + }; + let sub_filters = GithubMutationFilters { + required_labels: &config.sub_required_labels, + required_title_prefix: config.sub_title_prefix.as_deref(), + }; + if let Err(result) = validate_github_mutation_filters(&parent_metadata, parent_filters) { + return Ok(Err(result)); + } + if let Err(result) = validate_github_mutation_filters(&sub_metadata, sub_filters) { + return Ok(Err(result)); + } + Ok(Ok((parent_metadata, sub_metadata))) +} - debug!( - "Linking GitHub issue {}#{} as a sub-issue of #{}", - parent.repository, sub_issue.number, parent.number - ); - let mutation = match client - .graphql( - "Link GitHub sub-issue", - ADD_SUB_ISSUE, - serde_json::json!({ - "parentId": parent_node_id, - "subIssueId": sub_node_id, - }), - ) - .await? - { - Ok(data) => data, - Err(error) => { - return Ok(ExecutionResult::failure(format!( - "GitHub addSubIssue mutation is unsupported or failed: {error}" - ))); - } - }; - let mutated_parent = mutation - .pointer("/addSubIssue/issue/number") - .and_then(Value::as_u64); - let mutated_sub = mutation - .pointer("/addSubIssue/subIssue/number") - .and_then(Value::as_u64); - if mutated_parent != Some(parent.number) || mutated_sub != Some(sub_issue.number) { - return Ok(ExecutionResult::failure( - "GitHub addSubIssue response did not identify the requested parent and sub-issue", - )); +/// Checks whether the sub-issue already has a parent, returning an early +/// result (success if already linked to the same parent, failure if linked +/// to a different one) or `None` if linking should proceed. +async fn check_existing_parent( + client: &GithubClient, + sub_node_id: &str, + parent_node_id: &str, + parent: &ResolvedGithubIssueTarget, + sub_issue: &ResolvedGithubIssueTarget, +) -> anyhow::Result> { + let preflight = match client + .graphql( + "Check GitHub sub-issue parent", + GET_SUB_ISSUE_PARENT, + serde_json::json!({ "id": sub_node_id }), + ) + .await? + { + Ok(data) => data, + Err(error) => { + return Ok(Some(ExecutionResult::failure(format!( + "GitHub sub-issues are unsupported or unavailable: {error}" + )))); } + }; + let Some(existing) = (match parse_existing_parent(&preflight) { + Ok(parent) => parent, + Err(message) => return Ok(Some(ExecutionResult::failure(message))), + }) else { + return Ok(None); + }; + if existing.id == parent_node_id { info!( - "Linked GitHub issue {}#{} as a sub-issue of #{}", + "GitHub issue {}#{} is already a sub-issue of #{}", parent.repository, sub_issue.number, parent.number ); - Ok(ExecutionResult::success_with_data( + return Ok(Some(ExecutionResult::success_with_data( format!( - "Linked GitHub issue {}#{} as a sub-issue of #{}", + "GitHub issue {}#{} is already a sub-issue of #{}", parent.repository, sub_issue.number, parent.number ), serde_json::json!({ "parent_issue_number": parent.number, "sub_issue_number": sub_issue.number, "target_repo": parent.repository, - "already_linked": false, + "already_linked": true, + }), + ))); + } + let existing_target = format!("{}#{}", existing.repository, existing.number); + Ok(Some(ExecutionResult::failure(format!( + "GitHub issue {}#{} is already linked to a different parent ({existing_target}); refusing to replace it", + sub_issue.repository, sub_issue.number + )))) +} + +/// Performs the `addSubIssue` mutation and validates the response identifies +/// the requested parent and sub-issue. +async fn link_sub_issue( + client: &GithubClient, + parent_node_id: &str, + sub_node_id: &str, + parent: &ResolvedGithubIssueTarget, + sub_issue: &ResolvedGithubIssueTarget, +) -> anyhow::Result { + debug!( + "Linking GitHub issue {}#{} as a sub-issue of #{}", + parent.repository, sub_issue.number, parent.number + ); + let mutation = match client + .graphql( + "Link GitHub sub-issue", + ADD_SUB_ISSUE, + serde_json::json!({ + "parentId": parent_node_id, + "subIssueId": sub_node_id, }), - )) + ) + .await? + { + Ok(data) => data, + Err(error) => { + return Ok(ExecutionResult::failure(format!( + "GitHub addSubIssue mutation is unsupported or failed: {error}" + ))); + } + }; + let mutated_parent = mutation + .pointer("/addSubIssue/issue/number") + .and_then(Value::as_u64); + let mutated_sub = mutation + .pointer("/addSubIssue/subIssue/number") + .and_then(Value::as_u64); + if mutated_parent != Some(parent.number) || mutated_sub != Some(sub_issue.number) { + return Ok(ExecutionResult::failure( + "GitHub addSubIssue response did not identify the requested parent and sub-issue", + )); } + + info!( + "Linked GitHub issue {}#{} as a sub-issue of #{}", + parent.repository, sub_issue.number, parent.number + ); + Ok(ExecutionResult::success_with_data( + format!( + "Linked GitHub issue {}#{} as a sub-issue of #{}", + parent.repository, sub_issue.number, parent.number + ), + serde_json::json!({ + "parent_issue_number": parent.number, + "sub_issue_number": sub_issue.number, + "target_repo": parent.repository, + "already_linked": false, + }), + )) } pub(crate) fn validate_link_github_sub_issue_config( diff --git a/src/safe_outputs/missing_data.rs b/src/safe_outputs/missing_data.rs index f47d7873e..bec47c2be 100644 --- a/src/safe_outputs/missing_data.rs +++ b/src/safe_outputs/missing_data.rs @@ -57,7 +57,14 @@ impl Executor for MissingDataResult { if let Some(context) = &self.context { message.push_str(&format!(" [{context}]")); } - Ok(ExecutionResult::success(message)) + Ok(ExecutionResult::success_with_data( + message, + serde_json::json!({ + "data_type": self.data_type, + "reason": self.reason, + "context": self.context, + }), + )) } } @@ -87,4 +94,30 @@ mod tests { Some("checked GitHub and internal wiki, neither had it".to_string()) ); } + + #[tokio::test] + async fn test_execute_impl_preserves_report_details() { + let result: MissingDataResult = MissingDataParams { + data_type: "API docs".to_string(), + reason: "needed for integration".to_string(), + context: Some("checked available sources".to_string()), + } + .try_into() + .unwrap(); + + let exec = result + .execute_impl(&crate::safe_outputs::ExecutionContext::default()) + .await + .unwrap(); + + assert!(exec.success); + assert_eq!( + exec.data, + Some(serde_json::json!({ + "data_type": "API docs", + "reason": "needed for integration", + "context": "checked available sources", + })) + ); + } } diff --git a/src/safe_outputs/missing_tool.rs b/src/safe_outputs/missing_tool.rs index d05508601..4c5264e22 100644 --- a/src/safe_outputs/missing_tool.rs +++ b/src/safe_outputs/missing_tool.rs @@ -48,7 +48,13 @@ impl Executor for MissingToolResult { if let Some(context) = &self.context { message.push_str(&format!(" [{context}]")); } - Ok(ExecutionResult::success(message)) + Ok(ExecutionResult::success_with_data( + message, + serde_json::json!({ + "tool_name": self.tool_name, + "context": self.context, + }), + )) } } @@ -120,5 +126,12 @@ mod tests { exec.message, "Missing tool reported: bash [needed for script execution]" ); + assert_eq!( + exec.data, + Some(serde_json::json!({ + "tool_name": "bash", + "context": "needed for script execution", + })) + ); } } diff --git a/src/safe_outputs/set_github_issue_field.rs b/src/safe_outputs/set_github_issue_field.rs index 0595774f2..2dfa53598 100644 --- a/src/safe_outputs/set_github_issue_field.rs +++ b/src/safe_outputs/set_github_issue_field.rs @@ -9,10 +9,10 @@ use serde_json::Value; use crate::safe_outputs::{ ExecutionContext, ExecutionResult, Executor, GithubClient, GithubIssueNumber, - GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, Validate, - resolve_github_issue_target, validate_github_mutation_filter_config, - validate_github_mutation_filters, validate_github_repository, - validate_github_target_capability, + GithubMutationFilters, GithubRepositoryPolicy, GithubTargetCapabilities, + ResolvedGithubIssueTarget, Validate, resolve_github_issue_target, + validate_github_mutation_filter_config, validate_github_mutation_filters, + validate_github_repository, validate_github_target_capability, }; use crate::sanitize::{SanitizeContent, sanitize_config}; use crate::tool_result; @@ -220,6 +220,61 @@ impl Executor for SetGithubIssueFieldResult { ))); }; + let field = match self + .discover_and_select_field(&client, &target, &config) + .await? + { + Ok(field) => field, + Err(result) => return Ok(result), + }; + + let field_input = match coerce_field_value(&field, &self.value) { + Ok(input) => input, + Err(message) => return Ok(ExecutionResult::failure(message)), + }; + debug!( + "Setting GitHub issue field {} ({}) on {}#{}", + field.name, field.kind, target.repository, target.number + ); + + if let Err(result) = + apply_field_mutation(&client, issue_node_id, &field_input, &target).await? + { + return Ok(result); + } + + info!( + "Set GitHub issue field '{}' on {}#{}", + field.name, target.repository, target.number + ); + Ok(ExecutionResult::success_with_data( + format!( + "Set GitHub issue field '{}' on {}#{}", + field.name, target.repository, target.number + ), + serde_json::json!({ + "number": target.number, + "target_repo": target.repository, + "field_name": field.name, + "field_node_id": field.id, + "field_type": field.kind, + "value": self.value, + }), + )) + } +} + +impl SetGithubIssueFieldResult { + /// Discover a repository's custom issue fields, select the one requested + /// by the agent, and enforce the built-in-field and allowed-fields + /// policies. Returns `Ok(Err(result))` for any policy or API failure that + /// should be surfaced to the caller as an [`ExecutionResult::failure`]. + async fn discover_and_select_field( + &self, + client: &GithubClient, + target: &ResolvedGithubIssueTarget, + config: &SetGithubIssueFieldConfig, + ) -> anyhow::Result> { let (owner, repo) = target .repository .split_once('/') @@ -234,14 +289,14 @@ impl Executor for SetGithubIssueFieldResult { { Ok(data) => data, Err(error) => { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "GitHub issue fields are unsupported or unavailable: {error}" - ))); + )))); } }; let fields = match parse_issue_fields(&discovery) { Ok(fields) => fields, - Err(message) => return Ok(ExecutionResult::failure(message)), + Err(message) => return Ok(Err(ExecutionResult::failure(message))), }; let field = match select_issue_field( &fields, @@ -249,76 +304,61 @@ impl Executor for SetGithubIssueFieldResult { self.field_node_id.as_deref(), ) { Ok(field) => field, - Err(message) => return Ok(ExecutionResult::failure(message)), + Err(message) => return Ok(Err(ExecutionResult::failure(message))), }; if is_builtin_issue_field(&field.name) { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "GitHub field '{}' is a built-in issue field; use its dedicated safe-output tool", crate::sanitize::neutralize_pipeline_commands(&field.name) - ))); + )))); } if !github_issue_field_is_allowed(&config.allowed_fields, &field.name) { - return Ok(ExecutionResult::failure(format!( + return Ok(Err(ExecutionResult::failure(format!( "GitHub issue field '{}' is not in allowed-fields: {}", crate::sanitize::neutralize_pipeline_commands(&field.name), config.allowed_fields.join(", ") - ))); - } - - let field_input = match coerce_field_value(field, &self.value) { - Ok(input) => input, - Err(message) => return Ok(ExecutionResult::failure(message)), - }; - debug!( - "Setting GitHub issue field {} ({}) on {}#{}", - field.name, field.kind, target.repository, target.number - ); - let mutation = match client - .graphql( - "Set GitHub issue field value", - SET_ISSUE_FIELD_VALUE, - serde_json::json!({ - "issueId": issue_node_id, - "issueFields": [field_input], - }), - ) - .await? - { - Ok(data) => data, - Err(error) => { - return Ok(ExecutionResult::failure(format!( - "GitHub issue field mutation is unsupported or failed: {error}" - ))); - } - }; - let updated_number = mutation - .pointer("/setIssueFieldValue/issue/number") - .and_then(Value::as_u64); - if updated_number != Some(target.number) { - return Ok(ExecutionResult::failure( - "GitHub setIssueFieldValue response did not identify the updated issue", - )); + )))); } + Ok(Ok(field.clone())) + } +} - info!( - "Set GitHub issue field '{}' on {}#{}", - field.name, target.repository, target.number - ); - Ok(ExecutionResult::success_with_data( - format!( - "Set GitHub issue field '{}' on {}#{}", - field.name, target.repository, target.number - ), +/// Send the `setIssueFieldValue` mutation and confirm the response identifies +/// the expected issue. Returns `Ok(Err(result))` on any API or response-shape +/// failure that should be surfaced as an [`ExecutionResult::failure`]. +async fn apply_field_mutation( + client: &GithubClient, + issue_node_id: &str, + field_input: &Value, + target: &ResolvedGithubIssueTarget, +) -> anyhow::Result> { + let mutation = match client + .graphql( + "Set GitHub issue field value", + SET_ISSUE_FIELD_VALUE, serde_json::json!({ - "number": target.number, - "target_repo": target.repository, - "field_name": field.name, - "field_node_id": field.id, - "field_type": field.kind, - "value": self.value, + "issueId": issue_node_id, + "issueFields": [field_input], }), - )) + ) + .await? + { + Ok(data) => data, + Err(error) => { + return Ok(Err(ExecutionResult::failure(format!( + "GitHub issue field mutation is unsupported or failed: {error}" + )))); + } + }; + let updated_number = mutation + .pointer("/setIssueFieldValue/issue/number") + .and_then(Value::as_u64); + if updated_number != Some(target.number) { + return Ok(Err(ExecutionResult::failure( + "GitHub setIssueFieldValue response did not identify the updated issue", + ))); } + Ok(Ok(())) } fn github_issue_field_is_allowed(allowed_fields: &[String], field_name: &str) -> bool { diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index 70dd486e0..4e4962c3b 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -238,6 +238,399 @@ impl Default for UploadBuildAttachmentConfig { } } +/// Resolve the current run's build ID and reconcile it with any agent-supplied +/// `build_id`. A build attachment can only ever be added to the *current* +/// job's timeline record (see module docs), so `build_id`, when the agent +/// supplies it, must match the current run. +fn resolve_effective_build_id( + requested_build_id: Option, + ctx: &ExecutionContext, +) -> anyhow::Result> { + let current_build_id: Option = match ctx.build_id { + Some(current) => Some(i64::try_from(current).context("BUILD_BUILDID value overflows i64")?), + None => None, + }; + let outcome = match (requested_build_id, current_build_id) { + // Agent supplied a build_id that differs from the current run — not + // possible for a build attachment; fail with a clear message. + (Some(requested), Some(current)) if requested != current => { + Err(ExecutionResult::failure(format!( + "build_id {requested} does not match the current build ({current}). Build \ + attachments can only be added to the current run — omit build_id (or set it \ + to {current}) to attach to this build." + ))) + } + (Some(requested), Some(_current)) => Ok(requested), + // Agent supplied a build_id but the current build is unknown; we + // cannot prove it targets the current run, so refuse. + (Some(requested), None) => Err(ExecutionResult::failure(format!( + "build_id {requested} was specified but the current build id (BUILD_BUILDID) \ + is not set, so it cannot be confirmed to target the current run. Build \ + attachments can only be added to the current run — omit build_id." + ))), + (None, Some(current)) => Ok(current), + (None, None) => Err(ExecutionResult::failure( + "Cannot attach a build attachment: BUILD_BUILDID is not set, so the current \ + run cannot be determined." + .to_string(), + )), + }; + Ok(outcome) +} + +/// Apply the operator-configured `name-prefix` to the agent-supplied artifact +/// name, re-validate the resulting name's charset, and enforce the +/// `allowed-artifact-names` allow-list. +fn resolve_final_artifact_name( + artifact_name: &str, + config: &UploadBuildAttachmentConfig, +) -> Result { + // Validate name-prefix length before applying. A long prefix would + // be caught later by the final_name.len() > 100 check, but rejecting + // early gives operators a clearer error message. + if let Some(prefix) = &config.name_prefix + && prefix.len() > 50 + { + return Err(ExecutionResult::failure(format!( + "name-prefix '{}...' is too long ({} chars, max 50)", + prefix.chars().take(20).collect::(), + prefix.len() + ))); + } + + // Apply name-prefix and re-validate the resulting name's charset (the + // prefix itself is operator-controlled and sanitized at config load, + // but we still defensively check the joined string). + let final_name = match &config.name_prefix { + Some(prefix) => format!("{}{}", prefix, artifact_name), + None => artifact_name.to_string(), + }; + if final_name.starts_with('.') || final_name.len() > 100 || !is_valid_artifact_name(&final_name) + { + return Err(ExecutionResult::failure(format!( + "Resolved artifact name '{}' is not a valid Azure DevOps artifact name", + final_name + ))); + } + debug!("Final artifact name (after prefix): {}", final_name); + + // Check artifact-name allow-list (if configured). + if !config.allowed_artifact_names.is_empty() { + let allowed = config + .allowed_artifact_names + .iter() + .any(|pattern| super::name_matches_pattern(&final_name, pattern)); + if !allowed { + return Err(ExecutionResult::failure(format!( + "Artifact name '{}' is not in the allowed list", + final_name + ))); + } + } + + Ok(final_name) +} + +/// Validate the staged file's extension against the operator-configured +/// `allowed-extensions` list (if any). Uses `Path::extension()` for a precise +/// match rather than suffix matching on the full path — this prevents "log" +/// from matching filenames like "catalog" when the operator omits the leading +/// dot. +fn validate_file_extension( + file_path: &str, + config: &UploadBuildAttachmentConfig, +) -> Result<(), ExecutionResult> { + if config.allowed_extensions.is_empty() { + return Ok(()); + } + let file_ext = std::path::Path::new(file_path) + .extension() + .and_then(|e| e.to_str()) + .unwrap_or(""); + let has_valid_ext = config + .allowed_extensions + .iter() + .any(|ext| ext.trim_start_matches('.').eq_ignore_ascii_case(file_ext)); + if !has_valid_ext { + return Err(ExecutionResult::failure(format!( + "File '{}' has an extension not in the allowed list: {:?}", + file_path, config.allowed_extensions + ))); + } + Ok(()) +} + +/// Resolve the attachment type: operator config wins, otherwise the default. +/// Re-validate the charset defensively even though `SanitizeConfig` strips +/// control characters, because the type is interpolated into a URL path +/// segment. +fn resolve_attachment_type(config: &UploadBuildAttachmentConfig) -> Result<&str, ExecutionResult> { + let attachment_type = config + .attachment_type + .as_deref() + .unwrap_or(DEFAULT_ATTACHMENT_TYPE); + if attachment_type.is_empty() + || attachment_type.starts_with('.') + || attachment_type.len() > 100 + || !is_valid_artifact_name(attachment_type) + { + return Err(ExecutionResult::failure(format!( + "attachment-type '{}' is not a valid value (must be non-empty, ≤100 chars, no leading '.', alphanumeric/'-'/'_'/'.')", + attachment_type + ))); + } + debug!("Attachment type: {}", attachment_type); + Ok(attachment_type) +} + +/// Resolve the staged file inside the safe-outputs working directory and +/// validate it, returning its canonical path and size. +/// +/// Stage 1 (MCP) copied the agent's file there under `staged_file`; the +/// sandbox workspace where the original lived is no longer accessible. +/// Canonicalize and verify it stays inside `working_directory` so a malicious +/// staged_file value can't escape (defense in depth — MCP generates the name +/// itself). Also enforces the recorded file-size integrity check and the +/// operator's `max-file-size` limit. +fn resolve_staged_file( + staged_file: &str, + recorded_file_size: u64, + ctx: &ExecutionContext, + config: &UploadBuildAttachmentConfig, +) -> anyhow::Result> { + let staged_path = ctx.working_directory.join(staged_file); + debug!("Staged file path: {}", staged_path.display()); + + let canonical = staged_path.canonicalize().context( + "Failed to canonicalize staged file path — file may be missing or contains broken symlinks", + )?; + let canonical_base = ctx + .working_directory + .canonicalize() + .context("Failed to canonicalize working directory")?; + if !canonical.starts_with(&canonical_base) { + return Ok(Err(ExecutionResult::failure(format!( + "Staged file '{}' resolves outside the safe-outputs directory", + staged_file + )))); + } + + // Reject directories defensively — the staged entry must always be a + // single file (Stage 1 only copies single files). + let metadata = std::fs::metadata(&canonical).context("Failed to read file metadata")?; + if metadata.is_dir() { + return Ok(Err(ExecutionResult::failure(format!( + "Staged path '{}' is a directory; upload-build-attachment only supports single files", + staged_file + )))); + } + let file_size = metadata.len(); + debug!("File size: {} bytes", file_size); + + // Integrity check: compare the live file size against the size + // recorded in Stage 1. A mismatch means the staged file was modified + // between stages — fail hard rather than uploading tampered content. + if file_size != recorded_file_size { + return Ok(Err(ExecutionResult::failure(format!( + "Staged file size ({} bytes) differs from size recorded at Stage 1 ({} bytes) — \ + the file may have been modified between stages", + file_size, recorded_file_size + )))); + } + + if file_size > config.max_file_size { + return Ok(Err(ExecutionResult::failure(format!( + "File size ({} bytes) exceeds maximum allowed size ({} bytes)", + file_size, config.max_file_size + )))); + } + + Ok(Ok((canonical, file_size))) +} + +/// Read the staged file's bytes and verify its SHA-256 hash matches the one +/// recorded at Stage 1 — catches same-size replacements between stages that +/// the size check alone would miss. +async fn read_and_verify_staged_bytes( + canonical: &std::path::Path, + expected_sha256: &str, +) -> anyhow::Result, ExecutionResult>> { + let file_bytes = tokio::fs::read(canonical) + .await + .context("Failed to read file contents")?; + + let live_hash = crate::hash::sha256_hex(&file_bytes); + if live_hash != expected_sha256 { + return Ok(Err(ExecutionResult::failure(format!( + "Staged file SHA-256 mismatch: expected {} (recorded at Stage 1), got {} — \ + the file may have been tampered with between stages", + expected_sha256, live_hash + )))); + } + Ok(Ok(file_bytes)) +} + +/// ADO API coordinates required to write a timeline attachment for the +/// current job's record. +struct TimelineAttachmentCoords<'a> { + org_url: &'a str, + project: &'a str, + token: &'a str, + project_id: &'a str, + plan_id: &'a str, + timeline_id: &'a str, + record_id: &'a str, +} + +/// Resolve the ADO API context (collection URL, token) and the current job's +/// timeline coordinates. A build attachment is a DistributedTask **timeline +/// attachment** on the running job's record (the same object +/// `##vso[task.addattachment]` creates), so we need the plan / timeline / +/// record IDs of the current run — these come from the auto-injected +/// SYSTEM_* predefined variables and only exist for the current job. +fn resolve_timeline_coords(ctx: &ExecutionContext) -> anyhow::Result> { + let org_url = ctx + .ado_org_url + .as_deref() + .context("AZURE_DEVOPS_ORG_URL not set")?; + let project = ctx + .ado_project + .as_deref() + .context("SYSTEM_TEAMPROJECT not set")?; + let token = ctx + .access_token + .as_deref() + .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; + // The DistributedTask hub route's `{scopeIdentifier}` is the **project + // GUID** (SYSTEM_TEAMPROJECTID), not the project name — the name routes + // but is rejected with HTTP 400. + let project_id = ctx.ado_project_id.as_deref().context( + "SYSTEM_TEAMPROJECTID is not set — required as the scope identifier for the build \ + attachment (timeline attachment) API", + )?; + let plan_id = ctx.plan_id.as_deref().context( + "SYSTEM_PLANID is not set — required to attach to the current build (build attachments \ + are written to the current job's timeline record)", + )?; + let timeline_id = ctx.timeline_id.as_deref().context( + "SYSTEM_TIMELINEID is not set — required to attach to the current build (build \ + attachments are written to the current job's timeline record)", + )?; + let record_id = ctx.job_id.as_deref().context( + "SYSTEM_JOBID is not set — required to attach to the current build (build \ + attachments are written to the current job's timeline record)", + )?; + debug!( + "ADO org: {}, project: {} ({})", + org_url, project, project_id + ); + Ok(TimelineAttachmentCoords { + org_url, + project, + token, + project_id, + plan_id, + timeline_id, + record_id, + }) +} + +/// PUT the file bytes to the timeline attachment endpoint and translate the +/// HTTP response into an [`ExecutionResult`]. +async fn upload_timeline_attachment( + coords: &TimelineAttachmentCoords<'_>, + attachment_type: &str, + final_name: &str, + file_path: &str, + file_size: u64, + effective_build_id: i64, + file_bytes: Vec, +) -> anyhow::Result { + // Build the DistributedTask timeline-attachment URL. This is the write + // side of a build attachment — the object is read back via the Build ▸ + // Attachments Get/List API by `{type}`/`{name}`. The `build` hub covers + // build/YAML pipelines. The route's `{scopeIdentifier}` is the project + // **GUID**; released api-version is 7.1. + // PUT {org}/{projectId}/_apis/distributedtask/hubs/build/plans/{planId} + // /timelines/{timelineId}/records/{recordId} + // /attachments/{type}/{name}?api-version=7.1 + let url = format!( + "{}/{}/_apis/distributedtask/hubs/build/plans/{}/timelines/{}/records/{}/attachments/{}/{}?api-version=7.1", + coords.org_url.trim_end_matches('/'), + utf8_percent_encode(coords.project_id, PATH_SEGMENT), + utf8_percent_encode(coords.plan_id, PATH_SEGMENT), + utf8_percent_encode(coords.timeline_id, PATH_SEGMENT), + utf8_percent_encode(coords.record_id, PATH_SEGMENT), + utf8_percent_encode(attachment_type, PATH_SEGMENT), + utf8_percent_encode(final_name, PATH_SEGMENT), + ); + debug!("Attachment URL: {}", url); + + let client = reqwest::Client::new(); + info!( + "Uploading {} bytes to build #{} as attachment '{}/{}'", + file_size, effective_build_id, attachment_type, final_name + ); + let response = client + .put(&url) + .header("Content-Type", "application/octet-stream") + .basic_auth("", Some(coords.token)) + .body(file_bytes) + .send() + .await + .context("Failed to send attachment upload request to Azure DevOps")?; + + if response.status().is_success() { + let resp_body: serde_json::Value = response.json().await.unwrap_or_else(|e| { + warn!( + "Build attachment uploaded for build #{} but the response JSON could not be parsed: {} — proceeding without attachment URL", + effective_build_id, e + ); + serde_json::Value::Null + }); + // The timeline-attachment response carries the attachment URL under + // `_links.self.href` (there is no top-level `url` field); fall back + // to a top-level `url` defensively for forward compatibility. + let attachment_url = resp_body + .get("_links") + .and_then(|l| l.get("self")) + .and_then(|s| s.get("href")) + .and_then(|v| v.as_str()) + .or_else(|| resp_body.get("url").and_then(|v| v.as_str())) + .map(|s| s.to_string()); + info!( + "Attached '{}' to build #{} as '{}'", + file_path, effective_build_id, final_name + ); + + Ok(ExecutionResult::success_with_data( + format!( + "Attached '{}' to build #{} as artifact '{}'", + file_path, effective_build_id, final_name + ), + serde_json::json!({ + "build_id": effective_build_id, + "artifact_name": final_name, + "attachment_type": attachment_type, + "file_path": file_path, + "size_bytes": file_size, + "attachment_url": attachment_url, + "project": coords.project, + }), + )) + } else { + let status = response.status(); + let error_body = response + .text() + .await + .unwrap_or_else(|_| "Unknown error".to_string()); + Ok(ExecutionResult::failure(format!( + "Failed to attach artifact to build #{} (HTTP {}): {}", + effective_build_id, status, error_body + ))) + } +} + #[async_trait::async_trait] impl Executor for UploadBuildAttachmentResult { fn dry_run_summary(&self) -> String { @@ -254,43 +647,9 @@ impl Executor for UploadBuildAttachmentResult { } async fn execute_impl(&self, ctx: &ExecutionContext) -> anyhow::Result { - // Resolve the current run's build ID. A build attachment can only ever - // be added to the *current* job's timeline record (see module docs), so - // `build_id`, when the agent supplies it, must match the current run. - let current_build_id: Option = match ctx.build_id { - Some(current) => { - Some(i64::try_from(current).context("BUILD_BUILDID value overflows i64")?) - } - None => None, - }; - let effective_build_id: i64 = match (self.build_id, current_build_id) { - // Agent supplied a build_id that differs from the current run — not - // possible for a build attachment; fail with a clear message. - (Some(requested), Some(current)) if requested != current => { - return Ok(ExecutionResult::failure(format!( - "build_id {requested} does not match the current build ({current}). Build \ - attachments can only be added to the current run — omit build_id (or set it \ - to {current}) to attach to this build." - ))); - } - (Some(requested), Some(_current)) => requested, - // Agent supplied a build_id but the current build is unknown; we - // cannot prove it targets the current run, so refuse. - (Some(requested), None) => { - return Ok(ExecutionResult::failure(format!( - "build_id {requested} was specified but the current build id (BUILD_BUILDID) \ - is not set, so it cannot be confirmed to target the current run. Build \ - attachments can only be added to the current run — omit build_id." - ))); - } - (None, Some(current)) => current, - (None, None) => { - return Ok(ExecutionResult::failure( - "Cannot attach a build attachment: BUILD_BUILDID is not set, so the current \ - run cannot be determined." - .to_string(), - )); - } + let effective_build_id = match resolve_effective_build_id(self.build_id, ctx)? { + Ok(id) => id, + Err(result) => return Ok(result), }; info!( @@ -310,144 +669,25 @@ impl Executor for UploadBuildAttachmentResult { config.allowed_artifact_names ); - // Validate name-prefix length before applying. A long prefix would - // be caught later by the final_name.len() > 100 check, but rejecting - // early gives operators a clearer error message. - if let Some(prefix) = &config.name_prefix - && prefix.len() > 50 - { - return Ok(ExecutionResult::failure(format!( - "name-prefix '{}...' is too long ({} chars, max 50)", - prefix.chars().take(20).collect::(), - prefix.len() - ))); - } - - // Apply name-prefix and re-validate the resulting name's charset (the - // prefix itself is operator-controlled and sanitized at config load, - // but we still defensively check the joined string). - let final_name = match &config.name_prefix { - Some(prefix) => format!("{}{}", prefix, self.artifact_name), - None => self.artifact_name.clone(), + let final_name = match resolve_final_artifact_name(&self.artifact_name, &config) { + Ok(name) => name, + Err(result) => return Ok(result), }; - if final_name.starts_with('.') - || final_name.len() > 100 - || !is_valid_artifact_name(&final_name) - { - return Ok(ExecutionResult::failure(format!( - "Resolved artifact name '{}' is not a valid Azure DevOps artifact name", - final_name - ))); - } - debug!("Final artifact name (after prefix): {}", final_name); - - // Check artifact-name allow-list (if configured). - if !config.allowed_artifact_names.is_empty() { - let allowed = config - .allowed_artifact_names - .iter() - .any(|pattern| super::name_matches_pattern(&final_name, pattern)); - if !allowed { - return Ok(ExecutionResult::failure(format!( - "Artifact name '{}' is not in the allowed list", - final_name - ))); - } - } - // Validate file extension against allowed-extensions (if configured). - // Uses Path::extension() for a precise match rather than suffix - // matching on the full path — this prevents "log" from matching - // filenames like "catalog" when the operator omits the leading dot. - if !config.allowed_extensions.is_empty() { - let file_ext = std::path::Path::new(&self.file_path) - .extension() - .and_then(|e| e.to_str()) - .unwrap_or(""); - let has_valid_ext = config - .allowed_extensions - .iter() - .any(|ext| ext.trim_start_matches('.').eq_ignore_ascii_case(file_ext)); - if !has_valid_ext { - return Ok(ExecutionResult::failure(format!( - "File '{}' has an extension not in the allowed list: {:?}", - self.file_path, config.allowed_extensions - ))); - } + if let Err(result) = validate_file_extension(&self.file_path, &config) { + return Ok(result); } - // Resolve the attachment type. Operator config wins; otherwise use the - // default. Re-validate the charset defensively even though - // `SanitizeConfig` strips control characters, because the type is - // interpolated into a URL path segment. - let attachment_type = config - .attachment_type - .as_deref() - .unwrap_or(DEFAULT_ATTACHMENT_TYPE); - if attachment_type.is_empty() - || attachment_type.starts_with('.') - || attachment_type.len() > 100 - || !is_valid_artifact_name(attachment_type) - { - return Ok(ExecutionResult::failure(format!( - "attachment-type '{}' is not a valid value (must be non-empty, ≤100 chars, no leading '.', alphanumeric/'-'/'_'/'.')", - attachment_type - ))); - } - debug!("Attachment type: {}", attachment_type); - - // Resolve the staged file inside the safe-outputs working directory. - // Stage 1 (MCP) copied the agent's file there under `self.staged_file`; - // the sandbox workspace where the original lived is no longer - // accessible. Canonicalize and verify it stays inside - // `working_directory` so a malicious staged_file value can't escape - // (defense in depth — MCP generates the name itself). - let staged_path = ctx.working_directory.join(&self.staged_file); - debug!("Staged file path: {}", staged_path.display()); - - let canonical = staged_path.canonicalize().context( - "Failed to canonicalize staged file path — file may be missing or contains broken symlinks", - )?; - let canonical_base = ctx - .working_directory - .canonicalize() - .context("Failed to canonicalize working directory")?; - if !canonical.starts_with(&canonical_base) { - return Ok(ExecutionResult::failure(format!( - "Staged file '{}' resolves outside the safe-outputs directory", - self.staged_file - ))); - } - - // Reject directories defensively — the staged entry must always be a - // single file (Stage 1 only copies single files). - let metadata = std::fs::metadata(&canonical).context("Failed to read file metadata")?; - if metadata.is_dir() { - return Ok(ExecutionResult::failure(format!( - "Staged path '{}' is a directory; upload-build-attachment only supports single files", - self.staged_file - ))); - } - let file_size = metadata.len(); - debug!("File size: {} bytes", file_size); - - // Integrity check: compare the live file size against the size - // recorded in Stage 1. A mismatch means the staged file was modified - // between stages — fail hard rather than uploading tampered content. - if file_size != self.file_size { - return Ok(ExecutionResult::failure(format!( - "Staged file size ({} bytes) differs from size recorded at Stage 1 ({} bytes) — \ - the file may have been modified between stages", - file_size, self.file_size - ))); - } + let attachment_type = match resolve_attachment_type(&config) { + Ok(t) => t, + Err(result) => return Ok(result), + }; - if file_size > config.max_file_size { - return Ok(ExecutionResult::failure(format!( - "File size ({} bytes) exceeds maximum allowed size ({} bytes)", - file_size, config.max_file_size - ))); - } + let (canonical, file_size) = + match resolve_staged_file(&self.staged_file, self.file_size, ctx, &config)? { + Ok(pair) => pair, + Err(result) => return Ok(result), + }; if ctx.dry_run { return Ok(ExecutionResult::success(format!( @@ -459,147 +699,24 @@ impl Executor for UploadBuildAttachmentResult { // Read the file bytes for upload (after the dry-run guard to avoid // reading up to 50 MB into memory only to discard it). Uses async I/O // to avoid blocking the tokio runtime for large files. - let file_bytes = tokio::fs::read(&canonical) - .await - .context("Failed to read file contents")?; - - // SHA-256 integrity check: verify the staged file hasn't been swapped - // between stages. This catches same-size replacements that the size - // check alone would miss. - let live_hash = crate::hash::sha256_hex(&file_bytes); - if live_hash != self.staged_sha256 { - return Ok(ExecutionResult::failure(format!( - "Staged file SHA-256 mismatch: expected {} (recorded at Stage 1), got {} — \ - the file may have been tampered with between stages", - self.staged_sha256, live_hash - ))); - } - - // Resolve the ADO API context (collection URL, token) and the current - // job's timeline coordinates. A build attachment is a DistributedTask - // **timeline attachment** on the running job's record (the same object - // `##vso[task.addattachment]` creates), so we need the plan / timeline / - // record IDs of the current run — these come from the auto-injected - // SYSTEM_* predefined variables and only exist for the current job. - let org_url = ctx - .ado_org_url - .as_ref() - .context("AZURE_DEVOPS_ORG_URL not set")?; - let project = ctx - .ado_project - .as_ref() - .context("SYSTEM_TEAMPROJECT not set")?; - let token = ctx - .access_token - .as_ref() - .context("No access token available (SYSTEM_ACCESSTOKEN or AZURE_DEVOPS_EXT_PAT)")?; - // The DistributedTask hub route's `{scopeIdentifier}` is the **project - // GUID** (SYSTEM_TEAMPROJECTID), not the project name — the name routes - // but is rejected with HTTP 400. - let project_id = ctx.ado_project_id.as_ref().context( - "SYSTEM_TEAMPROJECTID is not set — required as the scope identifier for the build \ - attachment (timeline attachment) API", - )?; - let plan_id = ctx.plan_id.as_ref().context( - "SYSTEM_PLANID is not set — required to attach to the current build (build attachments \ - are written to the current job's timeline record)", - )?; - let timeline_id = ctx.timeline_id.as_ref().context( - "SYSTEM_TIMELINEID is not set — required to attach to the current build (build \ - attachments are written to the current job's timeline record)", - )?; - let record_id = ctx.job_id.as_ref().context( - "SYSTEM_JOBID is not set — required to attach to the current build (build attachments \ - are written to the current job's timeline record)", - )?; - debug!( - "ADO org: {}, project: {} ({})", - org_url, project, project_id - ); + let file_bytes = match read_and_verify_staged_bytes(&canonical, &self.staged_sha256).await? + { + Ok(bytes) => bytes, + Err(result) => return Ok(result), + }; - // Build the DistributedTask timeline-attachment URL. This is the write - // side of a build attachment — the object is read back via the Build ▸ - // Attachments Get/List API by `{type}`/`{name}`. The `build` hub covers - // build/YAML pipelines. The route's `{scopeIdentifier}` is the project - // **GUID**; released api-version is 7.1. - // PUT {org}/{projectId}/_apis/distributedtask/hubs/build/plans/{planId} - // /timelines/{timelineId}/records/{recordId} - // /attachments/{type}/{name}?api-version=7.1 - let url = format!( - "{}/{}/_apis/distributedtask/hubs/build/plans/{}/timelines/{}/records/{}/attachments/{}/{}?api-version=7.1", - org_url.trim_end_matches('/'), - utf8_percent_encode(project_id, PATH_SEGMENT), - utf8_percent_encode(plan_id, PATH_SEGMENT), - utf8_percent_encode(timeline_id, PATH_SEGMENT), - utf8_percent_encode(record_id, PATH_SEGMENT), - utf8_percent_encode(attachment_type, PATH_SEGMENT), - utf8_percent_encode(&final_name, PATH_SEGMENT), - ); - debug!("Attachment URL: {}", url); + let coords = resolve_timeline_coords(ctx)?; - let client = reqwest::Client::new(); - info!( - "Uploading {} bytes to build #{} as attachment '{}/{}'", - file_size, effective_build_id, attachment_type, final_name - ); - let response = client - .put(&url) - .header("Content-Type", "application/octet-stream") - .basic_auth("", Some(token)) - .body(file_bytes) - .send() - .await - .context("Failed to send attachment upload request to Azure DevOps")?; - - if response.status().is_success() { - let resp_body: serde_json::Value = response.json().await.unwrap_or_else(|e| { - warn!( - "Build attachment uploaded for build #{} but the response JSON could not be parsed: {} — proceeding without attachment URL", - effective_build_id, e - ); - serde_json::Value::Null - }); - // The timeline-attachment response carries the attachment URL under - // `_links.self.href` (there is no top-level `url` field); fall back - // to a top-level `url` defensively for forward compatibility. - let attachment_url = resp_body - .get("_links") - .and_then(|l| l.get("self")) - .and_then(|s| s.get("href")) - .and_then(|v| v.as_str()) - .or_else(|| resp_body.get("url").and_then(|v| v.as_str())) - .map(|s| s.to_string()); - info!( - "Attached '{}' to build #{} as '{}'", - self.file_path, effective_build_id, final_name - ); - - Ok(ExecutionResult::success_with_data( - format!( - "Attached '{}' to build #{} as artifact '{}'", - self.file_path, effective_build_id, final_name - ), - serde_json::json!({ - "build_id": effective_build_id, - "artifact_name": final_name, - "attachment_type": attachment_type, - "file_path": self.file_path, - "size_bytes": file_size, - "attachment_url": attachment_url, - "project": project, - }), - )) - } else { - let status = response.status(); - let error_body = response - .text() - .await - .unwrap_or_else(|_| "Unknown error".to_string()); - Ok(ExecutionResult::failure(format!( - "Failed to attach artifact to build #{} (HTTP {}): {}", - effective_build_id, status, error_body - ))) - } + upload_timeline_attachment( + &coords, + attachment_type, + &final_name, + &self.file_path, + file_size, + effective_build_id, + file_bytes, + ) + .await } } diff --git a/tests/audit_it.rs b/tests/audit_it.rs index e6412cef8..672911a22 100644 --- a/tests/audit_it.rs +++ b/tests/audit_it.rs @@ -892,6 +892,86 @@ async fn audit_pipeline_artifact_layouts_are_equivalent_end_to_end() { let trace: Value = serde_json::from_slice(&trace.stdout).expect("trace JSON"); assert_eq!(trace["build_id"], 630125); + let step_trace_cache = TempDir::new().expect("create step trace cache"); + let step_trace = Command::new(binary()) + .current_dir(workspace.path()) + .env("CI", "1") + .env("TMPDIR", step_trace_cache.path()) + .env("ADO_AW_TEST_ORG_URL", flat_server.uri()) + .args([ + "trace", + "630125", + "--step", + "threatAnalysis", + "--json", + "--org", + "test-org", + "--project", + "test-project", + "--pat", + "test-pat", + ]) + .output() + .await + .expect("run trace --step"); + assert!( + step_trace.status.success(), + "trace --step should succeed: stdout={} stderr={}", + String::from_utf8_lossy(&step_trace.stdout), + String::from_utf8_lossy(&step_trace.stderr) + ); + assert!( + !String::from_utf8_lossy(&step_trace.stderr) + .contains("requested step was not found in the local IR graph"), + "trace --step for a step present in the local IR graph should not warn: stderr={}", + String::from_utf8_lossy(&step_trace.stderr) + ); + let step_trace: Value = serde_json::from_slice(&step_trace.stdout).expect("trace --step JSON"); + assert_eq!(step_trace["build_id"], 630125); + assert_eq!(step_trace["step"]["step"], "threatAnalysis"); + assert_eq!(step_trace["step"]["location"]["job"], "Detection"); + + let missing_step_cache = TempDir::new().expect("create missing-step trace cache"); + let missing_step_trace = Command::new(binary()) + .current_dir(workspace.path()) + .env("CI", "1") + .env("TMPDIR", missing_step_cache.path()) + .env("ADO_AW_TEST_ORG_URL", flat_server.uri()) + .args([ + "trace", + "630125", + "--step", + "does-not-exist", + "--json", + "--org", + "test-org", + "--project", + "test-project", + "--pat", + "test-pat", + ]) + .output() + .await + .expect("run trace --step for a missing step"); + assert!( + missing_step_trace.status.success(), + "trace --step for an unknown step id should still exit 0: stdout={} stderr={}", + String::from_utf8_lossy(&missing_step_trace.stdout), + String::from_utf8_lossy(&missing_step_trace.stderr) + ); + assert!( + String::from_utf8_lossy(&missing_step_trace.stderr) + .contains("requested step was not found in the local IR graph"), + "trace --step for an unknown step id should warn on stderr: stderr={}", + String::from_utf8_lossy(&missing_step_trace.stderr) + ); + let missing_step_trace: Value = + serde_json::from_slice(&missing_step_trace.stdout).expect("trace --step (missing) JSON"); + assert!( + missing_step_trace["step"].is_null(), + "trace --step for an unknown step id should omit the step section: {missing_step_trace}" + ); + let mcp_cache = TempDir::new().expect("create MCP cache"); let responses = run_mcp_author(workspace.path(), mcp_cache.path(), &flat_server).await; let audit_build = responses diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 46383ee25..4da1c35b8 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -2453,7 +2453,10 @@ fn test_mcpg_container_azure_auth_emits_refresher_and_rotating_token_mount() { .collect(); assert_eq!(identity_keys.len(), 2); for name in ["Agent", "Detection"] { - let job = jobs.iter().find(|job| job["job"].as_str() == Some(name)).unwrap(); + let job = jobs + .iter() + .find(|job| job["job"].as_str() == Some(name)) + .unwrap(); let run = job["steps"] .as_sequence() .unwrap() @@ -11038,3 +11041,148 @@ fn test_template_targets_emit_no_top_level_trigger() { ); } } + +/// A `mounts:` entry that maps the host Docker socket into a container MCP +/// must surface `validate_mount_source`'s container-escape warning on +/// stderr during `compile`. `validate_mount_source` itself is unit-tested in +/// `src/compile/common.rs`, but nothing previously exercised the full +/// front-matter → compile → stderr path, so a regression that dropped the +/// `eprintln!` call in `validate_stdio_mcp` (or stopped iterating +/// `opts.mounts`) would go unnoticed. +#[test] +fn test_compile_warns_on_docker_socket_mount() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-mcp-docker-sock-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let input = "---\nname: \"Docker Socket Mount Test\"\ndescription: \"Tests docker.sock mount warning\"\nmcp-servers:\n my-tool:\n container: \"ghcr.io/example/my-tool:latest\"\n mounts:\n - \"/var/run/docker.sock:/var/run/docker.sock:rw\"\n---\n\n## Test\n"; + + let input_path = temp_dir.join("docker-sock-mcp.md"); + let output_path = temp_dir.join("docker-sock-mcp.yml"); + fs::write(&input_path, input).unwrap(); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + input_path.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed (mount warnings are non-fatal): {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("exposes the Docker socket"), + "expected a Docker-socket container-escape warning on stderr, got:\n{stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// A Docker arg smuggling `--privileged` into a container MCP must surface +/// `validate_docker_args`'s elevated-privileges warning on stderr during +/// `compile`. This exercises the full front-matter → `validate_stdio_mcp` → +/// stderr path for `args:` (as opposed to `mounts:`), guarding against a +/// regression that stopped iterating `opts.args` or dropped the warning +/// `eprintln!`. +#[test] +fn test_compile_warns_on_privileged_docker_arg() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-mcp-privileged-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let input = "---\nname: \"Privileged Docker Arg Test\"\ndescription: \"Tests --privileged arg warning\"\nmcp-servers:\n my-tool:\n container: \"ghcr.io/example/my-tool:latest\"\n args: [\"--privileged\"]\n---\n\n## Test\n"; + + let input_path = temp_dir.join("privileged-mcp.md"); + let output_path = temp_dir.join("privileged-mcp.yml"); + fs::write(&input_path, input).unwrap(); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + input_path.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed (docker-arg warnings are non-fatal): {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("grants elevated privileges"), + "expected an elevated-privileges warning on stderr, got:\n{stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} + +/// A `-v`/`--volume` Docker arg smuggling a sensitive host mount (bypassing +/// the dedicated `mounts:` field) must surface **both** +/// `validate_docker_args`'s bypass warning and the delegated +/// `validate_mount_source` sensitive-path warning on stderr during +/// `compile`. This exercises the args→mounts delegation path in +/// `validate_docker_args` (`src/validate.rs`), which is unit-tested with a +/// safe `/data` mount in `src/compile/common.rs` but never against a +/// genuinely sensitive source path end-to-end through the compiler. +#[test] +fn test_compile_warns_on_volume_arg_smuggling_sensitive_mount() { + let temp_dir = std::env::temp_dir().join(format!( + "agentic-pipeline-mcp-volume-arg-{}", + std::process::id() + )); + fs::create_dir_all(&temp_dir).expect("Failed to create temp directory"); + + let input = "---\nname: \"Volume Arg Smuggling Test\"\ndescription: \"Tests -v arg bypassing mounts validation\"\nmcp-servers:\n my-tool:\n container: \"ghcr.io/example/my-tool:latest\"\n args: [\"-v\", \"/etc:/host-etc:ro\"]\n---\n\n## Test\n"; + + let input_path = temp_dir.join("volume-arg-mcp.md"); + let output_path = temp_dir.join("volume-arg-mcp.yml"); + fs::write(&input_path, input).unwrap(); + + let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); + let output = std::process::Command::new(&binary_path) + .args([ + "compile", + input_path.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]) + .output() + .expect("Failed to run compiler"); + + assert!( + output.status.success(), + "Compiler should succeed (docker-arg warnings are non-fatal): {}", + String::from_utf8_lossy(&output.stderr) + ); + + let stderr = String::from_utf8_lossy(&output.stderr); + assert!( + stderr.contains("bypasses mounts validation"), + "expected a mounts-bypass warning on stderr, got:\n{stderr}" + ); + assert!( + stderr.contains("sensitive host path"), + "expected the delegated sensitive-path warning on stderr, got:\n{stderr}" + ); + + let _ = fs::remove_dir_all(&temp_dir); +} diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index bd1f52f4e..a63505849 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -48,6 +48,10 @@ All deterministically-assertable ADO-write safe outputs plus the flagship - **Signals:** `noop`, `missing-tool`, `missing-data`, `report-incomplete` (no ADO write path; assert that the executor emits the expected status) +- **Conclusion work-item filing:** `conclusion-noop`, + `conclusion-missing-tool`, `conclusion-missing-data` and + `conclusion-report-as-work-item-false` — see [Conclusion + scenarios](#conclusion-scenarios) below - **Work items:** `create-work-item`, `assign-work-item`, `update-work-item`, `comment-on-work-item`, `link-work-items`, `upload-workitem-attachment`, plus two rendering-fidelity scenarios (see [Rendering @@ -133,6 +137,36 @@ definition/queue-time variable first, then falls back to > now-deleted per-tool agentic smoke pipelines. Adding them here closes > the coverage gap while keeping the test deterministic. +## Conclusion scenarios + +The signal safe-outputs have no ADO write path of their own: `ado-aw execute` +only records them in `safe-outputs-executed.ndjson`. Their user-visible effect +is produced one job later by the **Conclusion job**, which reads that manifest +and files (or appends to) an Azure DevOps work item per signal — see +[`docs/conclusion.md`](../../docs/conclusion.md). + +These scenarios extend the harness past Stage 3: after `ado-aw execute` +succeeds, the runner's `postExecute` phase runs the **real compiled +`conclusion.js`** against the manifest that run just wrote, with the same flat +`AW_*` env contract the compiler emits. The work item is then asserted (and +deleted) through the ADO REST API. + +| Scenario id | Signal | What it proves | +| --- | --- | --- | +| `conclusion-noop` | `noop` | a work item is created with the configured title, type and tags, and its description carries the rendered noop report plus the conclusion stats block | +| `conclusion-missing-tool` | `missing-tool` | same for `missing-tool`, including the reported tool name; a **second** conclusion run over the same manifest appends one comment instead of filing a duplicate (title deduplication) | +| `conclusion-missing-data` | `missing-data` | same for `missing-data`, including the reported data type and reason | +| `conclusion-report-as-work-item-false` | `noop` | the per-tool `report-as-work-item: false` opt-out files nothing | + +Each scenario uses a title unique to the build +(`[ado-aw-e2e conclusion] ado-aw-det--`) so concurrent runs +never dedup into each other's work item, and deletes it in `cleanup`. + +The bundle is a build artifact, not a checked-in file: the scenarios read its +path from `EXECUTOR_E2E_CONCLUSION_BUNDLE` and **skip** when that is unset or +points at a missing file. The pipeline builds it with `npm run +build:conclusion` alongside the harness. + ## GitHub issue scenarios `create-github-issue` and `set-github-issue-type` had **zero runtime @@ -273,6 +307,8 @@ Some scenarios need optional infrastructure and **skip** (rather than fail) when it is not available: - `queue-build` — needs a target pipeline id in `E2E_QUEUE_PIPELINE_ID`. +- The four `conclusion-*` scenarios — need a compiled `conclusion.js` in + `EXECUTOR_E2E_CONCLUSION_BUNDLE`. - `create-wiki-page` / `update-wiki-page` — need a wiki in the project. The harness auto-discovers the first wiki; set `E2E_WIKI_NAME` to force one. When no wiki exists, both skip. @@ -303,13 +339,15 @@ You need a write-capable ADO token (PAT) and a checkout-built binary: ```bash cargo build --release --bin ado-aw -cd scripts/ado-script && npm ci && npm run build:executor-e2e && cd ../.. +cd scripts/ado-script && npm ci && npm run build:executor-e2e && npm run build:conclusion && cd ../.. export SYSTEM_COLLECTIONURI="https://dev.azure.com/msazuresphere/" export SYSTEM_TEAMPROJECT="AgentPlayground" export SYSTEM_ACCESSTOKEN="" export EXECUTOR_E2E_ADO_AW_BIN="$PWD/target/release/ado-aw" export EXECUTOR_E2E_ADO_REPO="agent-definitions" +# Enables the conclusion work-item scenarios (they skip when unset): +export EXECUTOR_E2E_CONCLUSION_BUNDLE="$PWD/scripts/ado-script/conclusion.js" # Optional: # export EXECUTOR_E2E_GITHUB_TOKEN="" # export EXECUTOR_E2E_ISSUE_REPO="jamesadevine/ado-aw-issues" diff --git a/tests/executor-e2e/azure-pipelines.yml b/tests/executor-e2e/azure-pipelines.yml index 2b6a7bd7c..a63fa012a 100644 --- a/tests/executor-e2e/azure-pipelines.yml +++ b/tests/executor-e2e/azure-pipelines.yml @@ -24,6 +24,7 @@ pr: - src/sanitize.rs - src/sanitize/** - src/safe_outputs/** + - scripts/ado-script/src/conclusion/** - scripts/ado-script/src/executor-e2e/** - tests/executor-e2e/** @@ -119,8 +120,12 @@ steps: set -euo pipefail npm ci npm run build:executor-e2e + # The conclusion scenarios run the real Conclusion reporter over the + # manifest the executor writes, so the bundle must be built too. + npm run build:conclusion + echo "##vso[task.setvariable variable=CONCLUSION_BUNDLE]$(Build.SourcesDirectory)/scripts/ado-script/conclusion.js" workingDirectory: scripts/ado-script - displayName: Build executor-e2e harness (not shipped in ado-script.zip) + displayName: Build executor-e2e harness + conclusion bundle (not shipped in ado-script.zip) - task: AzureCLI@2 displayName: Acquire ADO token (SC_WRITE_TOKEN) @@ -145,6 +150,9 @@ steps: # explicitly so the harness (and the ado-aw binary it spawns) can write. SYSTEM_ACCESSTOKEN: $(SC_WRITE_TOKEN) EXECUTOR_E2E_ADO_AW_BIN: $(ADO_AW_BIN) + # Compiled conclusion.js driven by the conclusion work-item scenarios. + # When unset those scenarios skip rather than fail. + EXECUTOR_E2E_CONCLUSION_BUNDLE: $(CONCLUSION_BUNDLE) EXECUTOR_E2E_ADO_REPO: $(EFFECTIVE_EXECUTOR_E2E_ADO_REPO) EXECUTOR_E2E_ISSUE_REPO: $(EXECUTOR_E2E_ISSUE_REPO) # Secret PAT for filing failure issues on the configured repository.