From 0cd2f668a0004461cd674b1ededd0fbb35561015 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:10:20 +0000 Subject: [PATCH 1/7] Initial plan From 05b8577b6e7dff32442a519a2507356a9d5e6e66 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Thu, 30 Jul 2026 10:20:23 +0000 Subject: [PATCH 2/7] fix(compile): checkout additional repos in SafeOutputs job for create-pull-request (issue #1731) Co-authored-by: jamesadevine <4742697+jamesadevine@users.noreply.github.com> --- src/compile/agentic_pipeline.rs | 28 +++++++ tests/compiler_tests.rs | 144 ++++++++++++++++++++++++++++++++ 2 files changed, 172 insertions(+) diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index fe1ea408..ba966c14 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1499,6 +1499,34 @@ fn build_safeoutputs_job( ) -> Result { let mut steps: Vec = Vec::new(); steps.push(checkout_self_step(&cfg.self_checkout_fetch)); + // When `create-pull-request` is configured and there are additional + // checked-out repos, the SafeOutputs job must replicate the Agent job's + // multi-checkout layout (issue #1731). Without these checkouts: + // • The additional repo directories don't exist in the SafeOutputs + // workspace, so `prepare-pr-base.js` and `ado-aw execute` fail. + // • A single `checkout: self` places `self` at + // `$(Build.SourcesDirectory)` instead of the multi-checkout path + // `$(Build.SourcesDirectory)/$(Build.Repository.Name)`, causing + // the executor --source path to not resolve. + // Only emit these for the variant that actually runs `create-pull-request`; + // other variants (and split-approval auto-SafeOutputs) don't need them. + if variant.runs_create_pull_request { + for repo in &front_matter.checkout { + let fetch = front_matter + .checkout_fetch + .get(repo) + .cloned() + .unwrap_or_default(); + steps.push(Step::Checkout(CheckoutStep { + repository: CheckoutRepo::Named(repo.clone()), + clean: None, + submodules: None, + fetch_depth: fetch.depth_for_emit(), + fetch_tags: fetch.fetch_tags, + persist_credentials: None, + })); + } + } // Acquire write token (when configured) push_raw_yaml_if_nonempty(&mut steps, &cfg.acquire_write_token)?; // Download analyzed outputs diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index c6f2de9a..1b4760f6 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -9293,6 +9293,150 @@ fn test_no_create_pull_request_omits_prepare_pr_base_step() { ); } +// ── Issue #1731: SafeOutputs must check out additional repos for create-pr ── + +/// Issue #1731: when `create-pull-request` is configured with additional +/// checked-out repos, the SafeOutputs job must emit a `checkout` step for each +/// `checkout: true` repo so that: +/// • The additional repo directories exist before `prepare-pr-base.js` runs. +/// • ADO uses the multi-checkout workspace layout, placing `self` at +/// `$(Build.SourcesDirectory)/$(Build.Repository.Name)` rather than +/// directly at `$(Build.SourcesDirectory)`. +/// +/// A `checkout: false` repo must remain resource-only (no checkout step). +#[test] +fn test_issue_1731_safeoutputs_checks_out_additional_repos_for_create_pr() { + let compiled = compile_inline_agent( + "issue-1731-multi-checkout", + "---\nname: \"Multi-repo PR\"\ndescription: \"Opens a PR in an additional repo\"\nworkspace: root\nrepos:\n - my-org/tools\n - my-org/docs\n - name: my-org/scripts\n checkout: false\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + ); + let agent = job_block(&compiled, "Agent"); + let safeoutputs = job_block(&compiled, "SafeOutputs"); + + // Agent job: self + both checked-out repos. + assert!( + agent.contains("- checkout: self"), + "Agent must check out self:\n{agent}" + ); + assert!( + agent.contains("- checkout: tools"), + "Agent must check out tools:\n{agent}" + ); + assert!( + agent.contains("- checkout: docs"), + "Agent must check out docs:\n{agent}" + ); + // scripts has checkout: false — resource-only, no checkout step in Agent. + assert!( + !agent.contains("- checkout: scripts"), + "Agent must NOT check out scripts (checkout: false):\n{agent}" + ); + + // SafeOutputs job: same three checkouts must precede prepare-pr-base.js. + assert!( + safeoutputs.contains("- checkout: self"), + "SafeOutputs must check out self:\n{safeoutputs}" + ); + assert!( + safeoutputs.contains("- checkout: tools"), + "SafeOutputs must check out tools (issue #1731):\n{safeoutputs}" + ); + assert!( + safeoutputs.contains("- checkout: docs"), + "SafeOutputs must check out docs (issue #1731):\n{safeoutputs}" + ); + // scripts has checkout: false — must not appear in SafeOutputs either. + assert!( + !safeoutputs.contains("- checkout: scripts"), + "SafeOutputs must NOT check out scripts (checkout: false):\n{safeoutputs}" + ); + + // Checkouts must appear before the prepare-pr-base.js invocation. + let tools_checkout_at = safeoutputs + .find("- checkout: tools") + .expect("tools checkout present"); + let prepare_at = safeoutputs + .find("prepare-pr-base.js") + .expect("prepare-pr-base.js present"); + assert!( + tools_checkout_at < prepare_at, + "additional checkouts must precede prepare-pr-base.js in SafeOutputs:\n{safeoutputs}" + ); +} + +/// Issue #1731: with additional checked-out repos the executor `--source` path +/// uses the multi-checkout layout `$(Build.SourcesDirectory)/$(Build.Repository.Name)/...` +/// (because `trigger_repo_directory` expands to the subdirectory form when +/// `checkout` is non-empty). The SafeOutputs job's additional checkout steps +/// are what forces ADO to apply that layout rather than placing `self` directly +/// at `$(Build.SourcesDirectory)`. +#[test] +fn test_issue_1731_safeoutputs_executor_source_path_uses_multi_checkout_layout() { + let compiled = compile_inline_agent( + "issue-1731-source-path", + "---\nname: \"Multi-repo PR\"\ndescription: \"Opens a PR in an additional repo\"\nworkspace: root\nrepos:\n - my-org/tools\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + ); + let safeoutputs = job_block(&compiled, "SafeOutputs"); + // With additional repos, self is placed at $(Build.SourcesDirectory)/$(Build.Repository.Name). + assert!( + safeoutputs.contains("$(Build.Repository.Name)"), + "SafeOutputs executor --source must use the multi-checkout layout path:\n{safeoutputs}" + ); +} + +/// Issue #1731, split-approval variant: when `create-pull-request` is +/// review-gated and additional repos are checked out, the additional checkout +/// steps must appear ONLY in `SafeOutputs_Reviewed` (the variant that actually +/// runs the PR tool). The auto `SafeOutputs` job must not have them — it never +/// executes `create-pull-request` and should not pay for the extra checkouts. +#[test] +fn test_issue_1731_split_approval_additional_checkouts_only_in_pr_variant() { + let compiled = compile_inline_agent( + "issue-1731-split-checkout", + "---\nname: \"Split PR\"\ndescription: \"Gated PR with extra repos\"\nworkspace: root\nrepos:\n - my-org/tools\nsafe-outputs:\n add-build-tag:\n tag: ci\n create-pull-request:\n target-branch: main\n require-approval: true\n---\n\n## Agent\n\nDo work.\n", + ); + let auto = job_block(&compiled, "SafeOutputs"); + let reviewed = job_block(&compiled, "SafeOutputs_Reviewed"); + + // Reviewed job runs create-pull-request → must check out additional repos. + assert!( + reviewed.contains("- checkout: tools"), + "SafeOutputs_Reviewed must check out additional repos (issue #1731):\n{reviewed}" + ); + + // Auto job excludes create-pull-request → must NOT check out additional repos. + assert!( + !auto.contains("- checkout: tools"), + "auto SafeOutputs must NOT check out additional repos (it never runs create-pull-request):\n{auto}" + ); +} + +/// Mirror of the split-approval case: when `create-pull-request` is NOT gated +/// but a sibling tool is, the PR tool runs in the auto `SafeOutputs` job. +/// Additional checkouts must therefore appear in the auto job and NOT in +/// `SafeOutputs_Reviewed`. +#[test] +fn test_issue_1731_split_approval_additional_checkouts_in_auto_when_sibling_gated() { + let compiled = compile_inline_agent( + "issue-1731-split-checkout-auto", + "---\nname: \"Split PR auto\"\ndescription: \"Non-gated PR with gated sibling and extra repos\"\nworkspace: root\nrepos:\n - my-org/tools\nsafe-outputs:\n add-build-tag:\n tag: ci\n require-approval: true\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + ); + let auto = job_block(&compiled, "SafeOutputs"); + let reviewed = job_block(&compiled, "SafeOutputs_Reviewed"); + + // Auto job runs create-pull-request → must check out additional repos. + assert!( + auto.contains("- checkout: tools"), + "auto SafeOutputs must check out additional repos when it runs create-pull-request:\n{auto}" + ); + + // Reviewed job excludes create-pull-request → must NOT check out additional repos. + assert!( + !reviewed.contains("- checkout: tools"), + "SafeOutputs_Reviewed must NOT check out additional repos when it doesn't run create-pull-request:\n{reviewed}" + ); +} + #[test] fn test_smoke_failure_reporter_uses_registered_ado_names_and_staging_repo() { let reporter_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")) From bab32d3b8313478e5083a713fdeb196781b72fee Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 30 Jul 2026 18:54:17 +0100 Subject: [PATCH 3/7] fix(compile): stabilize multi-checkout self resolution Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 10ccd9bb-6fb7-4a24-8e36-d2ff13031733 --- docs/ado-script.md | 2 +- docs/cli.md | 1 + docs/codemods.md | 2 +- docs/front-matter.md | 16 +- docs/runtime-imports.md | 7 +- .../src/executor-e2e/__tests__/index.test.ts | 10 + .../scenarios/create-pull-request.ts | 306 +++++++++++------- .../prepare-pr-base/__tests__/index.test.ts | 26 ++ .../ado-script/src/prepare-pr-base/index.ts | 6 +- src/compile/agentic_pipeline.rs | 115 +++++-- .../codemods/0004_legacy_path_markers.rs | 6 +- src/compile/common.rs | 293 +++++++---------- src/compile/extensions/ado_script.rs | 9 + src/compile/ir/emit.rs | 1 + src/compile/ir/job.rs | 1 + src/compile/ir/lower.rs | 24 ++ src/compile/ir/step.rs | 3 + src/compile/path_layout_check.rs | 61 +++- src/execute.rs | 4 + src/main.rs | 12 +- src/mcp.rs | 89 ++++- src/safe_outputs/add_pr_comment.rs | 44 ++- src/safe_outputs/create_pull_request.rs | 93 +++--- src/safe_outputs/mod.rs | 145 +++++++-- src/safe_outputs/result.rs | 83 ++++- src/safe_outputs/upload_build_attachment.rs | 1 + tests/compiler_tests.rs | 203 +++++++++--- tests/executor-e2e/README.md | 5 +- 28 files changed, 1075 insertions(+), 493 deletions(-) diff --git a/docs/ado-script.md b/docs/ado-script.md index 789d27fb..51571b5f 100644 --- a/docs/ado-script.md +++ b/docs/ado-script.md @@ -171,7 +171,7 @@ node import.js --base "$(Build.SourcesDirectory)" \ compiler always passes `$(Build.SourcesDirectory)` (ADO expands the macro before node runs), and the compiler-emitted marker for the agent body is a **trigger-repo-relative** path (e.g. `agents/foo.md`, or - `$(Build.Repository.Name)/agents/foo.md` under multi-checkout) — not + `self/agents/foo.md` under multi-checkout) — not absolute. `import.js` rejects absolute and `..` paths. - `--var name=value` (repeatable) — a small, **compiler-owned allowlist** of ADO path-anchor variables (currently `Build.SourcesDirectory` and diff --git a/docs/cli.md b/docs/cli.md index 23775963..2b91e5f2 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -25,6 +25,7 @@ Global flags (apply to all subcommands): `--verbose, -v` (enable info-level logg - Useful for CI checks to ensure pipelines are regenerated after source changes - `mcp ` - Run SafeOutputs as a stdio MCP server. **This is what compiled pipelines use**: MCPG spawns it as a hardened, network-isolated sibling container entrypoint (see [`docs/mcpg.md`](mcpg.md)). - `--enabled-tools ` - Restrict available tools to those named (repeatable) + - `--self-repository-directory ` - Exact `self` checkout used for Git patch generation when the bounding workspace is a multi-checkout root or another repository. - `mcp-author` - Run the author-facing stdio MCP server for IDE/Copilot Chat integrations. See [`mcp-author.md`](mcp-author.md) for the full tool surface and trust model. - `execute` - Execute safe outputs from Stage 1 (Stage 3 of pipeline) - `--source, -s ` - Path to source markdown file diff --git a/docs/codemods.md b/docs/codemods.md index e75281fa..129e1810 100644 --- a/docs/codemods.md +++ b/docs/codemods.md @@ -369,7 +369,7 @@ fold substituted the directory markers: | Marker | Resolved to | |--------|-------------| -| `{{ workspace }}`, `{{ working_directory }}` | the resolved working directory (`$(Build.SourcesDirectory)`, `$(Build.SourcesDirectory)/$(Build.Repository.Name)`, or `$(Build.SourcesDirectory)/`) | +| `{{ workspace }}`, `{{ working_directory }}` | the resolved working directory (`$(Build.SourcesDirectory)`, `$(Build.SourcesDirectory)/self`, or `$(Build.SourcesDirectory)/`) | | `{{ trigger_repo_directory }}` | the trigger ("self") repo dir | After the IR migration these markers flow through verbatim and are no diff --git a/docs/front-matter.md b/docs/front-matter.md index f7c9a336..789bee21 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -385,8 +385,8 @@ Reference the explicit ADO path instead: - `$(Build.SourcesDirectory)` — the checkout root (the trigger repo root when only `self` is checked out). -- `$(Build.SourcesDirectory)/$(Build.Repository.Name)` — the trigger repo when - one or more additional repositories are checked out. +- `$(Build.SourcesDirectory)/self` — the compiler-owned `self` checkout path + when one or more additional repositories are checked out. - `$(Build.SourcesDirectory)/` — a specific checked-out repository. The `legacy_path_markers` codemod automatically rewrites any remaining markers @@ -423,6 +423,10 @@ Object fields: | `fetch-depth` | *(ADO default)* | Shallow-clone depth for this repo's checkout (ADO `fetchDepth`). `0` = full history | | `fetch-tags` | *(ADO default)* | Whether to fetch git tags during checkout (ADO `fetchTags`) | +Aliases must be unique case-insensitively because they become checkout +directory names on Windows agents. `root`, `repo`, and `self` are reserved in +every casing; `self` is the compiler-owned path for the pipeline repository. + ### Tuning checkout fetch behavior (`fetch-depth` / `fetch-tags`) On large monorepos the checkout step can dominate the run. Azure DevOps can @@ -614,9 +618,11 @@ The trade-off is that the generated YAML is larger, and prompt-body edits require `ado-aw compile` plus committing the updated pipeline file. -A small, fixed set of ADO path-anchor variables — -`$(Build.SourcesDirectory)` and `$(Build.Repository.Name)` — is -substituted into the prompt consistently in **both** modes. Arbitrary +A small, fixed set of ADO path-anchor variables — including +`$(Build.SourcesDirectory)` and `$(Build.Repository.Name)` — is substituted +into the prompt consistently in **both** modes. `Build.Repository.Name` +identifies the triggering repository and is not the compiler-owned `self` +checkout path. Arbitrary `$(...)` macros and pipeline/secret variables are not expanded; see [ADO variables in the prompt](runtime-imports.md#ado-variables-in-the-prompt). diff --git a/docs/runtime-imports.md b/docs/runtime-imports.md index 1bca50fd..a1eeb397 100644 --- a/docs/runtime-imports.md +++ b/docs/runtime-imports.md @@ -45,8 +45,8 @@ along with any author-written markers. `{{#runtime-import ../../../../etc/passwd}}` are both compile-time errors. - **Compiler-generated marker for the agent body** uses a **trigger-repo-relative** path resolved against `--base "$(Build.SourcesDirectory)"` — e.g. `agents/foo.md`, - or `$(Build.Repository.Name)/agents/foo.md` under multi-checkout (ADO expands the - macro before the resolver runs). It is deliberately relative, not absolute: + or `self/agents/foo.md` under multi-checkout. It is deliberately relative, + not absolute: `import.js` rejects absolute paths for everyone. The compile-time `..`/absolute restriction is a compile-host guard and does not constrain this tooling-generated path beyond the runtime resolver's own absolute/`..` rejection. @@ -59,7 +59,7 @@ by the runtime resolver: | Variable | Expands to | |----------|------------| | `$(Build.SourcesDirectory)` | the checkout root | -| `$(Build.Repository.Name)` | the trigger repo's subfolder name (multi-checkout) | +| `$(Build.Repository.Name)` | the triggering repository name (not the compiler-owned `self` path) | The compiler passes these to `import.js` as `--var name=$(name)` flags (ADO expands the macro at runtime); the resolver then replaces every `$(name)` occurrence in the @@ -113,4 +113,3 @@ compile time instead of on the pipeline runner. - **Compile time**: `resolve_imports_inline()` in `src/compile/extensions/ado_script.rs` performs the inline expansion when `inlined-imports: true`. - diff --git a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts index 93ed4154..91a19121 100644 --- a/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts +++ b/scripts/ado-script/src/executor-e2e/__tests__/index.test.ts @@ -1,6 +1,7 @@ import { describe, expect, it } from "vitest"; import { summarise } from "../index.js"; +import { allScenarios } from "../scenarios/index.js"; import type { ScenarioResult } from "../scenario.js"; describe("summarise", () => { @@ -17,3 +18,12 @@ describe("summarise", () => { expect(text).toContain("Total: 3 | Passed: 1 | Failed: 1 | Skipped: 1"); }); }); + +describe("scenario registry", () => { + it("registers both create-pull-request checkout layouts with unique ids", () => { + const ids = allScenarios.map((scenario) => scenario.id ?? scenario.tool); + expect(new Set(ids).size).toBe(ids.length); + expect(ids).toContain("create-pull-request"); + expect(ids).toContain("create-pull-request-self-multi-checkout"); + }); +}); diff --git a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts index 9b424491..fe1d3a1c 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts @@ -1,14 +1,19 @@ /** - * Flagship deterministic scenario: create-pull-request. + * Flagship deterministic scenarios: create-pull-request. * * Stage 3's create-pull-request executor operates on a real git checkout on - * disk (`/`): it reads a staged patch file, - * verifies its SHA-256, applies it via `git apply --3way` on top of the target - * branch, and pushes a new source branch + opens the PR via ADO REST using the - * recorded `base_commit` as the parent. + * disk: it reads a staged patch file, verifies its SHA-256, applies it via + * `git apply --3way` on top of the target branch, and pushes a new source + * branch + opens the PR via ADO REST using the recorded `base_commit` as the + * parent. * - * We reproduce that deterministically (no LLM): - * - clone `agent-definitions` into the source-checkout dir, + * We reproduce both supported checkout layouts deterministically (no LLM): + * - a named additional checkout at `/`, + * - `repository: self` beneath a non-Git multi-checkout root, located via + * `ADO_AW_SELF_REPOSITORY_DIRECTORY`. + * + * Each scenario: + * - clone the configured ADO test repo into the source-checkout dir, * - record `base_commit` = main HEAD, * - make a deterministic edit and capture a `git diff` patch, * - compute `patch_sha256`, @@ -35,11 +40,20 @@ interface CreatePrState { patchContent: string; /** BUILD_SOURCESDIRECTORY passed to the executor. */ sourcesDir: string; + /** Actual git checkout beneath sourcesDir. */ + checkoutDir: string; + /** ADO repository id, required when the executor selector is `self`. */ + repositoryId?: string; /** PR id, populated in assert() so cleanup can abandon it. */ prId?: number; } -const PATCH_REL_PATH = "create-pr.patch"; +interface CreatePrScenarioOptions { + readonly id: string; + readonly repositorySelector: "named" | "self"; + readonly patchRelPath: string; + readonly changedFileSuffix?: string; +} function runGit( args: string[], @@ -97,9 +111,10 @@ async function git( args: string[], cwd: string, extraHeader: string, + scenarioId: string, ): Promise { // Safe to log: the auth token is passed via env (GIT_CONFIG_*), not argv. - ctx.log(`[create-pull-request] git ${args.join(" ")}`); + ctx.log(`[${scenarioId}] git ${args.join(" ")}`); const res = await runGit(args, cwd, extraHeader); if (res.code !== 0) { throw new Error(`git ${args.join(" ")} failed (${res.code}): ${res.stderr.trim()}`); @@ -107,114 +122,177 @@ async function git( return res.stdout; } -export const createPullRequest: Scenario = { - tool: "create-pull-request", - targetsAdoRepo: true, - config: (_ctx, state) => ({ - // Target the repo's actual default branch (state.targetBranch), which is - // also where base_commit was taken from, rather than hardcoding "main". - "target-branch": state.targetBranch, - "allowed-repositories": [state.repo], - "delete-source-branch": true, - "if-no-changes": "error", - "include-stats": false, - }), - setup: async (ctx) => { - const repo = ctx.adoRepo; - const authHeader = "Basic " + Buffer.from(":" + ctx.token).toString("base64"); - const sourcesDir = join(ctx.workDir, "create-pull-request", "src-checkout"); - await mkdir(sourcesDir, { recursive: true }); - const checkout = join(sourcesDir, repo); +async function setupCreatePullRequest( + ctx: ScenarioContext, + options: CreatePrScenarioOptions, +): Promise { + const repo = ctx.adoRepo; + const authHeader = "Basic " + Buffer.from(":" + ctx.token).toString("base64"); + const sourcesDir = join(ctx.workDir, options.id, "src-checkout"); + await mkdir(sourcesDir, { recursive: true }); + const checkoutDir = join(sourcesDir, repo); + const repositoryId = + options.repositorySelector === "self" + ? (await ctx.rest.getRepository(repo)).id + : undefined; + + const cloneUrl = `${ctx.orgUrl.replace(/\/+$/, "")}/${encodeURIComponent(ctx.project)}/_git/${encodeURIComponent(repo)}`; + ctx.log(`[${options.id}] cloning ${repo}`); + await git(ctx, ["clone", cloneUrl, checkoutDir], sourcesDir, authHeader, options.id); + + // Determine the default branch and its HEAD (the patch base commit). + // `symbolic-ref refs/remotes/origin/HEAD` exits non-zero (not empty) when + // the remote HEAD symref isn't configured, which git() turns into a throw. + // Catch that and fall back to "main" so the `|| "main"` isn't dead code. + let targetBranch = "main"; + try { + const symref = await git( + ctx, + ["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], + checkoutDir, + authHeader, + options.id, + ); + targetBranch = symref.trim().replace(/^origin\//, "") || "main"; + } catch { + ctx.log(`[${options.id}] origin/HEAD symref not set; defaulting target branch to 'main'`); + } + const baseCommit = ( + await git(ctx, ["rev-parse", "HEAD"], checkoutDir, authHeader, options.id) + ).trim(); - const cloneUrl = `${ctx.orgUrl.replace(/\/+$/, "")}/${encodeURIComponent(ctx.project)}/_git/${encodeURIComponent(repo)}`; - ctx.log(`[create-pull-request] cloning ${repo}`); - await git(ctx, ["clone", cloneUrl, checkout], sourcesDir, authHeader); + // Deterministic edit: add a new file, then capture a git diff (which + // `git apply --3way` applies cleanly on top of the target branch). + // The buildId-scoped path is assumed untracked: `git add -N` on an + // already-tracked path is a no-op, so `git diff` would be empty and setup + // would (cleanly) fail with "generated patch is empty". + const relFile = `ado-aw-det/${ctx.buildId}${options.changedFileSuffix ?? ""}.md`; + const absFile = join(checkoutDir, relFile); + await mkdir(join(absFile, ".."), { recursive: true }); + await writeFile(absFile, `${detBody(ctx, options.id)}\n`, "utf8"); + await git(ctx, ["add", "-N", relFile], checkoutDir, authHeader, options.id); + const patchContent = await git( + ctx, + ["diff", "--", relFile], + checkoutDir, + authHeader, + options.id, + ); + if (!patchContent.trim()) throw new Error("generated patch is empty"); + // Reset the intent-to-add so the checkout stays clean. + await git(ctx, ["reset", "--", relFile], checkoutDir, authHeader, options.id); - // Determine the default branch and its HEAD (the patch base commit). - // `symbolic-ref refs/remotes/origin/HEAD` exits non-zero (not empty) when - // the remote HEAD symref isn't configured, which git() turns into a throw. - // Catch that and fall back to "main" so the `|| "main"` isn't dead code. - let targetBranch = "main"; - try { - const symref = await git( - ctx, - ["symbolic-ref", "--short", "refs/remotes/origin/HEAD"], - checkout, - authHeader, - ); - targetBranch = symref.trim().replace(/^origin\//, "") || "main"; - } catch { - ctx.log(`[create-pull-request] origin/HEAD symref not set; defaulting target branch to 'main'`); - } - const baseCommit = (await git(ctx, ["rev-parse", "HEAD"], checkout, authHeader)).trim(); + const patchSha256 = createHash("sha256").update(patchContent, "utf8").digest("hex"); - // Deterministic edit: add a new file, then capture a git diff (which - // `git apply --3way` applies cleanly on top of the target branch). - // The buildId-scoped path is assumed untracked: `git add -N` on an - // already-tracked path is a no-op, so `git diff` would be empty and setup - // would (cleanly) fail with "generated patch is empty". - const relFile = `ado-aw-det/${ctx.buildId}.md`; - const absFile = join(checkout, relFile); - await mkdir(join(absFile, ".."), { recursive: true }); - await writeFile(absFile, `${detBody(ctx, "create-pull-request")}\n`, "utf8"); - await git(ctx, ["add", "-N", relFile], checkout, authHeader); - const patchContent = await git(ctx, ["diff", "--", relFile], checkout, authHeader); - if (!patchContent.trim()) throw new Error("generated patch is empty"); - // Reset the intent-to-add so the checkout stays clean. - await git(ctx, ["reset", "--", relFile], checkout, authHeader); + return { + repo, + sourceBranch: ctx.prefix(options.id), + targetBranch, + baseCommit, + patchRelPath: options.patchRelPath, + patchSha256, + patchContent, + sourcesDir, + checkoutDir, + repositoryId, + }; +} + +function createPullRequestScenario( + options: CreatePrScenarioOptions, +): Scenario { + return { + id: options.id, + tool: "create-pull-request", + targetsAdoRepo: true, + config: (_ctx, state) => ({ + // Target the repo's actual default branch (state.targetBranch), which is + // also where base_commit was taken from, rather than hardcoding "main". + "target-branch": state.targetBranch, + "allowed-repositories": [state.repo], + "delete-source-branch": true, + "if-no-changes": "error", + "include-stats": false, + }), + setup: (ctx) => setupCreatePullRequest(ctx, options), + files: async (_ctx, state) => ({ [state.patchRelPath]: state.patchContent }), + env: async (_ctx, state) => { + const env: Record = { + BUILD_SOURCESDIRECTORY: state.sourcesDir, + }; + if (options.repositorySelector === "self") { + if (!state.repositoryId) { + throw new Error("self create-pull-request scenario has no ADO repository id"); + } + Object.assign(env, { + ADO_AW_SELF_REPOSITORY_DIRECTORY: state.checkoutDir, + ADO_AW_SELF_REPOSITORY_ID: state.repositoryId, + ADO_AW_SELF_REPOSITORY_NAME: state.repo, + // Model a repository-resource-triggered run: Build.Repository.* + // identifies the trigger, while the compiler-owned values above + // must continue to identify checkout: self. + BUILD_REPOSITORY_ID: "00000000-0000-0000-0000-000000000000", + BUILD_REPOSITORY_NAME: "external-trigger-repository", + }); + } + return env; + }, + ndjson: async (ctx, state) => ({ + title: `${ctx.prefix(options.id)} (do not merge)`, + description: detBody(ctx, options.id), + source_branch: state.sourceBranch, + patch_file: state.patchRelPath, + repository: options.repositorySelector === "self" ? "self" : state.repo, + agent_labels: [], + base_commit: state.baseCommit, + patch_sha256: state.patchSha256, + }), + assert: async (ctx, state, record) => { + const prId = numResult(record, "pull_request_id"); + // Record the PR id up front so cleanup abandons it even if a later + // assertion (or the getPullRequest call itself) throws. + state.prId = prId; + const pr = await ctx.rest.getPullRequest(state.repo, prId); + if (pr.status === "abandoned") throw new Error(`PR #${prId} is abandoned`); + const sha = await ctx.rest.getRefObjectId(state.repo, `heads/${state.sourceBranch}`); + if (!sha) throw new Error(`source branch '${state.sourceBranch}' was not pushed`); + }, + cleanup: async (ctx, state) => { + // Attempt each cleanup independently so an early failure never skips the + // rest — otherwise a throwing abandonPullRequest would leak the source + // branch and the local checkout dir. + const teardown = new Teardown(); + if (state.prId !== undefined) { + const prId = state.prId; + teardown.add("abandon PR", () => ctx.rest.abandonPullRequest(state.repo, prId)); + } + await teardown + .add("delete source branch", () => + ctx.rest.deleteRef(state.repo, `refs/heads/${state.sourceBranch}`), + ) + // Remove the cloned checkout so repeated local runs don't accumulate it. + .add("remove local checkout", () => + rm(state.sourcesDir, { recursive: true, force: true }), + ) + .run(); + }, + }; +} - const patchSha256 = createHash("sha256").update(patchContent, "utf8").digest("hex"); +export const createPullRequest = createPullRequestScenario({ + id: "create-pull-request", + repositorySelector: "named", + patchRelPath: "create-pr.patch", +}); - return { - repo, - sourceBranch: ctx.prefix("create-pull-request"), - targetBranch, - baseCommit, - patchRelPath: PATCH_REL_PATH, - patchSha256, - patchContent, - sourcesDir, - }; - }, - files: async (_ctx, state) => ({ [state.patchRelPath]: state.patchContent }), - env: async (_ctx, state) => ({ BUILD_SOURCESDIRECTORY: state.sourcesDir }), - ndjson: async (ctx, state) => ({ - title: `${ctx.prefix("create-pull-request")} (do not merge)`, - description: detBody(ctx, "create-pull-request"), - source_branch: state.sourceBranch, - patch_file: state.patchRelPath, - repository: ctx.adoRepo, - agent_labels: [], - base_commit: state.baseCommit, - patch_sha256: state.patchSha256, - }), - assert: async (ctx, state, record) => { - const prId = numResult(record, "pull_request_id"); - // Record the PR id up front so cleanup abandons it even if a later - // assertion (or the getPullRequest call itself) throws. - state.prId = prId; - const pr = await ctx.rest.getPullRequest(state.repo, prId); - if (pr.status === "abandoned") throw new Error(`PR #${prId} is abandoned`); - const sha = await ctx.rest.getRefObjectId(state.repo, `heads/${state.sourceBranch}`); - if (!sha) throw new Error(`source branch '${state.sourceBranch}' was not pushed`); - }, - cleanup: async (ctx, state) => { - // Attempt each cleanup independently so an early failure never skips the - // rest — otherwise a throwing abandonPullRequest would leak the source - // branch and the local checkout dir. - const teardown = new Teardown(); - if (state.prId !== undefined) { - const prId = state.prId; - teardown.add("abandon PR", () => ctx.rest.abandonPullRequest(state.repo, prId)); - } - await teardown - .add("delete source branch", () => - ctx.rest.deleteRef(state.repo, `refs/heads/${state.sourceBranch}`), - ) - // Remove the cloned checkout so repeated local runs don't accumulate it. - .add("remove local checkout", () => rm(state.sourcesDir, { recursive: true, force: true })) - .run(); - }, -}; +export const createPullRequestSelfMultiCheckout = createPullRequestScenario({ + id: "create-pull-request-self-multi-checkout", + repositorySelector: "self", + patchRelPath: "create-pr-self-multi-checkout.patch", + changedFileSuffix: "-self-multi-checkout", +}); -export const createPullRequestScenarios: Scenario[] = [createPullRequest]; +export const createPullRequestScenarios: Scenario[] = [ + createPullRequest, + createPullRequestSelfMultiCheckout, +]; diff --git a/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts b/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts index 3b65f905..b81afa93 100644 --- a/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts +++ b/scripts/ado-script/src/prepare-pr-base/__tests__/index.test.ts @@ -188,6 +188,32 @@ describe("prepare-pr-base main", () => { ); }); + it("prefers the self resource ref over the triggering repository branch", async () => { + vi.spyOn(process.stdout, "write").mockImplementation(() => true); + const { deps, calls } = dependencies({ + remote: "https://github.com/example/repo.git", + }); + await main( + { + mode: "patch-base", + repos: [{ dir: "/src", target: "main" }], + fallbackTarget: "main", + }, + { + ADO_AW_SELF_REPOSITORY_REF: "refs/heads/self-default", + BUILD_SOURCEBRANCH: "refs/heads/external-trigger", + }, + deps, + ); + const fetch = calls.find((call) => call.args[0] === "fetch"); + expect(fetch?.args).toContain( + "+refs/heads/self-default:refs/remotes/origin/ado-aw-prepare-source", + ); + expect(fetch?.args).not.toContain( + "+refs/heads/external-trigger:refs/remotes/origin/ado-aw-prepare-source", + ); + }); + it("target-worktree fetches only the target tip at depth one", async () => { vi.spyOn(process.stdout, "write").mockImplementation(() => true); const { deps, calls } = dependencies(); diff --git a/scripts/ado-script/src/prepare-pr-base/index.ts b/scripts/ado-script/src/prepare-pr-base/index.ts index 08cf6556..5cc63669 100644 --- a/scripts/ado-script/src/prepare-pr-base/index.ts +++ b/scripts/ado-script/src/prepare-pr-base/index.ts @@ -158,7 +158,11 @@ async function preparePatchBase( ): Promise { const { runners } = deps; const headSha = runners.gitOk(["rev-parse", "HEAD"]) ?? ""; - const sourceRef = repo.sourceRef ?? env.BUILD_SOURCEBRANCH ?? headSha; + const sourceRef = + repo.sourceRef ?? + env.ADO_AW_SELF_REPOSITORY_REF ?? + env.BUILD_SOURCEBRANCH ?? + headSha; let restReason = "origin is not an eligible same-organization Azure Repos remote"; const remote = runners.gitOk(["remote", "get-url", "origin"]) ?? ""; diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index ba966c14..33b4c751 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -135,11 +135,6 @@ pub(crate) fn build_pipeline_context( // ─── Validations (reuse all shared validators) ──────────────── common::validate_front_matter_identity(front_matter)?; common::validate_variable_groups(front_matter)?; - common::validate_checkout_self_collision( - &front_matter.repositories, - &front_matter.checkout, - ctx.ado_context.as_ref().map(|c| c.repo_name.as_str()), - )?; common::validate_safe_outputs_keys(front_matter)?; front_matter .validate_threat_detection_config(&threat_detection, &detection_engine_config)?; @@ -315,6 +310,7 @@ pub(crate) fn build_pipeline_context( ) })?; crate::validate::reject_pipeline_injection(source_path_suffix, "workflow source path")?; + let source_relative_path = source_path_suffix.to_string(); let source_path = source_path_raw.replace("{{ trigger_repo_directory }}", &trigger_repo_directory); let pipeline_path = common::generate_pipeline_path(output_path); @@ -413,6 +409,7 @@ pub(crate) fn build_pipeline_context( mcpg_docker_env, mcpg_step_env, source_path, + source_relative_path, pipeline_path: pipeline_path.clone(), acquire_read_token, acquire_write_token, @@ -594,6 +591,9 @@ pub(crate) struct StandaloneCtx { /// `env:` block for the MCPG step (`env:\n KEY: ...`). pub(crate) mcpg_step_env: String, pub(crate) source_path: String, + /// Validated path to the workflow source relative to the trigger repository. + /// SafeOutputs variants combine this with their job-local checkout layout. + pub(crate) source_relative_path: String, pub(crate) pipeline_path: String, /// `AzureCLI@2` task YAML body (or empty when no read service connection). pub(crate) acquire_read_token: String, @@ -846,7 +846,7 @@ fn build_setup_job( return Ok(None); } let mut steps: Vec = Vec::new(); - steps.push(checkout_self_step(&cfg.self_checkout_fetch)); + steps.push(checkout_self_step(&cfg.self_checkout_fetch, false)); steps.extend(ext_setup_steps.iter().cloned()); // User setup steps as RawYaml — they're arbitrary user-authored ADO YAML @@ -904,7 +904,10 @@ fn build_agent_job( let mut steps: Vec = Vec::new(); // 1. checkout: self - steps.push(checkout_self_step(&cfg.self_checkout_fetch)); + steps.push(checkout_self_step( + &cfg.self_checkout_fetch, + !front_matter.checkout.is_empty(), + )); // 2. additional repo checkouts for repo in &front_matter.checkout { let fetch = front_matter @@ -914,6 +917,7 @@ fn build_agent_job( .unwrap_or_default(); steps.push(Step::Checkout(CheckoutStep { repository: CheckoutRepo::Named(repo.clone()), + path: Some(format!("s/{repo}")), clean: None, submodules: None, fetch_depth: fetch.depth_for_emit(), @@ -1009,7 +1013,7 @@ fn build_agent_job( // only, so it never double-prints when the same step is also emitted in // the SafeOutputs job (issue #1453). warn_create_pr_target_inference(front_matter); - let repos = create_pr_prepare_repos(front_matter, &cfg.working_directory); + let repos = create_pr_prepare_repos(front_matter, &cfg.trigger_repo_directory); steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( super::extensions::ado_script::PreparePrBaseMode::PatchBase, &repos, @@ -1205,7 +1209,7 @@ fn build_detection_job( prefix: &JobPrefix<'_>, ) -> Result { let mut steps: Vec = Vec::new(); - steps.push(checkout_self_step(&cfg.self_checkout_fetch)); + steps.push(checkout_self_step(&cfg.self_checkout_fetch, false)); // Detection job pulls the Agent's output artifact via cross-job download steps.push(Step::Download(DownloadStep { source: "current".to_string(), @@ -1360,6 +1364,42 @@ struct SafeOutputsVariant { is_reviewed: bool, } +/// Checkout-dependent paths for one SafeOutputs job. +/// +/// Split approval can produce two Stage 3 jobs with different checkout sets, +/// so workflow-global paths cannot be reused blindly by both variants. +struct SafeOutputsCheckoutLayout { + source_path: String, + self_repository_directory: String, + multi_checkout: bool, +} + +impl SafeOutputsCheckoutLayout { + fn for_variant( + front_matter: &FrontMatter, + cfg: &StandaloneCtx, + variant: &SafeOutputsVariant, + ) -> Self { + let has_additional_checkouts = + variant.runs_create_pull_request && !front_matter.checkout.is_empty(); + let self_repository_directory = if has_additional_checkouts { + cfg.trigger_repo_directory.clone() + } else { + common::generate_trigger_repo_directory(&[]) + }; + let source_path = format!( + "{}/{}", + self_repository_directory, cfg.source_relative_path + ); + + Self { + source_path, + self_repository_directory, + multi_checkout: has_additional_checkouts, + } + } +} + impl SafeOutputsVariant { /// The default single-job variant: no filter, canonical names. Runs every /// configured tool, so it executes `create-pull-request` iff configured. @@ -1418,10 +1458,10 @@ fn filter_flags(flag: &str, tools: &[String]) -> String { } /// Build the `(dir, target-branch)` pairs the `prepare-pr-base` bundle must -/// fetch/deepen — one per allowed `create-pull-request` repo, mirroring -/// `mcp.rs::resolve_git_dir_for_patch`: `working_directory` (for `self`) and -/// `working_directory/` for each `checkout:` alias. Each dir is paired -/// with THAT repo's resolved target branch +/// fetch/deepen — one per allowed `create-pull-request` repo. `self` uses its +/// exact checkout directory, while named repositories are siblings beneath +/// `$(Build.SourcesDirectory)`. Each dir is paired with THAT repo's resolved +/// target branch /// (`CreatePrConfig::resolve_target_branch` — explicit override, inferred /// checkout ref, or the literal default), so a PR to any repo deepens the branch /// it actually targets. A single `self` checkout ⇒ one pair. Returns an empty @@ -1433,7 +1473,7 @@ fn filter_flags(flag: &str, tools: &[String]) -> String { /// jobs (issue #1453). fn create_pr_prepare_repos( front_matter: &FrontMatter, - working_directory: &str, + self_repository_directory: &str, ) -> Vec { use super::extensions::ado_script::PreparePrBaseRepo; @@ -1442,7 +1482,7 @@ fn create_pr_prepare_repos( }; let repo_refs = front_matter.checkout_repo_refs(); let mut repos = vec![PreparePrBaseRepo { - dir: working_directory.to_string(), + dir: self_repository_directory.to_string(), // Read BUILD_SOURCEBRANCH directly in the Node process. Embedding the // runtime branch value into bash argv would make valid `$()`/backtick // ref characters subject to shell command substitution. @@ -1451,7 +1491,7 @@ fn create_pr_prepare_repos( }]; for alias in &front_matter.checkout { repos.push(PreparePrBaseRepo { - dir: format!("{working_directory}/{alias}"), + dir: format!("$(Build.SourcesDirectory)/{alias}"), source_ref: repo_refs.get(alias).cloned(), target_branch: pr_cfg.resolve_target_branch(alias, &repo_refs), }); @@ -1497,17 +1537,20 @@ fn build_safeoutputs_job( prefix: &JobPrefix<'_>, variant: &SafeOutputsVariant, ) -> Result { + let layout = SafeOutputsCheckoutLayout::for_variant(front_matter, cfg, variant); let mut steps: Vec = Vec::new(); - steps.push(checkout_self_step(&cfg.self_checkout_fetch)); + steps.push(checkout_self_step( + &cfg.self_checkout_fetch, + layout.multi_checkout, + )); // When `create-pull-request` is configured and there are additional // checked-out repos, the SafeOutputs job must replicate the Agent job's // multi-checkout layout (issue #1731). Without these checkouts: // • The additional repo directories don't exist in the SafeOutputs // workspace, so `prepare-pr-base.js` and `ado-aw execute` fail. - // • A single `checkout: self` places `self` at - // `$(Build.SourcesDirectory)` instead of the multi-checkout path - // `$(Build.SourcesDirectory)/$(Build.Repository.Name)`, causing - // the executor --source path to not resolve. + // `self` uses the compiler-owned `s/self` path in this layout so neither a + // repository-resource trigger nor an additional alias can change where the + // executor finds the workflow source. // Only emit these for the variant that actually runs `create-pull-request`; // other variants (and split-approval auto-SafeOutputs) don't need them. if variant.runs_create_pull_request { @@ -1519,6 +1562,7 @@ fn build_safeoutputs_job( .unwrap_or_default(); steps.push(Step::Checkout(CheckoutStep { repository: CheckoutRepo::Named(repo.clone()), + path: Some(format!("s/{repo}")), clean: None, submodules: None, fetch_depth: fetch.depth_for_emit(), @@ -1573,7 +1617,7 @@ fn build_safeoutputs_job( front_matter.supply_chain(), ), ); - let repos = create_pr_prepare_repos(front_matter, &cfg.working_directory); + let repos = create_pr_prepare_repos(front_matter, &layout.self_repository_directory); steps.push(super::extensions::ado_script::prepare_pr_base_step_typed( super::extensions::ado_script::PreparePrBaseMode::TargetWorktree, &repos, @@ -1581,8 +1625,8 @@ fn build_safeoutputs_job( } // Execute safe outputs (Stage 3) — typed BashStep with typed env block steps.push(Step::Bash(execute_safe_outputs_step( - &cfg.source_path, - &cfg.working_directory, + &layout.source_path, + &layout.self_repository_directory, &cfg.executor_ado_env, &variant.filter_args, )?)); @@ -1851,7 +1895,7 @@ fn build_teardown_job( return Ok(None); } let mut steps: Vec = Vec::new(); - steps.push(checkout_self_step(&cfg.self_checkout_fetch)); + steps.push(checkout_self_step(&cfg.self_checkout_fetch, false)); for user_step_val in &front_matter.teardown { steps.push(Step::RawYaml(step_to_raw_yaml_string(user_step_val)?)); } @@ -2218,9 +2262,10 @@ fn wire_explicit_dependencies(jobs: &mut [Job], prefix: &JobPrefix<'_>) -> Resul // Step body builders — typed BashStep/TaskStep with format!() bodies // ───────────────────────────────────────────────────────────────────── -fn checkout_self_step(fetch: &CheckoutFetchOpts) -> Step { +fn checkout_self_step(fetch: &CheckoutFetchOpts, multi_checkout: bool) -> Step { Step::Checkout(CheckoutStep { repository: CheckoutRepo::Self_, + path: multi_checkout.then(|| common::MULTI_CHECKOUT_SELF_PATH.to_string()), clean: None, submodules: None, fetch_depth: fetch.depth_for_emit(), @@ -2232,6 +2277,7 @@ fn checkout_self_step(fetch: &CheckoutFetchOpts) -> Step { fn checkout_none_step() -> Step { Step::Checkout(CheckoutStep { repository: CheckoutRepo::None, + path: None, clean: None, submodules: None, fetch_depth: None, @@ -3099,7 +3145,7 @@ fn run_agent_step( fn execute_safe_outputs_step( source_path: &str, - working_directory: &str, + self_repository_directory: &str, executor_ado_env: &str, filter_args: &str, ) -> Result { @@ -3115,10 +3161,22 @@ fn execute_safe_outputs_step( exit $EXIT_CODE\n" ); let mut step = bash("Execute safe outputs (Stage 3)", script); - step.working_directory = Some(working_directory.to_string()); + step.working_directory = Some(self_repository_directory.to_string()); for (k, v) in parse_env_block(executor_ado_env)? { step = step.with_env(k, v); } + step = step.with_env( + "ADO_AW_SELF_REPOSITORY_DIRECTORY", + EnvValue::literal(self_repository_directory), + ); + step = step.with_env( + "ADO_AW_SELF_REPOSITORY_ID", + EnvValue::runtime_expression("resources.repositories['self'].id"), + ); + step = step.with_env( + "ADO_AW_SELF_REPOSITORY_NAME", + EnvValue::runtime_expression("resources.repositories['self'].name"), + ); Ok(step) } @@ -4125,6 +4183,7 @@ mod tests { mcpg_docker_env: String::new(), mcpg_step_env: String::new(), source_path: "source.md".to_string(), + source_relative_path: "source.md".to_string(), pipeline_path: "source.lock.yml".to_string(), acquire_read_token: String::new(), acquire_write_token: String::new(), diff --git a/src/compile/codemods/0004_legacy_path_markers.rs b/src/compile/codemods/0004_legacy_path_markers.rs index 78472e69..d420063b 100644 --- a/src/compile/codemods/0004_legacy_path_markers.rs +++ b/src/compile/codemods/0004_legacy_path_markers.rs @@ -7,7 +7,7 @@ //! //! - `{{ workspace }}` and `{{ working_directory }}` → the resolved //! working directory (`$(Build.SourcesDirectory)`, -//! `$(Build.SourcesDirectory)/$(Build.Repository.Name)`, or +//! `$(Build.SourcesDirectory)/self`, or //! `$(Build.SourcesDirectory)/`). //! - `{{ trigger_repo_directory }}` → the trigger ("self") repo dir. //! @@ -254,7 +254,7 @@ mod tests { assert!(changed); assert_eq!( step_script(&fm), - "cd $(Build.SourcesDirectory)/$(Build.Repository.Name)" + "cd $(Build.SourcesDirectory)/self" ); } @@ -275,7 +275,7 @@ mod tests { apply_codemod(&mut fm, &ctx()).expect("apply"); assert_eq!( step_script(&fm), - "cat $(Build.SourcesDirectory)/$(Build.Repository.Name)/file" + "cat $(Build.SourcesDirectory)/self/file" ); } diff --git a/src/compile/common.rs b/src/compile/common.rs index 65846e14..e50ed3e7 100644 --- a/src/compile/common.rs +++ b/src/compile/common.rs @@ -701,20 +701,27 @@ pub fn lower_repos(items: &[ReposItem]) -> Result { ); } - // Reject duplicate aliases - if !seen_aliases.insert(alias.clone()) { + // Reject duplicate aliases case-insensitively. Checkout paths are + // filesystem paths, so aliases that differ only by case collide on + // Windows agents. + if !seen_aliases.insert(alias.to_ascii_lowercase()) { anyhow::bail!( - "Duplicate repository alias '{}' in repos. \ + "Duplicate repository alias '{}' in repos (aliases are compared \ + case-insensitively). \ Use the `alias` field (or `alias=org/repo` shorthand) to disambiguate.", alias ); } - // Reject reserved names - if RESERVED_WORKSPACE_NAMES.contains(&alias.as_str()) { + // Reject reserved names case-insensitively. In particular, no casing + // of `self` may collide with the compiler-owned `s/self` checkout. + if RESERVED_WORKSPACE_NAMES + .iter() + .any(|reserved| reserved.eq_ignore_ascii_case(&alias)) + { anyhow::bail!( "Repository alias '{}' is reserved by the 'workspace:' resolver ({:?}). \ - Rename the alias to avoid ambiguity.", + Reserved names are case-insensitive; rename the alias to avoid ambiguity.", alias, RESERVED_WORKSPACE_NAMES ); @@ -858,71 +865,6 @@ pub fn resolve_repos(front_matter: &FrontMatter) -> Result { /// wrong working directory. We reject this at compile time instead. const RESERVED_WORKSPACE_NAMES: &[&str] = &["root", "repo", "self"]; -/// Validate that no entry in `checkout` resolves to the same on-disk -/// directory as the `self` checkout. -/// -/// In ADO multi-repo checkout, both `checkout: self` and an additional -/// `checkout: ` land in `s/`, where -/// `` is `Build.Repository.Name` for `self` and the -/// trailing path segment of the `name:` field for each `repositories:` -/// entry. When these collide, the second checkout runs `git clean -ffdx` -/// and resets to its configured ref, silently wiping files that exist on -/// the trigger branch but not on the workspace ref. Failing fast at -/// compile time is much more discoverable than the resulting runtime -/// "file not found" errors downstream. -/// -/// `self_repo_name` is the trigger repo's `Build.Repository.Name` — -/// usually the trailing segment of the trigger repo's full name, inferred -/// from the local git remote. When `None` (e.g. compiling outside an ADO -/// clone, or in unit tests) the check is skipped because we have no -/// reliable identity for `self`. -pub fn validate_checkout_self_collision( - repositories: &[Repository], - checkout: &[String], - self_repo_name: Option<&str>, -) -> Result<()> { - let Some(self_name) = self_repo_name else { - return Ok(()); - }; - if checkout.is_empty() { - return Ok(()); - } - - for alias in checkout { - let Some(repo) = repositories.iter().find(|r| r.repository == *alias) else { - // Unknown aliases are reported by `validate_checkout_list`. - continue; - }; - // `rsplit('/').next()` on any &str always yields `Some` — even for - // names without a slash the whole string is returned. - let last_segment = repo - .name - .rsplit('/') - .next() - .expect("rsplit always yields one item"); - // ADO is case-insensitive on Windows agents and case-sensitive on - // Linux. Use a case-insensitive comparison so the collision is - // caught regardless of agent OS — the resulting pipeline would - // break on at least one platform either way. - if last_segment.eq_ignore_ascii_case(self_name) { - anyhow::bail!( - "Checkout entry '{}' (repository name '{}') resolves to the same \ - directory ('s/{}') as the trigger repository checked out as 'self'. \ - The second checkout would overwrite the first, replacing files \ - from the trigger branch with the workspace ref. Remove '{}' from \ - 'checkout:' — the 'self' checkout already provides access to this \ - repository.", - alias, - repo.name, - self_name, - alias, - ); - } - } - - Ok(()) -} - /// Validate that all entries in checkout list exist in repositories pub fn validate_checkout_list(repositories: &[Repository], checkout: &[String]) -> Result<()> { if checkout.is_empty() { @@ -940,10 +882,13 @@ pub fn validate_checkout_list(repositories: &[Repository], checkout: &[String]) repo_names ); } - if RESERVED_WORKSPACE_NAMES.contains(&name.as_str()) { + if RESERVED_WORKSPACE_NAMES + .iter() + .any(|reserved| reserved.eq_ignore_ascii_case(name)) + { anyhow::bail!( "Checkout entry '{}' uses a name reserved by the 'workspace:' resolver \ - ({:?}). Rename the repository alias to avoid ambiguity with \ + ({:?}). Reserved names are case-insensitive; rename the repository alias to avoid ambiguity with \ 'workspace: {}'.", name, RESERVED_WORKSPACE_NAMES, @@ -1030,7 +975,10 @@ fn resolve_effective_workspace( let ws = ws.as_str(); match ws { "root" => Ok(("root".to_string(), false)), - "repo" | "self" => Ok(("repo".to_string(), !has_additional_checkouts)), + "repo" | "self" if has_additional_checkouts => { + Ok(("repo".to_string(), false)) + } + "repo" | "self" => Ok(("root".to_string(), true)), alias => { // Defense in depth: even though aliases are constrained // by `validate_checkout_list` to match a `repository:` @@ -1109,15 +1057,23 @@ pub(crate) fn contains_template_marker(input: &str, name: &str) -> bool { false } +/// Checkout path for `self` when a job checks out additional repositories. +/// +/// The `self` alias is reserved, so using the same fixed segment for its +/// compiler-owned path cannot collide with a user-declared repository alias. +pub const MULTI_CHECKOUT_SELF_PATH: &str = "s/self"; + +/// Absolute ADO expression corresponding to [`MULTI_CHECKOUT_SELF_PATH`]. +pub const MULTI_CHECKOUT_SELF_DIRECTORY: &str = "$(Build.SourcesDirectory)/self"; + /// Generate the directory where the trigger ("self") repository is checked out. /// /// This is independent of `workspace:` — it depends only on whether any /// additional repositories are checked out: /// - No additional checkouts → `$(Build.SourcesDirectory)` (ADO checks `self` /// into the root). -/// - One or more additional checkouts → `$(Build.SourcesDirectory)/$(Build.Repository.Name)` -/// (ADO puts each checked-out repo, including `self`, into a subfolder named -/// after the repository). +/// - One or more additional checkouts → `$(Build.SourcesDirectory)/self` +/// (the compiler gives `self` a fixed explicit path). /// /// Used to anchor paths to files that ship in the trigger repo (e.g. the agent /// markdown source and the compiled pipeline yaml itself), regardless of where @@ -1126,7 +1082,7 @@ pub fn generate_trigger_repo_directory(checkout: &[String]) -> String { if checkout.is_empty() { "$(Build.SourcesDirectory)".to_string() } else { - "$(Build.SourcesDirectory)/$(Build.Repository.Name)".to_string() + MULTI_CHECKOUT_SELF_DIRECTORY.to_string() } } @@ -1136,7 +1092,7 @@ pub fn generate_working_directory(effective_workspace: &str) -> String { return format!("$(Build.SourcesDirectory)/{}", alias); } match effective_workspace { - "repo" => "$(Build.SourcesDirectory)/$(Build.Repository.Name)".to_string(), + "repo" => MULTI_CHECKOUT_SELF_DIRECTORY.to_string(), "root" => "$(Build.SourcesDirectory)".to_string(), // compute_effective_workspace only ever returns "root", "repo", or an // "alias:" sentinel; any other value indicates a programming @@ -2405,6 +2361,7 @@ pub fn generate_mcpg_config( &front_matter.name, )?; let working_directory = generate_working_directory(&effective_workspace); + let trigger_repo_directory = generate_trigger_repo_directory(&front_matter.checkout); let registry_base = front_matter .supply_chain() .and_then(|sc| sc.registry.as_ref()) @@ -2421,9 +2378,23 @@ pub fn generate_mcpg_config( .map(str::to_string), ); safeoutputs_entrypoint_args.extend([ + "--self-repository-directory".to_string(), + trigger_repo_directory.clone(), "/safeoutputs".to_string(), working_directory.clone(), ]); + let mut safeoutputs_mounts = vec![ + "/tmp/awf-tools/ado-aw:/usr/local/bin/ado-aw:ro".to_string(), + format!("{working_directory}:{working_directory}:rw"), + ]; + if trigger_repo_directory != working_directory + && !trigger_repo_directory.starts_with(&format!("{working_directory}/")) + { + safeoutputs_mounts.push(format!( + "{trigger_repo_directory}:{trigger_repo_directory}:rw" + )); + } + safeoutputs_mounts.push("/tmp/awf-tools/staging:/safeoutputs:rw".to_string()); mcp_servers.insert( "safeoutputs".to_string(), McpgServerConfig { @@ -2431,11 +2402,7 @@ pub fn generate_mcpg_config( container: Some(safeoutputs_image), entrypoint: Some("/usr/local/bin/ado-aw".to_string()), entrypoint_args: Some(safeoutputs_entrypoint_args), - mounts: Some(vec![ - "/tmp/awf-tools/ado-aw:/usr/local/bin/ado-aw:ro".to_string(), - format!("{working_directory}:{working_directory}:rw"), - "/tmp/awf-tools/staging:/safeoutputs:rw".to_string(), - ]), + mounts: Some(safeoutputs_mounts), args: Some(vec![ "--network".to_string(), "none".to_string(), @@ -3460,7 +3427,7 @@ mod tests { assert_eq!(ws, "repo"); assert_eq!( generate_working_directory(&ws), - "$(Build.SourcesDirectory)/$(Build.Repository.Name)" + "$(Build.SourcesDirectory)/self" ); } @@ -3473,7 +3440,7 @@ mod tests { assert_eq!(ws, "repo"); assert_eq!( generate_working_directory(&ws), - "$(Build.SourcesDirectory)/$(Build.Repository.Name)" + "$(Build.SourcesDirectory)/self" ); } @@ -3491,10 +3458,10 @@ mod tests { } #[test] - fn test_workspace_explicit_repo_no_checkouts_still_returns_repo() { - // Emits a warning but still returns "repo" + fn test_workspace_explicit_repo_no_checkouts_resolves_to_root() { + // Emits a warning and preserves the single-checkout root layout. let ws = compute_effective_workspace(&Some("repo".to_string()), &[], "agent").unwrap(); - assert_eq!(ws, "repo"); + assert_eq!(ws, "root"); } #[test] @@ -3626,82 +3593,6 @@ mod tests { assert!(msg.contains("'repo'"), "msg: {msg}"); } - // ─── validate_checkout_self_collision ──────────────────────────────────── - - #[test] - fn test_validate_self_collision_detects_match() { - // Workspace repo's name last segment matches the self repo's name, - // so both `checkout: self` and `checkout: my-repo` would land in - // `s/my-repo`. Must error. - let repos = vec![Repository { - repository: "my-repo".to_string(), - repo_type: "git".to_string(), - name: "some-org/my-repo".to_string(), - repo_ref: "refs/heads/main".to_string(), - }]; - let checkout = vec!["my-repo".to_string()]; - let err = validate_checkout_self_collision(&repos, &checkout, Some("my-repo")).unwrap_err(); - let msg = err.to_string(); - assert!(msg.contains("'my-repo'"), "msg: {msg}"); - assert!(msg.contains("same"), "msg: {msg}"); - assert!(msg.contains("'self'"), "msg: {msg}"); - } - - #[test] - fn test_validate_self_collision_no_collision_passes() { - // Different repo name → different `s/` directory, no collision. - let repos = vec![Repository { - repository: "other".to_string(), - repo_type: "git".to_string(), - name: "some-org/other".to_string(), - repo_ref: "refs/heads/main".to_string(), - }]; - let checkout = vec!["other".to_string()]; - let result = validate_checkout_self_collision(&repos, &checkout, Some("my-repo")); - assert!(result.is_ok()); - } - - #[test] - fn test_validate_self_collision_case_insensitive() { - // ADO is case-insensitive on Windows; treat differing-only-by-case - // names as a collision so the pipeline doesn't break on one OS. - let repos = vec![Repository { - repository: "my-repo".to_string(), - repo_type: "git".to_string(), - name: "Some-Org/My-Repo".to_string(), - repo_ref: "refs/heads/main".to_string(), - }]; - let checkout = vec!["my-repo".to_string()]; - let err = validate_checkout_self_collision(&repos, &checkout, Some("my-repo")).unwrap_err(); - assert!(err.to_string().contains("same")); - } - - #[test] - fn test_validate_self_collision_no_self_name_skipped() { - // No git remote / no inferred self name → can't detect, skip. - let repos = vec![Repository { - repository: "my-repo".to_string(), - repo_type: "git".to_string(), - name: "org/my-repo".to_string(), - repo_ref: "refs/heads/main".to_string(), - }]; - let checkout = vec!["my-repo".to_string()]; - let result = validate_checkout_self_collision(&repos, &checkout, None); - assert!(result.is_ok()); - } - - #[test] - fn test_validate_self_collision_empty_checkout_passes() { - let repos = vec![Repository { - repository: "my-repo".to_string(), - repo_type: "git".to_string(), - name: "org/my-repo".to_string(), - repo_ref: "refs/heads/main".to_string(), - }]; - let result = validate_checkout_self_collision(&repos, &[], Some("my-repo")); - assert!(result.is_ok()); - } - // ─── Engine::args (copilot params) ────────────────────────────────────── #[test] @@ -4291,11 +4182,10 @@ mod tests { #[test] fn test_generate_trigger_repo_directory_with_additional_checkouts() { - // As soon as any additional repo is checked out, ADO places every - // checked-out repo (including `self`) into a subdirectory named - // after the repository. + // The compiler pins self to a reserved path so trigger metadata cannot + // change the checkout location. let result = generate_trigger_repo_directory(&["exp23-a7-nw".to_string()]); - assert_eq!(result, "$(Build.SourcesDirectory)/$(Build.Repository.Name)"); + assert_eq!(result, "$(Build.SourcesDirectory)/self"); } #[test] @@ -4311,10 +4201,7 @@ mod tests { .unwrap(); let working_dir = generate_working_directory(&workspace); - assert_eq!( - trigger, - "$(Build.SourcesDirectory)/$(Build.Repository.Name)" - ); + assert_eq!(trigger, "$(Build.SourcesDirectory)/self"); assert_eq!(working_dir, "$(Build.SourcesDirectory)/exp23-a7-nw"); assert_ne!( trigger, working_dir, @@ -5929,6 +5816,33 @@ safe-outputs: entrypoint_args.starts_with(&["mcp".to_string()]), "SafeOutputs should use the stdio MCP subcommand: {entrypoint_args:?}" ); + assert!( + entrypoint_args.windows(2).any(|args| { + args + == [ + "--self-repository-directory".to_string(), + "$(Build.SourcesDirectory)".to_string(), + ] + }), + "SafeOutputs should receive the exact self checkout: {entrypoint_args:?}" + ); + } + + #[test] + fn test_generate_mcpg_config_mounts_self_when_workspace_is_sibling_alias() { + let mut fm = minimal_front_matter(); + fm.checkout = vec!["tools".to_string()]; + fm.workspace = Some("tools".to_string()); + let config = generate_mcpg_config(&fm, &collect_exts_and_decls(&fm).1).unwrap(); + let so = config.mcp_servers.get("safeoutputs").unwrap(); + + assert!( + so.mounts.as_ref().unwrap().iter().any(|mount| { + mount + == "$(Build.SourcesDirectory)/self:$(Build.SourcesDirectory)/self:rw" + }), + "self checkout must be mounted when it is outside the selected workspace" + ); } #[test] @@ -6894,6 +6808,19 @@ safe-outputs: ); } + #[test] + fn test_repos_rejects_case_only_duplicate_aliases() { + let items = vec![ + ReposItem::Shorthand("org/Tools".to_string()), + ReposItem::Shorthand("other-org/tools".to_string()), + ]; + let err = lower_repos(&items).unwrap_err(); + assert!( + err.to_string().contains("case-insensitively"), + "{err}" + ); + } + #[test] fn test_repos_rejects_reserved_alias() { let items = vec![ReposItem::Shorthand("org/self".to_string())]; @@ -6923,6 +6850,26 @@ safe-outputs: assert!(err.to_string().contains("reserved"), "{err}"); } + #[test] + fn test_repos_rejects_reserved_alias_case_insensitively() { + for alias in ["Self", "SELF", "Repo", "ROOT"] { + let items = vec![ReposItem::Full(RepoEntry { + name: "org/fine-repo".to_string(), + alias: Some(alias.to_string()), + repo_type: "git".to_string(), + repo_ref: "refs/heads/main".to_string(), + checkout: true, + fetch_depth: None, + fetch_tags: None, + })]; + let err = lower_repos(&items).unwrap_err(); + assert!( + err.to_string().contains("case-insensitive"), + "{alias}: {err}" + ); + } + } + #[test] fn test_repos_rejects_shell_unsafe_alias() { // An explicit alias carrying a shell metacharacter is rejected — it diff --git a/src/compile/extensions/ado_script.rs b/src/compile/extensions/ado_script.rs index bdf5c716..52bbbb1b 100644 --- a/src/compile/extensions/ado_script.rs +++ b/src/compile/extensions/ado_script.rs @@ -670,6 +670,10 @@ pub fn prepare_pr_base_step_typed(mode: PreparePrBaseMode, repos: &[PreparePrBas BashStep::new(mode.display_name(), script).with_condition(Condition::Succeeded), crate::compile::ado_bundle::Bundle::PreparePrBase, crate::compile::ado_bundle::TokenSource::SystemAccessToken, + ) + .with_env( + "ADO_AW_SELF_REPOSITORY_REF", + EnvValue::runtime_expression("resources.repositories['self'].ref"), ); Step::Bash(step) } @@ -1568,6 +1572,11 @@ mod tests { step.env.get("SYSTEM_ACCESSTOKEN"), Some(EnvValue::Secret(v)) if v == "System.AccessToken" )); + assert!(matches!( + step.env.get("ADO_AW_SELF_REPOSITORY_REF"), + Some(EnvValue::RuntimeExpression(v)) + if v == "resources.repositories['self'].ref" + )); } #[test] diff --git a/src/compile/ir/emit.rs b/src/compile/ir/emit.rs index f489f166..eb8eee73 100644 --- a/src/compile/ir/emit.rs +++ b/src/compile/ir/emit.rs @@ -56,6 +56,7 @@ mod tests { let mut setup = Job::new(JobId::new("Setup").unwrap(), "Setup", pool()); setup.push_step(Step::Checkout(CheckoutStep { repository: CheckoutRepo::Self_, + path: None, clean: Some(true), submodules: None, fetch_depth: None, diff --git a/src/compile/ir/job.rs b/src/compile/ir/job.rs index e78c8936..ff377d46 100644 --- a/src/compile/ir/job.rs +++ b/src/compile/ir/job.rs @@ -273,6 +273,7 @@ mod tests { ); j.push_step(Step::Checkout(super::super::step::CheckoutStep { repository: super::super::step::CheckoutRepo::Self_, + path: None, clean: None, submodules: None, fetch_depth: None, diff --git a/src/compile/ir/lower.rs b/src/compile/ir/lower.rs index 9af9870b..ba701bb8 100644 --- a/src/compile/ir/lower.rs +++ b/src/compile/ir/lower.rs @@ -1025,6 +1025,9 @@ fn lower_checkout(c: &CheckoutStep) -> Value { m.insert(s("checkout"), s(name)); } } + if let Some(path) = &c.path { + m.insert(s("path"), s(path)); + } if let Some(clean) = c.clean { m.insert(s("clean"), Value::Bool(clean)); } @@ -2880,6 +2883,7 @@ mod tests { fn lower_checkout_emits_fetch_depth_and_tags_when_set() { let c = CheckoutStep { repository: CheckoutRepo::Self_, + path: None, clean: None, submodules: None, fetch_depth: Some(1), @@ -2893,10 +2897,28 @@ mod tests { assert_eq!(m.get(s("fetchTags")).unwrap(), &Value::Bool(false)); } + #[test] + fn lower_checkout_emits_explicit_path() { + let c = CheckoutStep { + repository: CheckoutRepo::Named("build-tools".to_string()), + path: Some("s/build-tools".to_string()), + clean: None, + submodules: None, + fetch_depth: None, + fetch_tags: None, + persist_credentials: None, + }; + let v = lower_checkout(&c); + let m = v.as_mapping().unwrap(); + assert_eq!(m.get(s("checkout")).unwrap(), &s("build-tools")); + assert_eq!(m.get(s("path")).unwrap(), &s("s/build-tools")); + } + #[test] fn lower_checkout_emits_zero_fetch_depth_explicitly() { let c = CheckoutStep { repository: CheckoutRepo::Self_, + path: None, clean: None, submodules: None, fetch_depth: Some(0), @@ -2912,6 +2934,7 @@ mod tests { fn lower_checkout_omits_fetch_keys_when_none() { let c = CheckoutStep { repository: CheckoutRepo::Self_, + path: None, clean: None, submodules: None, fetch_depth: None, @@ -2928,6 +2951,7 @@ mod tests { fn lower_checkout_none_emits_checkout_none() { let c = CheckoutStep { repository: CheckoutRepo::None, + path: None, clean: None, submodules: None, fetch_depth: None, diff --git a/src/compile/ir/step.rs b/src/compile/ir/step.rs index ad485021..05777262 100644 --- a/src/compile/ir/step.rs +++ b/src/compile/ir/step.rs @@ -189,6 +189,8 @@ impl TaskStep { pub struct CheckoutStep { /// `self`, or a named repository resource. pub repository: CheckoutRepo, + /// Checkout path relative to `$(Agent.BuildDirectory)`. + pub path: Option, pub clean: Option, pub submodules: Option, pub fetch_depth: Option, @@ -261,6 +263,7 @@ mod tests { fn step_id_returns_none_for_anchorless_kinds() { let chk = Step::Checkout(CheckoutStep { repository: CheckoutRepo::Self_, + path: None, clean: None, submodules: None, fetch_depth: None, diff --git a/src/compile/path_layout_check.rs b/src/compile/path_layout_check.rs index 5deedc7c..dcf466f0 100644 --- a/src/compile/path_layout_check.rs +++ b/src/compile/path_layout_check.rs @@ -7,17 +7,14 @@ //! *is* the trigger repo root. //! - **Multi-checkout** (≥1 additional repo): every repo, including the //! trigger repo, lives in a subfolder of `$(Build.SourcesDirectory)` -//! named after its alias — the trigger repo under -//! `$(Build.Repository.Name)`, each additional repo under its `repos:` -//! alias. +//! named after its alias — the trigger repo under the compiler-owned +//! `self` path, each additional repo under its `repos:` alias. //! //! When authors hand-write paths anchored at `$(Build.SourcesDirectory)` //! (or reference repos via `{{#runtime-import …}}`), it is easy to point //! at a path that will not exist under the resolved layout. This pass //! surfaces those mistakes as **warnings** — it never fails the compile, -//! because the compiler cannot always resolve a path (e.g. the trigger -//! repo's literal name behind `$(Build.Repository.Name)`), and false -//! positives must not block builds. +//! because false positives must not block builds. //! //! It also warns when deprecated directory markers //! (`{{ workspace }}` / `{{ working_directory }}` / @@ -35,9 +32,14 @@ use crate::compile::types::FrontMatter; /// reference is not treated as having a sub-segment). const SOURCES_DIR_PREFIX: &str = "$(Build.SourcesDirectory)/"; -/// The runtime macro under which the trigger ("self") repo is checked -/// out in multi-checkout mode. -const SELF_REPO_SEGMENT: &str = "$(Build.Repository.Name)"; +/// Compiler-owned segment under which the trigger ("self") repo is checked out +/// in multi-checkout mode. +const SELF_REPO_SEGMENT: &str = "self"; + +/// Legacy self path segment. `Build.Repository.Name` identifies the triggering +/// repository on repository-resource-triggered runs, so it is never a stable +/// checkout path. +const LEGACY_SELF_REPO_SEGMENT: &str = "$(Build.Repository.Name)"; /// Deprecated directory markers that the `legacy_path_markers` codemod /// migrates in front matter but cannot touch in the agent body. @@ -92,7 +94,18 @@ pub(crate) fn collect_path_layout_warnings(front_matter: &FrontMatter, markdown_ } for scalar in &step_scalars { for seg in sources_dir_segments(scalar) { - if declared_not_checked_out.contains(&seg.as_str()) { + if seg == LEGACY_SELF_REPO_SEGMENT { + let replacement = if multi { + "`$(Build.SourcesDirectory)/self`" + } else { + "`$(Build.SourcesDirectory)`" + }; + warnings.push(format!( + "step references `$(Build.SourcesDirectory)/$(Build.Repository.Name)`, but \ + `Build.Repository.Name` identifies the triggering repository and is not a \ + stable `self` checkout path. Use {replacement}." + )); + } else if declared_not_checked_out.contains(&seg.as_str()) { warnings.push(format!( "step references `$(Build.SourcesDirectory)/{seg}`, but repository `{seg}` is \ declared in `repos:` with `checkout: false`; its sources will not be present \ @@ -100,7 +113,7 @@ pub(crate) fn collect_path_layout_warnings(front_matter: &FrontMatter, markdown_ )); } else if !multi && seg == SELF_REPO_SEGMENT { warnings.push( - "step references `$(Build.SourcesDirectory)/$(Build.Repository.Name)`, but with \ + "step references `$(Build.SourcesDirectory)/self`, but with \ only the trigger repository checked out `$(Build.SourcesDirectory)` already IS \ the repository root; that subfolder will not exist. Use \ `$(Build.SourcesDirectory)` directly." @@ -239,7 +252,7 @@ mod tests { #[test] fn warns_on_self_subfolder_in_single_checkout() { let fm = fm( - "name: a\ndescription: d\nsteps:\n - script: cd $(Build.SourcesDirectory)/$(Build.Repository.Name)\n", + "name: a\ndescription: d\nsteps:\n - script: cd $(Build.SourcesDirectory)/self\n", ); let w = collect_path_layout_warnings(&fm, "body"); assert_eq!(w.len(), 1, "{w:?}"); @@ -249,11 +262,25 @@ mod tests { #[test] fn no_self_subfolder_warning_in_multi_checkout() { let fm = fm( - "name: a\ndescription: d\nrepos:\n - org/other\nsteps:\n - script: cd $(Build.SourcesDirectory)/$(Build.Repository.Name)\n", + "name: a\ndescription: d\nrepos:\n - org/other\nsteps:\n - script: cd $(Build.SourcesDirectory)/self\n", ); assert!(collect_path_layout_warnings(&fm, "body").is_empty()); } + #[test] + fn warns_on_trigger_scoped_repository_name_in_multi_checkout() { + let fm = fm( + "name: a\ndescription: d\nrepos:\n - org/other\nsteps:\n - script: cd $(Build.SourcesDirectory)/$(Build.Repository.Name)\n", + ); + let warnings = collect_path_layout_warnings(&fm, "body"); + assert_eq!(warnings.len(), 1, "{warnings:?}"); + assert!( + warnings[0].contains("triggering repository") + && warnings[0].contains("$(Build.SourcesDirectory)/self"), + "{warnings:?}" + ); + } + #[test] fn warns_on_reference_to_not_checked_out_repo() { let fm = fm( @@ -309,7 +336,7 @@ mod tests { #[test] fn deduplicates_repeated_warnings() { let fm = fm( - "name: a\ndescription: d\nsteps:\n - script: cd $(Build.SourcesDirectory)/$(Build.Repository.Name)\n - script: ls $(Build.SourcesDirectory)/$(Build.Repository.Name)\n", + "name: a\ndescription: d\nsteps:\n - script: cd $(Build.SourcesDirectory)/self\n - script: ls $(Build.SourcesDirectory)/self\n", ); let w = collect_path_layout_warnings(&fm, "body"); assert_eq!(w.len(), 1, "{w:?}"); @@ -323,9 +350,13 @@ mod tests { ); assert!(sources_dir_segments("cd $(Build.SourcesDirectory)").is_empty()); assert_eq!( - sources_dir_segments("$(Build.SourcesDirectory)/$(Build.Repository.Name)/x"), + sources_dir_segments("$(Build.SourcesDirectory)/self/x"), vec![SELF_REPO_SEGMENT.to_string()] ); + assert_eq!( + sources_dir_segments("$(Build.SourcesDirectory)/$(Build.Repository.Name)/x"), + vec![LEGACY_SELF_REPO_SEGMENT.to_string()] + ); // Back-to-back double prefix: the inner `$(Build.SourcesDirectory)` // is extracted as the segment and the loop advances past it (rather // than re-scanning from just after the outer prefix), so the segment diff --git a/src/execute.rs b/src/execute.rs index 0e033022..254f9abf 100644 --- a/src/execute.rs +++ b/src/execute.rs @@ -233,6 +233,10 @@ fn log_execution_context(safe_output_dir: &Path, ctx: &ExecutionContext) { info!("Stage 3 execution starting"); debug!("Safe output directory: {}", safe_output_dir.display()); debug!("Source directory: {}", ctx.source_directory.display()); + debug!( + "Self repository directory: {}", + ctx.self_repository_directory.display() + ); debug!( "ADO org: {}", ctx.ado_org_url.as_deref().unwrap_or("") diff --git a/src/main.rs b/src/main.rs index 13d16fd8..55f471fb 100644 --- a/src/main.rs +++ b/src/main.rs @@ -239,6 +239,9 @@ enum Commands { output_directory: String, /// Guard against directory traversal attacks by specifying the agent cannot influence folders outside this path bounding_directory: String, + /// Exact checkout directory for the pipeline's self repository. + #[arg(long)] + self_repository_directory: Option, /// Only expose these safe output tools (can be repeated). If omitted, all tools are exposed. #[arg(long = "enabled-tools")] enabled_tools: Vec, @@ -1058,6 +1061,7 @@ async fn main() -> Result<()> { Commands::Mcp { output_directory, bounding_directory, + self_repository_directory, enabled_tools, } => { let filter = if enabled_tools.is_empty() { @@ -1065,7 +1069,13 @@ async fn main() -> Result<()> { } else { Some(enabled_tools) }; - mcp::run(&output_directory, &bounding_directory, filter.as_deref()).await?; + mcp::run( + &output_directory, + &bounding_directory, + self_repository_directory.as_deref(), + filter.as_deref(), + ) + .await?; } Commands::McpAuthor {} => { mcp_author::run_stdio().await?; diff --git a/src/mcp.rs b/src/mcp.rs index 80d7befe..8ef08d04 100644 --- a/src/mcp.rs +++ b/src/mcp.rs @@ -199,6 +199,7 @@ async fn try_root_commit_fallback(git_dir: &std::path::Path) -> Option { #[derive(Clone, Debug)] pub struct SafeOutputs { bounding_directory: PathBuf, + self_repository_directory: PathBuf, output_directory: PathBuf, /// ToolRouter is used by the rmcp framework's #[tool_handler] macro for /// dispatching MCP tool calls. Clippy doesn't see this usage. @@ -208,15 +209,16 @@ pub struct SafeOutputs { /// Resolve which git directory to use for patch generation. /// -/// `repository` of `None` or `"self"` maps to `bounding_directory` directly; -/// any other value is validated as a safe path segment and resolved as a -/// subdirectory of `bounding_directory`. +/// `repository` of `None` or `"self"` maps to the compiler-provided self +/// checkout; any other value is validated as a safe path segment and resolved +/// as a subdirectory of `bounding_directory`. fn resolve_git_dir_for_patch( bounding_directory: &std::path::Path, + self_repository_directory: &std::path::Path, repository: Option<&str>, ) -> Result { match repository { - Some("self") | None => Ok(bounding_directory.to_owned()), + Some("self") | None => Ok(self_repository_directory.to_owned()), Some(repo_alias) => { if !crate::validate::is_safe_path_segment(repo_alias) { return Err(anyhow_to_mcp_error(anyhow::anyhow!( @@ -484,16 +486,35 @@ impl SafeOutputs { Ok(true) } + #[cfg(test)] async fn new( bounding_directory: impl Into, output_directory: impl Into, enabled_tools: Option<&[String]>, + ) -> Result { + Self::new_with_self_repository_directory( + bounding_directory, + output_directory, + None, + enabled_tools, + ) + .await + } + + async fn new_with_self_repository_directory( + bounding_directory: impl Into, + output_directory: impl Into, + self_repository_directory: Option, + enabled_tools: Option<&[String]>, ) -> Result { let bounding_dir = bounding_directory.into(); let output_dir = output_directory.into(); + let self_repository_dir = + self_repository_directory.unwrap_or_else(|| bounding_dir.clone()); info!( - "Initializing SafeOutputs MCP server: bounding={}, output={}", + "Initializing SafeOutputs MCP server: bounding={}, self={}, output={}", bounding_dir.display(), + self_repository_dir.display(), output_dir.display() ); anyhow::ensure!( @@ -501,6 +522,11 @@ impl SafeOutputs { "bounding_directory: {:?} is not a valid path or directory", bounding_dir ); + anyhow::ensure!( + self_repository_dir.exists() && self_repository_dir.is_dir(), + "self_repository_directory: {:?} is not a valid path or directory", + self_repository_dir + ); anyhow::ensure!( output_dir.exists() && output_dir.is_dir(), "output_directory: {:?} is not a valid path or directory", @@ -524,18 +550,23 @@ impl SafeOutputs { Ok(Self { bounding_directory: bounding_dir, + self_repository_directory: self_repository_dir, output_directory: output_dir, tool_router, }) } /// Generate a git diff patch from a specific directory - /// If `repository` is Some, it's treated as a subdirectory of bounding_directory - /// If `repository` is None or "self", use bounding_directory directly + /// If `repository` is Some, it's treated as a subdirectory of bounding_directory. + /// If `repository` is None or "self", use the explicit self checkout. async fn generate_patch(&self, repository: Option<&str>) -> Result<(String, String), McpError> { use tokio::process::Command; - let git_dir = resolve_git_dir_for_patch(&self.bounding_directory, repository)?; + let git_dir = resolve_git_dir_for_patch( + &self.bounding_directory, + &self.self_repository_directory, + repository, + )?; // Generate patch using git format-patch for proper commit metadata, // rename detection, and binary file handling. @@ -1512,16 +1543,22 @@ impl ServerHandler for SafeOutputs { pub async fn run( output_directory: &str, bounding_directory: &str, + self_repository_directory: Option<&str>, enabled_tools: Option<&[String]>, ) -> Result<()> { // Create and run the server with STDIO transport - let service = SafeOutputs::new(bounding_directory, output_directory, enabled_tools) - .await? - .serve(stdio()) - .await - .inspect_err(|e| { - error!("Error starting MCP server: {}", e); - })?; + let service = SafeOutputs::new_with_self_repository_directory( + bounding_directory, + output_directory, + self_repository_directory.map(PathBuf::from), + enabled_tools, + ) + .await? + .serve(stdio()) + .await + .inspect_err(|e| { + error!("Error starting MCP server: {}", e); + })?; service .waiting() .await @@ -1542,6 +1579,28 @@ mod tests { (safe_outputs, temp_dir) } + #[test] + fn test_resolve_git_dir_for_patch_uses_explicit_self_checkout() { + let root = tempdir().unwrap(); + let self_dir = root.path().join("self-repo"); + let alias_dir = root.path().join("tools"); + std::fs::create_dir(&self_dir).unwrap(); + std::fs::create_dir(&alias_dir).unwrap(); + + assert_eq!( + resolve_git_dir_for_patch(root.path(), &self_dir, Some("self")).unwrap(), + self_dir + ); + assert_eq!( + resolve_git_dir_for_patch(root.path(), &self_dir, None).unwrap(), + self_dir + ); + assert_eq!( + resolve_git_dir_for_patch(root.path(), &self_dir, Some("tools")).unwrap(), + alias_dir.canonicalize().unwrap() + ); + } + #[test] fn test_slugify_title_basic() { assert_eq!(slugify_title("Fix bug in parser"), "fix-bug-in-parser"); diff --git a/src/safe_outputs/add_pr_comment.rs b/src/safe_outputs/add_pr_comment.rs index 30238308..f8f7d3f0 100644 --- a/src/safe_outputs/add_pr_comment.rs +++ b/src/safe_outputs/add_pr_comment.rs @@ -4,7 +4,7 @@ use log::{debug, info}; use percent_encoding::utf8_percent_encode; use schemars::JsonSchema; use serde::{Deserialize, Serialize}; -use std::path::{Path, PathBuf}; +use std::path::Path; use super::PATH_SEGMENT; use crate::safe_outputs::{ExecutionContext, ExecutionResult, Executor, Validate}; @@ -208,23 +208,6 @@ fn validate_file_path(path: &str) -> anyhow::Result<()> { Ok(()) } -fn repository_checkout_dir(repository: &str, ctx: &ExecutionContext) -> anyhow::Result { - if crate::safe_outputs::input_refers_to_self(repository, ctx) { - return Ok(ctx.source_directory.clone()); - } - - if let Some(alias) = - crate::safe_outputs::lookup_allowed_repository_alias(repository, &ctx.allowed_repositories) - { - return Ok(ctx.source_directory.join(alias)); - } - - anyhow::bail!( - "Repository alias '{}' not found in allowed repositories", - repository - ) -} - fn build_inline_thread_context( workspace_root: &Path, repo_root: &Path, @@ -415,7 +398,11 @@ impl Executor for AddPrCommentResult { if let Some(ref fp) = self.file_path { let end_line = self.line.unwrap_or(1); let start_line = self.start_line.unwrap_or(end_line); - let repo_root = match repository_checkout_dir(&self.repository, ctx).and_then(|path| { + let repo_root = match crate::safe_outputs::resolve_repository_checkout_dir( + &self.repository, + ctx, + ) + .and_then(|path| { crate::validate::ensure_path_within_base( &path, &ctx.source_directory, @@ -875,7 +862,11 @@ allowed-statuses: ..Default::default() }; - let resolved = repository_checkout_dir("4x4/sdk-ftdidevicecontrol", &ctx).unwrap(); + let resolved = crate::safe_outputs::resolve_repository_checkout_dir( + "4x4/sdk-ftdidevicecontrol", + &ctx, + ) + .unwrap(); assert_eq!(resolved, alias_dir); } @@ -899,7 +890,11 @@ allowed-statuses: ..Default::default() }; - let resolved = repository_checkout_dir("repo-sdk-ftdidevicecontrol", &ctx).unwrap(); + let resolved = crate::safe_outputs::resolve_repository_checkout_dir( + "repo-sdk-ftdidevicecontrol", + &ctx, + ) + .unwrap(); assert_eq!(resolved, alias_dir); } @@ -907,14 +902,17 @@ allowed-statuses: #[test] fn test_repository_checkout_dir_treats_empty_repository_as_self() { let workspace = tempdir().unwrap(); + let self_dir = workspace.path().join("current-repo"); let ctx = ExecutionContext { source_directory: workspace.path().to_path_buf(), + self_repository_directory: self_dir.clone(), repository_name: Some("4x4/current-repo".to_string()), ..Default::default() }; - let resolved = repository_checkout_dir("", &ctx).unwrap(); + let resolved = + crate::safe_outputs::resolve_repository_checkout_dir("", &ctx).unwrap(); - assert_eq!(resolved, workspace.path()); + assert_eq!(resolved, self_dir); } } diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 44f6812e..9c0aa092 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -643,34 +643,14 @@ impl Executor for CreatePrResult { "Validating repository '{}' against allowed list", self.repository ); - let repo_id = if crate::safe_outputs::input_refers_to_self(&self.repository, ctx) { - // "self" or a name match against the pipeline's own repository - debug!("Using 'self' repository (matched '{}')", self.repository); - ctx.repository_id - .as_ref() - .or(ctx.repository_name.as_ref()) - .context("Repository ID not configured for 'self'")? - .clone() - } else if let Some(ado_repo_name) = crate::safe_outputs::lookup_allowed_repository( - &self.repository, - &ctx.allowed_repositories, - ) { - // Matched against allowed list (by alias, full value, or trailing name) - debug!( - "Repository '{}' resolved to '{}'", - self.repository, ado_repo_name - ); - ado_repo_name.clone() - } else if ctx.allowed_repositories.is_empty() { - // No allowed_repositories configured - fall back to default repo (backward compat) - debug!("No allowed_repositories configured, using default repo"); - ctx.repository_id - .as_ref() - .or(ctx.repository_name.as_ref()) - .context("Repository ID not configured")? - .clone() - } else { - // Repository not in allowed list + let repository_alias = + crate::safe_outputs::canonical_repository_alias(&self.repository, ctx) + .or_else(|| { + ctx.allowed_repositories + .is_empty() + .then(|| "self".to_string()) + }); + let Some(repository_alias) = repository_alias else { warn!( "Repository '{}' not in allowed list: {:?}", self.repository, @@ -686,6 +666,26 @@ impl Executor for CreatePrResult { .join(", ") ))); }; + let repo_id = if repository_alias == "self" { + // "self" or a name match against the pipeline's own repository + debug!("Using 'self' repository (matched '{}')", self.repository); + ctx.repository_id + .as_ref() + .or(ctx.repository_name.as_ref()) + .context("Repository ID not configured for 'self'")? + .clone() + } else if let Some(ado_repo_name) = ctx.allowed_repositories.get(&repository_alias) { + debug!( + "Repository '{}' resolved through alias '{}' to '{}'", + self.repository, repository_alias, ado_repo_name + ); + ado_repo_name.clone() + } else { + return Ok(ExecutionResult::failure(format!( + "Repository alias '{}' has no configured repository", + repository_alias + ))); + }; debug!("Resolved repository ID: {}", repo_id); // Get ADO configuration @@ -830,20 +830,15 @@ impl Executor for CreatePrResult { // literal `target-branch`. Resolution is shared with the compiler's // prepare-pr-base deepening, so the branch we PR into is the branch that // was fetched/deepened. - let target_branch = config.resolve_target_branch(&self.repository, &ctx.repo_refs); + let target_branch = config.resolve_target_branch(&repository_alias, &ctx.repo_refs); let target_branch = target_branch.as_str(); let mut source_branch = self.source_branch.clone(); let mut source_ref = format!("refs/heads/{}", source_branch); let target_ref = format!("refs/heads/{}", target_branch); debug!("Source ref: {}, Target ref: {}", source_ref, target_ref); - // Determine the git repository directory from the source checkout - // For "self", use the source directory; for other repos, use the subdirectory - let repo_git_dir = if self.repository == "self" { - ctx.source_directory.clone() - } else { - ctx.source_directory.join(&self.repository) - }; + let repo_git_dir = + crate::safe_outputs::resolve_repository_checkout_dir(&repository_alias, ctx)?; debug!("Git repository directory: {}", repo_git_dir.display()); // Verify this is a git repository @@ -2559,6 +2554,31 @@ mod tests { assert_eq!(cfg.resolve_target_branch("other", &refs), "develop"); // scalar default } + #[test] + fn test_full_repository_name_canonicalizes_before_target_resolution() { + let ctx = ExecutionContext { + repository_name: Some("Project/meta".to_string()), + allowed_repositories: std::collections::HashMap::from([( + "tools".to_string(), + "Project/tools".to_string(), + )]), + repo_refs: std::collections::HashMap::from([( + "tools".to_string(), + "refs/heads/release".to_string(), + )]), + ..Default::default() + }; + let alias = + crate::safe_outputs::canonical_repository_alias("Project/tools", &ctx).unwrap(); + let cfg = CreatePrConfig { + infer_target_from_checkout_ref: true, + ..Default::default() + }; + + assert_eq!(alias, "tools"); + assert_eq!(cfg.resolve_target_branch(&alias, &ctx.repo_refs), "release"); + } + #[test] fn test_short_branch_strips_heads_prefix() { assert_eq!(short_branch("refs/heads/main"), "main"); @@ -3154,6 +3174,7 @@ index 0000000..abcdefg access_token: Some("fake-token".to_string()), github_token: None, source_directory: dir.path().to_path_buf(), + self_repository_directory: dir.path().to_path_buf(), working_directory: dir.path().to_path_buf(), tool_configs: std::collections::HashMap::new(), debug_enabled_tools: std::collections::HashSet::new(), diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index 34318db1..2b5f5781 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -181,34 +181,50 @@ pub(crate) async fn resolve_wiki_branch( /// 3. a case-insensitive match against the trailing repo-name part of the value /// (e.g. `sdk-FtdiDeviceControl` for `4x4/sdk-FtdiDeviceControl`). /// -/// Azure DevOps repository names are case-insensitive, so the trailing-name fallback -/// matches case-insensitively. Returns the resolved alias key on success, or `None` -/// if no entry matches. +/// Azure DevOps repository names are case-insensitive, so the name-based +/// fallbacks match case-insensitively. Returns the resolved alias key only when +/// the match is unique; ambiguous names are rejected. pub(crate) fn lookup_allowed_repository_alias<'a>( input: &str, allowed_repositories: &'a std::collections::HashMap, ) -> Option<&'a String> { + fn unique_alias<'a>( + mut matches: impl Iterator, + ) -> Option<&'a String> { + let first = matches.next()?; + if matches.next().is_some() { + return None; + } + Some(first) + } + // 1. Exact alias key match if let Some((alias, _)) = allowed_repositories.get_key_value(input) { return Some(alias); } - // 2. Case-insensitive value match (full "project/repo" or just "repo"). + // 2. Unique case-insensitive full-value match ("project/repo"). // ADO repo names are case-insensitive, so accept any case for the full path. - if let Some((alias, _)) = allowed_repositories - .iter() - .find(|(_, v)| v.eq_ignore_ascii_case(input)) - { + if let Some(alias) = unique_alias( + allowed_repositories + .iter() + .filter(|(_, value)| value.eq_ignore_ascii_case(input)) + .map(|(alias, _)| alias), + ) { return Some(alias); } - // 3. Trailing repo-name part match (case-insensitive) - allowed_repositories.iter().find_map(|(alias, v)| { - let trailing = v.rsplit('/').next().unwrap_or(v.as_str()); - if trailing.eq_ignore_ascii_case(input) { - Some(alias) - } else { - None - } - }) + // 3. Unique trailing repo-name match (case-insensitive). + unique_alias( + allowed_repositories + .iter() + .filter(|(_, value)| { + value + .rsplit('/') + .next() + .unwrap_or(value.as_str()) + .eq_ignore_ascii_case(input) + }) + .map(|(alias, _)| alias), + ) } /// Look up an ADO repo name in `allowed_repositories`, accepting either: @@ -250,6 +266,39 @@ pub(crate) fn input_refers_to_self(input: &str, ctx: &ExecutionContext) -> bool false } +/// Normalize a repository selector to the compiler/runtime alias key. +pub(crate) fn canonical_repository_alias( + repository: &str, + ctx: &ExecutionContext, +) -> Option { + if input_refers_to_self(repository, ctx) { + return Some("self".to_string()); + } + lookup_allowed_repository_alias(repository, &ctx.allowed_repositories).cloned() +} + +/// Resolve a repository selector to its checkout directory. +/// +/// The checkout root and `self` directory differ in multi-checkout jobs. +/// Named repositories are resolved through the configured alias map rather +/// than appended from untrusted selector text. +pub(crate) fn resolve_repository_checkout_dir( + repository: &str, + ctx: &ExecutionContext, +) -> anyhow::Result { + let Some(alias) = canonical_repository_alias(repository, ctx) else { + anyhow::bail!( + "Repository '{}' is not in the allowed repository list", + repository + ); + }; + if alias == "self" { + return Ok(ctx.self_repository_directory.clone()); + } + + Ok(ctx.source_directory.join(alias)) +} + /// Resolve a repository alias to its ADO repo name. /// /// Accepts `"self"` (or `None`) → `ctx.repository_name`, an alias key from @@ -262,13 +311,20 @@ pub(crate) fn resolve_repo_name( ctx: &ExecutionContext, ) -> Result { let alias = repo_alias.unwrap_or("self"); - if input_refers_to_self(alias, ctx) { + let Some(alias) = canonical_repository_alias(alias, ctx) else { + return Err(ExecutionResult::failure(format!( + "Repository '{}' is not in the allowed repository list", + alias + ))); + }; + if alias == "self" { return ctx .repository_name .clone() .ok_or_else(|| ExecutionResult::failure("BUILD_REPOSITORY_NAME not set")); } - lookup_allowed_repository(alias, &ctx.allowed_repositories) + ctx.allowed_repositories + .get(&alias) .cloned() .ok_or_else(|| { ExecutionResult::failure(format!( @@ -819,6 +875,21 @@ mod tests { ); } + #[test] + fn test_lookup_allowed_repository_rejects_ambiguous_bare_name() { + let allowed = std::collections::HashMap::from([ + ("tools-a".to_string(), "ProjectA/tools".to_string()), + ("tools-b".to_string(), "ProjectB/tools".to_string()), + ]); + + assert_eq!(lookup_allowed_repository_alias("tools", &allowed), None); + assert_eq!(lookup_allowed_repository("tools", &allowed), None); + assert_eq!( + lookup_allowed_repository_alias("ProjectA/tools", &allowed), + Some(&"tools-a".to_string()) + ); + } + // ─── resolve_repo_name ────────────────────────────────────────────── fn ctx_with( @@ -833,6 +904,42 @@ mod tests { } } + #[test] + fn test_resolve_repository_checkout_dir_distinguishes_root_and_self() { + let mut ctx = ctx_with(Some("4x4/current-repo"), sample_allowed()); + ctx.source_directory = std::path::PathBuf::from("checkout-root"); + ctx.self_repository_directory = + std::path::PathBuf::from("checkout-root").join("current-repo"); + + assert_eq!( + resolve_repository_checkout_dir("self", &ctx).unwrap(), + ctx.self_repository_directory + ); + assert_eq!( + resolve_repository_checkout_dir("4X4/CURRENT-REPO", &ctx).unwrap(), + ctx.self_repository_directory + ); + assert_eq!( + resolve_repository_checkout_dir("sdk-ftdidevicecontrol", &ctx).unwrap(), + ctx.source_directory.join("repo-sdk-ftdidevicecontrol") + ); + assert_eq!( + resolve_repository_checkout_dir("4x4/sdk-DeviceCommunication", &ctx).unwrap(), + ctx.source_directory.join("repo-sdk-devicecommunication") + ); + } + + #[test] + fn test_resolve_repository_checkout_dir_rejects_unknown_selector() { + let ctx = ctx_with(Some("4x4/current-repo"), sample_allowed()); + let err = resolve_repository_checkout_dir("../outside", &ctx).unwrap_err(); + assert!( + err.to_string() + .contains("not in the allowed repository list"), + "got: {err}" + ); + } + #[test] fn test_resolve_repo_name_self_literal() { let ctx = ctx_with(Some("4x4/sdk-FtdiDeviceControl"), sample_allowed()); diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 12057eea..17086799 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -67,6 +67,11 @@ pub struct ExecutionContext { pub working_directory: std::path::PathBuf, /// Source checkout directory (BUILD_SOURCESDIRECTORY) where git repos are checked out pub source_directory: std::path::PathBuf, + /// Exact checkout directory for the pipeline's `self` repository. + /// + /// In a multi-checkout job `BUILD_SOURCESDIRECTORY` is the common checkout + /// root, so the compiler passes this path explicitly. + pub self_repository_directory: std::path::PathBuf, /// Per-tool configuration, keyed by tool name pub tool_configs: HashMap, /// Debug-only tools (e.g. `create-issue`) that the operator authorized @@ -75,9 +80,13 @@ pub struct ExecutionContext { /// whose tool name is absent from this set — otherwise a forged entry /// could bypass the MCP-layer default-deny gate. Empty by default. pub debug_enabled_tools: HashSet, - /// Repository ID (from BUILD_REPOSITORY_ID) + /// Exact `self` repository ID. Compiled pipelines provide + /// `ADO_AW_SELF_REPOSITORY_ID`; direct/legacy invocations fall back to + /// `BUILD_REPOSITORY_ID`. pub repository_id: Option, - /// Repository name (from BUILD_REPOSITORY_NAME) + /// Exact `self` repository name. Compiled pipelines provide + /// `ADO_AW_SELF_REPOSITORY_NAME`; direct/legacy invocations fall back to + /// `BUILD_REPOSITORY_NAME`. pub repository_name: Option, /// Allowed repositories for PRs: "self" + checkout list aliases /// Maps alias to ADO repo name (e.g., "other-repo" -> "org/other-repo") @@ -239,6 +248,9 @@ impl ExecutionContext { let source_directory = env("BUILD_SOURCESDIRECTORY") .map(std::path::PathBuf::from) .unwrap_or_else(|| std::env::current_dir().unwrap_or_default()); + let self_repository_directory = env("ADO_AW_SELF_REPOSITORY_DIRECTORY") + .map(std::path::PathBuf::from) + .unwrap_or_else(|| source_directory.clone()); Self { ado_org_url, @@ -249,10 +261,13 @@ impl ExecutionContext { github_token: env("ADO_AW_DEBUG_GITHUB_TOKEN"), working_directory: std::env::current_dir().unwrap_or_default(), source_directory, + self_repository_directory, tool_configs: HashMap::new(), debug_enabled_tools: HashSet::new(), - repository_id: env("BUILD_REPOSITORY_ID"), - repository_name: env("BUILD_REPOSITORY_NAME"), + repository_id: env("ADO_AW_SELF_REPOSITORY_ID") + .or_else(|| env("BUILD_REPOSITORY_ID")), + repository_name: env("ADO_AW_SELF_REPOSITORY_NAME") + .or_else(|| env("BUILD_REPOSITORY_NAME")), allowed_repositories: HashMap::new(), repo_refs: HashMap::new(), agent_stats: None, @@ -900,6 +915,66 @@ mod tests { assert_eq!(ctx.source_version.as_deref(), Some("abc1234")); } + #[test] + fn test_from_env_lookup_populates_checkout_directories() { + let ctx = ExecutionContext::from_env_lookup(env_from(&[ + ("BUILD_SOURCESDIRECTORY", "C:\\agent\\s"), + ( + "ADO_AW_SELF_REPOSITORY_DIRECTORY", + "C:\\agent\\s\\ado-aw", + ), + ])); + + assert_eq!( + ctx.source_directory, + std::path::PathBuf::from("C:\\agent\\s") + ); + assert_eq!( + ctx.self_repository_directory, + std::path::PathBuf::from("C:\\agent\\s\\ado-aw") + ); + } + + #[test] + fn test_from_env_lookup_self_directory_falls_back_to_source_directory() { + let ctx = ExecutionContext::from_env_lookup(env_from(&[( + "BUILD_SOURCESDIRECTORY", + "C:\\agent\\s", + )])); + + assert_eq!(ctx.self_repository_directory, ctx.source_directory); + } + + #[test] + fn test_from_env_lookup_prefers_compiler_owned_self_identity() { + let ctx = ExecutionContext::from_env_lookup(env_from(&[ + ("ADO_AW_SELF_REPOSITORY_ID", "self-id"), + ("ADO_AW_SELF_REPOSITORY_NAME", "project/self-repo"), + ("BUILD_REPOSITORY_ID", "trigger-id"), + ("BUILD_REPOSITORY_NAME", "project/trigger-repo"), + ])); + + assert_eq!(ctx.repository_id.as_deref(), Some("self-id")); + assert_eq!( + ctx.repository_name.as_deref(), + Some("project/self-repo") + ); + } + + #[test] + fn test_from_env_lookup_self_identity_falls_back_to_build_variables() { + let ctx = ExecutionContext::from_env_lookup(env_from(&[ + ("BUILD_REPOSITORY_ID", "build-id"), + ("BUILD_REPOSITORY_NAME", "project/build-repo"), + ])); + + assert_eq!(ctx.repository_id.as_deref(), Some("build-id")); + assert_eq!( + ctx.repository_name.as_deref(), + Some("project/build-repo") + ); + } + #[test] fn test_from_env_lookup_build_id_none_for_non_numeric() { let ctx = ExecutionContext::from_env_lookup(env_from(&[("BUILD_BUILDID", "not-a-number")])); diff --git a/src/safe_outputs/upload_build_attachment.rs b/src/safe_outputs/upload_build_attachment.rs index d343d261..02de0f8b 100644 --- a/src/safe_outputs/upload_build_attachment.rs +++ b/src/safe_outputs/upload_build_attachment.rs @@ -913,6 +913,7 @@ attachment-type: "agent-artifact" access_token: None, github_token: None, source_directory: working_directory.clone(), + self_repository_directory: working_directory.clone(), working_directory, tool_configs: std::collections::HashMap::new(), debug_enabled_tools: std::collections::HashSet::new(), diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 1b4760f6..0eaa068a 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -6244,40 +6244,35 @@ fn test_execution_context_pr_emits_prepare_step_and_prompt_supplement() { // The Stage step's own env block must NOT contain a direct // `dependencies.Setup.outputs[...]` reference. (The same expression // IS expected at Agent-job-level `variables:` scope, the documented - // safe location — that hoist is asserted separately.) Scope this - // check by isolating the Stage step's bash + env body. - let stage_step = compiled - .split("Stage PR execution context") - .nth(1) - .map(|tail| { - // Stop at the next step (`- bash:` / `- task:` / `- script:`) - // or end of the job (a less-indented key). - let stop_at = ["\n - bash:", "\n - task:", "\n - script:"]; - let end = stop_at - .iter() - .filter_map(|needle| tail.find(needle)) - .min() - .unwrap_or(tail.len()); - &tail[..end] - }) - .unwrap_or(""); + // safe location — that hoist is asserted separately.) + let parsed = parse_compiled_yaml(&compiled); + let stage_step = find_job_mapping_by_display_name( + &parsed, + "Stage PR execution context (aw-context/pr/*)", + ) + .expect("compiled YAML must contain the PR execution-context step"); + let stage_env = stage_step + .get(yaml_key("env")) + .and_then(|value| value.as_mapping()) + .expect("PR execution-context step must have an env block"); + let stage_env = serde_yaml::to_string(stage_env).expect("stage env must serialize"); assert!( - !stage_step.contains("dependencies.Setup.outputs['synthPr."), + !stage_env.contains("dependencies.Setup.outputs['synthPr."), "Stage step's own env block must NOT reference \ `dependencies.Setup.outputs[...]` — that is cross-job syntax. The cross-job \ output is hoisted into Agent-job-level `variables:` (see \ `generate_agent_job_variables`) and the Stage step reads it via the \ - `$(AW_PR_*)` macros. Stage step body: {stage_step}" + `$(AW_PR_*)` macros. Stage step env: {stage_env}" ); // ADO does NOT evaluate `$[ ... ]` runtime expressions inside step // `env:` values — only inside `variables:` mappings and // `condition:` fields. Any `$[ ` in this step's env block would be // passed through to bash as a literal string (the bug fixed here). assert!( - !stage_step.contains("$["), + !stage_env.contains("$["), "Stage step's env block must not contain `$[ ` runtime expressions \ (ADO doesn't evaluate them at step-env scope). Use the Agent-job-level \ - `variables:` hoist + `$(name)` macros instead. Stage step body: {stage_step}" + `variables:` hoist + `$(name)` macros instead. Stage step env: {stage_env}" ); assert!( compiled.contains("SYSTEM_ACCESSTOKEN: $(System.AccessToken)"), @@ -9096,9 +9091,8 @@ fn test_create_pull_request_prepare_step_defaults_target_branch() { } /// Multi-repo: with additional `checkout:` repos, the prepare step deepens the -/// target branch in the self working dir AND each alias dir — mirroring -/// `mcp.rs::resolve_git_dir_for_patch` so a PR to any allowed repo works on a -/// shallow pool. +/// target branch in the exact self checkout AND each alias checkout so a PR to +/// any allowed repo works on a shallow pool. #[test] fn test_create_pull_request_prepare_step_covers_all_checkout_repos() { let compiled = compile_inline_agent( @@ -9106,15 +9100,15 @@ fn test_create_pull_request_prepare_step_covers_all_checkout_repos() { "---\nname: \"Multi PR Agent\"\ndescription: \"opens PRs across repos\"\nworkspace: root\nrepos:\n - my-org/tools\n - my-org/lib\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", ); let agent = job_block(&compiled, "Agent"); - // self (working_directory == $(Build.SourcesDirectory) under workspace: root) - // + one dir per derived alias (my-org/tools -> tools, my-org/lib -> lib). + // self uses the compiler-owned multi-checkout path; aliases are siblings + // under the checkout root (my-org/tools -> tools, my-org/lib -> lib). assert_eq!( agent.matches("--repo-dir ").count(), 3, "multi-repo agent must emit one --repo-dir per allowed repo (self + 2 aliases):\n{agent}" ); assert!( - agent.contains("--repo-dir \"$(Build.SourcesDirectory)\""), + agent.contains("--repo-dir \"$(Build.SourcesDirectory)/self\""), "self dir must be covered:\n{agent}" ); assert!( @@ -9139,7 +9133,7 @@ fn test_create_pull_request_prepare_step_per_repo_targets() { let agent = job_block(&compiled, "Agent"); // self ⇒ literal default 'main' (self never infers). assert!( - agent.contains("--repo-dir \"$(Build.SourcesDirectory)\" --target-branch 'main'"), + agent.contains("--repo-dir \"$(Build.SourcesDirectory)/self\" --target-branch 'main'"), "self must target the literal default 'main':\n{agent}" ); // tools ⇒ inferred from its checkout ref (refs/heads/release → release). @@ -9204,7 +9198,7 @@ fn test_create_pull_request_safeoutputs_prepare_step_covers_all_checkout_repos() ); let safeoutputs = job_block(&compiled, "SafeOutputs"); assert!( - safeoutputs.contains("--repo-dir \"$(Build.SourcesDirectory)\" --target-branch 'main'"), + safeoutputs.contains("--repo-dir \"$(Build.SourcesDirectory)/self\" --target-branch 'main'"), "self must target the literal default 'main' in the SafeOutputs job:\n{safeoutputs}" ); assert!( @@ -9299,28 +9293,28 @@ fn test_no_create_pull_request_omits_prepare_pr_base_step() { /// checked-out repos, the SafeOutputs job must emit a `checkout` step for each /// `checkout: true` repo so that: /// • The additional repo directories exist before `prepare-pr-base.js` runs. -/// • ADO uses the multi-checkout workspace layout, placing `self` at -/// `$(Build.SourcesDirectory)/$(Build.Repository.Name)` rather than -/// directly at `$(Build.SourcesDirectory)`. +/// • `self` stays at the compiler-owned `$(Build.SourcesDirectory)/self` +/// path regardless of trigger metadata or repository aliases. /// /// A `checkout: false` repo must remain resource-only (no checkout step). #[test] fn test_issue_1731_safeoutputs_checks_out_additional_repos_for_create_pr() { let compiled = compile_inline_agent( "issue-1731-multi-checkout", - "---\nname: \"Multi-repo PR\"\ndescription: \"Opens a PR in an additional repo\"\nworkspace: root\nrepos:\n - my-org/tools\n - my-org/docs\n - name: my-org/scripts\n checkout: false\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + "---\nname: \"Multi-repo PR\"\ndescription: \"Opens a PR in an additional repo\"\nworkspace: root\nrepos:\n - name: my-org/tools\n alias: build-tools\n - my-org/docs\n - name: my-org/scripts\n checkout: false\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", ); let agent = job_block(&compiled, "Agent"); let safeoutputs = job_block(&compiled, "SafeOutputs"); // Agent job: self + both checked-out repos. assert!( - agent.contains("- checkout: self"), - "Agent must check out self:\n{agent}" + agent.contains("- checkout: self") && agent.contains("path: s/self"), + "Agent must check out self at its fixed multi-checkout path:\n{agent}" ); assert!( - agent.contains("- checkout: tools"), - "Agent must check out tools:\n{agent}" + agent.contains("- checkout: build-tools") + && agent.contains("path: s/build-tools"), + "Agent must check out tools at its explicit alias path:\n{agent}" ); assert!( agent.contains("- checkout: docs"), @@ -9334,12 +9328,13 @@ fn test_issue_1731_safeoutputs_checks_out_additional_repos_for_create_pr() { // SafeOutputs job: same three checkouts must precede prepare-pr-base.js. assert!( - safeoutputs.contains("- checkout: self"), - "SafeOutputs must check out self:\n{safeoutputs}" + safeoutputs.contains("- checkout: self") && safeoutputs.contains("path: s/self"), + "SafeOutputs must check out self at its fixed multi-checkout path:\n{safeoutputs}" ); assert!( - safeoutputs.contains("- checkout: tools"), - "SafeOutputs must check out tools (issue #1731):\n{safeoutputs}" + safeoutputs.contains("- checkout: build-tools") + && safeoutputs.contains("path: s/build-tools"), + "SafeOutputs must check out tools at its explicit alias path (issue #1731):\n{safeoutputs}" ); assert!( safeoutputs.contains("- checkout: docs"), @@ -9350,10 +9345,14 @@ fn test_issue_1731_safeoutputs_checks_out_additional_repos_for_create_pr() { !safeoutputs.contains("- checkout: scripts"), "SafeOutputs must NOT check out scripts (checkout: false):\n{safeoutputs}" ); + assert!( + safeoutputs.contains("--repo-dir \"$(Build.SourcesDirectory)/build-tools\""), + "prepare-pr-base must use the explicit alias checkout path:\n{safeoutputs}" + ); // Checkouts must appear before the prepare-pr-base.js invocation. let tools_checkout_at = safeoutputs - .find("- checkout: tools") + .find("- checkout: build-tools") .expect("tools checkout present"); let prepare_at = safeoutputs .find("prepare-pr-base.js") @@ -9364,12 +9363,29 @@ fn test_issue_1731_safeoutputs_checks_out_additional_repos_for_create_pr() { ); } +#[test] +fn test_issue_1731_fixed_self_path_cannot_collide_with_repository_alias() { + let compiled = compile_inline_agent( + "issue-1731-self-alias-collision", + "---\nname: \"Collision proof\"\ndescription: \"Keeps self separate from a same-named alias\"\nrepos:\n - name: another-project/orchestrator\n alias: orchestrator\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + ); + let agent = job_block(&compiled, "Agent"); + let safeoutputs = job_block(&compiled, "SafeOutputs"); + + for job in [agent, safeoutputs] { + assert!( + job.contains("- checkout: self") + && job.contains("path: s/self") + && job.contains("- checkout: orchestrator") + && job.contains("path: s/orchestrator"), + "self and the named checkout must use distinct explicit paths:\n{job}" + ); + } +} + /// Issue #1731: with additional checked-out repos the executor `--source` path -/// uses the multi-checkout layout `$(Build.SourcesDirectory)/$(Build.Repository.Name)/...` -/// (because `trigger_repo_directory` expands to the subdirectory form when -/// `checkout` is non-empty). The SafeOutputs job's additional checkout steps -/// are what forces ADO to apply that layout rather than placing `self` directly -/// at `$(Build.SourcesDirectory)`. +/// uses the compiler-owned multi-checkout layout +/// `$(Build.SourcesDirectory)/self/...`. #[test] fn test_issue_1731_safeoutputs_executor_source_path_uses_multi_checkout_layout() { let compiled = compile_inline_agent( @@ -9377,11 +9393,24 @@ fn test_issue_1731_safeoutputs_executor_source_path_uses_multi_checkout_layout() "---\nname: \"Multi-repo PR\"\ndescription: \"Opens a PR in an additional repo\"\nworkspace: root\nrepos:\n - my-org/tools\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", ); let safeoutputs = job_block(&compiled, "SafeOutputs"); - // With additional repos, self is placed at $(Build.SourcesDirectory)/$(Build.Repository.Name). + // With additional repos, self is pinned to $(Build.SourcesDirectory)/self. assert!( - safeoutputs.contains("$(Build.Repository.Name)"), + safeoutputs + .contains("ado-aw execute --source \"$(Build.SourcesDirectory)/self/"), "SafeOutputs executor --source must use the multi-checkout layout path:\n{safeoutputs}" ); + assert!( + safeoutputs + .contains("ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self"), + "SafeOutputs must pass the exact self checkout to the executor:\n{safeoutputs}" + ); + assert!( + safeoutputs.contains("resources.repositories['self'].id") + && safeoutputs.contains("resources.repositories['self'].name") + && safeoutputs.contains("ADO_AW_SELF_REPOSITORY_ID:") + && safeoutputs.contains("ADO_AW_SELF_REPOSITORY_NAME:"), + "SafeOutputs must use self resource metadata rather than trigger-scoped Build.Repository.*:\n{safeoutputs}" + ); } /// Issue #1731, split-approval variant: when `create-pull-request` is @@ -9409,6 +9438,27 @@ fn test_issue_1731_split_approval_additional_checkouts_only_in_pr_variant() { !auto.contains("- checkout: tools"), "auto SafeOutputs must NOT check out additional repos (it never runs create-pull-request):\n{auto}" ); + assert!( + auto.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/") + && !auto.contains( + "ado-aw execute --source \"$(Build.SourcesDirectory)/self/" + ), + "self-only SafeOutputs must use its single-checkout source path:\n{auto}" + ); + assert!( + auto.contains("ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)") + && !auto.contains( + "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self" + ), + "self-only SafeOutputs must pass its checkout root as the self repo:\n{auto}" + ); + assert!( + reviewed.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/self/") + && reviewed.contains( + "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self" + ), + "PR-capable reviewed job must use its multi-checkout self path:\n{reviewed}" + ); } /// Mirror of the split-approval case: when `create-pull-request` is NOT gated @@ -9435,6 +9485,61 @@ fn test_issue_1731_split_approval_additional_checkouts_in_auto_when_sibling_gate !reviewed.contains("- checkout: tools"), "SafeOutputs_Reviewed must NOT check out additional repos when it doesn't run create-pull-request:\n{reviewed}" ); + assert!( + auto.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/self/") + && auto.contains( + "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self" + ), + "PR-capable automatic job must use its multi-checkout self path:\n{auto}" + ); + assert!( + reviewed.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/") + && !reviewed.contains( + "ado-aw execute --source \"$(Build.SourcesDirectory)/self/" + ), + "self-only reviewed job must use its single-checkout source path:\n{reviewed}" + ); + assert!( + reviewed.contains("ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)") + && !reviewed.contains( + "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)/self" + ), + "self-only reviewed job must pass its checkout root as the self repo:\n{reviewed}" + ); +} + +#[test] +fn test_issue_1731_split_checkout_layout_compiles_for_every_target() { + for target in ["standalone", "1es", "job", "stage"] { + let compiled = compile_inline_agent( + &format!("issue-1731-{target}"), + &format!( + "---\nname: \"Split PR {target}\"\ndescription: \"Cross-target split checkout layout\"\ntarget: {target}\nworkspace: root\nrepos:\n - my-org/tools\nsafe-outputs:\n add-build-tag:\n tag: ci\n create-pull-request:\n target-branch: main\n require-approval: true\n---\n\n## Agent\n\nDo work.\n" + ), + ); + + assert_eq!( + compiled.matches("- checkout: tools").count(), + 2, + "{target}: tools must be checked out in Agent and the PR-capable Stage 3 job only:\n{compiled}" + ); + assert!( + compiled.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/self/"), + "{target}: PR-capable Stage 3 source must use multi-checkout layout:\n{compiled}" + ); + assert!( + compiled.contains("resources.repositories['self'].id") + && compiled.contains("resources.repositories['self'].name"), + "{target}: Stage 3 must use self resource metadata:\n{compiled}" + ); + assert!( + compiled.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/") + && compiled.contains( + "ADO_AW_SELF_REPOSITORY_DIRECTORY: $(Build.SourcesDirectory)" + ), + "{target}: self-only Stage 3 sibling must use single-checkout layout:\n{compiled}" + ); + } } #[test] diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index 43a23f1f..7ed12750 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -56,7 +56,10 @@ All deterministically-assertable ADO-write safe outputs plus the flagship - **Git:** `create-branch`, `create-git-tag` - **Build:** `add-build-tag`, `queue-build`, `upload-build-attachment`, `upload-pipeline-artifact` -- **Flagship:** `create-pull-request` +- **Flagship:** `create-pull-request` covers both a named additional checkout at + `/` and `repository: self` beneath a non-Git + multi-checkout root, with compiler-owned self repository identity taking + precedence over trigger-scoped `BUILD_REPOSITORY_*` values. Excluded (out of scope or GitHub-only): the GitHub-only `create-issue`. From 39480fed48738f9f7f2a3f2fccb5140fe674d45d Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 30 Jul 2026 19:58:32 +0100 Subject: [PATCH 4/7] docs(safe-outputs): clarify repository resolution and self-path invariants Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 10ccd9bb-6fb7-4a24-8e36-d2ff13031733 --- src/compile/agentic_pipeline.rs | 18 ++++++++++++++++++ src/safe_outputs/create_pull_request.rs | 8 ++++++++ src/safe_outputs/mod.rs | 21 +++++++++++++++++++++ 3 files changed, 47 insertions(+) diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 33b4c751..8a1fb205 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -1383,8 +1383,20 @@ impl SafeOutputsCheckoutLayout { let has_additional_checkouts = variant.runs_create_pull_request && !front_matter.checkout.is_empty(); let self_repository_directory = if has_additional_checkouts { + // This job emits the same multi-checkout layout as the Agent job, so + // `self` sits at the compiler-owned `MULTI_CHECKOUT_SELF_PATH`. That + // is exactly what `generate_trigger_repo_directory` produces for a + // non-empty checkout list, which is how `cfg.trigger_repo_directory` + // was built — assert the two stay in agreement. + debug_assert_eq!( + cfg.trigger_repo_directory, + common::MULTI_CHECKOUT_SELF_DIRECTORY, + "multi-checkout self directory must match the fixed `s/self` path" + ); cfg.trigger_repo_directory.clone() } else { + // Only `checkout: self` runs here, so ADO places it at the root + // regardless of what the workflow-wide layout looks like. common::generate_trigger_repo_directory(&[]) }; let source_path = format!( @@ -3167,6 +3179,12 @@ fn execute_safe_outputs_step( } step = step.with_env( "ADO_AW_SELF_REPOSITORY_DIRECTORY", + // The value embeds `$(Build.SourcesDirectory)`, but it is still a + // `Literal`: ADO expands `$(...)` macros in step `env:` values at agent + // runtime, so the macro reaches the executor already resolved. + // `EnvValue::AdoMacro` is for values that are *only* a macro; this one + // is a macro-plus-suffix path, and `Concat` would add no value because + // no part of it needs separate lowering. EnvValue::literal(self_repository_directory), ); step = step.with_env( diff --git a/src/safe_outputs/create_pull_request.rs b/src/safe_outputs/create_pull_request.rs index 9c0aa092..b663d9ca 100644 --- a/src/safe_outputs/create_pull_request.rs +++ b/src/safe_outputs/create_pull_request.rs @@ -681,6 +681,14 @@ impl Executor for CreatePrResult { ); ado_repo_name.clone() } else { + // Unreachable: `repository_alias` is either "self" (handled above) + // or a key produced by iterating `ctx.allowed_repositories`, so the + // lookup cannot miss. Kept as a fail-closed guard in case the + // canonicalization and the map ever drift apart. + debug_assert!( + false, + "canonical alias '{repository_alias}' is absent from allowed_repositories" + ); return Ok(ExecutionResult::failure(format!( "Repository alias '{}' has no configured repository", repository_alias diff --git a/src/safe_outputs/mod.rs b/src/safe_outputs/mod.rs index 2b5f5781..417c1a87 100644 --- a/src/safe_outputs/mod.rs +++ b/src/safe_outputs/mod.rs @@ -267,6 +267,19 @@ pub(crate) fn input_refers_to_self(input: &str, ctx: &ExecutionContext) -> bool } /// Normalize a repository selector to the compiler/runtime alias key. +/// +/// Accepts a raw agent-supplied selector (`"self"`, `""`, an alias key, a full +/// `project/repo` value, or a bare repo name) and returns the canonical alias +/// key — `"self"` or a key of `ctx.allowed_repositories`. +/// +/// **Idempotent**: passing an already-canonical alias returns it unchanged +/// (`"self"` short-circuits on [`input_refers_to_self`]; an alias key hits the +/// exact-key arm of [`lookup_allowed_repository_alias`]), so callers may +/// canonicalize defensively without changing the result. +/// +/// **Precedence**: self-identity wins over the alias map. A selector matching +/// the pipeline repository's name resolves to `"self"` even if an alias of the +/// same name is configured for a different repository. pub(crate) fn canonical_repository_alias( repository: &str, ctx: &ExecutionContext, @@ -282,6 +295,14 @@ pub(crate) fn canonical_repository_alias( /// The checkout root and `self` directory differ in multi-checkout jobs. /// Named repositories are resolved through the configured alias map rather /// than appended from untrusted selector text. +/// +/// `repository` may be **either** a raw agent-supplied selector or an alias +/// already canonicalized by [`canonical_repository_alias`]; both are supported +/// because that helper is idempotent. `add-pr-comment` passes the raw value +/// straight from the agent, while `create-pull-request` canonicalizes first so +/// it can reuse the alias for target-branch resolution. Callers must not build +/// the path themselves — routing every selector through here is what keeps +/// untrusted text out of the path join. pub(crate) fn resolve_repository_checkout_dir( repository: &str, ctx: &ExecutionContext, From cdab52c9cc90fa9938dc3b4c715ed88af1350bae Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 30 Jul 2026 21:49:11 +0100 Subject: [PATCH 5/7] fix(compile): resolve self repository identity at compile time Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 10ccd9bb-6fb7-4a24-8e36-d2ff13031733 --- docs/front-matter.md | 9 ++ .../scenarios/create-pull-request.ts | 17 +--- src/compile/agentic_pipeline.rs | 43 ++++++++- src/safe_outputs/result.rs | 52 +++++++++-- tests/compiler_tests.rs | 93 +++++++++++++++---- tests/executor-e2e/README.md | 5 +- 6 files changed, 177 insertions(+), 42 deletions(-) diff --git a/docs/front-matter.md b/docs/front-matter.md index 789bee21..b2347f2b 100644 --- a/docs/front-matter.md +++ b/docs/front-matter.md @@ -389,6 +389,15 @@ Reference the explicit ADO path instead: when one or more additional repositories are checked out. - `$(Build.SourcesDirectory)/` — a specific checked-out repository. +> **`self` identity.** The compiler resolves the `self` repository's name at +> compile time from the Azure DevOps git remote (or +> `ADO_AW_COMPILE_REMOTE_URL`) and bakes it into the compiled pipeline, so +> safe outputs targeting `repository: self` never depend on +> `Build.Repository.*` — which names the *triggering* repository and differs +> from `self` on repository-resource-triggered runs. If no ADO remote can be +> resolved at compile time the compiler warns and falls back to +> `$(Build.Repository.Name)`; compile from an Azure DevOps clone to avoid it. + The `legacy_path_markers` codemod automatically rewrites any remaining markers in front matter to the path they resolved to on the next `compile` (see [`docs/codemods.md`](codemods.md)). Markers left in the **agent body** cannot be diff --git a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts index fe1d3a1c..dc634679 100644 --- a/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts +++ b/scripts/ado-script/src/executor-e2e/scenarios/create-pull-request.ts @@ -42,8 +42,6 @@ interface CreatePrState { sourcesDir: string; /** Actual git checkout beneath sourcesDir. */ checkoutDir: string; - /** ADO repository id, required when the executor selector is `self`. */ - repositoryId?: string; /** PR id, populated in assert() so cleanup can abandon it. */ prId?: number; } @@ -131,10 +129,6 @@ async function setupCreatePullRequest( const sourcesDir = join(ctx.workDir, options.id, "src-checkout"); await mkdir(sourcesDir, { recursive: true }); const checkoutDir = join(sourcesDir, repo); - const repositoryId = - options.repositorySelector === "self" - ? (await ctx.rest.getRepository(repo)).id - : undefined; const cloneUrl = `${ctx.orgUrl.replace(/\/+$/, "")}/${encodeURIComponent(ctx.project)}/_git/${encodeURIComponent(repo)}`; ctx.log(`[${options.id}] cloning ${repo}`); @@ -194,7 +188,6 @@ async function setupCreatePullRequest( patchContent, sourcesDir, checkoutDir, - repositoryId, }; } @@ -221,16 +214,16 @@ function createPullRequestScenario( BUILD_SOURCESDIRECTORY: state.sourcesDir, }; if (options.repositorySelector === "self") { - if (!state.repositoryId) { - throw new Error("self create-pull-request scenario has no ADO repository id"); - } Object.assign(env, { ADO_AW_SELF_REPOSITORY_DIRECTORY: state.checkoutDir, - ADO_AW_SELF_REPOSITORY_ID: state.repositoryId, + // Compiled pipelines identify `self` by name only; the executor + // resolves an ID from it where one is needed. Supplying just the + // name here mirrors what the compiler actually emits. ADO_AW_SELF_REPOSITORY_NAME: state.repo, // Model a repository-resource-triggered run: Build.Repository.* // identifies the trigger, while the compiler-owned values above - // must continue to identify checkout: self. + // must continue to identify checkout: self. The bogus ID must be + // ignored entirely rather than paired with the self name. BUILD_REPOSITORY_ID: "00000000-0000-0000-0000-000000000000", BUILD_REPOSITORY_NAME: "external-trigger-repository", }); diff --git a/src/compile/agentic_pipeline.rs b/src/compile/agentic_pipeline.rs index 8a1fb205..8c1ab1d1 100644 --- a/src/compile/agentic_pipeline.rs +++ b/src/compile/agentic_pipeline.rs @@ -172,6 +172,35 @@ pub(crate) fn build_pipeline_context( )?; let working_directory = common::generate_working_directory(&effective_workspace); let trigger_repo_directory = common::generate_trigger_repo_directory(&front_matter.checkout); + // Identity of the `self` repository, resolved at compile time. + // + // `self` is the repository this workflow is compiled in, and (because the + // executor's `--source` resolves beneath the `self` checkout) also the + // repository whose pipeline runs it — for template targets that is the + // parent pipeline's repository. The compiler therefore already knows the + // name, and the `ado-aw-marker` extension already bakes it into the lock, + // so `ado-aw check` fails loudly if the baked value ever drifts from the + // repository the pipeline actually runs in. + // + // The `$(Build.Repository.Name)` fallback is deliberately last: it names + // the *triggering* repository, which differs from `self` on + // repository-resource-triggered runs (issue #1731). Warn rather than fail, + // because compiling outside an ADO clone is a supported developer path. + let self_repository_name = match ctx.ado_context.as_ref() { + Some(ado) => EnvValue::literal(ado.repo_name.clone()), + None => { + eprintln!( + "Warning: could not resolve the Azure DevOps repository for agent '{}' \ + (no ADO git remote and no ADO_AW_COMPILE_REMOTE_URL). Falling back to \ + $(Build.Repository.Name) for the 'self' repository identity, which names \ + the TRIGGERING repository — safe-outputs targeting `repository: self` may \ + resolve to the wrong repository on repository-resource-triggered runs. \ + Compile from an Azure DevOps clone, or set ADO_AW_COMPILE_REMOTE_URL.", + front_matter.name + ); + EnvValue::ado_macro("Build.Repository.Name")? + } + }; let pools = common::resolve_pool_overrides_typed( front_matter.target.clone(), front_matter.pool.as_ref(), @@ -392,6 +421,7 @@ pub(crate) fn build_pipeline_context( .unwrap_or_default(), working_directory: working_directory.clone(), trigger_repo_directory: trigger_repo_directory.clone(), + self_repository_name, compiler_version: compiler_version.clone(), engine_install_steps_yaml, detection_engine_install_steps_yaml, @@ -563,6 +593,10 @@ pub(crate) struct StandaloneCtx { pub(crate) self_checkout_fetch: CheckoutFetchOpts, pub(crate) working_directory: String, pub(crate) trigger_repo_directory: String, + /// Identity of the `self` repository, resolved at compile time from the ADO + /// git remote. Falls back to the `$(Build.Repository.Name)` macro (with a + /// compile warning) when no ADO context is available. + pub(crate) self_repository_name: EnvValue, pub(crate) compiler_version: String, /// Engine install steps as a YAML string (`Engine::install_steps` /// returns YAML today). Lowered through `Step::RawYaml` because @@ -1639,6 +1673,7 @@ fn build_safeoutputs_job( steps.push(Step::Bash(execute_safe_outputs_step( &layout.source_path, &layout.self_repository_directory, + &cfg.self_repository_name, &cfg.executor_ado_env, &variant.filter_args, )?)); @@ -3158,6 +3193,7 @@ fn run_agent_step( fn execute_safe_outputs_step( source_path: &str, self_repository_directory: &str, + self_repository_name: &EnvValue, executor_ado_env: &str, filter_args: &str, ) -> Result { @@ -3187,13 +3223,9 @@ fn execute_safe_outputs_step( // no part of it needs separate lowering. EnvValue::literal(self_repository_directory), ); - step = step.with_env( - "ADO_AW_SELF_REPOSITORY_ID", - EnvValue::runtime_expression("resources.repositories['self'].id"), - ); step = step.with_env( "ADO_AW_SELF_REPOSITORY_NAME", - EnvValue::runtime_expression("resources.repositories['self'].name"), + self_repository_name.clone(), ); Ok(step) } @@ -4184,6 +4216,7 @@ mod tests { trigger_repo_directory: super::super::common::generate_trigger_repo_directory( &fm.checkout, ), + self_repository_name: EnvValue::literal("test-repo"), compiler_version: "0.0.0-test".to_string(), engine_install_steps_yaml: String::new(), detection_engine_install_steps_yaml: String::new(), diff --git a/src/safe_outputs/result.rs b/src/safe_outputs/result.rs index 17086799..c4874c74 100644 --- a/src/safe_outputs/result.rs +++ b/src/safe_outputs/result.rs @@ -80,9 +80,15 @@ pub struct ExecutionContext { /// whose tool name is absent from this set — otherwise a forged entry /// could bypass the MCP-layer default-deny gate. Empty by default. pub debug_enabled_tools: HashSet, - /// Exact `self` repository ID. Compiled pipelines provide - /// `ADO_AW_SELF_REPOSITORY_ID`; direct/legacy invocations fall back to - /// `BUILD_REPOSITORY_ID`. + /// Exact `self` repository ID. + /// + /// Compiled pipelines no longer project an ID: the compiler resolves the + /// `self` repository by **name** at compile time, and ADO's REST API + /// accepts a repository name wherever it accepts an ID. This is populated + /// only from `ADO_AW_SELF_REPOSITORY_ID`, or from `BUILD_REPOSITORY_ID` + /// for direct/legacy invocations with no compiler-supplied identity. See + /// [`ExecutionContext::from_env_lookup`] for why the two sources are never + /// mixed. pub repository_id: Option, /// Exact `self` repository name. Compiled pipelines provide /// `ADO_AW_SELF_REPOSITORY_NAME`; direct/legacy invocations fall back to @@ -252,6 +258,24 @@ impl ExecutionContext { .map(std::path::PathBuf::from) .unwrap_or_else(|| source_directory.clone()); + // Resolve `self`'s identity from exactly one source, never a mix. + // + // `BUILD_REPOSITORY_*` describes the repository that *triggered* the + // run, which is not `checkout: self` on repository-resource-triggered + // builds (issue #1731). Pairing a compiler-supplied self name with a + // trigger-scoped ID would be worse than either alone, because + // consumers that prefer the ID would silently target the wrong + // repository. So if the compiler supplied either half, the + // `BUILD_REPOSITORY_*` pair is ignored entirely. + let compiled_repository_id = env("ADO_AW_SELF_REPOSITORY_ID"); + let compiled_repository_name = env("ADO_AW_SELF_REPOSITORY_NAME"); + let (repository_id, repository_name) = + if compiled_repository_id.is_some() || compiled_repository_name.is_some() { + (compiled_repository_id, compiled_repository_name) + } else { + (env("BUILD_REPOSITORY_ID"), env("BUILD_REPOSITORY_NAME")) + }; + Self { ado_org_url, ado_organization, @@ -264,10 +288,8 @@ impl ExecutionContext { self_repository_directory, tool_configs: HashMap::new(), debug_enabled_tools: HashSet::new(), - repository_id: env("ADO_AW_SELF_REPOSITORY_ID") - .or_else(|| env("BUILD_REPOSITORY_ID")), - repository_name: env("ADO_AW_SELF_REPOSITORY_NAME") - .or_else(|| env("BUILD_REPOSITORY_NAME")), + repository_id, + repository_name, allowed_repositories: HashMap::new(), repo_refs: HashMap::new(), agent_stats: None, @@ -947,6 +969,7 @@ mod tests { #[test] fn test_from_env_lookup_prefers_compiler_owned_self_identity() { + // Both halves supplied explicitly (manual/testing): use them as a pair. let ctx = ExecutionContext::from_env_lookup(env_from(&[ ("ADO_AW_SELF_REPOSITORY_ID", "self-id"), ("ADO_AW_SELF_REPOSITORY_NAME", "project/self-repo"), @@ -961,6 +984,21 @@ mod tests { ); } + #[test] + fn test_from_env_lookup_name_only_self_identity_ignores_trigger_id() { + // The shape compiled pipelines emit: a compile-time self name and no + // ID. The trigger-scoped BUILD_REPOSITORY_ID must NOT be paired with + // it, or consumers preferring the ID would target the wrong repo. + let ctx = ExecutionContext::from_env_lookup(env_from(&[ + ("ADO_AW_SELF_REPOSITORY_NAME", "self-repo"), + ("BUILD_REPOSITORY_ID", "trigger-id"), + ("BUILD_REPOSITORY_NAME", "trigger-repo"), + ])); + + assert_eq!(ctx.repository_name.as_deref(), Some("self-repo")); + assert_eq!(ctx.repository_id, None); + } + #[test] fn test_from_env_lookup_self_identity_falls_back_to_build_variables() { let ctx = ExecutionContext::from_env_lookup(env_from(&[ diff --git a/tests/compiler_tests.rs b/tests/compiler_tests.rs index 0eaa068a..1568c47f 100644 --- a/tests/compiler_tests.rs +++ b/tests/compiler_tests.rs @@ -8595,6 +8595,14 @@ description: "no fetch tuning" /// Compile inline agent `content` in an isolated temp dir and return the /// compiled YAML. Panics on compile failure. fn compile_inline_agent(tag: &str, content: &str) -> String { + compile_inline_agent_with_env(tag, content, &[]) +} + +/// Compile inline agent `content` with additional environment variables set on +/// the compiler subprocess. Used to exercise compile-time ADO context +/// resolution (`ADO_AW_COMPILE_REMOTE_URL`) without mutating the test +/// process's own environment, which parallel tests share. +fn compile_inline_agent_with_env(tag: &str, content: &str, env: &[(&str, &str)]) -> String { let temp_dir = std::env::temp_dir().join(format!("agentic-pipeline-{tag}-{}", std::process::id())); let _ = fs::remove_dir_all(&temp_dir); @@ -8603,15 +8611,17 @@ fn compile_inline_agent(tag: &str, content: &str) -> String { fs::write(&input, content).expect("Failed to write test input"); let output_path = temp_dir.join(format!("{tag}-agent.yml")); let binary_path = PathBuf::from(env!("CARGO_BIN_EXE_ado-aw")); - let output = std::process::Command::new(&binary_path) - .args([ - "compile", - input.to_str().unwrap(), - "-o", - output_path.to_str().unwrap(), - ]) - .output() - .expect("Failed to run compiler"); + let mut command = std::process::Command::new(&binary_path); + command.args([ + "compile", + input.to_str().unwrap(), + "-o", + output_path.to_str().unwrap(), + ]); + for (key, value) in env { + command.env(key, value); + } + let output = command.output().expect("Failed to run compiler"); assert!( output.status.success(), "compile should succeed for {tag}.\nstderr: {}", @@ -9405,11 +9415,59 @@ fn test_issue_1731_safeoutputs_executor_source_path_uses_multi_checkout_layout() "SafeOutputs must pass the exact self checkout to the executor:\n{safeoutputs}" ); assert!( - safeoutputs.contains("resources.repositories['self'].id") - && safeoutputs.contains("resources.repositories['self'].name") - && safeoutputs.contains("ADO_AW_SELF_REPOSITORY_ID:") - && safeoutputs.contains("ADO_AW_SELF_REPOSITORY_NAME:"), - "SafeOutputs must use self resource metadata rather than trigger-scoped Build.Repository.*:\n{safeoutputs}" + safeoutputs.contains("ADO_AW_SELF_REPOSITORY_NAME:") + && !safeoutputs.contains("ADO_AW_SELF_REPOSITORY_ID:"), + "SafeOutputs must identify self by name only (the ID is resolved from it):\n{safeoutputs}" + ); + assert!( + !safeoutputs.contains("resources.repositories['self'].id") + && !safeoutputs.contains("resources.repositories['self'].name"), + "self identity must be resolved at compile time, not via runtime expressions:\n{safeoutputs}" + ); +} + +/// Issue #1731: `self`'s repository identity is resolved at compile time from +/// the ADO git remote, so Stage 3 never has to infer it from the +/// trigger-scoped `Build.Repository.*` variables. +#[test] +fn test_issue_1731_self_repository_name_is_resolved_at_compile_time() { + let compiled = compile_inline_agent_with_env( + "issue-1731-baked-identity", + "---\nname: \"Multi-repo PR\"\ndescription: \"Opens a PR in an additional repo\"\nworkspace: root\nrepos:\n - my-org/tools\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + &[( + "ADO_AW_COMPILE_REMOTE_URL", + "https://dev.azure.com/msazuresphere/AgentPlayground/_git/ado-aw-mirror", + )], + ); + let safeoutputs = job_block(&compiled, "SafeOutputs"); + assert!( + safeoutputs.contains("ADO_AW_SELF_REPOSITORY_NAME: ado-aw-mirror"), + "the compile-time repository name must be baked into the executor env:\n{safeoutputs}" + ); + assert!( + !safeoutputs.contains("$(Build.Repository.Name)"), + "the trigger-scoped macro must not appear when ADO context resolves:\n{safeoutputs}" + ); +} + +/// Without a resolvable ADO remote the compiler falls back to the +/// trigger-scoped macro. That is a degraded mode, so it must be accompanied by +/// a warning rather than silently emitting a value that can name the wrong +/// repository on repository-resource-triggered runs. +#[test] +fn test_issue_1731_unresolvable_ado_context_falls_back_with_warning() { + let (ok, compiled, stderr) = compile_inline_source( + "issue-1731-identity-fallback", + "---\nname: \"Fallback PR\"\ndescription: \"No ADO remote available\"\nsafe-outputs:\n create-pull-request:\n target-branch: main\n---\n\n## Agent\n\nDo work.\n", + ); + assert!(ok, "fallback must not fail the compile: {stderr}"); + assert!( + compiled.contains("ADO_AW_SELF_REPOSITORY_NAME: $(Build.Repository.Name)"), + "fallback must emit the trigger-scoped macro:\n{compiled}" + ); + assert!( + stderr.contains("TRIGGERING repository"), + "fallback must warn that the identity may be wrong:\n{stderr}" ); } @@ -9528,9 +9586,10 @@ fn test_issue_1731_split_checkout_layout_compiles_for_every_target() { "{target}: PR-capable Stage 3 source must use multi-checkout layout:\n{compiled}" ); assert!( - compiled.contains("resources.repositories['self'].id") - && compiled.contains("resources.repositories['self'].name"), - "{target}: Stage 3 must use self resource metadata:\n{compiled}" + compiled.contains("ADO_AW_SELF_REPOSITORY_NAME:") + && !compiled.contains("resources.repositories['self'].id") + && !compiled.contains("resources.repositories['self'].name"), + "{target}: Stage 3 self identity must be compile-time resolved:\n{compiled}" ); assert!( compiled.contains("ado-aw execute --source \"$(Build.SourcesDirectory)/") diff --git a/tests/executor-e2e/README.md b/tests/executor-e2e/README.md index 7ed12750..d2cf969b 100644 --- a/tests/executor-e2e/README.md +++ b/tests/executor-e2e/README.md @@ -59,7 +59,10 @@ All deterministically-assertable ADO-write safe outputs plus the flagship - **Flagship:** `create-pull-request` covers both a named additional checkout at `/` and `repository: self` beneath a non-Git multi-checkout root, with compiler-owned self repository identity taking - precedence over trigger-scoped `BUILD_REPOSITORY_*` values. + precedence over trigger-scoped `BUILD_REPOSITORY_*` values. The `self` + scenario supplies only `ADO_AW_SELF_REPOSITORY_NAME` — matching what the + compiler emits — so it also proves the executor resolves a repository from + its name alone. Excluded (out of scope or GitHub-only): the GitHub-only `create-issue`. From ed10257d94c3bbbea283f28819b68c2fcacfdc20 Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 30 Jul 2026 22:19:09 +0100 Subject: [PATCH 6/7] test(smoke): add multi-repo checkout fixture to the candidate lane Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 10ccd9bb-6fb7-4a24-8e36-d2ff13031733 --- .../__tests__/config.test.ts | 2 + .../__tests__/fixtures.test.ts | 30 ++++- .../__tests__/index.test.ts | 1 + .../src/compiler-smoke-e2e/config.ts | 2 + .../src/compiler-smoke-e2e/fixtures.ts | 52 +++++--- tests/compiler-smoke-e2e/README.md | 59 +++++++-- tests/compiler-smoke-e2e/REGISTERED.md | 30 ++++- tests/compiler-smoke-e2e/azure-pipelines.yml | 2 + .../compiler-smoke-e2e/fixtures/multi-repo.md | 124 ++++++++++++++++++ 9 files changed, 267 insertions(+), 35 deletions(-) create mode 100644 tests/compiler-smoke-e2e/fixtures/multi-repo.md diff --git a/scripts/ado-script/src/compiler-smoke-e2e/__tests__/config.test.ts b/scripts/ado-script/src/compiler-smoke-e2e/__tests__/config.test.ts index a284a2aa..d52c00dc 100644 --- a/scripts/ado-script/src/compiler-smoke-e2e/__tests__/config.test.ts +++ b/scripts/ado-script/src/compiler-smoke-e2e/__tests__/config.test.ts @@ -19,6 +19,7 @@ function baseEnv(overrides: Record = {}): NodeJS.Pro COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID: "2602", COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID: "2603", COMPILER_SMOKE_REPORTER_DEFINITION_ID: "2605", + COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID: "2606", ...overrides, }; } @@ -35,6 +36,7 @@ describe("loadConfig", () => { "azure-cli": 2602, "noop-target": 2603, "smoke-failure-reporter": 2605, + "multi-repo": 2606, }); expect(config.concurrency).toBe(5); expect(config.childTimeoutMs).toBe(7_200_000); diff --git a/scripts/ado-script/src/compiler-smoke-e2e/__tests__/fixtures.test.ts b/scripts/ado-script/src/compiler-smoke-e2e/__tests__/fixtures.test.ts index 588cdaf6..7b601336 100644 --- a/scripts/ado-script/src/compiler-smoke-e2e/__tests__/fixtures.test.ts +++ b/scripts/ado-script/src/compiler-smoke-e2e/__tests__/fixtures.test.ts @@ -1,6 +1,12 @@ import { describe, expect, it } from "vitest"; -import { ALL_FIXTURES, allowedChangedPaths, fixturePaths, FIXTURE_DIR } from "../fixtures.js"; +import { + ALL_FIXTURES, + allowedChangedPaths, + CANDIDATE_FIXTURE_DIR, + fixturePaths, + FIXTURE_DIR, +} from "../fixtures.js"; describe("fixturePaths", () => { it("builds repo-relative md/lock paths under tests/safe-outputs", () => { @@ -10,30 +16,40 @@ describe("fixturePaths", () => { relLock: "tests/safe-outputs/canary.lock.yml", }); }); + + it("keeps candidate-only fixtures out of the release-owned directory", () => { + expect(fixturePaths("multi-repo")).toEqual({ + name: "multi-repo", + relMd: "tests/compiler-smoke-e2e/fixtures/multi-repo.md", + relLock: "tests/compiler-smoke-e2e/fixtures/multi-repo.lock.yml", + }); + }); }); describe("ALL_FIXTURES", () => { - it("has exactly the four fixtures in the required stable order", () => { + it("has exactly the five fixtures in the required stable order", () => { expect(ALL_FIXTURES.map((f) => f.name)).toEqual([ "canary", "azure-cli", "noop-target", "smoke-failure-reporter", + "multi-repo", ]); }); - it("every fixture path lives under the shared FIXTURE_DIR", () => { + it("every fixture path lives under a known fixture directory", () => { for (const f of ALL_FIXTURES) { - expect(f.relMd.startsWith(`${FIXTURE_DIR}/`)).toBe(true); - expect(f.relLock.startsWith(`${FIXTURE_DIR}/`)).toBe(true); + const dir = f.name === "multi-repo" ? CANDIDATE_FIXTURE_DIR : FIXTURE_DIR; + expect(f.relMd.startsWith(`${dir}/`)).toBe(true); + expect(f.relLock.startsWith(`${dir}/`)).toBe(true); } }); }); describe("allowedChangedPaths", () => { - it("contains exactly the four md files, four lock files, and .gitattributes", () => { + it("contains exactly each md file, each lock file, and .gitattributes", () => { const allowed = allowedChangedPaths(); - expect(allowed.size).toBe(9); + expect(allowed.size).toBe(ALL_FIXTURES.length * 2 + 1); expect(allowed.has(".gitattributes")).toBe(true); for (const f of ALL_FIXTURES) { expect(allowed.has(f.relMd)).toBe(true); diff --git a/scripts/ado-script/src/compiler-smoke-e2e/__tests__/index.test.ts b/scripts/ado-script/src/compiler-smoke-e2e/__tests__/index.test.ts index 59bdd761..d8c08603 100644 --- a/scripts/ado-script/src/compiler-smoke-e2e/__tests__/index.test.ts +++ b/scripts/ado-script/src/compiler-smoke-e2e/__tests__/index.test.ts @@ -18,6 +18,7 @@ const baseEnv = { COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID: "3002", COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID: "3003", COMPILER_SMOKE_REPORTER_DEFINITION_ID: "3005", + COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID: "3006", COMPILER_SMOKE_CHILD_TIMEOUT_MS: "5000", COMPILER_SMOKE_POLL_MS: "1", }; diff --git a/scripts/ado-script/src/compiler-smoke-e2e/config.ts b/scripts/ado-script/src/compiler-smoke-e2e/config.ts index a1426180..6b096587 100644 --- a/scripts/ado-script/src/compiler-smoke-e2e/config.ts +++ b/scripts/ado-script/src/compiler-smoke-e2e/config.ts @@ -33,6 +33,7 @@ export const FIXTURE_NAMES = [ "azure-cli", "noop-target", "smoke-failure-reporter", + "multi-repo", ] as const; export type FixtureName = (typeof FIXTURE_NAMES)[number]; @@ -89,6 +90,7 @@ const DEFINITION_ID_ENV_BY_FIXTURE: Readonly> = { "azure-cli": "COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID", "noop-target": "COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID", "smoke-failure-reporter": "COMPILER_SMOKE_REPORTER_DEFINITION_ID", + "multi-repo": "COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID", }; /** ADO macros that failed to expand look like `$(NAME)`; treat them as unset. */ diff --git a/scripts/ado-script/src/compiler-smoke-e2e/fixtures.ts b/scripts/ado-script/src/compiler-smoke-e2e/fixtures.ts index 1a6fed27..aa339dfb 100644 --- a/scripts/ado-script/src/compiler-smoke-e2e/fixtures.ts +++ b/scripts/ado-script/src/compiler-smoke-e2e/fixtures.ts @@ -1,26 +1,45 @@ /** - * Manifest of the four fixed compiler-smoke fixtures. + * Manifest of the fixed compiler-smoke fixtures. * - * These are selected release-backed agentic-pipeline sources documented in + * Most are selected release-backed agentic-pipeline sources documented in * `tests/safe-outputs/README.md` (canary, azure-cli, noop-target, and - * smoke-failure-reporter). The weekly janitor remains in the release lane but - * is not part of candidate checks. The harness reads the exact files from the - * detached candidate worktree (an exact checkout of `BUILD_SOURCEVERSION`, at - * `/tests/safe-outputs/.md` — never from + * smoke-failure-reporter); the weekly janitor remains in the release lane but + * is not part of candidate checks. `multi-repo` is candidate-only — it has no + * released lock file and lives under `tests/compiler-smoke-e2e/fixtures/`. + * + * The harness reads the exact files from the detached candidate worktree (an + * exact checkout of `BUILD_SOURCEVERSION` — never from * `BUILD_SOURCESDIRECTORY`, which may sit at a different commit), stages a * pinned `supply-chain.pipeline-artifact` transform of each onto the mirror - * repo, recompiles, and queues the four FIXED "candidate lane" pipeline - * definitions tracked in `tests/compiler-smoke-e2e/REGISTERED.md` (distinct - * from the release-backed definitions those same sources also feed). + * repo, recompiles, and queues the FIXED "candidate lane" pipeline definitions + * tracked in `tests/compiler-smoke-e2e/REGISTERED.md` (distinct from the + * release-backed definitions those same sources also feed). * * Test-harness module; not shipped in `ado-script.zip`. */ import type { FixtureName } from "./config.js"; import { FIXTURE_NAMES } from "./config.js"; -/** Repo-relative directory containing every fixture source + compiled lock. */ +/** Repo-relative directory containing the release-backed fixture sources. */ export const FIXTURE_DIR = "tests/safe-outputs"; +/** + * Repo-relative directory holding fixtures that exist **only** for the + * candidate lane. These have no released lock file and no release-backed + * definition, so they must not live beside the release-owned sources in + * {@link FIXTURE_DIR}. + */ +export const CANDIDATE_FIXTURE_DIR = "tests/compiler-smoke-e2e/fixtures"; + +/** Directory each fixture's source lives in. */ +const FIXTURE_DIR_BY_NAME: Readonly> = { + canary: FIXTURE_DIR, + "azure-cli": FIXTURE_DIR, + "noop-target": FIXTURE_DIR, + "smoke-failure-reporter": FIXTURE_DIR, + "multi-repo": CANDIDATE_FIXTURE_DIR, +}; + export interface FixturePaths { readonly name: FixtureName; /** Repo-relative path to the fixture markdown source, e.g. tests/safe-outputs/canary.md. */ @@ -31,21 +50,22 @@ export interface FixturePaths { /** Repo-relative path for a fixture's markdown source. Always POSIX-separated (a git path, not a filesystem path). */ export function fixturePaths(name: FixtureName): FixturePaths { + const dir = FIXTURE_DIR_BY_NAME[name]; return { name, - relMd: `${FIXTURE_DIR}/${name}.md`, - relLock: `${FIXTURE_DIR}/${name}.lock.yml`, + relMd: `${dir}/${name}.md`, + relLock: `${dir}/${name}.lock.yml`, }; } -/** All four fixtures in the stable declaration order used throughout the harness. */ +/** Every fixture in the stable declaration order used throughout the harness. */ export const ALL_FIXTURES: readonly FixturePaths[] = FIXTURE_NAMES.map(fixturePaths); /** * The exact set of repo-relative paths the candidate-staging commit is - * allowed to touch: the four markdown sources, their four compiled locks, - * and the compiler-managed `.gitattributes` block. Any other changed path - * fails the run before it pushes anything. + * allowed to touch: every markdown source, its compiled lock, and the + * compiler-managed `.gitattributes` block. Any other changed path fails the + * run before it pushes anything. */ export function allowedChangedPaths(): Set { const paths = new Set([".gitattributes"]); diff --git a/tests/compiler-smoke-e2e/README.md b/tests/compiler-smoke-e2e/README.md index fbfd9a03..2602e30f 100644 --- a/tests/compiler-smoke-e2e/README.md +++ b/tests/compiler-smoke-e2e/README.md @@ -1,11 +1,47 @@ # Candidate compiler agentic smoke This suite validates compiler changes before release. It builds `ado-aw` and -`ado-script` from the exact pull-request or `main` commit, recompiles four -selected workflows under [`tests/safe-outputs/`](../safe-outputs/) (canary, -azure-cli, noop-target, and failure reporter), and queues four fixed -AgentPlayground definitions against the regenerated YAML. The weekly janitor -remains covered by the release-backed lane and is intentionally excluded here. +`ado-script` from the exact pull-request or `main` commit, recompiles the +selected workflows below, and queues the matching fixed AgentPlayground +definitions against the regenerated YAML. The weekly janitor remains covered by +the release-backed lane and is intentionally excluded here. + +| Fixture | Source | Covers | +| --- | --- | --- | +| `canary` | [`tests/safe-outputs/`](../safe-outputs/) | Full agentic loop with two ADO write paths | +| `azure-cli` | [`tests/safe-outputs/`](../safe-outputs/) | AWF `az` extension and ADO control-plane reach | +| `noop-target` | [`tests/safe-outputs/`](../safe-outputs/) | Minimal Stage 1 → 3 shape | +| `smoke-failure-reporter` | [`tests/safe-outputs/`](../safe-outputs/) | Debug-only `create-issue` path | +| `multi-repo` | [`fixtures/`](fixtures/) | **Candidate-only.** Multi-checkout layout and compile-time `self` identity | + +The first four are release-backed sources that also feed the release lane. +`multi-repo` has no released lock file and no release definition, so it lives +under [`fixtures/`](fixtures/) instead — a lock committed there would have no +released compiler that owns it. + +### What `multi-repo` proves + +Compiler tests assert the *emitted YAML*; the executor-e2e suite asserts Stage 3 +against directories it creates itself. Neither proves that **Azure DevOps puts +repositories where the compiler said**. `multi-repo` closes that gap on live +ADO by checking, before the agent starts, that: + +1. `self` is at `$(Build.SourcesDirectory)/self` (the compiler-owned + `path: s/self`) and holds the exact candidate commit; +2. the additional repository is at `$(Build.SourcesDirectory)/pinned-base` and + resolves to a *different* commit, so a collapsed or missing checkout is + detectable; +3. the compiled pipeline is reachable beneath the `self` checkout — the same + path Stage 3 passes to `ado-aw execute --source`; +4. the `self` repository name baked in at compile time matches the repository + the build is actually running against. + +It declares `create-pull-request` so the compiler emits the multi-checkout +SafeOutputs job shape (issue #1731), but instructs the agent to emit only +`noop` and grants no `edit` tool — so Stage 3 exercises the layout while +proposing nothing and writing nothing. The SafeOutputs job's own layout is +verified implicitly: if its `self` checkout were misplaced, `--source` would +not resolve and Stage 3 would fail. It complements rather than replaces the release-backed daily smoke: @@ -29,7 +65,7 @@ binary cannot drift. `ado-aw-linux-x64`, `awf-linux-x64`, `ado-script.zip`, `checksums.txt`, and `provenance.json`. 4. The test-only `compiler-smoke-e2e` TypeScript harness creates a detached - worktree, adds an exact `supply-chain.pipeline-artifact` source to all four + worktree, adds an exact `supply-chain.pipeline-artifact` source to every smoke files, removes their schedules, recompiles them with the candidate compiler, and runs `ado-aw check`. Both compiler subprocesses receive `ADO_AW_COMPILE_REMOTE_URL` set to the target `ado-aw-mirror` URL, so @@ -38,7 +74,7 @@ binary cannot drift. 5. The harness pushes the worktree commit to `refs/heads/ado-aw-smoke-candidate/` in the Azure Repo `ado-aw-mirror`. -6. Four fixed child definitions are queued concurrently with both that ref and +6. Each fixed child definition is queued concurrently with both that ref and its exact commit SHA. Each generated pipeline downloads and verifies the artifact from the still-running producer build. 7. The harness waits for all children, cancels non-terminal runs after a @@ -81,10 +117,10 @@ to that manifest. The definitions are registered against `ado-aw-mirror`, not GitHub. Their default branch is the permanent inert ref -`refs/heads/ado-aw-smoke-candidate-base`, whose four lock-file paths contain +`refs/heads/ado-aw-smoke-candidate-base`, whose lock-file paths contain hand-authored `trigger: none`, `pr: none`, schedule-free placeholders. A child therefore runs only when the orchestrator explicitly supplies a candidate ref. -The checked-in [`inert-child.yml`](inert-child.yml) is copied to those four +The checked-in [`inert-child.yml`](inert-child.yml) is copied to each of those paths when the base ref is created. See [`REGISTERED.md`](REGISTERED.md) for definition IDs and variables. @@ -95,7 +131,7 @@ The principal behind `agent-playground-write`, used only after artifact publication, needs: - Contribute/Create branch/Delete refs on `ado-aw-mirror`; -- Queue builds and Stop builds on the four child definitions; +- Queue builds and Stop builds on the child definitions; - Read builds and artifacts in AgentPlayground. Child build identities need Code Read on `ado-aw-mirror` and Build Read on the @@ -120,6 +156,7 @@ COMPILER_SMOKE_CANARY_DEFINITION_ID COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID COMPILER_SMOKE_REPORTER_DEFINITION_ID +COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID ``` Optional overrides: @@ -150,7 +187,7 @@ The full live contract requires AgentPlayground and the fixed definitions: 1. the producer remains in progress after publishing its artifact; 2. every child downloads the exact producer `run-id`; -3. all four children succeed; +3. every child succeeds; 4. child provenance identifies the producer definition, build, and source SHA; 5. the per-run mirror ref is removed. diff --git a/tests/compiler-smoke-e2e/REGISTERED.md b/tests/compiler-smoke-e2e/REGISTERED.md index 8003fd13..1bc37ece 100644 --- a/tests/compiler-smoke-e2e/REGISTERED.md +++ b/tests/compiler-smoke-e2e/REGISTERED.md @@ -11,12 +11,40 @@ These definitions live in | `Candidate compiler smoke - azure-cli` | `ado-aw-mirror` | `tests/safe-outputs/azure-cli.lock.yml` | `2555` | | `Candidate compiler smoke - noop-target` | `ado-aw-mirror` | `tests/safe-outputs/noop-target.lock.yml` | `2556` | | `Candidate compiler smoke - failure reporter` | `ado-aw-mirror` | `tests/safe-outputs/smoke-failure-reporter.lock.yml` | `2558` | +| `Candidate compiler smoke - multi-repo` | `ado-aw-mirror` | `tests/compiler-smoke-e2e/fixtures/multi-repo.lock.yml` | *(pending registration)* | -All four child definitions use +All child definitions use `refs/heads/ado-aw-smoke-candidate-base` as their default branch. The ref is permanent and inert; the harness never deletes it. Its seed commit is `2b5fa7c336bd1f55a867cfc281e665472730b84c`. +## Registering the multi-repo child + +The `multi-repo` fixture is **candidate-only** — it has no released lock file +and no release-backed definition, so it lives under +`tests/compiler-smoke-e2e/fixtures/` rather than `tests/safe-outputs/`. +Registering it requires: + +1. Copy [`inert-child.yml`](inert-child.yml) to + `tests/compiler-smoke-e2e/fixtures/multi-repo.lock.yml` on + `refs/heads/ado-aw-smoke-candidate-base`, so the definition has an inert + YAML path before the orchestrator ever supplies a candidate ref. +2. Create the definition against `ado-aw-mirror` at that YAML path, with + `refs/heads/ado-aw-smoke-candidate-base` as its default branch and CI/PR + triggers disabled. +3. Provision its own secret `GITHUB_TOKEN` (server-side definition cloning does + not copy secret values). It needs no `ADO_AW_DEBUG_GITHUB_TOKEN`. +4. Authorize `agent-playground-read` on it. It needs no write service + connection: the fixture proposes no safe output that writes to ADO. +5. Record the returned ID above and set + `COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID` on the orchestrator. + +The fixture checks out `ado-aw-mirror` a second time under the alias +`pinned-base`, pinned to `refs/heads/ado-aw-smoke-candidate-base`. Because that +is the same repository the child already reads, it needs no additional +repository-resource authorization — only Code Read on `ado-aw-mirror`, which +every child already has. + Candidate janitor definition `2557` was retired. The release-backed janitor definition `2548` remains scheduled weekly and is not part of this lane. diff --git a/tests/compiler-smoke-e2e/azure-pipelines.yml b/tests/compiler-smoke-e2e/azure-pipelines.yml index 10500155..90298749 100644 --- a/tests/compiler-smoke-e2e/azure-pipelines.yml +++ b/tests/compiler-smoke-e2e/azure-pipelines.yml @@ -39,6 +39,7 @@ variables: EFFECTIVE_COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID: $[ coalesce(variables['COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID'], '') ] EFFECTIVE_COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID: $[ coalesce(variables['COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID'], '') ] EFFECTIVE_COMPILER_SMOKE_REPORTER_DEFINITION_ID: $[ coalesce(variables['COMPILER_SMOKE_REPORTER_DEFINITION_ID'], '') ] + EFFECTIVE_COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID: $[ coalesce(variables['COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID'], '') ] EFFECTIVE_CRATES_IO_FEED: $[ coalesce(variables['CRATES_IO_FEED'], 'sparse+https://pkgs.dev.azure.com/msazuresphere/AgentPlayground/_packaging/cargo/Cargo/index/') ] jobs: @@ -305,6 +306,7 @@ jobs: COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID: $(EFFECTIVE_COMPILER_SMOKE_AZURE_CLI_DEFINITION_ID) COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID: $(EFFECTIVE_COMPILER_SMOKE_NOOP_TARGET_DEFINITION_ID) COMPILER_SMOKE_REPORTER_DEFINITION_ID: $(EFFECTIVE_COMPILER_SMOKE_REPORTER_DEFINITION_ID) + COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID: $(EFFECTIVE_COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID) - task: PublishPipelineArtifact@1 condition: always() diff --git a/tests/compiler-smoke-e2e/fixtures/multi-repo.md b/tests/compiler-smoke-e2e/fixtures/multi-repo.md new file mode 100644 index 00000000..9f4526a2 --- /dev/null +++ b/tests/compiler-smoke-e2e/fixtures/multi-repo.md @@ -0,0 +1,124 @@ +--- +name: "ado-aw candidate smoke: multi-repo checkout" +description: "Proves Azure DevOps places multi-checkout repositories where the compiler says, and that the baked self identity matches the running repository" +target: standalone +pool: + name: AZS-1ES-L-Playground-ubuntu-22.04 +engine: + id: copilot + model: gpt-5-mini + timeout-minutes: 5 +permissions: + read: agent-playground-read +repos: + # Deliberately the mirror repo itself, pinned to the permanent inert base + # ref. Reusing `self`'s own repository keeps this fixture free of new ADO + # repository-resource infrastructure, and the differing ref is what makes a + # missing or collapsed checkout detectable: the two working trees must + # resolve to different commits. + - name: ado-aw-mirror + alias: pinned-base + ref: refs/heads/ado-aw-smoke-candidate-base + fetch-depth: 1 +safe-outputs: + noop: {} + # Configured but never invoked. `create-pull-request` is what makes the + # SafeOutputs job replicate the Agent job's multi-checkout layout (issue + # #1731), so declaring it is how this fixture reaches Stage 3 coverage. The + # agent is told to emit only `noop` and is granted no `edit` tool, so there + # is nothing to propose and no write ever reaches ADO. + create-pull-request: + target-branch: ado-aw-smoke-candidate-base +steps: + - bash: | + set -euo pipefail + + echo "checkout root:" + ls -la "$CHECKOUT_ROOT" + + # 1. Both repositories must exist at the exact paths the compiler emitted + # (`path: s/self` and `path: s/pinned-base`). + if [ ! -d "$SELF_DIR/.git" ]; then + echo "expected the self checkout at $SELF_DIR" >&2 + exit 1 + fi + if [ ! -d "$PINNED_DIR/.git" ]; then + echo "expected the pinned checkout at $PINNED_DIR" >&2 + exit 1 + fi + + self_head="$(git -C "$SELF_DIR" rev-parse HEAD)" + pinned_head="$(git -C "$PINNED_DIR" rev-parse HEAD)" + echo "self HEAD=$self_head" + echo "pinned HEAD=$pinned_head" + + # 2. `self` must be the exact candidate commit the orchestrator queued. + if [ "$self_head" != "$SOURCE_VERSION" ]; then + echo "self is at $self_head, expected candidate $SOURCE_VERSION" >&2 + exit 1 + fi + + # 3. The pinned alias tracks a different ref, so equal commits would mean + # the two checkouts collapsed into one directory. + if [ "$self_head" = "$pinned_head" ]; then + echo "pinned checkout matches self; the checkouts collided" >&2 + exit 1 + fi + + # 4. The workflow source must be reachable beneath the self checkout: + # this is the path Stage 3 passes to `ado-aw execute --source`. + if [ ! -f "$LOCK_FILE" ]; then + echo "compiled pipeline missing beneath $SELF_DIR" >&2 + exit 1 + fi + + # 5. The `self` repository identity is resolved at COMPILE time and baked + # into the lock. Prove the baked value matches the repository this + # build is actually running against — a silent mismatch here is what + # would send a `repository: self` safe output to the wrong repo. + baked="$(sed -n 's/^ *ADO_AW_SELF_REPOSITORY_NAME: *//p' "$LOCK_FILE" | head -n 1)" + if [ -z "$baked" ]; then + echo "no baked self repository identity found in $LOCK_FILE" >&2 + exit 1 + fi + # Written without the macro's leading '$(' so ADO cannot expand it here. + case "$baked" in + *Build.Repository.Name*) + echo "self identity fell back to the trigger-scoped macro: $baked" >&2 + exit 1 + ;; + esac + + remote_url="$(git -C "$SELF_DIR" remote get-url origin)" + actual="${remote_url##*/}" + echo "baked self repository=$baked actual=$actual" + if [ "$baked" != "$actual" ]; then + echo "baked self repository '$baked' does not match '$actual'" >&2 + exit 1 + fi + + echo "multi-repo checkout layout and self identity verified" + displayName: Assert multi-repo checkout layout and self identity + env: + CHECKOUT_ROOT: $(Build.SourcesDirectory) + SELF_DIR: $(Build.SourcesDirectory)/self + PINNED_DIR: $(Build.SourcesDirectory)/pinned-base + SOURCE_VERSION: $(Build.SourceVersion) + LOCK_FILE: $(Build.SourcesDirectory)/self/tests/compiler-smoke-e2e/fixtures/multi-repo.lock.yml +--- + +## Multi-repo checkout smoke + +The pipeline verified its own checkout layout and `self` identity before you +were started. Your only job is to emit a single proof-of-life safe output. + +Call **exactly one** safe-output tool: + +1. `noop` + + - context: "ado-aw-smoke-$(Build.BuildId)-multi-repo checkout layout verified" + +Do not call any other tool. In particular do **not** call +`create-pull-request`: it is configured only so the compiler emits the +multi-checkout SafeOutputs job shape, and there are no changes to propose. +After the safe output is emitted, stop. From 4e9b3efc3726bf9ad395bdf7eda164b2758d4268 Mon Sep 17 00:00:00 2001 From: James Devine Date: Thu, 30 Jul 2026 22:56:18 +0100 Subject: [PATCH 7/7] docs(smoke): document adding a candidate smoke fixture Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 10ccd9bb-6fb7-4a24-8e36-d2ff13031733 --- tests/compiler-smoke-e2e/README.md | 70 +++++++++++++++++++++++++- tests/compiler-smoke-e2e/REGISTERED.md | 28 +---------- 2 files changed, 71 insertions(+), 27 deletions(-) diff --git a/tests/compiler-smoke-e2e/README.md b/tests/compiler-smoke-e2e/README.md index 2602e30f..f130de0f 100644 --- a/tests/compiler-smoke-e2e/README.md +++ b/tests/compiler-smoke-e2e/README.md @@ -121,10 +121,78 @@ default branch is the permanent inert ref hand-authored `trigger: none`, `pr: none`, schedule-free placeholders. A child therefore runs only when the orchestrator explicitly supplies a candidate ref. The checked-in [`inert-child.yml`](inert-child.yml) is copied to each of those -paths when the base ref is created. +paths — when the base ref is first created, and again for each fixture added +later (see **Adding a new candidate fixture** below). See [`REGISTERED.md`](REGISTERED.md) for definition IDs and variables. +## Adding a new candidate fixture + +`` is the fixture name; `multi-repo` is the worked example. + +> **Order matters.** Phase B must be complete before Phase A merges — the +> harness fails closed on an unset `*_DEFINITION_ID`, so an unregistered +> fixture breaks the whole lane. + +### Phase A — repository (one PR) + +1. Author `fixtures/.md`. Do **not** commit a lock file: candidate-only + fixtures have no released compiler that owns one. +2. Wire it up in five places: + + | File | Change | + | --- | --- | + | `config.ts` | add `` to `FIXTURE_NAMES` | + | `config.ts` | add `COMPILER_SMOKE__DEFINITION_ID` to `DEFINITION_ID_ENV_BY_FIXTURE` | + | `fixtures.ts` | add `` to `FIXTURE_DIR_BY_NAME` | + | `azure-pipelines.yml` | add the `EFFECTIVE_*` variable **and** its env passthrough | + | `__tests__/{config,index}.test.ts` | add the ID to both env builders | + + (All five live under `scripts/ado-script/src/compiler-smoke-e2e/` except the + pipeline YAML.) +3. `npx vitest run src/compiler-smoke-e2e && npm run typecheck`. + +### Phase B — Azure DevOps (once, needs mirror push + definition-create rights) + +```bash +# 1. Seed an inert placeholder so the definition has a valid default branch. +git clone -b ado-aw-smoke-candidate-base \ + https://dev.azure.com/msazuresphere/AgentPlayground/_git/ado-aw-mirror +cd ado-aw-mirror +mkdir -p tests/compiler-smoke-e2e/fixtures +cp /path/to/ado-aw/tests/compiler-smoke-e2e/inert-child.yml \ + tests/compiler-smoke-e2e/fixtures/.lock.yml +git add -A && git commit -m "chore(smoke): seed inert child" && git push + +# 2. Create the definition (no run; triggers stay off). +az pipelines create \ + --org https://dev.azure.com/msazuresphere --project AgentPlayground \ + --name "Candidate compiler smoke - " \ + --repository ado-aw-mirror --repository-type tfsgit \ + --branch ado-aw-smoke-candidate-base \ + --yaml-path tests/compiler-smoke-e2e/fixtures/.lock.yml \ + --folder-path '\compiler-smoke-e2e' --skip-run true +``` + +Then, on the new definition: + +- provision its **own** secret `GITHUB_TOKEN` — server-side definition cloning + does not copy secret values; +- authorize the service connections the fixture's `permissions:` block declares + (`agent-playground-read`, plus `agent-playground-write` only if it proposes a + safe output that writes to ADO); +- set `COMPILER_SMOKE__DEFINITION_ID` on orchestrator `2559`. + +### Phase C — verify + +`/azp run` the orchestrator on the Phase A PR, confirm the new child appears in +the results table, then record its ID in [`REGISTERED.md`](REGISTERED.md). + +> Checking out an **additional** repository needs no extra authorization when +> it is one the child already reads (`multi-repo` checks out `ado-aw-mirror` +> twice). A genuinely different repository needs Code Read granted to the +> child's build identity. + ## Required permissions The principal behind `agent-playground-write`, used only after artifact diff --git a/tests/compiler-smoke-e2e/REGISTERED.md b/tests/compiler-smoke-e2e/REGISTERED.md index 1bc37ece..a40dc328 100644 --- a/tests/compiler-smoke-e2e/REGISTERED.md +++ b/tests/compiler-smoke-e2e/REGISTERED.md @@ -18,32 +18,8 @@ All child definitions use permanent and inert; the harness never deletes it. Its seed commit is `2b5fa7c336bd1f55a867cfc281e665472730b84c`. -## Registering the multi-repo child - -The `multi-repo` fixture is **candidate-only** — it has no released lock file -and no release-backed definition, so it lives under -`tests/compiler-smoke-e2e/fixtures/` rather than `tests/safe-outputs/`. -Registering it requires: - -1. Copy [`inert-child.yml`](inert-child.yml) to - `tests/compiler-smoke-e2e/fixtures/multi-repo.lock.yml` on - `refs/heads/ado-aw-smoke-candidate-base`, so the definition has an inert - YAML path before the orchestrator ever supplies a candidate ref. -2. Create the definition against `ado-aw-mirror` at that YAML path, with - `refs/heads/ado-aw-smoke-candidate-base` as its default branch and CI/PR - triggers disabled. -3. Provision its own secret `GITHUB_TOKEN` (server-side definition cloning does - not copy secret values). It needs no `ADO_AW_DEBUG_GITHUB_TOKEN`. -4. Authorize `agent-playground-read` on it. It needs no write service - connection: the fixture proposes no safe output that writes to ADO. -5. Record the returned ID above and set - `COMPILER_SMOKE_MULTI_REPO_DEFINITION_ID` on the orchestrator. - -The fixture checks out `ado-aw-mirror` a second time under the alias -`pinned-base`, pinned to `refs/heads/ado-aw-smoke-candidate-base`. Because that -is the same repository the child already reads, it needs no additional -repository-resource authorization — only Code Read on `ado-aw-mirror`, which -every child already has. +To add a new child definition, follow **Adding a new candidate fixture** in +[`README.md`](README.md), then record the returned ID in the table above. Candidate janitor definition `2557` was retired. The release-backed janitor definition `2548` remains scheduled weekly and is not part of this lane.