diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 7869cf329f7..2337566f7c4 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -56,6 +56,7 @@ async function main(config = {}) { const envLabels = config.labels ? (Array.isArray(config.labels) ? config.labels : config.labels.split(",")).map(label => String(label).trim()).filter(label => label) : []; const ifNoChanges = config.if_no_changes || "warn"; const ignoreMissingBranchFailure = config.ignore_missing_branch_failure === true; + const fallbackAsPullRequest = config.fallback_as_pull_request !== false; const commitTitleSuffix = config.commit_title_suffix || ""; const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; const maxCount = config.max || 0; // 0 means no limit @@ -91,6 +92,7 @@ async function main(config = {}) { } core.info(`If no changes: ${ifNoChanges}`); core.info(`Ignore missing branch failure: ${ignoreMissingBranchFailure}`); + core.info(`Fallback as pull request: ${fallbackAsPullRequest}`); if (commitTitleSuffix) { core.info(`Commit title suffix: ${commitTitleSuffix}`); } @@ -770,6 +772,58 @@ async function main(config = {}) { core.warning(`Push failed and branch existence re-check errored for ${branchName}: ${getErrorMessage(diagnosisError)}`); } + // Fallback path for diverged branches: create a new pull request so changes + // can still be reviewed and merged into the original PR branch. + if (isNonFastForward && fallbackAsPullRequest) { + const fallbackBranchName = normalizeBranchName(`${branchName}-fallback`, String(Date.now())); + core.warning(`Non-fast-forward push detected; creating fallback pull request from '${fallbackBranchName}' to '${branchName}'`); + try { + await exec.exec("git", ["checkout", "-b", fallbackBranchName]); + await exec.exec("git", ["push", "origin", fallbackBranchName], { + env: { ...process.env, ...gitAuthEnv }, + }); + + const fallbackBody = [ + "> [!NOTE]", + "> Direct push to the original pull request branch failed because the branch diverged (non-fast-forward).", + `> Original PR branch: \`${branchName}\``, + "", + `This fallback PR contains the prepared changes for PR #${pullNumber}.`, + "Merge this fallback PR into the original PR branch to apply them.", + "", + `Workflow run: ${buildWorkflowRunUrl(context, context.repo)}`, + ].join("\n"); + + const { data: fallbackPR } = await githubClient.rest.pulls.create({ + owner: repoParts.owner, + repo: repoParts.repo, + title: `[fallback] ${prTitle || `Changes for #${pullNumber}`}`, + body: fallbackBody, + head: fallbackBranchName, + base: branchName, + }); + + core.info(`Created fallback pull request #${fallbackPR.number}: ${fallbackPR.html_url}`); + await updateActivationComment(github, context, core, fallbackPR.html_url, fallbackPR.number, "pull_request"); + + return { + success: true, + fallback_used: true, + fallback_type: "pull_request", + pull_request_number: fallbackPR.number, + pull_request_url: fallbackPR.html_url, + branch_name: fallbackBranchName, + repo: itemRepo, + number: fallbackPR.number, + url: fallbackPR.html_url, + }; + } catch (fallbackError) { + const fallbackErrorMessage = getErrorMessage(fallbackError); + core.error(`Failed to create fallback pull request: ${fallbackErrorMessage}`); + userMessage = `${userMessage} Fallback pull request creation also failed: ${fallbackErrorMessage}`; + } + } + return { success: false, error_type: "push_failed", error: userMessage }; } diff --git a/actions/setup/js/push_to_pull_request_branch.test.cjs b/actions/setup/js/push_to_pull_request_branch.test.cjs index 4fdc66c4d4d..b7f94059c63 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -144,6 +144,12 @@ describe("push_to_pull_request_branch.cjs", () => { labels: [], }, }), + create: vi.fn().mockResolvedValue({ + data: { + number: 999, + html_url: "https://github.com/test-owner/test-repo/pull/999", + }, + }), }, repos: { get: vi.fn().mockResolvedValue({ @@ -767,7 +773,7 @@ index 0000000..abc1234 expect(mockCore.info).toHaveBeenCalledWith("Investigating patch failure..."); }); - it("should handle git push rejection (concurrent changes)", async () => { + it("should create fallback pull request on non-fast-forward push rejection by default", async () => { const patchPath = createPatchFile(); // Set up successful operations until push @@ -798,8 +804,40 @@ index 0000000..abc1234 const handler = await module.main({}); const result = await handler({ patch_path: patchPath }, {}); - // The error happens during push + expect(result.success).toBe(true); + expect(result.fallback_used).toBe(true); + expect(result.fallback_type).toBe("pull_request"); + expect(result.pull_request_number).toBe(999); + expect(mockGithub.rest.pulls.create).toHaveBeenCalled(); + }); + + it("should not create fallback pull request when fallback-as-pull-request is disabled", async () => { + const patchPath = createPatchFile(); + + 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: "" }); // git rev-parse HEAD (before patch) + + mockExec.exec.mockResolvedValueOnce(0); // git am + + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "abc123\n", stderr: "" }); // git rev-list + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "remote-oid\trefs/heads/feature-branch\n", stderr: "" }); // git ls-remote + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "Test commit\n", stderr: "" }); // git log -1 + mockExec.getExecOutput.mockResolvedValueOnce({ exitCode: 0, stdout: "", stderr: "" }); // git diff --name-status + + mockGithub.graphql.mockRejectedValueOnce(new Error("GraphQL error: branch protection")); + mockExec.exec.mockRejectedValueOnce(new Error("! [rejected] feature-branch -> feature-branch (non-fast-forward)")); + + const module = await loadModule(); + const handler = await module.main({ fallback_as_pull_request: false }); + const result = await handler({ patch_path: patchPath }, {}); + expect(result.success).toBe(false); + expect(result.error_type).toBe("push_failed"); + expect(result.error).toContain("non-fast-forward"); + expect(mockGithub.rest.pulls.create).not.toHaveBeenCalled(); }); it("should diagnose deleted branch when push fails", async () => { diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 189f4dc010a..b5559147f4e 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -360,10 +360,10 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) /** @type {Array<{type: string, error: string}>} */ const codePushFailures = []; - // Track when a code-push operation falls back to creating a review issue instead. + // Track when a code-push operation falls back to creating an issue or pull request instead. // When set, subsequent add_comment messages will receive a correction note prepended - // to their body so the posted comment accurately reflects the actual outcome. - /** @type {{type: string, issueNumber: number, issueUrl: string}|null} */ + // to their body so the posted comment accurately reflects the actual fallback target. + /** @type {{type: string, fallbackTargetType: "issue" | "pull_request", number: number, url: string}|null} */ let codePushFallbackInfo = null; // Load custom safe output job types (from GH_AW_SAFE_OUTPUT_JOBS env var) @@ -481,9 +481,12 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) // If a previous code-push operation fell back to a review issue, prepend a correction note // so the posted comment accurately reflects the outcome. if (codePushFallbackInfo) { - const fallbackNote = `\n\n---\n> [!NOTE]\n> The pull request was not created — a fallback review issue was created instead due to protected file changes: [#${codePushFallbackInfo.issueNumber}](${codePushFallbackInfo.issueUrl})\n\n`; + const fallbackNote = + codePushFallbackInfo.fallbackTargetType === "pull_request" + ? `\n\n---\n> [!NOTE]\n> Direct push to the original pull request branch was not possible (diverged/non-fast-forward). A fallback pull request was created instead: [#${codePushFallbackInfo.number}](${codePushFallbackInfo.url})\n\n` + : `\n\n---\n> [!NOTE]\n> The pull request was not created — a fallback review issue was created instead due to protected file changes: [#${codePushFallbackInfo.number}](${codePushFallbackInfo.url})\n\n`; effectiveMessage = { ...effectiveMessage, body: fallbackNote + (effectiveMessage.body || "") }; - core.info(`Prepending fallback correction note to add_comment body (fallback issue: #${codePushFallbackInfo.issueNumber})`); + core.info(`Prepending fallback correction note to add_comment body (fallback ${codePushFallbackInfo.fallbackTargetType}: #${codePushFallbackInfo.number})`); } // If a previous code-push operation failed outright (e.g. patch application error), // prepend a failure warning so the status comment accurately reflects that the @@ -585,11 +588,26 @@ async function processMessages(messageHandlers, messages, onItemCreated = null) } } - // Track when a code-push operation falls back to a review issue so subsequent + // Track when a code-push operation falls back to an issue or pull request so subsequent // add_comment messages can include a correction note. - if (CODE_PUSH_TYPES.has(messageType) && result && result.fallback_used === true && result.issue_number != null && result.issue_url) { - codePushFallbackInfo = { type: messageType, issueNumber: result.issue_number, issueUrl: result.issue_url }; - core.info(`Code push '${messageType}' fell back to review issue #${result.issue_number} — add_comment messages will be annotated`); + if (CODE_PUSH_TYPES.has(messageType) && result && result.fallback_used === true) { + if (result.issue_number != null && result.issue_url) { + codePushFallbackInfo = { + type: messageType, + fallbackTargetType: "issue", + number: result.issue_number, + url: result.issue_url, + }; + core.info(`Code push '${messageType}' fell back to review issue #${result.issue_number} — add_comment messages will be annotated`); + } else if (result.pull_request_number != null && result.pull_request_url) { + codePushFallbackInfo = { + type: messageType, + fallbackTargetType: "pull_request", + number: result.pull_request_number, + url: result.pull_request_url, + }; + core.info(`Code push '${messageType}' fell back to pull request #${result.pull_request_number} — add_comment messages will be annotated`); + } } // Check if this output was created with unresolved temporary IDs diff --git a/actions/setup/js/safe_output_handler_manager.test.cjs b/actions/setup/js/safe_output_handler_manager.test.cjs index 50b85111096..89e7b73b386 100644 --- a/actions/setup/js/safe_output_handler_manager.test.cjs +++ b/actions/setup/js/safe_output_handler_manager.test.cjs @@ -1333,6 +1333,34 @@ describe("Safe Output Handler Manager", () => { expect(calledMessage.body).toContain("#7"); }); + it("should prepend fallback note to add_comment body when push_to_pull_request_branch falls back to pull request", async () => { + const messages = [ + { type: "push_to_pull_request_branch", branch: "fix-branch" }, + { type: "add_comment", body: "Changes pushed." }, + ]; + + const pushHandler = vi.fn().mockResolvedValue({ + success: true, + fallback_used: true, + fallback_type: "pull_request", + pull_request_number: 71, + pull_request_url: "https://github.com/owner/repo/pull/71", + }); + const commentHandler = vi.fn().mockResolvedValue([{ _tracking: null }]); + + const handlers = new Map([ + ["push_to_pull_request_branch", pushHandler], + ["add_comment", commentHandler], + ]); + + await processMessages(handlers, messages); + + const calledMessage = commentHandler.mock.calls[0][0]; + expect(calledMessage.body).toContain("Direct push to the original pull request branch was not possible"); + expect(calledMessage.body).toContain("#71"); + expect(calledMessage.body).toContain("https://github.com/owner/repo/pull/71"); + }); + it("should NOT prepend fallback note when create_pull_request succeeds normally", async () => { const messages = [ { type: "create_pull_request", title: "My Fix PR" }, diff --git a/actions/setup/js/safe_output_summary.cjs b/actions/setup/js/safe_output_summary.cjs index 94b0ef776e7..1e17ed89b29 100644 --- a/actions/setup/js/safe_output_summary.cjs +++ b/actions/setup/js/safe_output_summary.cjs @@ -30,28 +30,41 @@ function generateSafeOutputSummary(options) { .map(word => word.charAt(0).toUpperCase() + word.slice(1)) .join(" "); - // Detect fallback-to-issue outcome for code-push types + // Detect fallback outcomes for code-push types. + // Prefer explicit fallback_type when available; infer only for backward compatibility. const isFallback = success && result && result.fallback_used === true; + const inferredFallbackType = isFallback && (result.pull_request_url || result.pull_request_number != null) ? "pull_request" : "issue"; + const fallbackType = isFallback && result?.fallback_type ? result.fallback_type : inferredFallbackType; // Choose emoji and status based on success and fallback const emoji = isFallback ? "⚠️" : success ? "✅" : "❌"; - const status = isFallback ? "Fallback Issue Created" : success ? "Success" : "Failed"; + const status = isFallback ? (fallbackType === "pull_request" ? "Fallback Pull Request Created" : "Fallback Issue Created") : success ? "Success" : "Failed"; // Start building the summary let summary = `
\n${emoji} ${displayType} - ${status} (Message ${messageIndex})\n\n`; // Add message details - const sectionTitle = isFallback ? `### ${displayType} — Fallback Issue\n\n` : `### ${displayType}\n\n`; + const sectionTitle = isFallback ? `### ${displayType} — ${fallbackType === "pull_request" ? "Fallback Pull Request" : "Fallback Issue"}\n\n` : `### ${displayType}\n\n`; summary += sectionTitle; if (isFallback) { - // Explain why the fallback occurred and show the created issue - summary += `> ℹ️ Pull request creation was blocked due to protected file changes. A review issue was created instead.\n\n`; - if (result.issue_url) { - summary += `**Fallback Issue:** ${result.issue_url}\n\n`; - } - if (result.issue_number != null && result.repo) { - summary += `**Location:** ${result.repo}#${result.issue_number}\n\n`; + // Explain why the fallback occurred and show the created fallback target + if (fallbackType === "pull_request") { + summary += `> ℹ️ Direct push to the original pull request branch was not possible (diverged/non-fast-forward). A fallback pull request was created instead.\n\n`; + if (result.pull_request_url) { + summary += `**Fallback Pull Request:** ${result.pull_request_url}\n\n`; + } + if (result.pull_request_number != null && result.repo) { + summary += `**Location:** ${result.repo}#${result.pull_request_number}\n\n`; + } + } else { + summary += `> ℹ️ Pull request creation was blocked due to protected file changes. A review issue was created instead.\n\n`; + if (result.issue_url) { + summary += `**Fallback Issue:** ${result.issue_url}\n\n`; + } + if (result.issue_number != null && result.repo) { + summary += `**Location:** ${result.repo}#${result.issue_number}\n\n`; + } } if (result.branch_name) { summary += `**Branch:** \`${result.branch_name}\`\n\n`; diff --git a/actions/setup/js/safe_output_summary.test.cjs b/actions/setup/js/safe_output_summary.test.cjs index bce462f9e43..e37f78cdf08 100644 --- a/actions/setup/js/safe_output_summary.test.cjs +++ b/actions/setup/js/safe_output_summary.test.cjs @@ -360,6 +360,59 @@ describe("safe_output_summary", () => { expect(summary).not.toContain("⚠️"); expect(summary).not.toContain("Fallback"); }); + + it("should show fallback pull request status when push_to_pull_request_branch falls back to pull request", () => { + const options = { + type: "push_to_pull_request_branch", + messageIndex: 3, + success: true, + result: { + fallback_used: true, + fallback_type: "pull_request", + pull_request_number: 71, + pull_request_url: "https://github.com/owner/repo/pull/71", + repo: "owner/repo", + }, + message: { + body: "Pushing to PR branch.", + }, + }; + + const summary = generateSafeOutputSummary(options); + + expect(summary).toContain("⚠️"); + expect(summary).toContain("Fallback Pull Request Created"); + expect(summary).toContain("https://github.com/owner/repo/pull/71"); + expect(summary).toContain("owner/repo#71"); + expect(summary).toContain("non-fast-forward"); + }); + + it("should prefer explicit fallback_type over inferred shape for backward compatibility", () => { + const options = { + type: "push_to_pull_request_branch", + messageIndex: 4, + success: true, + result: { + fallback_used: true, + fallback_type: "issue", + // pull_request_url present by shape, but explicit fallback_type should win + pull_request_url: "https://github.com/owner/repo/pull/72", + issue_number: 123, + issue_url: "https://github.com/owner/repo/issues/123", + repo: "owner/repo", + }, + message: { + body: "Pushing to PR branch.", + }, + }; + + const summary = generateSafeOutputSummary(options); + + expect(summary).toContain("Fallback Issue Created"); + expect(summary).toContain("Fallback Issue:"); + expect(summary).toContain("https://github.com/owner/repo/issues/123"); + expect(summary).not.toContain("Fallback Pull Request Created"); + }); }); describe("writeSafeOutputSummaries", () => { diff --git a/actions/setup/js/types/handler-factory.d.ts b/actions/setup/js/types/handler-factory.d.ts index 848817b2a20..f5ef66af9f3 100644 --- a/actions/setup/js/types/handler-factory.d.ts +++ b/actions/setup/js/types/handler-factory.d.ts @@ -18,6 +18,8 @@ interface HandlerConfig { protected_path_prefixes?: string[]; /** Policy for how protected file matches are handled: "blocked" (default), "fallback-to-issue", or "allowed" */ protected_files_policy?: string; + /** When true (default), create a fallback pull request if direct push to PR branch fails with non-fast-forward/diverged branch. */ + fallback_as_pull_request?: boolean; /** Additional handler-specific configuration properties */ [key: string]: any; } diff --git a/docs/src/content/docs/reference/frontmatter-full.md b/docs/src/content/docs/reference/frontmatter-full.md index e29e729d7a6..69b3930eb56 100644 --- a/docs/src/content/docs/reference/frontmatter-full.md +++ b/docs/src/content/docs/reference/frontmatter-full.md @@ -4599,6 +4599,13 @@ safe-outputs: # (optional) github-token-for-extra-empty-commit: "example-value" + # When true (default), if pushing to the PR branch fails due to a + # non-fast-forward/diverged branch, create a fallback pull request that targets + # the original PR branch. Set to false to disable this behavior and avoid + # requiring pull-requests: write permission. + # (optional) + fallback-as-pull-request: true + # Target repository in format 'owner/repo' for cross-repository push to pull # request branch. Takes precedence over trial target repo settings. # (optional) diff --git a/docs/src/content/docs/reference/safe-outputs-pull-requests.md b/docs/src/content/docs/reference/safe-outputs-pull-requests.md index 6d339a5a146..fcd57dd4350 100644 --- a/docs/src/content/docs/reference/safe-outputs-pull-requests.md +++ b/docs/src/content/docs/reference/safe-outputs-pull-requests.md @@ -234,6 +234,7 @@ safe-outputs: - "**/*.lock" github-token: ${{ secrets.SOME_CUSTOM_TOKEN }} # optional custom token for permissions github-token-for-extra-empty-commit: ${{ secrets.CI_TOKEN }} # optional token to push empty commit triggering CI + fallback-as-pull-request: true # default: on non-fast-forward push failure, create fallback PR to original PR branch protected-files: fallback-to-issue # create review issue if protected files modified ``` @@ -283,6 +284,8 @@ checkout: If `push-to-pull-request-branch` (or `create-pull-request`) fails, the safe-output pipeline cancels all remaining non-code-push outputs. Each cancelled output is marked with an explicit reason such as "Cancelled: code push operation failed". The failure details appear in the agent failure issue or comment generated by the conclusion job. +When `fallback-as-pull-request` is enabled (default), non-fast-forward push failures trigger a fallback pull request that targets the original PR branch. Set `fallback-as-pull-request: false` to disable this fallback behavior. + ## Protected Files Both `create-pull-request` and `push-to-pull-request-branch` enforce protected file protection by default. Patches that modify package manifests, agent instruction files, or repository security configuration are refused unless you explicitly configure a policy. diff --git a/pkg/parser/schemas/main_workflow_schema.json b/pkg/parser/schemas/main_workflow_schema.json index 23994195e59..c41bd8294c6 100644 --- a/pkg/parser/schemas/main_workflow_schema.json +++ b/pkg/parser/schemas/main_workflow_schema.json @@ -7096,6 +7096,11 @@ "type": "string", "description": "Token used to push an empty commit after pushing changes to trigger CI events. Works around the GITHUB_TOKEN limitation where pushes don't trigger workflow runs. Defaults to the magic secret GH_AW_CI_TRIGGER_TOKEN if set in the repository. Use a secret expression (e.g. '${{ secrets.CI_TOKEN }}') for a custom token, or 'app' for GitHub App auth." }, + "fallback-as-pull-request": { + "type": "boolean", + "description": "When true (default), if pushing to the PR branch fails due to a non-fast-forward/diverged branch, create a fallback pull request that targets the original PR branch. Set to false to disable this behavior and avoid requiring pull-requests: write permission.", + "default": true + }, "target-repo": { "type": "string", "description": "Target repository in format 'owner/repo' for cross-repository push to pull request branch. Takes precedence over trial target repo settings." diff --git a/pkg/workflow/compiler_safe_outputs_handlers.go b/pkg/workflow/compiler_safe_outputs_handlers.go index c42bb7abc61..1386b9aef3b 100644 --- a/pkg/workflow/compiler_safe_outputs_handlers.go +++ b/pkg/workflow/compiler_safe_outputs_handlers.go @@ -425,6 +425,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddStringSlice("allowed_files", c.AllowedFiles). AddStringSlice("excluded_files", c.ExcludedFiles). AddIfNotEmpty("patch_format", c.PatchFormat). + AddBoolPtr("fallback_as_pull_request", c.FallbackAsPullRequest). Build() }, "update_pull_request": func(cfg *SafeOutputsConfig) map[string]any { diff --git a/pkg/workflow/push_to_pull_request_branch.go b/pkg/workflow/push_to_pull_request_branch.go index e3a31c1538d..4530a332a20 100644 --- a/pkg/workflow/push_to_pull_request_branch.go +++ b/pkg/workflow/push_to_pull_request_branch.go @@ -26,6 +26,7 @@ type PushToPullRequestBranchConfig struct { AllowedFiles []string `yaml:"allowed-files,omitempty"` // Strict allowlist of glob patterns for files eligible for push. Checked independently of protected-files; both checks must pass. ExcludedFiles []string `yaml:"excluded-files,omitempty"` // List of glob patterns for files to exclude from the patch using git :(exclude) pathspecs. Matching files are stripped by git at generation time and will not appear in the commit or be subject to allowed-files or protected-files checks. PatchFormat string `yaml:"patch-format,omitempty"` // Transport format for packaging changes: "am" (default, uses git format-patch) or "bundle" (uses git bundle, preserves merge topology and per-commit metadata). + FallbackAsPullRequest *bool `yaml:"fallback-as-pull-request,omitempty"` // When true (default), creates a fallback pull request if direct push fails due to diverged/non-fast-forward branch. When false, fallback is disabled and pull-requests: write is not requested. AllowWorkflows bool `yaml:"allow-workflows,omitempty"` // When true, adds workflows: write to the GitHub App token. Requires safe-outputs.github-app to be configured. } @@ -173,6 +174,13 @@ func (c *Compiler) parsePushToPullRequestBranchConfig(outputMap map[string]any) } } + // Parse fallback-as-pull-request (optional, defaults to true) + if fallbackAsPullRequest, exists := configMap["fallback-as-pull-request"]; exists { + if fallbackAsPullRequestBool, ok := fallbackAsPullRequest.(bool); ok { + pushToBranchConfig.FallbackAsPullRequest = &fallbackAsPullRequestBool + } + } + // Parse allow-workflows: when true, adds workflows: write to the GitHub App token if allowWorkflows, exists := configMap["allow-workflows"]; exists { if allowWorkflowsBool, ok := allowWorkflows.(bool); ok { diff --git a/pkg/workflow/push_to_pull_request_branch_test.go b/pkg/workflow/push_to_pull_request_branch_test.go index d42c6ef7397..989d018b4a2 100644 --- a/pkg/workflow/push_to_pull_request_branch_test.go +++ b/pkg/workflow/push_to_pull_request_branch_test.go @@ -3,6 +3,7 @@ package workflow import ( + "encoding/json" "os" "path/filepath" "strings" @@ -11,8 +12,66 @@ import ( "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/testutil" + "go.yaml.in/yaml/v3" ) +func extractPushToPullRequestBranchHandlerConfig(t *testing.T, lockContent []byte) map[string]any { + t.Helper() + + var workflowDoc map[string]any + if err := yaml.Unmarshal(lockContent, &workflowDoc); err != nil { + t.Fatalf("Failed to unmarshal lock workflow YAML: %v", err) + } + + jobsRaw, ok := workflowDoc["jobs"].(map[string]any) + if !ok { + t.Fatalf("Generated workflow should contain jobs map") + } + + safeOutputsJobRaw, ok := jobsRaw["safe_outputs"].(map[string]any) + if !ok { + t.Fatalf("Generated workflow should contain safe_outputs job") + } + + stepsRaw, ok := safeOutputsJobRaw["steps"].([]any) + if !ok { + t.Fatalf("Generated workflow safe_outputs job should contain steps array") + } + + var handlerConfigJSON string + for _, step := range stepsRaw { + stepMap, ok := step.(map[string]any) + if !ok { + continue + } + envMap, ok := stepMap["env"].(map[string]any) + if !ok { + continue + } + rawConfig, ok := envMap["GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG"].(string) + if ok && rawConfig != "" { + handlerConfigJSON = rawConfig + break + } + } + + if handlerConfigJSON == "" { + t.Fatalf("Generated workflow should contain GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG env var") + } + + var handlerConfig map[string]any + if err := json.Unmarshal([]byte(handlerConfigJSON), &handlerConfig); err != nil { + t.Fatalf("Failed to unmarshal GH_AW_SAFE_OUTPUTS_HANDLER_CONFIG JSON: %v", err) + } + + pushCfgRaw, ok := handlerConfig["push_to_pull_request_branch"].(map[string]any) + if !ok { + t.Fatalf("Handler config should contain push_to_pull_request_branch object") + } + + return pushCfgRaw +} + func TestPushToPullRequestBranchConfigParsing(t *testing.T) { // Create a temporary directory for the test tmpDir := testutil.TempDir(t, "test-*") @@ -130,6 +189,104 @@ safe-outputs: } } +func TestPushToPullRequestBranchFallbackAsPullRequestDisabled(t *testing.T) { + tmpDir := testutil.TempDir(t, "test-*") + + testMarkdown := `--- +on: + pull_request: + types: [opened, synchronize] +safe-outputs: + push-to-pull-request-branch: + fallback-as-pull-request: false +--- + +# Test Push to PR Branch Fallback Disabled +` + + mdFile := filepath.Join(tmpDir, "test-push-to-pull-request-branch-fallback-disabled.md") + if err := os.WriteFile(mdFile, []byte(testMarkdown), 0644); err != nil { + t.Fatalf("Failed to write test markdown file: %v", err) + } + + compiler := NewCompiler() + if err := compiler.CompileWorkflow(mdFile); err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + lockFile := stringutil.MarkdownToLockFile(mdFile) + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + pushConfig := extractPushToPullRequestBranchHandlerConfig(t, lockContent) + fallbackAsPullRequest, exists := pushConfig["fallback_as_pull_request"] + if !exists { + t.Errorf("Generated workflow should contain fallback_as_pull_request in handler config JSON") + } + fallbackAsPullRequestBool, isBool := fallbackAsPullRequest.(bool) + if !isBool { + t.Errorf("Expected fallback_as_pull_request to be a bool, got %#v", fallbackAsPullRequest) + } + if fallbackAsPullRequestBool { + t.Errorf("Expected fallback_as_pull_request=false, got %#v", fallbackAsPullRequestBool) + } + if strings.Contains(lockContentStr, "pull-requests: write") { + t.Errorf("Generated workflow should NOT have pull-requests: write permission when fallback-as-pull-request is false") + } +} + +func TestPushToPullRequestBranchFallbackAsPullRequestEnabled(t *testing.T) { + tmpDir := testutil.TempDir(t, "test-*") + + testMarkdown := `--- +on: + pull_request: + types: [opened, synchronize] +safe-outputs: + push-to-pull-request-branch: + fallback-as-pull-request: true +--- + +# Test Push to PR Branch Fallback Enabled +` + + mdFile := filepath.Join(tmpDir, "test-push-to-pull-request-branch-fallback-enabled.md") + if err := os.WriteFile(mdFile, []byte(testMarkdown), 0644); err != nil { + t.Fatalf("Failed to write test markdown file: %v", err) + } + + compiler := NewCompiler() + if err := compiler.CompileWorkflow(mdFile); err != nil { + t.Fatalf("Failed to compile workflow: %v", err) + } + + lockFile := stringutil.MarkdownToLockFile(mdFile) + lockContent, err := os.ReadFile(lockFile) + if err != nil { + t.Fatalf("Failed to read lock file: %v", err) + } + + lockContentStr := string(lockContent) + pushConfig := extractPushToPullRequestBranchHandlerConfig(t, lockContent) + fallbackAsPullRequest, exists := pushConfig["fallback_as_pull_request"] + if !exists { + t.Errorf("Generated workflow should contain fallback_as_pull_request in handler config JSON") + } + fallbackAsPullRequestBool, isBool := fallbackAsPullRequest.(bool) + if !isBool { + t.Errorf("Expected fallback_as_pull_request to be a bool, got %#v", fallbackAsPullRequest) + } + if !fallbackAsPullRequestBool { + t.Errorf("Expected fallback_as_pull_request=true, got %#v", fallbackAsPullRequestBool) + } + if !strings.Contains(lockContentStr, "pull-requests: write") { + t.Errorf("Generated workflow should have pull-requests: write permission when fallback-as-pull-request is true") + } +} + func TestPushToPullRequestBranchWithTargetAsterisk(t *testing.T) { // Create a temporary directory for the test tmpDir := testutil.TempDir(t, "test-*") diff --git a/pkg/workflow/safe_outputs_permissions.go b/pkg/workflow/safe_outputs_permissions.go index 74ff34d27cc..940ea72f2e6 100644 --- a/pkg/workflow/safe_outputs_permissions.go +++ b/pkg/workflow/safe_outputs_permissions.go @@ -51,6 +51,14 @@ func isHandlerStaged(globalStaged, handlerStaged bool) bool { return globalStaged || handlerStaged } +// getPushFallbackAsPullRequest returns the effective fallback-as-pull-request setting (defaults to true). +func getPushFallbackAsPullRequest(config *PushToPullRequestBranchConfig) bool { + if config == nil || config.FallbackAsPullRequest == nil { + return true // Default + } + return *config.FallbackAsPullRequest +} + // ComputePermissionsForSafeOutputs computes the minimal required permissions // based on the configured safe-outputs. This function is used by both the // consolidated safe outputs job and the conclusion job to ensure they only @@ -137,8 +145,13 @@ func ComputePermissionsForSafeOutputs(safeOutputs *SafeOutputsConfig) *Permissio } } if safeOutputs.PushToPullRequestBranch != nil && !isHandlerStaged(safeOutputs.Staged, safeOutputs.PushToPullRequestBranch.Staged) { - safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch") - permissions.Merge(NewPermissionsContentsWritePRWrite()) + if getPushFallbackAsPullRequest(safeOutputs.PushToPullRequestBranch) { + safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch with fallback-as-pull-request") + permissions.Merge(NewPermissionsContentsWritePRWrite()) + } else { + safeOutputsPermissionsLog.Print("Adding permissions for push-to-pull-request-branch without fallback-as-pull-request") + permissions.Merge(NewPermissionsContentsWrite()) + } // Add workflows: write when allow-workflows is true (GitHub App-only permission) if safeOutputs.PushToPullRequestBranch.AllowWorkflows { safeOutputsPermissionsLog.Print("Adding workflows: write for push-to-pull-request-branch (allow-workflows: true)") diff --git a/pkg/workflow/safe_outputs_permissions_test.go b/pkg/workflow/safe_outputs_permissions_test.go index f1ffc6bb765..a4533a2856b 100644 --- a/pkg/workflow/safe_outputs_permissions_test.go +++ b/pkg/workflow/safe_outputs_permissions_test.go @@ -245,7 +245,7 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { }, }, { - name: "push-to-pull-request-branch - no issues permission", + name: "push-to-pull-request-branch default fallback - requires pull-requests write", safeOutputs: &SafeOutputsConfig{ PushToPullRequestBranch: &PushToPullRequestBranchConfig{ BaseSafeOutputConfig: BaseSafeOutputConfig{}, @@ -256,6 +256,18 @@ func TestComputePermissionsForSafeOutputs(t *testing.T) { PermissionPullRequests: PermissionWrite, }, }, + { + name: "push-to-pull-request-branch with fallback-as-pull-request false - no pull-requests permission", + safeOutputs: &SafeOutputsConfig{ + PushToPullRequestBranch: &PushToPullRequestBranchConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{}, + FallbackAsPullRequest: boolPtr(false), + }, + }, + expected: map[PermissionScope]PermissionLevel{ + PermissionContents: PermissionWrite, + }, + }, { name: "multiple safe outputs without discussions - no discussions permission", safeOutputs: &SafeOutputsConfig{