diff --git a/.github/workflows/code-scanning-fixer.lock.yml b/.github/workflows/code-scanning-fixer.lock.yml index 93cded9aca..eb3485cb63 100644 --- a/.github/workflows/code-scanning-fixer.lock.yml +++ b/.github/workflows/code-scanning-fixer.lock.yml @@ -229,8 +229,6 @@ jobs: contents: read pull-requests: read security-events: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/codex-github-remote-mcp-test.lock.yml b/.github/workflows/codex-github-remote-mcp-test.lock.yml index 08a79efe70..2ee992a9f1 100644 --- a/.github/workflows/codex-github-remote-mcp-test.lock.yml +++ b/.github/workflows/codex-github-remote-mcp-test.lock.yml @@ -204,8 +204,6 @@ jobs: permissions: contents: read issues: read - concurrency: - group: "gh-aw-codex-${{ github.workflow }}" env: GH_AW_WORKFLOW_ID_SANITIZED: codexgithubremotemcptest outputs: diff --git a/.github/workflows/commit-changes-analyzer.lock.yml b/.github/workflows/commit-changes-analyzer.lock.yml index ac4cb5bacc..7fc028305d 100644 --- a/.github/workflows/commit-changes-analyzer.lock.yml +++ b/.github/workflows/commit-changes-analyzer.lock.yml @@ -227,8 +227,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-claude-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/example-permissions-warning.lock.yml b/.github/workflows/example-permissions-warning.lock.yml index 9f3a3a1ade..0369cc372c 100644 --- a/.github/workflows/example-permissions-warning.lock.yml +++ b/.github/workflows/example-permissions-warning.lock.yml @@ -204,8 +204,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: GH_AW_WORKFLOW_ID_SANITIZED: examplepermissionswarning outputs: diff --git a/.github/workflows/firewall.lock.yml b/.github/workflows/firewall.lock.yml index f995f6c588..a6d844b889 100644 --- a/.github/workflows/firewall.lock.yml +++ b/.github/workflows/firewall.lock.yml @@ -206,8 +206,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: GH_AW_WORKFLOW_ID_SANITIZED: firewall outputs: diff --git a/.github/workflows/notion-issue-summary.lock.yml b/.github/workflows/notion-issue-summary.lock.yml index d8d91e53c8..6a23ebc71b 100644 --- a/.github/workflows/notion-issue-summary.lock.yml +++ b/.github/workflows/notion-issue-summary.lock.yml @@ -225,8 +225,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/python-data-charts.lock.yml b/.github/workflows/python-data-charts.lock.yml index a9efd030dd..548accf432 100644 --- a/.github/workflows/python-data-charts.lock.yml +++ b/.github/workflows/python-data-charts.lock.yml @@ -236,8 +236,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: ".png,.jpg,.jpeg" diff --git a/.github/workflows/release.lock.yml b/.github/workflows/release.lock.yml index 814feb8bce..942e907b17 100644 --- a/.github/workflows/release.lock.yml +++ b/.github/workflows/release.lock.yml @@ -230,8 +230,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/repo-audit-analyzer.lock.yml b/.github/workflows/repo-audit-analyzer.lock.yml index 9ec1a819bb..7bdd9a23be 100644 --- a/.github/workflows/repo-audit-analyzer.lock.yml +++ b/.github/workflows/repo-audit-analyzer.lock.yml @@ -234,8 +234,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/research.lock.yml b/.github/workflows/research.lock.yml index d432f268ab..c4ed63cf6e 100644 --- a/.github/workflows/research.lock.yml +++ b/.github/workflows/research.lock.yml @@ -231,8 +231,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/technical-doc-writer.lock.yml b/.github/workflows/technical-doc-writer.lock.yml index 71251e83c1..86f14d831f 100644 --- a/.github/workflows/technical-doc-writer.lock.yml +++ b/.github/workflows/technical-doc-writer.lock.yml @@ -242,8 +242,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: ".png,.jpg,.jpeg" diff --git a/.github/workflows/test-create-pr-error-handling.lock.yml b/.github/workflows/test-create-pr-error-handling.lock.yml index f352073d15..c88c7ef70d 100644 --- a/.github/workflows/test-create-pr-error-handling.lock.yml +++ b/.github/workflows/test-create-pr-error-handling.lock.yml @@ -219,8 +219,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-claude-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/test-dispatcher.lock.yml b/.github/workflows/test-dispatcher.lock.yml index acfbea6831..88c7f2200d 100644 --- a/.github/workflows/test-dispatcher.lock.yml +++ b/.github/workflows/test-dispatcher.lock.yml @@ -207,8 +207,6 @@ jobs: permissions: contents: read issues: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/test-project-url-default.lock.yml b/.github/workflows/test-project-url-default.lock.yml index c866f0da56..19e33d8ed1 100644 --- a/.github/workflows/test-project-url-default.lock.yml +++ b/.github/workflows/test-project-url-default.lock.yml @@ -206,8 +206,6 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/.github/workflows/test-workflow.lock.yml b/.github/workflows/test-workflow.lock.yml index c5dbae4531..345ec25577 100644 --- a/.github/workflows/test-workflow.lock.yml +++ b/.github/workflows/test-workflow.lock.yml @@ -206,8 +206,6 @@ jobs: runs-on: ubuntu-latest permissions: contents: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: GH_AW_WORKFLOW_ID_SANITIZED: testworkflow outputs: diff --git a/.github/workflows/video-analyzer.lock.yml b/.github/workflows/video-analyzer.lock.yml index fb8cc30ec0..23cd6aae1c 100644 --- a/.github/workflows/video-analyzer.lock.yml +++ b/.github/workflows/video-analyzer.lock.yml @@ -227,8 +227,6 @@ jobs: contents: read issues: read pull-requests: read - concurrency: - group: "gh-aw-copilot-${{ github.workflow }}" env: DEFAULT_BRANCH: ${{ github.event.repository.default_branch }} GH_AW_ASSETS_ALLOWED_EXTS: "" diff --git a/actions/setup/js/checkout_pr_branch.test.cjs b/actions/setup/js/checkout_pr_branch.test.cjs index bdaa893645..3a3b405ed0 100644 --- a/actions/setup/js/checkout_pr_branch.test.cjs +++ b/actions/setup/js/checkout_pr_branch.test.cjs @@ -15,6 +15,7 @@ describe("checkout_pr_branch.cjs", () => { setOutput: vi.fn(), startGroup: vi.fn(), endGroup: vi.fn(), + exportVariable: vi.fn(), summary: { addRaw: vi.fn().mockReturnThis(), write: vi.fn().mockResolvedValue(undefined), diff --git a/actions/setup/js/create_discussion_category_normalization.test.cjs b/actions/setup/js/create_discussion_category_normalization.test.cjs index b4d4010a86..a1ae88da50 100644 --- a/actions/setup/js/create_discussion_category_normalization.test.cjs +++ b/actions/setup/js/create_discussion_category_normalization.test.cjs @@ -105,8 +105,13 @@ describe("create_discussion category normalization", () => { }); afterEach(() => { - // Restore environment - process.env = originalEnv; + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); vi.clearAllMocks(); }); diff --git a/actions/setup/js/create_discussion_fallback.test.cjs b/actions/setup/js/create_discussion_fallback.test.cjs index 38c0a456a9..2f2b787f1f 100644 --- a/actions/setup/js/create_discussion_fallback.test.cjs +++ b/actions/setup/js/create_discussion_fallback.test.cjs @@ -102,8 +102,13 @@ describe("create_discussion fallback with close_older_discussions", () => { }); afterEach(() => { - // Restore environment - process.env = originalEnv; + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); vi.clearAllMocks(); }); diff --git a/actions/setup/js/create_discussion_labels.test.cjs b/actions/setup/js/create_discussion_labels.test.cjs index d5e4c569b4..8537c39039 100644 --- a/actions/setup/js/create_discussion_labels.test.cjs +++ b/actions/setup/js/create_discussion_labels.test.cjs @@ -136,8 +136,13 @@ describe("create_discussion with labels", () => { }); afterEach(() => { - // Restore environment - process.env = originalEnv; + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); // Clear mocks vi.clearAllMocks(); diff --git a/actions/setup/js/create_issue.test.cjs b/actions/setup/js/create_issue.test.cjs index 4375ee72ee..e6d71690e4 100644 --- a/actions/setup/js/create_issue.test.cjs +++ b/actions/setup/js/create_issue.test.cjs @@ -1,4 +1,3 @@ -// @ts-check import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createRequire } from "module"; @@ -81,8 +80,13 @@ describe("create_issue", () => { }); afterEach(() => { - // Restore original environment - process.env = originalEnv; + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); vi.clearAllMocks(); }); diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index 1c1bbc9275..2dd78d6007 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -553,6 +553,10 @@ async function main(config = {}) { core.info(`Created new branch from base: ${branchName}`); // Apply the patch using git CLI (skip if empty) + // Track number of new commits pushed so we can restrict the extra empty commit + // to branches with exactly one new commit (security: prevents use of CI trigger + // token on multi-commit branches where workflow files may have been modified). + let newCommitCount = 0; if (!isEmpty) { core.info("Applying patch..."); @@ -565,8 +569,10 @@ async function main(config = {}) { } // Patches are created with git format-patch, so use git am to apply them + // Use --3way to handle cross-repo patches where the patch base may differ from target repo + // This allows git to resolve create-vs-modify mismatches when a file exists in target but not source try { - await exec.exec(`git am ${patchFilePath}`); + await exec.exec(`git am --3way ${patchFilePath}`); core.info("Patch applied successfully"); } catch (patchError) { core.error(`Failed to apply patch: ${patchError instanceof Error ? patchError.message : String(patchError)}`); @@ -616,6 +622,17 @@ async function main(config = {}) { await exec.exec(`git push origin ${branchName}`); core.info("Changes pushed to branch"); + + // Count new commits on PR branch relative to base, used to restrict + // the extra empty CI-trigger commit to exactly 1 new commit. + try { + const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `origin/${baseBranch}..HEAD`]); + newCommitCount = parseInt(countStr.trim(), 10); + core.info(`${newCommitCount} new commit(s) on branch relative to origin/${baseBranch}`); + } catch { + // Non-fatal - newCommitCount stays 0, extra empty commit will be skipped + core.info("Could not count new commits - extra empty commit will be skipped"); + } } catch (pushError) { // Push failed - create fallback issue instead of PR (if fallback is enabled) core.error(`Git push failed: ${pushError instanceof Error ? pushError.message : String(pushError)}`); @@ -664,8 +681,8 @@ To apply the patch locally: gh run download ${runId} -n agent-artifacts -D /tmp/agent-artifacts-${runId} # The patch file will be at agent-artifacts/tmp/gh-aw/${patchFileName} after download -# Apply the patch -git am /tmp/agent-artifacts-${runId}/${patchFileName} +# Apply the patch (--3way handles cross-repo patches where files may already exist) +git am --3way /tmp/agent-artifacts-${runId}/${patchFileName} \`\`\` ${patchPreview}`; @@ -750,6 +767,16 @@ ${patchPreview}`; await exec.exec(`git push origin ${branchName}`); core.info("Empty branch pushed successfully"); + + // Count new commits (will be 1 from the Initialize commit) + try { + const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `origin/${baseBranch}..HEAD`]); + newCommitCount = parseInt(countStr.trim(), 10); + core.info(`${newCommitCount} new commit(s) on branch relative to origin/${baseBranch}`); + } catch { + // Non-fatal - newCommitCount stays 0, extra empty commit will be skipped + core.info("Could not count new commits - extra empty commit will be skipped"); + } } catch (pushError) { const error = `Failed to push empty branch: ${pushError instanceof Error ? pushError.message : String(pushError)}`; core.error(error); @@ -840,12 +867,15 @@ ${patchPreview}`; ) .write(); - // Push an extra empty commit if a token is configured. + // Push an extra empty commit if a token is configured and exactly 1 new commit was pushed. // This works around the GITHUB_TOKEN limitation where pushes don't trigger CI events. + // Restricting to exactly 1 new commit prevents the CI trigger token being used on + // multi-commit branches where workflow files may have been iteratively modified. const ciTriggerResult = await pushExtraEmptyCommit({ branchName, repoOwner: repoParts.owner, repoName: repoParts.repo, + newCommitCount, }); if (ciTriggerResult.success && !ciTriggerResult.skipped) { core.info("Extra empty commit pushed - CI checks should start shortly"); diff --git a/actions/setup/js/extra_empty_commit.cjs b/actions/setup/js/extra_empty_commit.cjs index 6819bf8b5b..41ee5fa827 100644 --- a/actions/setup/js/extra_empty_commit.cjs +++ b/actions/setup/js/extra_empty_commit.cjs @@ -27,9 +27,12 @@ * @param {string} options.repoOwner - Repository owner * @param {string} options.repoName - Repository name * @param {string} [options.commitMessage] - Custom commit message (default: "ci: trigger CI checks") + * @param {number} [options.newCommitCount] - Number of new commits being pushed. Only pushes the + * empty commit when exactly 1 new commit was pushed, preventing accidental workflow-file + * modifications on multi-commit branches and reducing loop risk. * @returns {Promise<{success: boolean, skipped?: boolean, error?: string}>} */ -async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMessage }) { +async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMessage, newCommitCount }) { const token = process.env.GH_AW_CI_TRIGGER_TOKEN; if (!token || !token.trim()) { @@ -37,6 +40,11 @@ async function pushExtraEmptyCommit({ branchName, repoOwner, repoName, commitMes return { success: true, skipped: true }; } + if (newCommitCount !== undefined && newCommitCount !== 1) { + core.info(`Skipping extra empty commit: ${newCommitCount} new commit(s) pushed (only triggers for exactly 1 commit)`); + return { success: true, skipped: true }; + } + core.info("Extra empty commit token detected - pushing empty commit to trigger CI events"); try { diff --git a/actions/setup/js/extra_empty_commit.test.cjs b/actions/setup/js/extra_empty_commit.test.cjs index d3b69e7488..27f16a6da0 100644 --- a/actions/setup/js/extra_empty_commit.test.cjs +++ b/actions/setup/js/extra_empty_commit.test.cjs @@ -321,6 +321,78 @@ describe("extra_empty_commit.cjs", () => { }); }); + // ────────────────────────────────────────────────────── + // newCommitCount restriction + // ────────────────────────────────────────────────────── + + describe("newCommitCount restriction", () => { + beforeEach(() => { + process.env.GH_AW_CI_TRIGGER_TOKEN = "ghp_test_token_123"; + // No empty commits in log (so cycle prevention doesn't interfere) + mockGitLogOutput("COMMIT:abc123\nfile.txt\n"); + ({ pushExtraEmptyCommit } = require("./extra_empty_commit.cjs")); + }); + + it("should proceed when newCommitCount is exactly 1", async () => { + const result = await pushExtraEmptyCommit({ + branchName: "feature-branch", + repoOwner: "test-owner", + repoName: "test-repo", + newCommitCount: 1, + }); + + expect(result).toEqual({ success: true }); + // Should have committed and pushed + const commitCall = mockExec.exec.mock.calls.find(c => c[0] === "git" && c[1] && c[1][0] === "commit"); + expect(commitCall).toBeDefined(); + }); + + it("should skip when newCommitCount is 0", async () => { + const result = await pushExtraEmptyCommit({ + branchName: "feature-branch", + repoOwner: "test-owner", + repoName: "test-repo", + newCommitCount: 0, + }); + + expect(result).toEqual({ success: true, skipped: true }); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("0 new commit(s)")); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("only triggers for exactly 1 commit")); + // Should NOT have committed + const commitCall = mockExec.exec.mock.calls.find(c => c[0] === "git" && c[1] && c[1][0] === "commit"); + expect(commitCall).toBeUndefined(); + }); + + it("should skip when newCommitCount is 2", async () => { + const result = await pushExtraEmptyCommit({ + branchName: "feature-branch", + repoOwner: "test-owner", + repoName: "test-repo", + newCommitCount: 2, + }); + + expect(result).toEqual({ success: true, skipped: true }); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("2 new commit(s)")); + // Should NOT have committed + const commitCall = mockExec.exec.mock.calls.find(c => c[0] === "git" && c[1] && c[1][0] === "commit"); + expect(commitCall).toBeUndefined(); + }); + + it("should proceed when newCommitCount is not provided (backward compatibility)", async () => { + const result = await pushExtraEmptyCommit({ + branchName: "feature-branch", + repoOwner: "test-owner", + repoName: "test-repo", + // newCommitCount omitted + }); + + expect(result).toEqual({ success: true }); + // Should have committed (no restriction when parameter is absent) + const commitCall = mockExec.exec.mock.calls.find(c => c[0] === "git" && c[1] && c[1][0] === "commit"); + expect(commitCall).toBeDefined(); + }); + }); + // ────────────────────────────────────────────────────── // Error handling // ────────────────────────────────────────────────────── diff --git a/actions/setup/js/generate_git_patch.cjs b/actions/setup/js/generate_git_patch.cjs index ce1c572209..d4b0c8fbb6 100644 --- a/actions/setup/js/generate_git_patch.cjs +++ b/actions/setup/js/generate_git_patch.cjs @@ -8,6 +8,17 @@ const { getBaseBranch } = require("./get_base_branch.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { execGitSync } = require("./git_helpers.cjs"); +/** + * Debug logging helper - logs to stderr when DEBUG env var matches + * @param {string} message - Debug message to log + */ +function debugLog(message) { + const debug = process.env.DEBUG || ""; + if (debug === "*" || debug.includes("generate_git_patch") || debug.includes("patch")) { + console.error(`[generate_git_patch] ${message}`); + } +} + /** * Sanitize a branch name for use as a patch filename * Replaces path separators and special characters with dashes @@ -36,14 +47,27 @@ function getPatchPath(branchName) { /** * Generates a git patch file for the current changes * @param {string} branchName - The branch name to generate patch for + * @param {Object} [options] - Optional parameters + * @param {string} [options.mode="full"] - Patch generation mode: + * - "full": Include all commits since merge-base with default branch (for create_pull_request) + * - "incremental": Only include commits since origin/branchName (for push_to_pull_request_branch) + * In incremental mode, origin/branchName is fetched explicitly and merge-base fallback is disabled. * @returns {Object} Object with patch info or error */ -function generateGitPatch(branchName) { +function generateGitPatch(branchName, options = {}) { + const mode = options.mode || "full"; const patchPath = getPatchPath(branchName); const cwd = process.env.GITHUB_WORKSPACE || process.cwd(); + // NOTE: In cross-repo scenarios, DEFAULT_BRANCH comes from the workflow repository + // (via github.event.repository.default_branch), not the checked-out target repository. + // If the target repo has a different default branch (e.g., "master" vs "main"), + // Strategy 1's merge-base calculation may fail. Strategy 3 handles this gracefully. const defaultBranch = process.env.DEFAULT_BRANCH || getBaseBranch(); const githubSha = process.env.GITHUB_SHA; + debugLog(`Starting patch generation: mode=${mode}, branch=${branchName}, defaultBranch=${defaultBranch}`); + debugLog(`Environment: cwd=${cwd}, GITHUB_SHA=${githubSha || "(not set)"}`); + // Ensure /tmp/gh-aw directory exists const patchDir = path.dirname(patchPath); if (!fs.existsSync(patchDir)) { @@ -56,24 +80,89 @@ function generateGitPatch(branchName) { try { // Strategy 1: If we have a branch name, check if that branch exists and get its diff if (branchName) { + debugLog(`Strategy 1: Checking if branch '${branchName}' exists locally`); // Check if the branch exists locally try { execGitSync(["show-ref", "--verify", "--quiet", `refs/heads/${branchName}`], { cwd }); + debugLog(`Strategy 1: Branch '${branchName}' exists locally`); // Determine base ref for patch generation let baseRef; - try { - // Check if origin/branchName exists - execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd }); - baseRef = `origin/${branchName}`; - } catch { - // Use merge-base with default branch - execGitSync(["fetch", "origin", defaultBranch], { cwd }); - baseRef = execGitSync(["merge-base", `origin/${defaultBranch}`, branchName], { cwd }).trim(); + + if (mode === "incremental") { + // INCREMENTAL MODE (for push_to_pull_request_branch): + // Only include commits that are new since origin/branchName. + // This prevents including commits that already exist on the PR branch. + // We must explicitly fetch origin/branchName and fail if it doesn't exist. + + debugLog(`Strategy 1 (incremental): Fetching origin/${branchName}`); + try { + // Explicitly fetch origin/branchName to ensure we have the latest + // Use "--" to prevent branch names starting with "-" from being interpreted as options + execGitSync(["fetch", "origin", "--", `${branchName}:refs/remotes/origin/${branchName}`], { cwd }); + baseRef = `origin/${branchName}`; + debugLog(`Strategy 1 (incremental): Successfully fetched, baseRef=${baseRef}`); + } catch (fetchError) { + // In incremental mode, we MUST have origin/branchName - no fallback + debugLog(`Strategy 1 (incremental): Fetch failed - ${getErrorMessage(fetchError)}`); + errorMessage = `Cannot generate incremental patch: failed to fetch origin/${branchName}. ` + `This typically happens when the remote branch doesn't exist yet or was force-pushed. ` + `Error: ${getErrorMessage(fetchError)}`; + // Don't try other strategies in incremental mode + return { + success: false, + error: errorMessage, + patchPath: patchPath, + }; + } + } else { + // FULL MODE (for create_pull_request): + // Include all commits since merge-base with default branch. + // This is appropriate for creating new PRs where we want all changes. + + debugLog(`Strategy 1 (full): Checking if origin/${branchName} exists`); + try { + // Check if origin/branchName exists + execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${branchName}`], { cwd }); + baseRef = `origin/${branchName}`; + debugLog(`Strategy 1 (full): Using existing origin/${branchName} as baseRef`); + } catch { + // origin/branchName doesn't exist - use merge-base with default branch + debugLog(`Strategy 1 (full): origin/${branchName} not found, trying merge-base with ${defaultBranch}`); + // First check if origin/ already exists locally (e.g., from checkout with fetch-depth: 0) + // This is important for cross-repo checkouts where persist-credentials: false prevents fetching + let hasLocalDefaultBranch = false; + try { + execGitSync(["show-ref", "--verify", "--quiet", `refs/remotes/origin/${defaultBranch}`], { cwd }); + hasLocalDefaultBranch = true; + debugLog(`Strategy 1 (full): origin/${defaultBranch} exists locally`); + } catch { + // origin/ doesn't exist locally, try to fetch it + debugLog(`Strategy 1 (full): origin/${defaultBranch} not found locally, attempting fetch`); + try { + // Use "--" to prevent branch names starting with "-" from being interpreted as options + execGitSync(["fetch", "origin", "--", defaultBranch], { cwd }); + hasLocalDefaultBranch = true; + debugLog(`Strategy 1 (full): Successfully fetched origin/${defaultBranch}`); + } catch (fetchErr) { + // Fetch failed (likely due to persist-credentials: false in cross-repo checkout) + // We'll try other strategies below + debugLog(`Strategy 1 (full): Fetch failed - ${getErrorMessage(fetchErr)} (will try other strategies)`); + } + } + + if (hasLocalDefaultBranch) { + baseRef = execGitSync(["merge-base", "--", `origin/${defaultBranch}`, branchName], { cwd }).trim(); + debugLog(`Strategy 1 (full): Computed merge-base: ${baseRef}`); + } else { + // No remote refs available - fall through to Strategy 2 + debugLog(`Strategy 1 (full): No remote refs available, falling through to Strategy 2`); + throw new Error("No remote refs available for merge-base calculation"); + } + } } // Count commits to be included const commitCount = parseInt(execGitSync(["rev-list", "--count", `${baseRef}..${branchName}`], { cwd }).trim(), 10); + debugLog(`Strategy 1: Found ${commitCount} commits between ${baseRef} and ${branchName}`); if (commitCount > 0) { // Generate patch from the determined base to the branch @@ -82,43 +171,148 @@ function generateGitPatch(branchName) { if (patchContent && patchContent.trim()) { fs.writeFileSync(patchPath, patchContent, "utf8"); patchGenerated = true; + debugLog(`Strategy 1: SUCCESS - Generated patch with ${patchContent.split("\n").length} lines`); } + } else if (mode === "incremental") { + // In incremental mode, zero commits means nothing new to push + return { + success: false, + error: "No new commits to push - your changes may already be on the remote branch", + patchPath: patchPath, + patchSize: 0, + patchLines: 0, + }; } } catch (branchError) { // Branch does not exist locally + debugLog(`Strategy 1: Branch '${branchName}' does not exist locally - ${getErrorMessage(branchError)}`); + if (mode === "incremental") { + return { + success: false, + error: `Branch ${branchName} does not exist locally. Cannot generate incremental patch.`, + patchPath: patchPath, + }; + } } } // Strategy 2: Check if commits were made to current HEAD since checkout if (!patchGenerated) { + debugLog(`Strategy 2: Checking commits since GITHUB_SHA`); const currentHead = execGitSync(["rev-parse", "HEAD"], { cwd }).trim(); + debugLog(`Strategy 2: currentHead=${currentHead}, GITHUB_SHA=${githubSha || "(not set)"}`); if (!githubSha) { + debugLog(`Strategy 2: GITHUB_SHA not set, cannot use this strategy`); errorMessage = "GITHUB_SHA environment variable is not set"; } else if (currentHead === githubSha) { // No commits have been made since checkout + debugLog(`Strategy 2: HEAD equals GITHUB_SHA - no new commits`); } else { - // Check if GITHUB_SHA is an ancestor of current HEAD + // First verify GITHUB_SHA exists in this repo's git history + // In cross-repo checkout scenarios, GITHUB_SHA is from the workflow repo, + // not the target repo that's currently checked out + let shaExistsInRepo = false; try { - execGitSync(["merge-base", "--is-ancestor", githubSha, "HEAD"], { cwd }); + execGitSync(["cat-file", "-e", githubSha], { cwd }); + shaExistsInRepo = true; + debugLog(`Strategy 2: GITHUB_SHA exists in this repo`); + } catch { + // GITHUB_SHA doesn't exist in this repo - likely a cross-repo checkout + // This is expected when workflow repo != checked out repo + debugLog(`Strategy 2: GITHUB_SHA not found in repo (cross-repo checkout?)`); + } + + if (shaExistsInRepo) { + // Check if GITHUB_SHA is an ancestor of current HEAD + try { + execGitSync(["merge-base", "--is-ancestor", githubSha, "HEAD"], { cwd }); + debugLog(`Strategy 2: GITHUB_SHA is an ancestor of HEAD`); - // Count commits between GITHUB_SHA and HEAD - const commitCount = parseInt(execGitSync(["rev-list", "--count", `${githubSha}..HEAD`], { cwd }).trim(), 10); + // Count commits between GITHUB_SHA and HEAD + const commitCount = parseInt(execGitSync(["rev-list", "--count", `${githubSha}..HEAD`], { cwd }).trim(), 10); + debugLog(`Strategy 2: Found ${commitCount} commits between GITHUB_SHA and HEAD`); - if (commitCount > 0) { - // Generate patch from GITHUB_SHA to HEAD - const patchContent = execGitSync(["format-patch", `${githubSha}..HEAD`, "--stdout"], { cwd }); + if (commitCount > 0) { + // Generate patch from GITHUB_SHA to HEAD + const patchContent = execGitSync(["format-patch", `${githubSha}..HEAD`, "--stdout"], { cwd }); - if (patchContent && patchContent.trim()) { - fs.writeFileSync(patchPath, patchContent, "utf8"); - patchGenerated = true; + if (patchContent && patchContent.trim()) { + fs.writeFileSync(patchPath, patchContent, "utf8"); + patchGenerated = true; + debugLog(`Strategy 2: SUCCESS - Generated patch with ${patchContent.split("\n").length} lines`); + } } + } catch (ancestorErr) { + // GITHUB_SHA is not an ancestor of HEAD - repository state has diverged + debugLog(`Strategy 2: GITHUB_SHA is not an ancestor of HEAD - ${getErrorMessage(ancestorErr)}`); } - } catch { - // GITHUB_SHA is not an ancestor of HEAD - repository state has diverged } } } + + // Strategy 3: Cross-repo fallback - find commits not reachable from any remote ref + // This handles cases where: + // - Cross-repo checkout with persist-credentials: false (can't fetch) + // - GITHUB_SHA is from a different repo + // - No origin/ available locally + if (!patchGenerated && branchName) { + debugLog(`Strategy 3: Cross-repo fallback - finding commits not reachable from remote refs`); + try { + // Get all remote refs + const remoteRefsOutput = execGitSync(["for-each-ref", "--format=%(refname)", "refs/remotes/"], { cwd }).trim(); + + if (remoteRefsOutput) { + // Build exclusion list from all remote refs + const remoteRefs = remoteRefsOutput.split("\n").filter(r => r); + debugLog(`Strategy 3: Found ${remoteRefs.length} remote refs: ${remoteRefs.slice(0, 5).join(", ")}${remoteRefs.length > 5 ? "..." : ""}`); + + if (remoteRefs.length > 0) { + // Find commits on current branch not reachable from any remote ref + // This gets commits the agent added that haven't been pushed anywhere + const excludeArgs = remoteRefs.flatMap(ref => ["--not", ref]); + const revListArgs = ["rev-list", "--count", branchName, ...excludeArgs]; + + const commitCount = parseInt(execGitSync(revListArgs, { cwd }).trim(), 10); + debugLog(`Strategy 3: Found ${commitCount} commits not reachable from any remote ref`); + + if (commitCount > 0) { + // Get the merge-base with the first remote ref (typically origin/HEAD or origin/main) + // to determine the starting point for the patch + let baseCommit; + for (const ref of remoteRefs) { + try { + baseCommit = execGitSync(["merge-base", ref, branchName], { cwd }).trim(); + if (baseCommit) { + debugLog(`Strategy 3: Found merge-base ${baseCommit} with ref ${ref}`); + break; + } + } catch { + // Try next ref + } + } + + if (baseCommit) { + const patchContent = execGitSync(["format-patch", `${baseCommit}..${branchName}`, "--stdout"], { cwd }); + + if (patchContent && patchContent.trim()) { + fs.writeFileSync(patchPath, patchContent, "utf8"); + patchGenerated = true; + debugLog(`Strategy 3: SUCCESS - Generated patch with ${patchContent.split("\n").length} lines`); + } + } else { + debugLog(`Strategy 3: Could not find merge-base with any remote ref`); + } + } + } + } else { + debugLog(`Strategy 3: No remote refs found`); + } + } catch (strategy3Err) { + // Strategy 3 failed - no remote refs available at all + debugLog(`Strategy 3: Failed - ${getErrorMessage(strategy3Err)}`); + } + } } catch (error) { errorMessage = `Failed to generate patch: ${getErrorMessage(error)}`; } @@ -131,6 +325,7 @@ function generateGitPatch(branchName) { if (!patchContent.trim()) { // Empty patch + debugLog(`Final: Patch file exists but is empty`); return { success: false, error: "No changes to commit - patch is empty", @@ -140,6 +335,7 @@ function generateGitPatch(branchName) { }; } + debugLog(`Final: SUCCESS - patchSize=${patchSize} bytes, patchLines=${patchLines}`); return { success: true, patchPath: patchPath, @@ -149,6 +345,7 @@ function generateGitPatch(branchName) { } // No patch generated + debugLog(`Final: FAILED - ${errorMessage || "No changes to commit - no commits found"}`); return { success: false, error: errorMessage || "No changes to commit - no commits found", diff --git a/actions/setup/js/generate_git_patch.test.cjs b/actions/setup/js/generate_git_patch.test.cjs index df5e45e3b9..686ef939a6 100644 --- a/actions/setup/js/generate_git_patch.test.cjs +++ b/actions/setup/js/generate_git_patch.test.cjs @@ -162,3 +162,187 @@ describe("generateGitPatch", () => { expect(result.success).toBe(false); }); }); + +describe("generateGitPatch - cross-repo checkout scenarios", () => { + let originalEnv; + + beforeEach(() => { + // Save original environment + originalEnv = { + GITHUB_SHA: process.env.GITHUB_SHA, + GITHUB_WORKSPACE: process.env.GITHUB_WORKSPACE, + DEFAULT_BRANCH: process.env.DEFAULT_BRANCH, + GH_AW_BASE_BRANCH: process.env.GH_AW_BASE_BRANCH, + }; + }); + + afterEach(() => { + // Restore original environment + Object.keys(originalEnv).forEach(key => { + if (originalEnv[key] !== undefined) { + process.env[key] = originalEnv[key]; + } else { + delete process.env[key]; + } + }); + }); + + it("should handle GITHUB_SHA not existing in cross-repo checkout", async () => { + const { generateGitPatch } = await import("./generate_git_patch.cjs"); + + // In cross-repo checkout, GITHUB_SHA is from the workflow repo, + // not the target repo that's checked out + process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; + process.env.GITHUB_SHA = "deadbeef123456789"; // SHA that doesn't exist in target repo + + const result = generateGitPatch("feature-branch"); + + // Should fail gracefully, not crash + expect(result).toHaveProperty("success"); + expect(result.success).toBe(false); + expect(result).toHaveProperty("error"); + }); + + it("should fall back gracefully when persist-credentials is false", async () => { + const { generateGitPatch } = await import("./generate_git_patch.cjs"); + + // Simulate cross-repo checkout where fetch fails due to persist-credentials: false + process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; + process.env.GITHUB_SHA = "abc123"; + process.env.DEFAULT_BRANCH = "main"; + + const result = generateGitPatch("feature-branch"); + + // Should try multiple strategies and fail gracefully + expect(result).toHaveProperty("success"); + expect(result.success).toBe(false); + expect(result).toHaveProperty("error"); + expect(result).toHaveProperty("patchPath"); + }); + + it("should check local refs before attempting network fetch", async () => { + const { generateGitPatch } = await import("./generate_git_patch.cjs"); + + // This tests that Strategy 1 checks for local refs before fetching + process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; + process.env.DEFAULT_BRANCH = "main"; + + const result = generateGitPatch("feature-branch"); + + // Should complete without hanging or crashing due to fetch attempts + expect(result).toHaveProperty("success"); + expect(result).toHaveProperty("patchPath"); + }); + + it("should return meaningful error for cross-repo scenarios", async () => { + const { generateGitPatch } = await import("./generate_git_patch.cjs"); + + process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; + process.env.GITHUB_SHA = "sha-from-workflow-repo"; + process.env.DEFAULT_BRANCH = "main"; + + const result = generateGitPatch("agent-created-branch"); + + expect(result.success).toBe(false); + expect(result).toHaveProperty("error"); + // Error should be informative + expect(typeof result.error).toBe("string"); + expect(result.error.length).toBeGreaterThan(0); + }); + + it("should handle incremental mode failure in cross-repo checkout", async () => { + const { generateGitPatch } = await import("./generate_git_patch.cjs"); + + process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; + process.env.DEFAULT_BRANCH = "main"; + + // Incremental mode requires origin/branchName to exist - should fail clearly + const result = generateGitPatch("feature-branch", { mode: "incremental" }); + + expect(result.success).toBe(false); + expect(result).toHaveProperty("error"); + // Should indicate the branch doesn't exist or can't be fetched + expect(result.error).toMatch(/branch|fetch|incremental/i); + }); + + it("should handle SideRepoOps pattern where workflow repo != target repo", async () => { + const { generateGitPatch } = await import("./generate_git_patch.cjs"); + + // Simulates: workflow in org/side-repo, checkout of org/target-repo + // GITHUB_SHA would be from side-repo, not target-repo + process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-target-repo"; + process.env.GITHUB_SHA = "side-repo-sha-not-in-target"; + process.env.DEFAULT_BRANCH = "main"; + + const result = generateGitPatch("agent-changes"); + + // Should not crash, should return failure with helpful error + expect(result).toHaveProperty("success"); + expect(result.success).toBe(false); + expect(result).toHaveProperty("patchPath"); + }); +}); + +describe("sanitizeBranchNameForPatch", () => { + it("should sanitize branch names with path separators", async () => { + const { sanitizeBranchNameForPatch } = await import("./generate_git_patch.cjs"); + + expect(sanitizeBranchNameForPatch("feature/add-login")).toBe("feature-add-login"); + expect(sanitizeBranchNameForPatch("user\\branch")).toBe("user-branch"); + }); + + it("should sanitize branch names with special characters", async () => { + const { sanitizeBranchNameForPatch } = await import("./generate_git_patch.cjs"); + + expect(sanitizeBranchNameForPatch("feature:test")).toBe("feature-test"); + expect(sanitizeBranchNameForPatch("branch*name")).toBe("branch-name"); + expect(sanitizeBranchNameForPatch('branch?"name')).toBe("branch-name"); + expect(sanitizeBranchNameForPatch("branch<>name")).toBe("branch-name"); + expect(sanitizeBranchNameForPatch("branch|name")).toBe("branch-name"); + }); + + it("should collapse multiple dashes", async () => { + const { sanitizeBranchNameForPatch } = await import("./generate_git_patch.cjs"); + + expect(sanitizeBranchNameForPatch("feature//double")).toBe("feature-double"); + expect(sanitizeBranchNameForPatch("a---b")).toBe("a-b"); + }); + + it("should remove leading and trailing dashes", async () => { + const { sanitizeBranchNameForPatch } = await import("./generate_git_patch.cjs"); + + expect(sanitizeBranchNameForPatch("/feature")).toBe("feature"); + expect(sanitizeBranchNameForPatch("feature/")).toBe("feature"); + expect(sanitizeBranchNameForPatch("/feature/")).toBe("feature"); + }); + + it("should convert to lowercase", async () => { + const { sanitizeBranchNameForPatch } = await import("./generate_git_patch.cjs"); + + expect(sanitizeBranchNameForPatch("Feature-Branch")).toBe("feature-branch"); + expect(sanitizeBranchNameForPatch("UPPER")).toBe("upper"); + }); + + it("should handle null and empty strings", async () => { + const { sanitizeBranchNameForPatch } = await import("./generate_git_patch.cjs"); + + expect(sanitizeBranchNameForPatch(null)).toBe("unknown"); + expect(sanitizeBranchNameForPatch("")).toBe("unknown"); + expect(sanitizeBranchNameForPatch(undefined)).toBe("unknown"); + }); +}); + +describe("getPatchPath", () => { + it("should return correct path format", async () => { + const { getPatchPath } = await import("./generate_git_patch.cjs"); + + expect(getPatchPath("feature-branch")).toBe("/tmp/gh-aw/aw-feature-branch.patch"); + }); + + it("should sanitize branch name in path", async () => { + const { getPatchPath } = await import("./generate_git_patch.cjs"); + + expect(getPatchPath("feature/branch")).toBe("/tmp/gh-aw/aw-feature-branch.patch"); + expect(getPatchPath("Feature/BRANCH")).toBe("/tmp/gh-aw/aw-feature-branch.patch"); + }); +}); diff --git a/actions/setup/js/git_patch_integration.test.cjs b/actions/setup/js/git_patch_integration.test.cjs new file mode 100644 index 0000000000..07ceda48b7 --- /dev/null +++ b/actions/setup/js/git_patch_integration.test.cjs @@ -0,0 +1,661 @@ +/** + * Integration tests for git patch generation and application + * + * These tests run REAL git commands to verify: + * 1. git format-patch generates valid patches + * 2. git am can apply patches correctly + * 3. Emoji and unicode in commit messages work + * 4. Merge conflict detection works + * 5. Concurrent push scenarios are handled + * + * These tests require git to be installed and create temporary git repos. + */ + +import { describe, it, expect, beforeEach, afterEach } from "vitest"; +import fs from "fs"; +import path from "path"; +import { spawnSync } from "child_process"; +import os from "os"; +import { generateGitPatch } from "./generate_git_patch.cjs"; + +/** + * Execute git command safely with args array + */ +function execGit(args, options = {}) { + const result = spawnSync("git", args, { + encoding: "utf8", + ...options, + }); + if (result.error) { + throw result.error; + } + if (result.status !== 0 && !options.allowFailure) { + throw new Error(`git ${args.join(" ")} failed: ${result.stderr}`); + } + return result; +} + +/** + * Create a temporary git repository for testing + */ +function createTestRepo() { + const repoDir = fs.mkdtempSync(path.join(os.tmpdir(), "git-patch-test-")); + + execGit(["init"], { cwd: repoDir }); + execGit(["config", "user.name", "Test User"], { cwd: repoDir }); + execGit(["config", "user.email", "test@example.com"], { cwd: repoDir }); + + // Create initial commit on main branch + fs.writeFileSync(path.join(repoDir, "README.md"), "# Test Repo\n"); + execGit(["add", "."], { cwd: repoDir }); + execGit(["commit", "-m", "Initial commit"], { cwd: repoDir }); + + // Create main as the default branch + execGit(["branch", "-M", "main"], { cwd: repoDir }); + + return repoDir; +} + +/** + * Clean up test repository + */ +function cleanupTestRepo(repoDir) { + if (repoDir && fs.existsSync(repoDir)) { + fs.rmSync(repoDir, { recursive: true, force: true }); + } +} + +describe("git patch integration tests", () => { + let repoDir; + let patchDir; + + beforeEach(() => { + repoDir = createTestRepo(); + patchDir = fs.mkdtempSync(path.join(os.tmpdir(), "git-patch-output-")); + }); + + afterEach(() => { + cleanupTestRepo(repoDir); + cleanupTestRepo(patchDir); + }); + + // ────────────────────────────────────────────────────── + // Basic Patch Generation and Application + // ────────────────────────────────────────────────────── + + describe("basic patch generation and application", () => { + it("should generate and apply a simple patch", () => { + // Create feature branch with changes + execGit(["checkout", "-b", "feature-branch"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "test.txt"), "Hello World\n"); + execGit(["add", "test.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Add test file"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "test.patch"); + const patchResult = execGit(["format-patch", "main..feature-branch", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Verify patch has content + const patchContent = fs.readFileSync(patchPath, "utf8"); + expect(patchContent).toContain("Subject:"); + expect(patchContent).toContain("Add test file"); + expect(patchContent).toContain("Hello World"); + + // Create a clean branch to test apply + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-test"], { cwd: repoDir }); + + // Apply patch + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + + // Verify file was created + const fileContent = fs.readFileSync(path.join(repoDir, "test.txt"), "utf8"); + expect(fileContent).toBe("Hello World\n"); + }); + + it("should handle multi-commit patches", () => { + execGit(["checkout", "-b", "multi-commit"], { cwd: repoDir }); + + // First commit + fs.writeFileSync(path.join(repoDir, "file1.txt"), "File 1\n"); + execGit(["add", "file1.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Add file 1"], { cwd: repoDir }); + + // Second commit + fs.writeFileSync(path.join(repoDir, "file2.txt"), "File 2\n"); + execGit(["add", "file2.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Add file 2"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "multi.patch"); + const patchResult = execGit(["format-patch", "main..multi-commit", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Apply to clean branch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-multi"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + + // Verify both files exist + expect(fs.existsSync(path.join(repoDir, "file1.txt"))).toBe(true); + expect(fs.existsSync(path.join(repoDir, "file2.txt"))).toBe(true); + }); + }); + + // ────────────────────────────────────────────────────── + // Emoji and Unicode in Commit Messages + // ────────────────────────────────────────────────────── + + describe("emoji and unicode handling", () => { + it("should handle emoji in commit message", () => { + execGit(["checkout", "-b", "emoji-branch"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "emoji.txt"), "Bug fixed!\n"); + execGit(["add", "emoji.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "🐛 Fix bug with login"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "emoji.patch"); + const patchResult = execGit(["format-patch", "main..emoji-branch", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Check patch content - git may RFC2047 encode or keep UTF-8 + const patchContent = fs.readFileSync(patchPath, "utf8"); + // Either raw emoji or RFC2047 encoded is acceptable + const hasEmoji = patchContent.includes("🐛") || patchContent.includes("=?UTF-8?"); + expect(hasEmoji).toBe(true); + + // Apply to clean branch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-emoji"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + + // Verify commit message was preserved + const logResult = execGit(["log", "-1", "--format=%s"], { cwd: repoDir }); + expect(logResult.stdout.trim()).toContain("Fix bug"); + }); + + it("should handle unicode characters in commit message", () => { + execGit(["checkout", "-b", "unicode-branch"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "unicode.txt"), "International text\n"); + execGit(["add", "unicode.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "日本語のコミットメッセージ"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "unicode.patch"); + const patchResult = execGit(["format-patch", "main..unicode-branch", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Apply to clean branch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-unicode"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + }); + + it("should handle multi-line commit messages", () => { + execGit(["checkout", "-b", "multiline-branch"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "multiline.txt"), "Content\n"); + execGit(["add", "multiline.txt"], { cwd: repoDir }); + + // Commit with multi-line message + const commitMsg = "Short title\n\nThis is the body of the commit.\nIt has multiple lines.\n\n- Item 1\n- Item 2"; + execGit(["commit", "-m", commitMsg], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "multiline.patch"); + const patchResult = execGit(["format-patch", "main..multiline-branch", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Verify patch structure + const patchContent = fs.readFileSync(patchPath, "utf8"); + expect(patchContent).toContain("Subject:"); + expect(patchContent).toContain("Short title"); + + // Apply to clean branch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-multiline"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + + // Verify body was preserved + const logResult = execGit(["log", "-1", "--format=%B"], { cwd: repoDir }); + expect(logResult.stdout).toContain("body of the commit"); + }); + + it("should handle special characters in commit message", () => { + execGit(["checkout", "-b", "special-chars"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "special.txt"), "Content\n"); + execGit(["add", "special.txt"], { cwd: repoDir }); + + // Commit with special characters that might cause issues + const commitMsg = "Fix: Handle \"quotes\" and 'apostrophes' and $variables and `backticks`"; + execGit(["commit", "-m", commitMsg], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "special.patch"); + const patchResult = execGit(["format-patch", "main..special-chars", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Apply to clean branch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-special"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + }); + }); + + // ────────────────────────────────────────────────────── + // Merge Conflict Scenarios + // ────────────────────────────────────────────────────── + + describe("merge conflict scenarios", () => { + it("should detect and report patch application failure due to conflict", () => { + // Create conflicting changes + fs.writeFileSync(path.join(repoDir, "conflict.txt"), "Original content\n"); + execGit(["add", "conflict.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Add conflict file"], { cwd: repoDir }); + + // Create feature branch with one change + execGit(["checkout", "-b", "feature-conflict"], { cwd: repoDir }); + fs.writeFileSync(path.join(repoDir, "conflict.txt"), "Feature branch content\n"); + execGit(["add", "conflict.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Feature change"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "conflict.patch"); + const patchResult = execGit(["format-patch", "main~1..feature-conflict", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Go back to main and make a different change + execGit(["checkout", "main"], { cwd: repoDir }); + fs.writeFileSync(path.join(repoDir, "conflict.txt"), "Main branch different content\n"); + execGit(["add", "conflict.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Main change"], { cwd: repoDir }); + + // Try to apply patch - should fail + const applyResult = execGit(["am", patchPath], { cwd: repoDir, allowFailure: true }); + expect(applyResult.status).not.toBe(0); + // Error message varies - could be "patch does not apply" or "already exists in index" + const errorOutput = applyResult.stderr.toLowerCase(); + expect(errorOutput.includes("patch does not apply") || errorOutput.includes("already exists") || errorOutput.includes("conflict")).toBe(true); + + // Abort the failed am + execGit(["am", "--abort"], { cwd: repoDir, allowFailure: true }); + }); + + it("should handle patch based on old commit that no longer exists in history", () => { + // This simulates force-push scenario where base commit was rewritten + execGit(["checkout", "-b", "force-push-branch"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "force.txt"), "Content\n"); + execGit(["add", "force.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Initial force change"], { cwd: repoDir }); + + // Get the commit SHA + const shaResult = execGit(["rev-parse", "HEAD"], { cwd: repoDir }); + const originalSha = shaResult.stdout.trim(); + + // Generate patch + const patchPath = path.join(patchDir, "force.patch"); + const patchResult = execGit(["format-patch", "main..force-push-branch", "--stdout"], { cwd: repoDir }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Simulate force-push by amending the commit + fs.writeFileSync(path.join(repoDir, "force.txt"), "Different content\n"); + execGit(["add", "force.txt"], { cwd: repoDir }); + execGit(["commit", "--amend", "-m", "Amended force change"], { cwd: repoDir }); + + // The original SHA should no longer match HEAD + const newShaResult = execGit(["rev-parse", "HEAD"], { cwd: repoDir }); + expect(newShaResult.stdout.trim()).not.toBe(originalSha); + + // The patch should still be applicable to main (it's based on main) + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-force"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + }); + }); + + // ────────────────────────────────────────────────────── + // Concurrent Push Scenarios + // ────────────────────────────────────────────────────── + + describe("concurrent push scenarios", () => { + let bareRepoDir; + let workingRepo1; + let workingRepo2; + + beforeEach(() => { + // Create a bare repository to simulate remote + bareRepoDir = fs.mkdtempSync(path.join(os.tmpdir(), "bare-repo-")); + execGit(["init", "--bare"], { cwd: bareRepoDir }); + + // Clone it twice to simulate two workers + workingRepo1 = fs.mkdtempSync(path.join(os.tmpdir(), "working1-")); + execGit(["clone", bareRepoDir, "."], { cwd: workingRepo1 }); + execGit(["config", "user.name", "User 1"], { cwd: workingRepo1 }); + execGit(["config", "user.email", "user1@example.com"], { cwd: workingRepo1 }); + + // Make initial commit + fs.writeFileSync(path.join(workingRepo1, "README.md"), "# Repo\n"); + execGit(["add", "."], { cwd: workingRepo1 }); + execGit(["commit", "-m", "Initial"], { cwd: workingRepo1 }); + execGit(["push", "-u", "origin", "main"], { cwd: workingRepo1, allowFailure: true }); + + workingRepo2 = fs.mkdtempSync(path.join(os.tmpdir(), "working2-")); + execGit(["clone", bareRepoDir, "."], { cwd: workingRepo2 }); + execGit(["config", "user.name", "User 2"], { cwd: workingRepo2 }); + execGit(["config", "user.email", "user2@example.com"], { cwd: workingRepo2 }); + }); + + afterEach(() => { + cleanupTestRepo(bareRepoDir); + cleanupTestRepo(workingRepo1); + cleanupTestRepo(workingRepo2); + }); + + it("should fail when branch was updated after patch was generated", () => { + // Create PR branch in working repo 1 + execGit(["checkout", "-b", "pr-branch"], { cwd: workingRepo1 }); + fs.writeFileSync(path.join(workingRepo1, "file1.txt"), "User 1 content\n"); + execGit(["add", "file1.txt"], { cwd: workingRepo1 }); + execGit(["commit", "-m", "User 1 commit"], { cwd: workingRepo1 }); + execGit(["push", "-u", "origin", "pr-branch"], { cwd: workingRepo1 }); + + // Fetch in working repo 2 and make changes + execGit(["fetch", "origin"], { cwd: workingRepo2 }); + execGit(["checkout", "-b", "pr-branch", "--track", "origin/pr-branch"], { cwd: workingRepo2 }); + + fs.writeFileSync(path.join(workingRepo2, "file2.txt"), "User 2 content\n"); + execGit(["add", "file2.txt"], { cwd: workingRepo2 }); + execGit(["commit", "-m", "User 2 commit"], { cwd: workingRepo2 }); + + // Generate patch in repo 2 (before pushing) + const patchPath = path.join(patchDir, "concurrent.patch"); + const patchResult = execGit(["format-patch", "origin/pr-branch..pr-branch", "--stdout"], { cwd: workingRepo2 }); + fs.writeFileSync(patchPath, patchResult.stdout); + + // Simulate concurrent push - User 1 pushes first + fs.writeFileSync(path.join(workingRepo1, "file3.txt"), "Another User 1 commit\n"); + execGit(["add", "file3.txt"], { cwd: workingRepo1 }); + execGit(["commit", "-m", "Concurrent commit from User 1"], { cwd: workingRepo1 }); + execGit(["push", "origin", "pr-branch"], { cwd: workingRepo1 }); + + // Now User 2 tries to push (without fetching) - should fail with non-fast-forward + const pushResult = execGit(["push", "origin", "pr-branch"], { cwd: workingRepo2, allowFailure: true }); + expect(pushResult.status).not.toBe(0); + // Error message varies by git version but contains rejection info + const errorOutput = pushResult.stderr.toLowerCase(); + expect(errorOutput.includes("rejected") || errorOutput.includes("failed") || errorOutput.includes("non-fast-forward")).toBe(true); + }); + }); + + // ────────────────────────────────────────────────────── + // Commit Title Suffix Modification + // ────────────────────────────────────────────────────── + + describe("commit title suffix modification", () => { + it("should correctly modify Subject line to add suffix", () => { + execGit(["checkout", "-b", "suffix-test"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "suffix.txt"), "Content\n"); + execGit(["add", "suffix.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "Original title"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "suffix.patch"); + const patchResult = execGit(["format-patch", "main..suffix-test", "--stdout"], { cwd: repoDir }); + let patchContent = patchResult.stdout; + + // Simulate the suffix modification logic from push_to_pull_request_branch.cjs + const commitTitleSuffix = " [bot]"; + patchContent = patchContent.replace(/^Subject: (?:\[PATCH\] )?(.*)$/gm, (match, title) => `Subject: [PATCH] ${title}${commitTitleSuffix}`); + + fs.writeFileSync(patchPath, patchContent); + + // Verify the modification + const modifiedPatch = fs.readFileSync(patchPath, "utf8"); + expect(modifiedPatch).toContain("Subject: [PATCH] Original title [bot]"); + + // Apply the modified patch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-suffix"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + + // Verify commit message has suffix + const logResult = execGit(["log", "-1", "--format=%s"], { cwd: repoDir }); + expect(logResult.stdout.trim()).toBe("Original title [bot]"); + }); + + it("should handle emoji in commit title with suffix modification", () => { + execGit(["checkout", "-b", "emoji-suffix"], { cwd: repoDir }); + + fs.writeFileSync(path.join(repoDir, "emoji.txt"), "Content\n"); + execGit(["add", "emoji.txt"], { cwd: repoDir }); + execGit(["commit", "-m", "🚀 Launch feature"], { cwd: repoDir }); + + // Generate patch + const patchPath = path.join(patchDir, "emoji-suffix.patch"); + const patchResult = execGit(["format-patch", "main..emoji-suffix", "--stdout"], { cwd: repoDir }); + let patchContent = patchResult.stdout; + + // Check if emoji is RFC2047 encoded + const isEncoded = patchContent.includes("=?UTF-8?"); + + // Apply suffix modification + const commitTitleSuffix = " [bot]"; + + if (isEncoded) { + // For RFC2047 encoded subjects, we need different handling + // This test documents the current limitation + console.log("Note: Emoji commit titles are RFC2047 encoded - suffix cannot be applied with simple regex"); + // The regex won't match encoded subjects properly + } else { + // If not encoded, apply the suffix + patchContent = patchContent.replace(/^Subject: (?:\[PATCH\] )?(.*)$/gm, (match, title) => `Subject: [PATCH] ${title}${commitTitleSuffix}`); + } + + fs.writeFileSync(patchPath, patchContent); + + // Apply the patch + execGit(["checkout", "main"], { cwd: repoDir }); + execGit(["checkout", "-b", "apply-emoji-suffix"], { cwd: repoDir }); + + const applyResult = execGit(["am", patchPath], { cwd: repoDir }); + expect(applyResult.status).toBe(0); + + // The commit should be applied (suffix may or may not be present depending on encoding) + const logResult = execGit(["log", "-1", "--format=%s"], { cwd: repoDir }); + expect(logResult.stdout).toContain("Launch feature"); + }); + }); + + // ────────────────────────────────────────────────────── + // Incremental Mode Tests + // ────────────────────────────────────────────────────── + + describe("incremental mode", () => { + let bareRepoDir; + let workingRepo; + + beforeEach(() => { + // Create a bare repo to simulate remote + bareRepoDir = fs.mkdtempSync(path.join(os.tmpdir(), "bare-incremental-")); + execGit(["init", "--bare", "--initial-branch=main"], { cwd: bareRepoDir }); + + // Clone the bare repo + workingRepo = fs.mkdtempSync(path.join(os.tmpdir(), "working-incremental-")); + execGit(["clone", bareRepoDir, "."], { cwd: workingRepo }); + + // Set up git config (after clone, we have a proper repo) + execGit(["config", "user.email", "test@test.com"], { cwd: workingRepo }); + execGit(["config", "user.name", "Test User"], { cwd: workingRepo }); + + // Checkout main (might be master on some systems) + execGit(["checkout", "-b", "main"], { cwd: workingRepo, allowFailure: true }); + + // Create initial commit on main + fs.writeFileSync(path.join(workingRepo, "README.md"), "# Initial\n"); + execGit(["add", "README.md"], { cwd: workingRepo }); + execGit(["commit", "-m", "Initial commit"], { cwd: workingRepo }); + + // Push main to origin (this MUST succeed for full mode tests) + execGit(["push", "-u", "origin", "main"], { cwd: workingRepo }); + }); + + afterEach(() => { + // Clean up + cleanupTestRepo(bareRepoDir); + cleanupTestRepo(workingRepo); + }); + + it("should only include new commits after origin/branch in incremental mode", () => { + // Create a feature branch with first commit + execGit(["checkout", "-b", "feature-branch"], { cwd: workingRepo }); + fs.writeFileSync(path.join(workingRepo, "feature.txt"), "First commit content\n"); + execGit(["add", "feature.txt"], { cwd: workingRepo }); + execGit(["commit", "-m", "First commit on feature branch"], { cwd: workingRepo }); + + // Push to origin + execGit(["push", "-u", "origin", "feature-branch"], { cwd: workingRepo }); + + // Add a second commit (the "new" commit) + fs.writeFileSync(path.join(workingRepo, "feature2.txt"), "Second commit content\n"); + execGit(["add", "feature2.txt"], { cwd: workingRepo }); + execGit(["commit", "-m", "Second commit - the new one"], { cwd: workingRepo }); + + // Delete the origin/feature-branch tracking ref to simulate a fresh checkout + // that hasn't fetched the remote branch yet + execGit(["update-ref", "-d", "refs/remotes/origin/feature-branch"], { cwd: workingRepo }); + + // Generate patch in incremental mode + // Set environment + const origWorkspace = process.env.GITHUB_WORKSPACE; + const origDefaultBranch = process.env.DEFAULT_BRANCH; + process.env.GITHUB_WORKSPACE = workingRepo; + process.env.DEFAULT_BRANCH = "main"; + + try { + const result = generateGitPatch("feature-branch", { mode: "incremental" }); + + expect(result.success).toBe(true); + expect(result.patchPath).toBeDefined(); + + // Read the patch content + const patchContent = fs.readFileSync(result.patchPath, "utf8"); + + // Should only have ONE patch [PATCH 1/1], not [PATCH 1/2], [PATCH 2/2] + expect(patchContent).toContain("Subject:"); + + // Should contain the second commit + expect(patchContent).toContain("Second commit - the new one"); + + // Should NOT contain the first commit (it's already on origin/feature-branch) + expect(patchContent).not.toContain("First commit on feature branch"); + + // Verify it's [PATCH 1/1] or just [PATCH], not [PATCH 1/2] + const patchHeaders = patchContent.match(/\[PATCH[^\]]*\]/g); + // If there are numbered patches, should be just 1 + if (patchHeaders) { + expect(patchHeaders.length).toBe(1); + } + } finally { + process.env.GITHUB_WORKSPACE = origWorkspace; + process.env.DEFAULT_BRANCH = origDefaultBranch; + } + }); + + it("should fail clearly when origin/branch doesnt exist in incremental mode", () => { + // Create a local branch without pushing + execGit(["checkout", "-b", "local-only-branch"], { cwd: workingRepo }); + fs.writeFileSync(path.join(workingRepo, "local.txt"), "Local content\n"); + execGit(["add", "local.txt"], { cwd: workingRepo }); + execGit(["commit", "-m", "Local commit"], { cwd: workingRepo }); + + // Don't push - origin/local-only-branch doesn't exist + + const origWorkspace = process.env.GITHUB_WORKSPACE; + const origDefaultBranch = process.env.DEFAULT_BRANCH; + process.env.GITHUB_WORKSPACE = workingRepo; + process.env.DEFAULT_BRANCH = "main"; + + try { + const result = generateGitPatch("local-only-branch", { mode: "incremental" }); + + // Should fail with a clear error message + expect(result.success).toBe(false); + expect(result.error).toContain("Cannot generate incremental patch"); + expect(result.error).toContain("origin/local-only-branch"); + } finally { + process.env.GITHUB_WORKSPACE = origWorkspace; + process.env.DEFAULT_BRANCH = origDefaultBranch; + } + }); + + it("should include all commits in full mode even when origin/branch exists", () => { + // Create a feature branch with first commit + execGit(["checkout", "-b", "full-mode-branch"], { cwd: workingRepo }); + fs.writeFileSync(path.join(workingRepo, "full1.txt"), "First commit\n"); + execGit(["add", "full1.txt"], { cwd: workingRepo }); + execGit(["commit", "-m", "First commit in full mode test"], { cwd: workingRepo }); + + // Push to origin + execGit(["push", "-u", "origin", "full-mode-branch"], { cwd: workingRepo }); + + // Add second commit + fs.writeFileSync(path.join(workingRepo, "full2.txt"), "Second commit\n"); + execGit(["add", "full2.txt"], { cwd: workingRepo }); + execGit(["commit", "-m", "Second commit in full mode test"], { cwd: workingRepo }); + + // Delete origin ref to force merge-base fallback + execGit(["update-ref", "-d", "refs/remotes/origin/full-mode-branch"], { cwd: workingRepo }); + + // Fetch origin/main so merge-base can work + execGit(["fetch", "origin", "main"], { cwd: workingRepo }); + + const origWorkspace = process.env.GITHUB_WORKSPACE; + const origDefaultBranch = process.env.DEFAULT_BRANCH; + process.env.GITHUB_WORKSPACE = workingRepo; + process.env.DEFAULT_BRANCH = "main"; + + try { + // Full mode (default) - should fall back to merge-base and include all commits + const result = generateGitPatch("full-mode-branch", { mode: "full" }); + + // Debug output if test fails + if (!result.success) { + console.log("Full mode test failed with error:", result.error); + } + + expect(result.success).toBe(true); + + const patchContent = fs.readFileSync(result.patchPath, "utf8"); + + // Should contain BOTH commits (using merge-base with main) + expect(patchContent).toContain("First commit in full mode test"); + expect(patchContent).toContain("Second commit in full mode test"); + } finally { + process.env.GITHUB_WORKSPACE = origWorkspace; + process.env.DEFAULT_BRANCH = origDefaultBranch; + } + }); + }); +}); diff --git a/actions/setup/js/handle_noop_message.test.cjs b/actions/setup/js/handle_noop_message.test.cjs index 0bb89343ec..58ba5af7e1 100644 --- a/actions/setup/js/handle_noop_message.test.cjs +++ b/actions/setup/js/handle_noop_message.test.cjs @@ -108,8 +108,13 @@ This issue helps you: }); afterEach(() => { - // Restore environment - process.env = originalEnv; + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); // Restore fs.readFileSync if (originalReadFileSync) { diff --git a/actions/setup/js/interpolate_prompt.test.cjs b/actions/setup/js/interpolate_prompt.test.cjs index 031e6a8e93..ef08d8fd2c 100644 --- a/actions/setup/js/interpolate_prompt.test.cjs +++ b/actions/setup/js/interpolate_prompt.test.cjs @@ -112,7 +112,11 @@ describe("interpolate_prompt", () => { core.setFailed.mockClear()); }), afterEach(() => { - (tmpDir && fs.existsSync(tmpDir) && fs.rmSync(tmpDir, { recursive: !0, force: !0 }), (process.env = originalEnv)); + (tmpDir && fs.existsSync(tmpDir) && fs.rmSync(tmpDir, { recursive: !0, force: !0 }), + Object.keys(process.env).forEach(k => { + if (!(k in originalEnv)) delete process.env[k]; + }), + Object.assign(process.env, originalEnv)); }), it("should fail when GH_AW_PROMPT is not set", () => { delete process.env.GH_AW_PROMPT; diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 58078fcc4e..6aedcc1d98 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -6,9 +6,10 @@ const fs = require("fs"); const { generateStagedPreview } = require("./staged_preview.cjs"); const { updateActivationCommentWithCommit } = require("./update_activation_comment.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); -const { replaceTemporaryIdReferences } = require("./temporary_id.cjs"); const { normalizeBranchName } = require("./normalize_branch_name.cjs"); const { pushExtraEmptyCommit } = require("./extra_empty_commit.cjs"); +const { detectForkPR } = require("./pr_helpers.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -33,6 +34,10 @@ async function main(config = {}) { const baseBranch = config.base_branch || ""; const maxCount = config.max || 0; // 0 means no limit + // Cross-repo support: resolve target repository from config + // This allows pushing to PRs in a different repository than the workflow + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + // Check if we're in staged mode const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true"; @@ -52,6 +57,10 @@ async function main(config = {}) { } core.info(`Max patch size: ${maxSizeKb} KB`); core.info(`Max count: ${maxCount || "unlimited"}`); + core.info(`Default target repo: ${defaultTargetRepo}`); + if (allowedRepos.size > 0) { + core.info(`Allowed repos: ${[...allowedRepos].join(", ")}`); + } // Track how many items we've processed for max limit let processedCount = 0; @@ -199,20 +208,47 @@ async function main(config = {}) { return { success: false, error: "Pull request number is required but not found" }; } + // Resolve and validate target repository + // For cross-repo scenarios, the PR may be in a different repository than the workflow + const repoResult = resolveAndValidateRepo(message, defaultTargetRepo, allowedRepos, "push to PR branch"); + if (!repoResult.success) { + return { success: false, error: repoResult.error }; + } + const itemRepo = repoResult.repo; + const repoParts = repoResult.repoParts; + + core.info(`Target repository: ${itemRepo}`); + // Fetch the specific PR to get its head branch, title, and labels + let pullRequest; try { - const { data: pullRequest } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, + const response = await github.rest.pulls.get({ + owner: repoParts.owner, + repo: repoParts.repo, pull_number: pullNumber, }); + pullRequest = response.data; branchName = pullRequest.head.ref; prTitle = pullRequest.title || ""; prLabels = pullRequest.labels.map(label => label.name); } catch (error) { - core.info(`Warning: Could not fetch PR ${pullNumber} details: ${getErrorMessage(error)}`); - return { success: false, error: `Failed to determine branch name for PR ${pullNumber}` }; + core.info(`Warning: Could not fetch PR ${pullNumber} from ${itemRepo}: ${getErrorMessage(error)}`); + return { success: false, error: `Failed to determine branch name for PR ${pullNumber} in ${itemRepo}` }; + } + + // SECURITY: Check if this is a fork PR - we cannot push to fork branches + // The workflow token only has access to the base repository, not the fork + const { isFork, reason: forkReason } = detectForkPR(pullRequest); + if (isFork) { + core.error(`Cannot push to fork PR branch: ${forkReason}`); + core.error("The workflow token does not have permission to push to fork repositories."); + core.error("Fork PRs must be updated by the fork owner or through other mechanisms."); + return { + success: false, + error: `Cannot push to fork PR: ${forkReason}. The workflow token does not have permission to push to fork repositories.`, + }; } + core.info(`Fork PR check: not a fork (${forkReason})`); // SECURITY: Sanitize branch name to prevent shell injection (CWE-78) // Branch names from GitHub API must be normalized before use in git commands @@ -283,6 +319,10 @@ async function main(config = {}) { } // Apply the patch using git CLI (skip if empty) + // Track number of new commits added so we can restrict the extra empty commit + // to branches with exactly one new commit (security: prevents use of CI trigger + // token on multi-commit branches where workflow files may have been modified). + let newCommitCount = 0; if (hasChanges) { core.info("Applying patch..."); try { @@ -310,12 +350,35 @@ async function main(config = {}) { } // Apply patch - await exec.exec(`git am ${patchFilePath}`); + // Capture HEAD before applying patch to compute new-commit count later + let remoteHeadBeforePatch = ""; + try { + const { stdout } = await exec.getExecOutput("git", ["rev-parse", "HEAD"]); + remoteHeadBeforePatch = stdout.trim(); + } catch { + // Non-fatal - extra empty commit will be skipped + } + + // Use --3way to handle cross-repo patches where the patch base may differ from target repo + // This allows git to resolve create-vs-modify mismatches when a file exists in target but not source + await exec.exec(`git am --3way ${patchFilePath}`); core.info("Patch applied successfully"); // Push the applied commits to the branch await exec.exec(`git push origin ${branchName}`); core.info(`Changes committed and pushed to branch: ${branchName}`); + + // Count new commits pushed for the CI trigger decision + if (remoteHeadBeforePatch) { + try { + const { stdout: countStr } = await exec.getExecOutput("git", ["rev-list", "--count", `${remoteHeadBeforePatch}..HEAD`]); + newCommitCount = parseInt(countStr.trim(), 10); + core.info(`${newCommitCount} new commit(s) pushed to branch`); + } catch { + // Non-fatal - newCommitCount stays 0, extra empty commit will be skipped + core.info("Could not count new commits - extra empty commit will be skipped"); + } + } } catch (error) { core.error(`Failed to apply patch: ${getErrorMessage(error)}`); @@ -374,8 +437,9 @@ async function main(config = {}) { const commitSha = commitShaRes.stdout.trim(); // Get repository base URL and construct URLs + // For cross-repo scenarios, use repoParts (the target repo) not context.repo (the workflow repo) const githubServer = process.env.GITHUB_SERVER_URL || "https://github.com"; - const repoUrl = context.payload.repository ? context.payload.repository.html_url : `${githubServer}/${context.repo.owner}/${context.repo.repo}`; + const repoUrl = `${githubServer}/${repoParts.owner}/${repoParts.repo}`; const pushUrl = `${repoUrl}/tree/${branchName}`; const commitUrl = `${repoUrl}/commit/${commitSha}`; @@ -403,13 +467,16 @@ async function main(config = {}) { await core.summary.addRaw(summaryContent).write(); - // Push an extra empty commit if a token is configured and changes were pushed. + // Push an extra empty commit if a token is configured and exactly 1 new commit was pushed. // This works around the GITHUB_TOKEN limitation where pushes don't trigger CI events. + // Restricting to exactly 1 new commit prevents the CI trigger token being used on + // multi-commit branches where workflow files may have been iteratively modified. if (hasChanges) { const ciTriggerResult = await pushExtraEmptyCommit({ branchName, - repoOwner: context.repo.owner, - repoName: context.repo.repo, + repoOwner: repoParts.owner, + repoName: repoParts.repo, + newCommitCount, }); if (ciTriggerResult.success && !ciTriggerResult.skipped) { core.info("Extra empty commit pushed - CI checks should start shortly"); diff --git a/actions/setup/js/push_to_pull_request_branch.test.cjs b/actions/setup/js/push_to_pull_request_branch.test.cjs new file mode 100644 index 0000000000..d62e2f11bd --- /dev/null +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -0,0 +1,1078 @@ +import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; +import * as fs from "fs"; +import * as path from "path"; +import * as os from "os"; + +/** + * Test Matrix for push_to_pull_request_branch.cjs + * + * This test file covers the following scenarios: + * + * 1. GitHub Actions Context Scenarios: + * - pull_request event (direct PR trigger) + * - issue_comment event (comment on PR) + * - schedule event (scheduled task) + * - workflow_dispatch (manual trigger) + * + * 2. Repository State Scenarios: + * - PR branch has had new pushes (needs merge/rebase) + * - Base branch (main/default) has new commits + * - PR branch has been force-pushed + * - Normal push scenario + * + * 3. Empty Commit Handling: + * - if-no-changes: warn (default) + * - if-no-changes: error + * - if-no-changes: ignore + * + * 4. Fork PR Scenarios: + * - Fork PR detection + * - Early failure for fork PRs (no push access) + * - Same-repo PR (normal case) + * + * 5. Error Scenarios: + * - git fetch failure + * - git checkout failure + * - git am (patch apply) failure + * - git push failure + * - Patch size too large + * - Patch file not found + * - Invalid patch content + */ + +describe("push_to_pull_request_branch.cjs", () => { + let mockCore; + let mockExec; + let mockContext; + let mockGithub; + let tempDir; + let originalEnv; + + beforeEach(async () => { + originalEnv = { ...process.env }; + + // Create temp directory for test artifacts + tempDir = fs.mkdtempSync(path.join(os.tmpdir(), "push-to-pr-test-")); + + // Mock core actions methods + mockCore = { + info: vi.fn(), + warning: vi.fn(), + error: vi.fn(), + debug: vi.fn(), + setFailed: vi.fn(), + setOutput: vi.fn(), + startGroup: vi.fn(), + endGroup: vi.fn(), + summary: { + addRaw: vi.fn().mockReturnThis(), + write: vi.fn().mockResolvedValue(undefined), + }, + }; + + // Default exec mock + mockExec = { + exec: vi.fn().mockResolvedValue(0), + getExecOutput: vi.fn().mockResolvedValue({ exitCode: 0, stdout: "", stderr: "" }), + }; + + // Default pull request context + mockContext = { + eventName: "pull_request", + sha: "abc123def456", + repo: { + owner: "test-owner", + repo: "test-repo", + }, + payload: { + pull_request: { + number: 123, + state: "open", + title: "Test PR", + labels: [], + head: { + ref: "feature-branch", + sha: "head-sha-123", + repo: { + full_name: "test-owner/test-repo", + fork: false, + owner: { + login: "test-owner", + }, + }, + }, + base: { + ref: "main", + sha: "base-sha-456", + repo: { + full_name: "test-owner/test-repo", + owner: { + login: "test-owner", + }, + }, + }, + }, + repository: { + html_url: "https://github.com/test-owner/test-repo", + }, + }, + }; + + // Default GitHub API mock + mockGithub = { + rest: { + pulls: { + get: vi.fn().mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { + full_name: "test-owner/test-repo", + fork: false, + }, + }, + base: { + repo: { + full_name: "test-owner/test-repo", + }, + }, + title: "Test PR", + labels: [], + }, + }), + }, + }, + graphql: vi.fn(), + }; + + global.core = mockCore; + global.exec = mockExec; + global.context = mockContext; + global.github = mockGithub; + + // Clear module cache + delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; + delete require.cache[require.resolve("./staged_preview.cjs")]; + delete require.cache[require.resolve("./update_activation_comment.cjs")]; + delete require.cache[require.resolve("./extra_empty_commit.cjs")]; + }); + + afterEach(() => { + // Restore environment by mutating process.env in place + // (replacing process.env with a plain object breaks Node's special env handling) + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); + + // Clean up temp directory + if (tempDir && fs.existsSync(tempDir)) { + fs.rmSync(tempDir, { recursive: true, force: true }); + } + + // Clean up globals + delete global.core; + delete global.exec; + delete global.context; + delete global.github; + vi.clearAllMocks(); + }); + + /** + * Helper to load the module with mocked dependencies + */ + async function loadModule() { + // Module loads when imported + const module = require("./push_to_pull_request_branch.cjs"); + return module; + } + + /** + * Helper to create a valid patch file + */ + function createPatchFile(content = null) { + const patchPath = path.join(tempDir, "test.patch"); + const defaultPatch = `From abc123 Mon Sep 17 00:00:00 2001 +From: Test Author +Date: Mon, 1 Jan 2024 00:00:00 +0000 +Subject: [PATCH] Test commit + +Test changes + +diff --git a/test.txt b/test.txt +new file mode 100644 +index 0000000..abc1234 +--- /dev/null ++++ b/test.txt +@@ -0,0 +1 @@ ++Hello World +-- +2.34.1 +`; + fs.writeFileSync(patchPath, content !== null ? content : defaultPatch); + return patchPath; + } + + // ────────────────────────────────────────────────────── + // Configuration Parsing Tests + // ────────────────────────────────────────────────────── + + describe("configuration parsing", () => { + it('should default target to "triggering" when not specified', async () => { + const module = await loadModule(); + const handler = await module.main({}); + + expect(mockCore.info).toHaveBeenCalledWith("Target: triggering"); + }); + + it("should accept target from config", async () => { + const module = await loadModule(); + const handler = await module.main({ target: "*" }); + + expect(mockCore.info).toHaveBeenCalledWith("Target: *"); + }); + + it("should accept numeric target for specific PR", async () => { + const module = await loadModule(); + const handler = await module.main({ target: "456" }); + + expect(mockCore.info).toHaveBeenCalledWith("Target: 456"); + }); + + it('should default if_no_changes to "warn"', async () => { + const module = await loadModule(); + const handler = await module.main({}); + + expect(mockCore.info).toHaveBeenCalledWith("If no changes: warn"); + }); + + it("should accept title_prefix configuration", async () => { + const module = await loadModule(); + const handler = await module.main({ title_prefix: "[bot] " }); + + expect(mockCore.info).toHaveBeenCalledWith("Title prefix: [bot] "); + }); + + it("should accept labels configuration as array", async () => { + const module = await loadModule(); + const handler = await module.main({ labels: ["automated", "bot"] }); + + expect(mockCore.info).toHaveBeenCalledWith("Required labels: automated, bot"); + }); + + it("should accept labels configuration as comma-separated string", async () => { + const module = await loadModule(); + const handler = await module.main({ labels: "automated,bot" }); + + expect(mockCore.info).toHaveBeenCalledWith("Required labels: automated, bot"); + }); + + it("should default max_patch_size to 1024 KB", async () => { + const module = await loadModule(); + const handler = await module.main({}); + + expect(mockCore.info).toHaveBeenCalledWith("Max patch size: 1024 KB"); + }); + }); + + // ────────────────────────────────────────────────────── + // GitHub Actions Context Tests + // ────────────────────────────────────────────────────── + + describe("GitHub Actions context scenarios", () => { + it('should handle pull_request event with target "triggering"', async () => { + mockContext.eventName = "pull_request"; + const patchPath = createPatchFile(); + + // Mock git commands + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.pulls.get).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + pull_number: 123, + }); + }); + + it("should handle issue_comment event (comment on PR)", async () => { + mockContext.eventName = "issue_comment"; + mockContext.payload.issue = { number: 123 }; + const patchPath = createPatchFile(); + + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + }); + + it('should fail for schedule event with target "triggering"', async () => { + mockContext.eventName = "schedule"; + delete mockContext.payload.pull_request; + delete mockContext.payload.issue; + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("pull request context"); + }); + + it('should handle schedule event with target "*" and explicit PR number', async () => { + mockContext.eventName = "schedule"; + delete mockContext.payload.pull_request; + const patchPath = createPatchFile(); + + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ target: "*" }); + const result = await handler({ patch_path: patchPath, pull_request_number: 456 }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.pulls.get).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + pull_number: 456, + }); + }); + + it("should handle workflow_dispatch event with explicit PR number", async () => { + mockContext.eventName = "workflow_dispatch"; + delete mockContext.payload.pull_request; + const patchPath = createPatchFile(); + + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ target: "789" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.pulls.get).toHaveBeenCalledWith({ + owner: "test-owner", + repo: "test-repo", + pull_number: 789, + }); + }); + }); + + // ────────────────────────────────────────────────────── + // Fork PR Detection Tests + // ────────────────────────────────────────────────────── + + describe("fork PR detection and handling", () => { + it("should detect fork PR via different repository names and fail early", async () => { + // Set up fork scenario - head repo is different from base repo + mockContext.payload.pull_request.head.repo.full_name = "fork-owner/test-repo"; + mockContext.payload.pull_request.head.repo.owner.login = "fork-owner"; + + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { + full_name: "fork-owner/test-repo", + fork: true, + }, + }, + base: { + repo: { + full_name: "test-owner/test-repo", + }, + }, + title: "Fork PR", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + // Fork PRs should fail early with a clear error + expect(result.success).toBe(false); + expect(result.error).toContain("fork"); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Cannot push to fork PR")); + // Should not attempt any git operations + expect(mockExec.exec).not.toHaveBeenCalled(); + }); + + it("should detect fork PR via fork flag and fail early", async () => { + mockContext.payload.pull_request.head.repo.fork = true; + + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { + full_name: "test-owner/test-repo", + fork: true, + }, + }, + base: { + repo: { + full_name: "test-owner/test-repo", + }, + }, + title: "Fork PR", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + // Fork PRs should fail early with a clear error + expect(result.success).toBe(false); + expect(result.error).toContain("fork"); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Cannot push to fork PR")); + }); + + it("should handle deleted head repo (likely a fork) and fail early", async () => { + delete mockContext.payload.pull_request.head.repo; + + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: null, // Deleted fork + }, + base: { + repo: { + full_name: "test-owner/test-repo", + }, + }, + title: "Deleted Fork PR", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({ target: "triggering" }); + const result = await handler({ patch_path: patchPath }, {}); + + // When head.repo is null, this is likely a deleted fork + // The handler should give a clear error about the fork + expect(result.success).toBe(false); + expect(result.error).toContain("fork"); + expect(mockCore.error).toHaveBeenCalledWith(expect.stringContaining("Cannot push to fork PR")); + }); + }); + + // ────────────────────────────────────────────────────── + // Repository State Scenarios + // ────────────────────────────────────────────────────── + + describe("repository state scenarios", () => { + it("should handle successful normal push", async () => { + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(result.branch_name).toBe("feature-branch"); + expect(result.commit_url).toContain("test-owner/test-repo/commit/"); + }); + + it("should handle git fetch failure", async () => { + const patchPath = createPatchFile(); + + // First exec call (git fetch) fails + mockExec.exec.mockRejectedValueOnce(new Error("Failed to fetch: network error")); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Failed to fetch branch"); + }); + + it("should handle branch not existing on origin", async () => { + const patchPath = createPatchFile(); + + // git fetch succeeds, but git rev-parse fails + mockExec.exec.mockResolvedValueOnce(0); // fetch + mockExec.exec.mockRejectedValueOnce(new Error("fatal: Needed a single revision")); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("does not exist on origin"); + }); + + it("should handle git checkout failure", async () => { + const patchPath = createPatchFile(); + + mockExec.exec.mockResolvedValueOnce(0); // fetch + mockExec.exec.mockResolvedValueOnce(0); // rev-parse + mockExec.exec.mockRejectedValueOnce(new Error("checkout failed")); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Failed to checkout branch"); + }); + + it("should handle git am (patch apply) failure with investigation", async () => { + const patchPath = createPatchFile(); + + // Set up successful fetch, rev-parse, checkout + mockExec.exec.mockResolvedValueOnce(0); // fetch + mockExec.exec.mockResolvedValueOnce(0); // rev-parse + mockExec.exec.mockResolvedValueOnce(0); // checkout + + // git rev-parse HEAD for commit tracking + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "before-sha\n", stderr: "" }); + + // git am fails + mockExec.exec.mockRejectedValueOnce(new Error("Patch does not apply")); + + // Investigation commands succeed + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "M file.txt\n", stderr: "" }); // git status + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "abc123 commit 1\n", stderr: "" }); // git log + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "" }); // git diff + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "patch diff", stderr: "" }); // git am --show-current-patch=diff + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "full patch", stderr: "" }); // git am --show-current-patch + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Failed to apply patch"); + expect(mockCore.info).toHaveBeenCalledWith("Investigating patch failure..."); + }); + + it("should handle git push rejection (concurrent changes)", async () => { + const patchPath = createPatchFile(); + + // Set up successful operations until push + mockExec.exec.mockResolvedValueOnce(0); // fetch + mockExec.exec.mockResolvedValueOnce(0); // rev-parse + mockExec.exec.mockResolvedValueOnce(0); // checkout + + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "before-sha\n", stderr: "" }); + + mockExec.exec.mockResolvedValueOnce(0); // git am + mockExec.exec.mockRejectedValueOnce(new Error("! [rejected] feature-branch -> feature-branch (non-fast-forward)")); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + // The error happens during push, which currently shows in patch apply failure + expect(result.success).toBe(false); + }); + + it("should detect force-pushed branch via ref mismatch", async () => { + const patchPath = createPatchFile(); + + // This tests the scenario where the branch SHA we have doesn't match remote + // The current implementation doesn't explicitly detect this, but git operations would fail + mockContext.payload.pull_request.head.sha = "old-sha-123"; + + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + sha: "new-sha-456", // Remote has different SHA + }, + title: "Test PR", + labels: [], + }, + }); + + mockExec.exec.mockResolvedValueOnce(0); // fetch + mockExec.exec.mockResolvedValueOnce(0); // rev-parse + mockExec.exec.mockResolvedValueOnce(0); // checkout + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "new-sha-456\n", stderr: "" }); + mockExec.exec.mockResolvedValueOnce(0); // git am + mockExec.exec.mockResolvedValueOnce(0); // git push + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "final-sha\n", stderr: "" }); + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "1\n", stderr: "" }); // commit count + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + // The push proceeds - force-push detection would need to be added + expect(mockGithub.rest.pulls.get).toHaveBeenCalled(); + }); + }); + + // ────────────────────────────────────────────────────── + // Empty Commit / No Changes Handling + // ────────────────────────────────────────────────────── + + describe("empty commit / no changes handling", () => { + it("should warn when patch is empty and if-no-changes is warn", async () => { + const patchPath = createPatchFile(""); // Empty patch + + const module = await loadModule(); + const handler = await module.main({ if_no_changes: "warn" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.skipped).toBe(true); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("noop")); + }); + + it("should error when patch is empty and if-no-changes is error", async () => { + const patchPath = createPatchFile(""); // Empty patch + + const module = await loadModule(); + const handler = await module.main({ if_no_changes: "error" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("No changes"); + }); + + it("should silently skip when patch is empty and if-no-changes is ignore", async () => { + const patchPath = createPatchFile(""); // Empty patch + + const module = await loadModule(); + const handler = await module.main({ if_no_changes: "ignore" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.skipped).toBe(true); + }); + + it("should handle missing patch file", async () => { + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: "/nonexistent/path.patch" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("No patch file"); + }); + + it("should handle patch file with error message", async () => { + const patchPath = createPatchFile("Failed to generate patch: some error"); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("error message"); + }); + }); + + // ────────────────────────────────────────────────────── + // Patch Size Validation + // ────────────────────────────────────────────────────── + + describe("patch size validation", () => { + it("should reject patches exceeding max size", async () => { + // Create a patch larger than 1KB + const largePatch = "x".repeat(2 * 1024 * 1024); // 2MB + const patchPath = createPatchFile(largePatch); + + const module = await loadModule(); + const handler = await module.main({ max_patch_size: 1024 }); // 1MB max + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("exceeds maximum"); + }); + + it("should accept patches within max size", async () => { + const patchPath = createPatchFile(); // Small valid patch + + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ max_patch_size: 1024 }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockCore.info).toHaveBeenCalledWith("Patch size validation passed"); + }); + }); + + // ────────────────────────────────────────────────────── + // Title Prefix and Labels Validation + // ────────────────────────────────────────────────────── + + describe("title prefix and labels validation", () => { + it("should reject PR without required title prefix", async () => { + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "Some PR Title", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({ title_prefix: "[bot] " }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("does not start with required prefix"); + }); + + it("should accept PR with required title prefix", async () => { + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "[bot] Automated PR", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ title_prefix: "[bot] " }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockCore.info).toHaveBeenCalledWith('✓ Title prefix validation passed: "[bot] "'); + }); + + it("should reject PR without required labels", async () => { + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "Test PR", + labels: [{ name: "bug" }], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({ labels: ["automated", "enhancement"] }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("missing required labels"); + }); + + it("should accept PR with all required labels", async () => { + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature-branch", + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "Test PR", + labels: [{ name: "automated" }, { name: "enhancement" }], + }, + }); + + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ labels: ["automated", "enhancement"] }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockCore.info).toHaveBeenCalledWith("✓ Labels validation passed: automated, enhancement"); + }); + }); + + // ────────────────────────────────────────────────────── + // Commit Title Suffix + // ────────────────────────────────────────────────────── + + describe("commit title suffix", () => { + it("should append suffix to commit messages in patch", async () => { + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ commit_title_suffix: " [skip ci]" }); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(mockCore.info).toHaveBeenCalledWith('Appending commit title suffix: " [skip ci]"'); + + // Verify patch was modified + const modifiedPatch = fs.readFileSync(patchPath, "utf8"); + expect(modifiedPatch).toContain("[skip ci]"); + }); + }); + + // ────────────────────────────────────────────────────── + // Staged Mode + // ────────────────────────────────────────────────────── + + describe("staged mode", () => { + it("should generate preview instead of pushing in staged mode", async () => { + process.env.GH_AW_SAFE_OUTPUTS_STAGED = "true"; + const patchPath = createPatchFile(); + + // Re-import the module to pick up env var + delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; + + const module = require("./push_to_pull_request_branch.cjs"); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + expect(result.staged).toBe(true); + // Should not have made any git commands + expect(mockExec.exec).not.toHaveBeenCalled(); + }); + }); + + // ────────────────────────────────────────────────────── + // Max Count Limiting + // ────────────────────────────────────────────────────── + + describe("max count limiting", () => { + it("should skip messages after max count reached", async () => { + const patchPath = createPatchFile(); + mockExec.getExecOutput.mockResolvedValue({ exitCode: 0, stdout: "abc123\n", stderr: "" }); + + const module = await loadModule(); + const handler = await module.main({ max: 1 }); + + // First call succeeds + const result1 = await handler({ patch_path: patchPath }, {}); + expect(result1.success).toBe(true); + + // Second call is skipped + const result2 = await handler({ patch_path: patchPath }, {}); + expect(result2.success).toBe(false); + expect(result2.skipped).toBe(true); + expect(result2.error).toContain("Max count"); + }); + }); + + // ────────────────────────────────────────────────────── + // Branch Name Sanitization + // ────────────────────────────────────────────────────── + + describe("branch name sanitization", () => { + it("should sanitize branch names with shell metacharacters", async () => { + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: "feature;rm -rf /", + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "Test PR", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + // The normalizeBranchName should sanitize dangerous characters + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Branch name sanitized")); + }); + + it("should reject empty branch name after sanitization", async () => { + mockGithub.rest.pulls.get.mockResolvedValue({ + data: { + head: { + ref: ";$`|&", // All dangerous chars + repo: { full_name: "test-owner/test-repo", fork: false }, + }, + base: { repo: { full_name: "test-owner/test-repo" } }, + title: "Test PR", + labels: [], + }, + }); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Invalid branch name"); + }); + }); + + // ────────────────────────────────────────────────────── + // PR API Error Handling + // ────────────────────────────────────────────────────── + + describe("PR API error handling", () => { + it("should handle PR not found error", async () => { + mockGithub.rest.pulls.get.mockRejectedValue(new Error("Not Found")); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Failed to determine branch name"); + }); + + it("should handle network error fetching PR", async () => { + mockGithub.rest.pulls.get.mockRejectedValue(new Error("Network timeout")); + + const patchPath = createPatchFile(); + + const module = await loadModule(); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("Failed to determine branch name"); + }); + }); + + // ────────────────────────────────────────────────────── + // Extra Empty Commit (CI Trigger) + // ────────────────────────────────────────────────────── + + describe("extra empty commit for CI trigger", () => { + it("should push extra empty commit when CI trigger token is set", async () => { + process.env.GH_AW_CI_TRIGGER_TOKEN = "ghp_test_token"; + + // Re-import modules to pick up env var + delete require.cache[require.resolve("./push_to_pull_request_branch.cjs")]; + delete require.cache[require.resolve("./extra_empty_commit.cjs")]; + + const patchPath = createPatchFile(); + + // Mock successful commands + mockExec.exec.mockResolvedValue(0); + mockExec.getExecOutput.mockImplementation(async (cmd, args) => { + if (args && args[0] === "rev-parse") { + return { exitCode: 0, stdout: "abc123\n", stderr: "" }; + } + if (args && args[0] === "rev-list") { + return { exitCode: 0, stdout: "1\n", stderr: "" }; // 1 new commit + } + if (args && args[0] === "log") { + return { exitCode: 0, stdout: "COMMIT:abc\nfile.txt\n", stderr: "" }; + } + return { exitCode: 0, stdout: "", stderr: "" }; + }); + + const module = require("./push_to_pull_request_branch.cjs"); + const handler = await module.main({}); + const result = await handler({ patch_path: patchPath }, {}); + + expect(result.success).toBe(true); + // The extra empty commit should have been attempted + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Extra empty commit")); + }); + }); + + // ────────────────────────────────────────────────────── + // Handler Type Export + // ────────────────────────────────────────────────────── + + describe("module exports", () => { + it("should export HANDLER_TYPE as push_to_pull_request_branch", async () => { + const module = await loadModule(); + + expect(module.HANDLER_TYPE).toBe("push_to_pull_request_branch"); + }); + + it("should export main function", async () => { + const module = await loadModule(); + + expect(typeof module.main).toBe("function"); + }); + }); +}); + +// ────────────────────────────────────────────────────── +// Integration Test Recommendations +// ────────────────────────────────────────────────────── + +/** + * INTEGRATION TEST RECOMMENDATIONS + * + * The following scenarios require integration testing with real Git repositories. + * These tests should be added to a separate file (e.g., push_to_pull_request_branch.integration.test.cjs) + * and run in a CI environment with actual Git operations. + * + * 1. CONCURRENT PUSH SCENARIOS: + * - Create a test repo with a PR branch + * - Push a commit to the PR branch from another process + * - Attempt to push from the handler + * - Verify proper error handling for non-fast-forward rejection + * + * 2. FORCE-PUSH DETECTION: + * - Create a test repo with a PR branch + * - Force-push to rewrite the branch history + * - Attempt to apply a patch based on old history + * - Verify proper error message about force-push + * + * 3. BASE BRANCH UPDATES: + * - Create a test repo with a PR branch + * - Push commits to the base branch + * - Verify that pushing to PR branch still works + * - (Note: This should work as PR branch is independent) + * + * 4. MERGE CONFLICT SCENARIOS: + * - Create a test repo with conflicting changes + * - Attempt to apply a patch that conflicts + * - Verify clear error message about merge conflict + * + * 5. FORK PR EARLY FAILURE: + * - Create a fork repository (or simulate fork context) + * - Verify early failure before attempting push + * - Verify clear error message about fork permissions + * + * TEST SETUP RECOMMENDATIONS: + * - Use GitHub API to create test repositories + * - Use git commands to set up test scenarios + * - Clean up test repos after each test + * - Run these tests on a schedule, not on every commit + */ diff --git a/actions/setup/js/safe_outputs_branch_detection.test.cjs b/actions/setup/js/safe_outputs_branch_detection.test.cjs index ec1236835f..e063e8b2c2 100644 --- a/actions/setup/js/safe_outputs_branch_detection.test.cjs +++ b/actions/setup/js/safe_outputs_branch_detection.test.cjs @@ -15,7 +15,11 @@ describe("safe_outputs_mcp_server.cjs branch detection", () => { (process.env.GH_AW_SAFE_OUTPUTS = tempOutputFile)); }), afterEach(() => { - ((process.env = originalEnv), fs.existsSync(tempOutputDir) && fs.rmSync(tempOutputDir, { recursive: !0, force: !0 })); + (Object.keys(process.env).forEach(k => { + if (!(k in originalEnv)) delete process.env[k]; + }), + Object.assign(process.env, originalEnv), + fs.existsSync(tempOutputDir) && fs.rmSync(tempOutputDir, { recursive: !0, force: !0 })); }), it("should use git branch when provided branch equals base branch", () => { const testRepoDir = path.join(tempOutputDir, "test_repo"); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index 5880d09c14..b994f4c1d0 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -281,6 +281,9 @@ function createHandlers(server, appendSafeOutput, config = {}) { * Handler for push_to_pull_request_branch tool * Resolves the current branch if branch is not provided or is the base branch * Generates git patch for the changes + * + * Note: Fork PR detection is handled by push_to_pull_request_branch.cjs handler + * which fetches the PR and calls detectForkPR() with full PR data. */ const pushToPullRequestBranchHandler = args => { const entry = { ...args, type: "push_to_pull_request_branch" }; @@ -300,9 +303,11 @@ function createHandlers(server, appendSafeOutput, config = {}) { entry.branch = detectedBranch; } - // Generate git patch - server.debug(`Generating patch for push_to_pull_request_branch with branch: ${entry.branch}`); - const patchResult = generateGitPatch(entry.branch); + // Generate git patch in incremental mode + // Incremental mode only includes commits since origin/branchName, + // preventing patches that include already-existing commits + server.debug(`Generating incremental patch for push_to_pull_request_branch with branch: ${entry.branch}`); + const patchResult = generateGitPatch(entry.branch, { mode: "incremental" }); if (!patchResult.success) { // Patch generation failed or patch is empty diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index a4a2667cce..3593b1edd0 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -1,4 +1,3 @@ -// @ts-check import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import fs from "fs"; import path from "path"; @@ -318,7 +317,7 @@ describe("safe_outputs_handlers", () => { const responseData = JSON.parse(result.content[0].text); expect(responseData.result).toBe("error"); expect(responseData.error).toBeDefined(); - expect(responseData.error).toContain("Failed to generate patch"); + expect(responseData.error).toContain("does not exist locally"); expect(responseData.details).toBeDefined(); expect(responseData.details).toContain("push to the pull request branch"); expect(responseData.details).toContain("git add and git commit"); diff --git a/actions/setup/js/safe_outputs_mcp_large_content.test.cjs b/actions/setup/js/safe_outputs_mcp_large_content.test.cjs index ca3ca780bd..4dd583ff5b 100644 --- a/actions/setup/js/safe_outputs_mcp_large_content.test.cjs +++ b/actions/setup/js/safe_outputs_mcp_large_content.test.cjs @@ -17,7 +17,11 @@ describe.sequential("safe_outputs_mcp_server.cjs large content handling", () => fs.writeFileSync(toolsJsonPath, toolsJsonContent); }), afterEach(() => { - ((process.env = originalEnv), fs.existsSync(tempOutputDir) && fs.rmSync(tempOutputDir, { recursive: !0, force: !0 })); + (Object.keys(process.env).forEach(k => { + if (!(k in originalEnv)) delete process.env[k]; + }), + Object.assign(process.env, originalEnv), + fs.existsSync(tempOutputDir) && fs.rmSync(tempOutputDir, { recursive: !0, force: !0 })); }), it("should write large content to file when exceeding 16000 tokens", async () => { ((process.env.GH_AW_SAFE_OUTPUTS = tempOutputFile), (process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH = tempConfigFile), (process.env.GH_AW_SAFE_OUTPUTS_TOOLS_PATH = path.join(tempOutputDir, "tools.json"))); diff --git a/actions/setup/js/safe_outputs_mcp_server_defaults.test.cjs b/actions/setup/js/safe_outputs_mcp_server_defaults.test.cjs index f00919196d..66dda8f1a9 100644 --- a/actions/setup/js/safe_outputs_mcp_server_defaults.test.cjs +++ b/actions/setup/js/safe_outputs_mcp_server_defaults.test.cjs @@ -17,7 +17,12 @@ import { spawn } from "child_process"; fs.writeFileSync(toolsJsonPath, toolsJsonContent); }), afterEach(() => { - ((process.env = originalEnv), fs.existsSync(tempConfigFile) && fs.unlinkSync(tempConfigFile), fs.existsSync(tempOutputDir) && fs.rmSync(tempOutputDir, { recursive: !0, force: !0 })); + (Object.keys(process.env).forEach(k => { + if (!(k in originalEnv)) delete process.env[k]; + }), + Object.assign(process.env, originalEnv), + fs.existsSync(tempConfigFile) && fs.unlinkSync(tempConfigFile), + fs.existsSync(tempOutputDir) && fs.rmSync(tempOutputDir, { recursive: !0, force: !0 })); }), it("should use default output file when GH_AW_SAFE_OUTPUTS is not set", async () => { (delete process.env.GH_AW_SAFE_OUTPUTS, delete process.env.GH_AW_SAFE_OUTPUTS_CONFIG_PATH, fs.existsSync("/opt/gh-aw/safeoutputs") || fs.mkdirSync("/opt/gh-aw/safeoutputs", { recursive: !0 })); diff --git a/actions/setup/js/workflow_metadata_helpers.test.cjs b/actions/setup/js/workflow_metadata_helpers.test.cjs index 8a5d59c3d0..d9db2a8ac9 100644 --- a/actions/setup/js/workflow_metadata_helpers.test.cjs +++ b/actions/setup/js/workflow_metadata_helpers.test.cjs @@ -23,8 +23,13 @@ describe("getWorkflowMetadata", () => { }); afterEach(() => { - // Restore original environment - process.env = originalEnv; + // Restore environment by mutating process.env in place + for (const key of Object.keys(process.env)) { + if (!(key in originalEnv)) { + delete process.env[key]; + } + } + Object.assign(process.env, originalEnv); // Restore original context global.context = originalContext; diff --git a/docs/src/content/docs/reference/safe-outputs.md b/docs/src/content/docs/reference/safe-outputs.md index 15a8abd846..b64a921dcd 100644 --- a/docs/src/content/docs/reference/safe-outputs.md +++ b/docs/src/content/docs/reference/safe-outputs.md @@ -826,6 +826,10 @@ safe-outputs: Pushes changes to a PR's branch. Validates via `title-prefix` and `labels` to ensure only approved PRs receive changes. Multiple pushes per run are supported by setting `max` higher than 1. +:::caution[Fork PRs Not Supported] +This safe output **cannot push to PRs from forks**. Fork PRs will fail early with a clear error message. This is a security restriction—the workflow does not have write access to fork repositories. +::: + ```yaml wrap safe-outputs: push-to-pull-request-branch: