diff --git a/.github/skills/checkout-credential-review/SKILL.md b/.github/skills/checkout-credential-review/SKILL.md new file mode 100644 index 00000000000..30e559f74e4 --- /dev/null +++ b/.github/skills/checkout-credential-review/SKILL.md @@ -0,0 +1,36 @@ +--- +name: checkout-credential-review +description: Review code that performs git or gh operations against repository checkouts in gh-aw, checking that the right credentials are available at the right time and that sparseness, shallowness and credential-free factors are properly considered. +--- + +# Checkout Credential Review + +Use this skill when reviewing or writing code in `pkg/workflow/`, `actions/setup/js/`, or compiled `.lock.yml` workflows that runs `git`, `gh`, or any other remote-touching operation against a repository checkout. + +## Background + +Each entry in a workflow's `checkout:` block may declare its own credentials (`github-token:`, `github-app:`), and the compiler wires those into the corresponding `actions/checkout` step ([pkg/workflow/checkout_step_generator.go](pkg/workflow/checkout_step_generator.go)). Generated checkouts always set `persist-credentials: false`, so the on-disk repo retains **no** credentials after the step finishes — only `actions/checkout`'s own internal token is used during the clone, and it is scrubbed in its post-step. + +A separate step that wants to authenticate later must either (a) re-inject a token at command level (e.g. `git -c http.extraheader=...`) or (b) be passed the per-checkout token via env. The compiler does *not* automatically thread per-checkout `github-token`s into downstream steps. + +Two important contexts deliberately run with **no git credentials**: + +- The **safe-outputs MCP server** and its handlers (`generate_git_bundle.cjs`, `generate_git_patch.cjs`, `create_pull_request.cjs`). Errors in these paths explicitly say "the safe-outputs MCP server has no credentials for private repositories" — fetch/push will fail for private repos. +- The **agent runtime** after `actions/checkout`. The agent prompt in [actions/setup/md/safe_outputs_push_to_pr_branch.md](actions/setup/md/safe_outputs_push_to_pr_branch.md) explicitly tells the model not to attempt `git fetch`, `git pull`, `git push`, or any other authenticated git operation, and to report unavailable branches rather than try to fetch them. + +## Review checklist + +When you see a new `git`, `gh`, `execFileSync('git'…)`, or compiled `run:` block: + +1. **Does it touch a remote?** Local-only commands (`symbolic-ref`, `rev-parse`, `log`, `show`, `merge-base`, `diff`, `status`) need no credentials. Anything in `fetch | pull | push | clone | ls-remote | remote (set-url|add|update)` does, plus on-demand blob fetches in partial clones. +2. **Which checkout is it operating on?** If it's a cross-repo entry from `checkout:`, the relevant credential is *that entry's* `github-token`, not the workflow's default `GITHUB_TOKEN`. Confirm the per-entry token is actually threaded into the step's env (or refuse to do remote operations and degrade gracefully). +3. **Which job/context emits it?** Agent job and safe-outputs MCP server both run without git credentials by design. Any remote git operation there must be wrapped in `try/catch`, fail soft, and surface a clear "no credentials" error rather than a raw git stderr. +4. **Sparse / shallow / monorepo concerns.** Avoid emitting steps that deepen (`git fetch --unshallow`, `--deepen=N`) or widen (`git fetch origin '+refs/heads/*'`) a sparse or shallow checkout of a large monorepo — these need credentials *and* can pull hundreds of MB. Prefer expanding `fetch:` / `fetch-depth:` / `sparse-checkout:` at compile time so it happens during `actions/checkout` with its internal token, never later. +5. **`gh` is REST, not git.** `gh api …` uses whatever `GH_TOKEN` is in the step's env — it does **not** automatically inherit per-checkout PATs. For cross-org private repos, either thread the right token in or accept the call will 404 and handle it. + +## Related + +- [docs/src/content/docs/reference/checkout.md](docs/src/content/docs/reference/checkout.md) — "Git Credentials After Checkout" +- [docs/sparseness.md](docs/sparseness.md) — sparse/blobless credential lifecycle +- [pkg/workflow/checkout_step_generator.go](pkg/workflow/checkout_step_generator.go) — token wiring per checkout +- [actions/setup/md/safe_outputs_push_to_pr_branch.md](actions/setup/md/safe_outputs_push_to_pr_branch.md) — agent-facing guidance diff --git a/.github/workflows/smoke-create-cross-repo-pr.lock.yml b/.github/workflows/smoke-create-cross-repo-pr.lock.yml index 7f2b2e5838b..33690526074 100644 --- a/.github/workflows/smoke-create-cross-repo-pr.lock.yml +++ b/.github/workflows/smoke-create-cross-repo-pr.lock.yml @@ -508,32 +508,17 @@ jobs: repository: github/gh-aw-side-repo token: ${{ secrets.GH_AW_SIDE_REPO_PAT }} - name: Build checkout manifest for safe-outputs handlers + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: GH_TOKEN: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} - run: | - set -euo pipefail - mkdir -p "${RUNNER_TEMP}/gh-aw" - manifest="${RUNNER_TEMP}/gh-aw/checkout-manifest.json" - printf '{}' > "$manifest" - resolve_default_branch() { - local repo="$1" path="$2" db="" - if [ -d "${GITHUB_WORKSPACE}/${path}/.git" ]; then - db=$(git -C "${GITHUB_WORKSPACE}/${path}" symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || true) - fi - if [ -z "$db" ]; then - db=$(gh api "repos/${repo}" --jq '.default_branch' 2>/dev/null || true) - fi - printf '%s' "$db" - } - repo='github/gh-aw-side-repo' - path='' - db=$(resolve_default_branch "$repo" "$path") - tmp=$(mktemp) - jq --arg repo "$repo" --arg path "$path" --arg db "$db" \ - '.[($repo | ascii_downcase)] = {repository: $repo, path: $path, default_branch: $db}' \ - "$manifest" > "$tmp" && mv "$tmp" "$manifest" - echo "checkout-manifest: ${repo} -> path=${path} default_branch=${db:-}" - cat "$manifest" + GH_AW_CHECKOUT_MANIFEST_COUNT: "1" + GH_AW_CHECKOUT_REPO_0: "github/gh-aw-side-repo" + GH_AW_CHECKOUT_PATH_0: "" + GH_AW_CHECKOUT_TOKEN_0: ${{ secrets.GH_AW_SIDE_REPO_PAT }} + with: + script: | + const { main } = require('${{ runner.temp }}/gh-aw/actions/build_checkout_manifest.cjs'); + await main(); - name: Create gh-aw temp directory run: bash "${RUNNER_TEMP}/gh-aw/actions/create_gh_aw_tmp_dir.sh" - name: Configure gh CLI for GitHub Enterprise diff --git a/.github/workflows/smoke-update-cross-repo-pr.lock.yml b/.github/workflows/smoke-update-cross-repo-pr.lock.yml index 8d6a5ca9818..0ccc6399f1e 100644 --- a/.github/workflows/smoke-update-cross-repo-pr.lock.yml +++ b/.github/workflows/smoke-update-cross-repo-pr.lock.yml @@ -527,32 +527,17 @@ jobs: header=$(printf "x-access-token:%s" "${GH_AW_FETCH_TOKEN}" | base64 -w 0) git -c "http.extraheader=Authorization: Basic ${header}" fetch origin '+refs/heads/main:refs/remotes/origin/main' '+refs/pull/*/head:refs/remotes/origin/pull/*/head' - name: Build checkout manifest for safe-outputs handlers + uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9.0.0 env: GH_TOKEN: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }} - run: | - set -euo pipefail - mkdir -p "${RUNNER_TEMP}/gh-aw" - manifest="${RUNNER_TEMP}/gh-aw/checkout-manifest.json" - printf '{}' > "$manifest" - resolve_default_branch() { - local repo="$1" path="$2" db="" - if [ -d "${GITHUB_WORKSPACE}/${path}/.git" ]; then - db=$(git -C "${GITHUB_WORKSPACE}/${path}" symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || true) - fi - if [ -z "$db" ]; then - db=$(gh api "repos/${repo}" --jq '.default_branch' 2>/dev/null || true) - fi - printf '%s' "$db" - } - repo='github/gh-aw-side-repo' - path='' - db=$(resolve_default_branch "$repo" "$path") - tmp=$(mktemp) - jq --arg repo "$repo" --arg path "$path" --arg db "$db" \ - '.[($repo | ascii_downcase)] = {repository: $repo, path: $path, default_branch: $db}' \ - "$manifest" > "$tmp" && mv "$tmp" "$manifest" - echo "checkout-manifest: ${repo} -> path=${path} default_branch=${db:-}" - cat "$manifest" + GH_AW_CHECKOUT_MANIFEST_COUNT: "1" + GH_AW_CHECKOUT_REPO_0: "github/gh-aw-side-repo" + GH_AW_CHECKOUT_PATH_0: "" + GH_AW_CHECKOUT_TOKEN_0: ${{ secrets.GH_AW_SIDE_REPO_PAT }} + with: + script: | + const { main } = require('${{ runner.temp }}/gh-aw/actions/build_checkout_manifest.cjs'); + await main(); - name: Create gh-aw temp directory run: bash "${RUNNER_TEMP}/gh-aw/actions/create_gh_aw_tmp_dir.sh" - name: Configure gh CLI for GitHub Enterprise diff --git a/AGENTS.md b/AGENTS.md index 195392da6f8..a6439ed93e3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -50,6 +50,7 @@ Use skills only when the task requires specialized guidance. Do not pre-load eve - GitHub MCP usage patterns → `.github/skills/github-mcp-server/SKILL.md` - Query helpers for issues/PRs/workflows/discussions/labels → matching `.github/skills/github-*-query/SKILL.md` - Doc-writing conventions → `.github/skills/documentation/SKILL.md` +- Reviewing or writing `git`/`gh`/remote operations against checkouts (per-checkout credentials, sparse/shallow monorepos, safe-outputs MCP runs without credentials) → `.github/skills/checkout-credential-review/SKILL.md` ## Why this file is intentionally short diff --git a/actions/setup/js/build_checkout_manifest.cjs b/actions/setup/js/build_checkout_manifest.cjs new file mode 100644 index 00000000000..0e77a571407 --- /dev/null +++ b/actions/setup/js/build_checkout_manifest.cjs @@ -0,0 +1,143 @@ +// @ts-check +/// + +require("./shim.cjs"); + +const fs = require("fs"); +const path = require("path"); +const { execFileSync } = require("child_process"); + +const { getErrorMessage } = require("./error_helpers.cjs"); + +function parseManifestEntries(entriesJSON = process.env.GH_AW_CHECKOUT_MANIFEST_ENTRIES || "[]") { + const parsed = JSON.parse(entriesJSON); + if (!Array.isArray(parsed)) { + throw new Error("GH_AW_CHECKOUT_MANIFEST_ENTRIES must be a JSON array"); + } + return parsed; +} + +function readManifestEntriesFromEnv() { + const count = Number.parseInt(process.env.GH_AW_CHECKOUT_MANIFEST_COUNT || "0", 10); + if (!Number.isFinite(count) || count < 0) { + throw new Error("GH_AW_CHECKOUT_MANIFEST_COUNT must be a non-negative integer"); + } + + const entries = []; + for (let i = 0; i < count; i += 1) { + entries.push({ + repository: process.env[`GH_AW_CHECKOUT_REPO_${i}`] || "", + path: process.env[`GH_AW_CHECKOUT_PATH_${i}`] || "", + token: process.env[`GH_AW_CHECKOUT_TOKEN_${i}`] || "", + }); + } + return entries; +} + +function resolveDefaultBranch(repository, checkoutPath, options = {}) { + const workspace = options.workspace || process.env.GITHUB_WORKSPACE || ""; + const runGit = options.runGit || ((args, execOptions = {}) => execFileSync("git", args, { encoding: "utf8", ...execOptions })); + const runGH = + options.runGH || + ((args, execOptions = {}) => + execFileSync("gh", args, { + encoding: "utf8", + env: { ...process.env, ...(execOptions.env || {}) }, + ...execOptions, + })); + let defaultBranch = ""; + + const repoPath = checkoutPath ? path.join(workspace, checkoutPath) : workspace; + if (repoPath && fs.existsSync(path.join(repoPath, ".git"))) { + try { + const output = runGit(["-C", repoPath, "symbolic-ref", "--short", "refs/remotes/origin/HEAD"], { + stdio: ["ignore", "pipe", "pipe"], + }); + defaultBranch = output.trim().replace(/^origin\//, ""); + core.debug(`build_checkout_manifest: git resolved default branch for ${repository}: ${defaultBranch}`); + } catch (error) { + core.debug(`build_checkout_manifest: git default branch lookup failed for ${repository}: ${getErrorMessage(error)}`); + } + } + + if (defaultBranch === "") { + try { + const checkoutToken = options.checkoutToken || ""; + const ghExecOptions = { + stdio: ["ignore", "pipe", "pipe"], + }; + if (checkoutToken !== "") { + ghExecOptions.env = { GH_TOKEN: checkoutToken }; + } + defaultBranch = runGH(["api", `repos/${repository}`, "--jq", ".default_branch"], ghExecOptions).trim(); + core.debug(`build_checkout_manifest: gh api resolved default branch for ${repository}: ${defaultBranch}`); + } catch (error) { + core.debug(`build_checkout_manifest: gh api default branch lookup failed for ${repository}: ${getErrorMessage(error)}`); + } + } + + return defaultBranch; +} + +function buildCheckoutManifest(entries, options = {}) { + const runnerTemp = options.runnerTemp || process.env.RUNNER_TEMP; + if (!runnerTemp) { + throw new Error("RUNNER_TEMP is required to build checkout manifest"); + } + + const runGit = options.runGit; + const runGH = options.runGH; + + const manifestDir = path.join(runnerTemp, "gh-aw"); + fs.mkdirSync(manifestDir, { recursive: true }); + const manifestPath = path.join(manifestDir, "checkout-manifest.json"); + const manifest = {}; + core.info(`checkout-manifest: building manifest for ${entries.length} checkout entries`); + + for (const entry of entries) { + if (!entry || typeof entry !== "object") { + core.debug("checkout-manifest: skipping non-object entry"); + continue; + } + const repository = String(entry.repository || "").trim(); + if (repository === "") { + core.debug("checkout-manifest: skipping entry with empty repository"); + continue; + } + const checkoutPath = String(entry.path || ""); + const defaultBranch = resolveDefaultBranch(repository, checkoutPath, { + workspace: options.workspace, + runGit, + runGH, + checkoutToken: entry.token || "", + }); + manifest[repository.toLowerCase()] = { + repository, + path: checkoutPath, + default_branch: defaultBranch, + }; + core.info(`checkout-manifest: ${repository} -> path=${checkoutPath} default_branch=${defaultBranch || ""}`); + } + + fs.writeFileSync(manifestPath, JSON.stringify(manifest, null, 2) + "\n", "utf8"); + core.info(`checkout-manifest written to ${manifestPath}`); + return { manifestPath, manifest }; +} + +async function main(options = {}) { + let entries; + if (typeof options.entriesJSON === "string" && options.entriesJSON.trim() !== "") { + entries = parseManifestEntries(options.entriesJSON); + } else { + entries = readManifestEntriesFromEnv(); + } + return buildCheckoutManifest(entries, options); +} + +module.exports = { + buildCheckoutManifest, + main, + parseManifestEntries, + readManifestEntriesFromEnv, + resolveDefaultBranch, +}; diff --git a/actions/setup/js/build_checkout_manifest.test.cjs b/actions/setup/js/build_checkout_manifest.test.cjs new file mode 100644 index 00000000000..1da95cb1635 --- /dev/null +++ b/actions/setup/js/build_checkout_manifest.test.cjs @@ -0,0 +1,136 @@ +import { afterEach, describe, expect, it } from "vitest"; +import fs from "fs"; +import os from "os"; +import path from "path"; +import { spawnSync } from "child_process"; + +import { buildCheckoutManifest, readManifestEntriesFromEnv, resolveDefaultBranch } from "./build_checkout_manifest.cjs"; + +function execGit(args, options = {}) { + const result = spawnSync("git", args, { encoding: "utf8", ...options }); + if (result.error) throw result.error; + if (result.status !== 0) { + throw new Error(`git ${args.join(" ")} failed:\nstdout: ${result.stdout}\nstderr: ${result.stderr}`); + } + return result.stdout; +} + +function createTempDir(prefix) { + return fs.mkdtempSync(path.join(os.tmpdir(), prefix)); +} + +function removeDir(dir) { + if (dir && fs.existsSync(dir)) { + fs.rmSync(dir, { recursive: true, force: true }); + } +} + +function setEnv(key, value) { + if (value === undefined) { + delete process.env[key]; + } else { + process.env[key] = value; + } +} + +describe("build_checkout_manifest.cjs", () => { + const originalEnv = { ...process.env }; + const tempDirs = []; + + afterEach(() => { + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); + while (tempDirs.length > 0) { + removeDir(tempDirs.pop()); + } + }); + + it("reads entries from environment variables", () => { + setEnv("GH_AW_CHECKOUT_MANIFEST_COUNT", "2"); + setEnv("GH_AW_CHECKOUT_REPO_0", "owner/a"); + setEnv("GH_AW_CHECKOUT_PATH_0", "./a"); + setEnv("GH_AW_CHECKOUT_TOKEN_0", "${{ secrets.REPO_A_TOKEN }}"); + setEnv("GH_AW_CHECKOUT_REPO_1", "owner/b"); + setEnv("GH_AW_CHECKOUT_PATH_1", ""); + + expect(readManifestEntriesFromEnv()).toEqual([ + { repository: "owner/a", path: "./a", token: "${{ secrets.REPO_A_TOKEN }}" }, + { repository: "owner/b", path: "", token: "" }, + ]); + }); + + it("resolves default branch from local git checkout before gh fallback", () => { + const workspace = createTempDir("checkout-manifest-workspace-"); + tempDirs.push(workspace); + const checkoutPath = "target"; + const repoDir = path.join(workspace, checkoutPath); + fs.mkdirSync(repoDir, { recursive: true }); + + execGit(["init", "-q"], { cwd: repoDir }); + execGit(["symbolic-ref", "refs/remotes/origin/HEAD", "refs/remotes/origin/main"], { cwd: repoDir }); + + const ghCalls = []; + const defaultBranch = resolveDefaultBranch("owner/repo", checkoutPath, { + workspace, + runGH: args => { + ghCalls.push(args); + return "should-not-be-used\n"; + }, + }); + + expect(defaultBranch).toBe("main"); + expect(ghCalls).toHaveLength(0); + }); + + it("falls back to gh api when local git default branch is unavailable", () => { + const workspace = createTempDir("checkout-manifest-workspace-"); + tempDirs.push(workspace); + let ghOptions = null; + + const defaultBranch = resolveDefaultBranch("owner/repo", "missing", { + workspace, + checkoutToken: "${{ secrets.CROSS_REPO_PAT }}", + runGH: (_args, options) => { + ghOptions = options; + return "trunk\n"; + }, + }); + + expect(defaultBranch).toBe("trunk"); + expect(ghOptions?.env?.GH_TOKEN).toBe("${{ secrets.CROSS_REPO_PAT }}"); + }); + + it("writes manifest with lowercase keys", () => { + const workspace = createTempDir("checkout-manifest-workspace-"); + const runnerTemp = createTempDir("checkout-manifest-runner-temp-"); + tempDirs.push(workspace, runnerTemp); + + const { manifestPath, manifest } = buildCheckoutManifest( + [ + { repository: "Owner/Repo", path: "./repo" }, + { repository: "", path: "./skip" }, + ], + { + workspace, + runnerTemp, + runGH: () => "main\n", + } + ); + + expect(manifestPath).toBe(path.join(runnerTemp, "gh-aw", "checkout-manifest.json")); + expect(manifest).toEqual({ + "owner/repo": { + repository: "Owner/Repo", + path: "./repo", + default_branch: "main", + }, + }); + + const fileContents = JSON.parse(fs.readFileSync(manifestPath, "utf8")); + expect(fileContents).toEqual(manifest); + }); +}); diff --git a/pkg/workflow/checkout_manager_test.go b/pkg/workflow/checkout_manager_test.go index 43a5838b362..9ab8c22d4fe 100644 --- a/pkg/workflow/checkout_manager_test.go +++ b/pkg/workflow/checkout_manager_test.go @@ -1376,59 +1376,63 @@ func TestWikiCheckout(t *testing.T) { // The manifest step is consumed by the safe-outputs MCP server to resolve a // per-repo default branch without making any network calls at request time. func TestGenerateCheckoutManifestStep(t *testing.T) { + getActionPin := func(action string) string { + return action + "@pin" + } + t.Run("no configs emits nothing", func(t *testing.T) { cm := NewCheckoutManager(nil) - assert.Empty(t, cm.GenerateCheckoutManifestStep(), "empty manager should not emit a manifest step") + assert.Empty(t, cm.GenerateCheckoutManifestStep(getActionPin), "empty manager should not emit a manifest step") }) t.Run("default-only checkout emits nothing", func(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ {Path: "."}, }) - assert.Empty(t, cm.GenerateCheckoutManifestStep(), "manifest is for cross-repo entries only; default checkout should not produce one") + assert.Empty(t, cm.GenerateCheckoutManifestStep(getActionPin), "manifest is for cross-repo entries only; default checkout should not produce one") }) t.Run("path-only additional checkout (no repository) emits nothing", func(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ {Path: "./workspace"}, }) - assert.Empty(t, cm.GenerateCheckoutManifestStep(), "additional checkout without repository should not be in manifest") + assert.Empty(t, cm.GenerateCheckoutManifestStep(getActionPin), "additional checkout without repository should not be in manifest") }) t.Run("wiki cross-repo checkout is excluded", func(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ {Repository: "owner/docs", Path: "./wiki", Wiki: true}, }) - assert.Empty(t, cm.GenerateCheckoutManifestStep(), "wiki checkouts must be excluded from manifest") + assert.Empty(t, cm.GenerateCheckoutManifestStep(getActionPin), "wiki checkouts must be excluded from manifest") }) t.Run("cross-repo additional checkout emits entry", func(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ - {Repository: "owner/other", Path: "./other"}, + {Repository: "owner/other", Path: "./other", GitHubToken: "${{ secrets.CROSS_REPO_PAT }}"}, }) - steps := cm.GenerateCheckoutManifestStep() + steps := cm.GenerateCheckoutManifestStep(getActionPin) require.Len(t, steps, 1, "should emit one manifest step") out := steps[0] assert.Contains(t, out, "name: Build checkout manifest for safe-outputs handlers") - assert.Contains(t, out, "${RUNNER_TEMP}/gh-aw/checkout-manifest.json") + assert.Contains(t, out, "uses: actions/github-script@pin") assert.Contains(t, out, "GH_TOKEN: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}") - assert.Contains(t, out, "resolve_default_branch", "should define the resolver helper") - assert.Contains(t, out, "git -C \"${GITHUB_WORKSPACE}/${path}\" symbolic-ref --short refs/remotes/origin/HEAD", "should attempt local git resolution first") - assert.Contains(t, out, "gh api \"repos/${repo}\" --jq '.default_branch'", "should fall back to gh api") - assert.Contains(t, out, "repo='owner/other'", "repo must be shell-single-quoted") - assert.Contains(t, out, "path='./other'", "path must be shell-single-quoted") - assert.Contains(t, out, "ascii_downcase", "manifest key must be lowercased via jq") + assert.Contains(t, out, `GH_AW_CHECKOUT_MANIFEST_COUNT: "1"`) + assert.Contains(t, out, `GH_AW_CHECKOUT_REPO_0: "owner/other"`) + assert.Contains(t, out, `GH_AW_CHECKOUT_PATH_0: "./other"`) + assert.Contains(t, out, "GH_AW_CHECKOUT_TOKEN_0: ${{ secrets.CROSS_REPO_PAT }}") + assert.Contains(t, out, "build_checkout_manifest.cjs") + assert.NotContains(t, out, "run: |", "manifest step should use github-script instead of shell run block") }) t.Run("cross-repo root checkout (empty path) is included", func(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ {Repository: "owner/other"}, }) - steps := cm.GenerateCheckoutManifestStep() + steps := cm.GenerateCheckoutManifestStep(getActionPin) require.Len(t, steps, 1, "cross-repo root checkout must be in manifest") out := steps[0] - assert.Contains(t, out, "repo='owner/other'") - assert.Contains(t, out, "path=''", "empty path should still be emitted (manifest consumer expects the key)") + assert.Contains(t, out, `GH_AW_CHECKOUT_REPO_0: "owner/other"`) + assert.Contains(t, out, `GH_AW_CHECKOUT_PATH_0: ""`, "empty path should still be emitted (manifest consumer expects the key)") }) t.Run("multiple cross-repo entries each get a jq update", func(t *testing.T) { @@ -1438,24 +1442,48 @@ func TestGenerateCheckoutManifestStep(t *testing.T) { {Path: "./local-only"}, // no repo → skipped {Repository: "owner/c", Path: "./c"}, // included }) - steps := cm.GenerateCheckoutManifestStep() + steps := cm.GenerateCheckoutManifestStep(getActionPin) require.Len(t, steps, 1) out := steps[0] - assert.Equal(t, 3, strings.Count(out, "resolve_default_branch \"$repo\" \"$path\""), "should invoke resolver once per cross-repo entry") - assert.Contains(t, out, "repo='owner/a'") - assert.Contains(t, out, "repo='owner/b'") - assert.Contains(t, out, "repo='owner/c'") + assert.Contains(t, out, `GH_AW_CHECKOUT_MANIFEST_COUNT: "3"`) + assert.Contains(t, out, `GH_AW_CHECKOUT_REPO_0: "owner/a"`) + assert.Contains(t, out, `GH_AW_CHECKOUT_REPO_1: "owner/b"`) + assert.Contains(t, out, `GH_AW_CHECKOUT_REPO_2: "owner/c"`) assert.NotContains(t, out, "./local-only", "path-only entries must not be in the manifest step") }) - t.Run("repository names containing single quotes are escaped", func(t *testing.T) { - // Repository names cannot actually contain single quotes per GitHub rules, - // but the shellSingleQuote helper must still defend against it. + t.Run("repository names containing single quotes are yaml-escaped", func(t *testing.T) { cm := NewCheckoutManager([]*CheckoutConfig{ {Repository: "weird'owner/repo", Path: "./x"}, }) - steps := cm.GenerateCheckoutManifestStep() + steps := cm.GenerateCheckoutManifestStep(getActionPin) + require.Len(t, steps, 1) + assert.Contains(t, steps[0], `GH_AW_CHECKOUT_REPO_0: "weird'owner/repo"`) + }) + + t.Run("dynamic repository expressions are emitted as raw env expressions", func(t *testing.T) { + cm := NewCheckoutManager([]*CheckoutConfig{ + {Repository: "${{ github.event.inputs.trigger_ref }}", Path: "./target"}, + }) + steps := cm.GenerateCheckoutManifestStep(getActionPin) + require.Len(t, steps, 1) + assert.Contains(t, steps[0], "GH_AW_CHECKOUT_REPO_0: ${{ github.event.inputs.trigger_ref }}") + }) + + t.Run("github-app token expression preserves checkout index in manifest env", func(t *testing.T) { + cm := NewCheckoutManager([]*CheckoutConfig{ + {Path: "./local"}, + { + Repository: "owner/private", + Path: "./private", + GitHubApp: &GitHubAppConfig{ + AppID: "${{ vars.APP_ID }}", + PrivateKey: "${{ secrets.APP_PRIVATE_KEY }}", + }, + }, + }) + steps := cm.GenerateCheckoutManifestStep(getActionPin) require.Len(t, steps, 1) - assert.Contains(t, steps[0], `repo='weird'\''owner/repo'`, "single quote must be escaped with '\\''") + assert.Contains(t, steps[0], "GH_AW_CHECKOUT_TOKEN_0: ${{ steps.checkout-app-token-1.outputs.token }}") }) } diff --git a/pkg/workflow/checkout_manifest_compile_test.go b/pkg/workflow/checkout_manifest_compile_test.go new file mode 100644 index 00000000000..ed92d453a16 --- /dev/null +++ b/pkg/workflow/checkout_manifest_compile_test.go @@ -0,0 +1,52 @@ +//go:build !integration + +package workflow + +import ( + "os" + "path/filepath" + "testing" + + "github.com/github/gh-aw/pkg/stringutil" + "github.com/stretchr/testify/require" +) + +func TestCompileDynamicCheckoutRepositoryManifestStepUsesGitHubScript(t *testing.T) { + tmpDir := t.TempDir() + workflowPath := filepath.Join(tmpDir, "repro.md") + content := `--- +on: + workflow_dispatch: + inputs: + trigger_ref: + type: string + required: true +engine: copilot +timeout-minutes: 5 +checkout: + - repository: ${{ github.event.inputs.trigger_ref }} + current: true +safe-outputs: + noop: + report-as-issue: false +--- +# Repro +Do nothing. +` + require.NoError(t, os.WriteFile(workflowPath, []byte(content), 0o644)) + + compiler := NewCompiler() + require.NoError(t, compiler.CompileWorkflow(workflowPath)) + + lockPath := stringutil.MarkdownToLockFile(workflowPath) + lockBytes, err := os.ReadFile(lockPath) + require.NoError(t, err) + lock := string(lockBytes) + + require.Contains(t, lock, "name: Build checkout manifest for safe-outputs handlers") + require.Contains(t, lock, "uses: actions/github-script") + require.Contains(t, lock, "build_checkout_manifest.cjs") + require.Contains(t, lock, "GH_AW_CHECKOUT_REPO_0: ${{ github.event.inputs.trigger_ref }}") + require.NotContains(t, lock, "repo='${{ github.event.inputs.trigger_ref }}'") + require.NotContains(t, lock, "Build checkout manifest for safe-outputs handlers\n run: |", "manifest step must not use run block") +} diff --git a/pkg/workflow/checkout_step_generator.go b/pkg/workflow/checkout_step_generator.go index 45020086ac8..11ef5c81d47 100644 --- a/pkg/workflow/checkout_step_generator.go +++ b/pkg/workflow/checkout_step_generator.go @@ -2,6 +2,7 @@ package workflow import ( "fmt" + "strconv" "strings" ) @@ -30,11 +31,11 @@ func wikiRepository(repository string) string { func (cm *CheckoutManager) GenerateCheckoutAppTokenSteps(c *Compiler, permissions *Permissions) []string { checkoutManagerLog.Printf("Building app token minting steps for %d checkout entries", len(cm.ordered)) var steps []string - for i, entry := range cm.ordered { + for checkoutIndex, entry := range cm.ordered { if entry.githubApp == nil { continue } - checkoutManagerLog.Printf("Generating app token minting step for checkout index=%d repo=%q", i, entry.key.repository) + checkoutManagerLog.Printf("Generating app token minting step for checkout index=%d repo=%q", checkoutIndex, entry.key.repository) // Pass empty fallback so the app token defaults to github.event.repository.name. // Checkout-specific cross-repo scoping is handled via the explicit repository field. steps = append(steps, c.buildGitHubAppTokenMintStepWithMeta( @@ -42,8 +43,8 @@ func (cm *CheckoutManager) GenerateCheckoutAppTokenSteps(c *Compiler, permission permissions, "", entry.key.repository, - fmt.Sprintf("Generate GitHub App token for checkout (%d)", i), - fmt.Sprintf("checkout-app-token-%d", i), + fmt.Sprintf("Generate GitHub App token for checkout (%d)", checkoutIndex), + fmt.Sprintf("checkout-app-token-%d", checkoutIndex), )...) } return steps @@ -55,12 +56,12 @@ func (cm *CheckoutManager) GenerateCheckoutAppTokenSteps(c *Compiler, permission func (cm *CheckoutManager) GenerateAdditionalCheckoutSteps(getActionPin func(string) string) []string { checkoutManagerLog.Printf("Generating additional checkout steps from %d configured entries", len(cm.ordered)) var lines []string - for i, entry := range cm.ordered { + for checkoutIndex, entry := range cm.ordered { // Skip the default checkout (handled separately) if entry.key.path == "" && entry.key.repository == "" { continue } - lines = append(lines, generateCheckoutStepLines(entry, i, getActionPin)...) + lines = append(lines, generateCheckoutStepLines(entry, checkoutIndex, getActionPin)...) } checkoutManagerLog.Printf("Generated %d additional checkout step(s)", len(lines)) return lines @@ -79,20 +80,25 @@ func (cm *CheckoutManager) GenerateAdditionalCheckoutSteps(getActionPin func(str // 2. `gh api repos// --jq .default_branch` as a credentialed fallback // // Returns an empty slice when there are no non-default cross-repo checkouts to record. -func (cm *CheckoutManager) GenerateCheckoutManifestStep() []string { +func (cm *CheckoutManager) GenerateCheckoutManifestStep(getActionPin func(string) string) []string { type manifestEntry struct { repository string path string + token string } var entries []manifestEntry - for _, entry := range cm.ordered { + for checkoutIndex, entry := range cm.ordered { if entry.key.wiki { continue } if entry.key.repository == "" { continue } - entries = append(entries, manifestEntry{repository: entry.key.repository, path: entry.key.path}) + entries = append(entries, manifestEntry{ + repository: entry.key.repository, + path: entry.key.path, + token: resolveCheckoutTokenExpression(entry, checkoutIndex, false), + }) } if len(entries) == 0 { return nil @@ -102,43 +108,39 @@ func (cm *CheckoutManager) GenerateCheckoutManifestStep() []string { var sb strings.Builder sb.WriteString(" - name: Build checkout manifest for safe-outputs handlers\n") + fmt.Fprintf(&sb, " uses: %s\n", getActionPin("actions/github-script")) sb.WriteString(" env:\n") sb.WriteString(" GH_TOKEN: ${{ secrets.GH_AW_GITHUB_TOKEN || secrets.GITHUB_TOKEN }}\n") - sb.WriteString(" run: |\n") - sb.WriteString(" set -euo pipefail\n") - sb.WriteString(" mkdir -p \"${RUNNER_TEMP}/gh-aw\"\n") - sb.WriteString(" manifest=\"${RUNNER_TEMP}/gh-aw/checkout-manifest.json\"\n") - sb.WriteString(" printf '{}' > \"$manifest\"\n") - sb.WriteString(" resolve_default_branch() {\n") - sb.WriteString(" local repo=\"$1\" path=\"$2\" db=\"\"\n") - sb.WriteString(" if [ -d \"${GITHUB_WORKSPACE}/${path}/.git\" ]; then\n") - sb.WriteString(" db=$(git -C \"${GITHUB_WORKSPACE}/${path}\" symbolic-ref --short refs/remotes/origin/HEAD 2>/dev/null | sed 's|^origin/||' || true)\n") - sb.WriteString(" fi\n") - sb.WriteString(" if [ -z \"$db\" ]; then\n") - sb.WriteString(" db=$(gh api \"repos/${repo}\" --jq '.default_branch' 2>/dev/null || true)\n") - sb.WriteString(" fi\n") - sb.WriteString(" printf '%s' \"$db\"\n") - sb.WriteString(" }\n") - for _, e := range entries { - // Both repo and path are static at compile time. Use shell-quoted literals. - fmt.Fprintf(&sb, " repo=%s\n", shellSingleQuote(e.repository)) - fmt.Fprintf(&sb, " path=%s\n", shellSingleQuote(e.path)) - sb.WriteString(" db=$(resolve_default_branch \"$repo\" \"$path\")\n") - sb.WriteString(" tmp=$(mktemp)\n") - sb.WriteString(" jq --arg repo \"$repo\" --arg path \"$path\" --arg db \"$db\" \\\n") - sb.WriteString(" '.[($repo | ascii_downcase)] = {repository: $repo, path: $path, default_branch: $db}' \\\n") - sb.WriteString(" \"$manifest\" > \"$tmp\" && mv \"$tmp\" \"$manifest\"\n") - sb.WriteString(" echo \"checkout-manifest: ${repo} -> path=${path} default_branch=${db:-}\"\n") - } - sb.WriteString(" cat \"$manifest\"\n") + writeYAMLEnv(&sb, " ", "GH_AW_CHECKOUT_MANIFEST_COUNT", strconv.Itoa(len(entries))) + for manifestIndex, e := range entries { + repoKey := fmt.Sprintf("GH_AW_CHECKOUT_REPO_%d", manifestIndex) + pathKey := fmt.Sprintf("GH_AW_CHECKOUT_PATH_%d", manifestIndex) + if strings.Contains(e.repository, "${{") { + fmt.Fprintf(&sb, " %s: %s\n", repoKey, githubExpressionWhitespaceReplacer.Replace(e.repository)) + } else { + writeYAMLEnv(&sb, " ", repoKey, e.repository) + } + if strings.Contains(e.path, "${{") { + fmt.Fprintf(&sb, " %s: %s\n", pathKey, githubExpressionWhitespaceReplacer.Replace(e.path)) + } else { + writeYAMLEnv(&sb, " ", pathKey, e.path) + } + if e.token != "" { + tokenKey := fmt.Sprintf("GH_AW_CHECKOUT_TOKEN_%d", manifestIndex) + if strings.Contains(e.token, "${{") { + fmt.Fprintf(&sb, " %s: %s\n", tokenKey, githubExpressionWhitespaceReplacer.Replace(e.token)) + } else { + writeYAMLEnv(&sb, " ", tokenKey, e.token) + } + } + } + sb.WriteString(" with:\n") + sb.WriteString(" script: |\n") + sb.WriteString(" const { main } = require('${{ runner.temp }}/gh-aw/actions/build_checkout_manifest.cjs');\n") + sb.WriteString(" await main();\n") return []string{sb.String()} } -// shellSingleQuote returns s wrapped in single quotes, escaping any embedded single quotes. -func shellSingleQuote(s string) string { - return "'" + strings.ReplaceAll(s, "'", `'\''`) + "'" -} - // GenerateGitHubFolderCheckoutStep generates YAML step lines for a sparse checkout of // the .github and .agents folders. This is used in the activation job to access workflow // configuration and runtime imports. @@ -339,15 +341,7 @@ func generateCheckoutStepLines(entry *resolvedCheckout, index int, getActionPin fmt.Fprintf(&sb, " path: %s\n", entry.key.path) } // Determine effective token: github-app-minted token takes precedence - effectiveToken := entry.token - if entry.githubApp != nil { - // The token is minted in the agent job itself (same-job step reference). - //nolint:gosec // G101: False positive - this is a GitHub Actions expression template placeholder, not a hardcoded credential - effectiveToken = fmt.Sprintf("${{ steps.checkout-app-token-%d.outputs.token }}", index) - if entry.githubApp.shouldIgnoreMissingKey() { - effectiveToken = combineTokenExpressions(effectiveToken, getEffectiveGitHubToken(entry.token)) - } - } + effectiveToken := resolveCheckoutTokenExpression(entry, index, false) if effectiveToken != "" { fmt.Fprintf(&sb, " token: %s\n", effectiveToken) } @@ -469,18 +463,7 @@ func generateFetchStepLines(entry *resolvedCheckout, index int) string { } // Determine authentication token - token := entry.token - if entry.githubApp != nil { - // The token is minted in the agent job itself (same-job step reference). - //nolint:gosec // G101: False positive - this is a GitHub Actions expression template placeholder, not a hardcoded credential - token = fmt.Sprintf("${{ steps.checkout-app-token-%d.outputs.token }}", index) - if entry.githubApp.shouldIgnoreMissingKey() { - token = combineTokenExpressions(token, getEffectiveGitHubToken(entry.token)) - } - } - if token == "" { - token = getEffectiveGitHubToken("") - } + token := resolveCheckoutTokenExpression(entry, index, true) // Build refspecs refspecs := make([]string, 0, len(entry.fetchRefs)) @@ -518,3 +501,19 @@ func generateFetchStepLines(entry *resolvedCheckout, index int) string { gitPrefix, depthFlag, strings.Join(refspecs, " ")) return sb.String() } + +func resolveCheckoutTokenExpression(entry *resolvedCheckout, checkoutIndex int, defaultWhenEmpty bool) string { + token := entry.token + if entry.githubApp != nil { + // The token is minted in the agent job itself (same-job step reference). + //nolint:gosec // G101: False positive - this is a GitHub Actions expression template placeholder, not a hardcoded credential + token = fmt.Sprintf("${{ steps.checkout-app-token-%d.outputs.token }}", checkoutIndex) + if entry.githubApp.shouldIgnoreMissingKey() { + token = combineTokenExpressions(token, getEffectiveGitHubToken(entry.token)) + } + } + if token == "" && defaultWhenEmpty { + token = getEffectiveGitHubToken("") + } + return token +} diff --git a/pkg/workflow/compiler_yaml_main_job.go b/pkg/workflow/compiler_yaml_main_job.go index f0fbb29f3b4..7043017e68e 100644 --- a/pkg/workflow/compiler_yaml_main_job.go +++ b/pkg/workflow/compiler_yaml_main_job.go @@ -143,7 +143,7 @@ func (c *Compiler) generateInitialAndCheckoutSteps(yaml *strings.Builder, data * // Emit a manifest step that records the path and resolved default branch for each // non-default cross-repo checkout. The safe-outputs MCP server reads this file to // resolve base branches without making any credentialed network calls. - for _, line := range checkoutMgr.GenerateCheckoutManifestStep() { + for _, line := range checkoutMgr.GenerateCheckoutManifestStep(c.getActionPin) { yaml.WriteString(line) }