diff --git a/actions/setup/js/copilot_harness.cjs b/actions/setup/js/copilot_harness.cjs index 7ac1789671b..144cc16e005 100644 --- a/actions/setup/js/copilot_harness.cjs +++ b/actions/setup/js/copilot_harness.cjs @@ -715,7 +715,7 @@ async function main() { // correct SDK endpoint URI. const sdkEnv = buildCopilotSDKEnv(); const copilotSDKMode = isCopilotSDKEnabled(); - let copilotConnectionToken; + let copilotConnectionToken = ""; if (copilotSDKMode) { // The harness always generates the connection token when SDK mode is active. // The token is injected into the driver subprocess env so the harness-managed diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index b7f4782171a..38da3fc5ed5 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -133,17 +133,94 @@ function isWorkflowsScopeRejection(stderr) { return lower.includes("`workflows` scope") || lower.includes("workflow can be created or updated due to timeout"); } +/** + * Returns the list of unique workflow file paths (.github/workflows/**) present in the + * local branch history beyond the PR's base branch. This is used as a pre-flight check + * before pushing a new branch ref: GitHub rejects such pushes when the token lacks the + * 'workflows' scope, even if the current changeset itself does not touch workflow files + * (the rejection is based on ALL commits reachable from the pushed ref). + * + * Uses `origin/${baseBranch}` as the exclusion baseline so that commits already on the + * PR's target branch (which GitHub has already accepted) are excluded. Falls back to + * `origin/HEAD` when `baseBranch` is not available, and to an empty array (no workflow + * changes detected) when the baseline ref is not resolvable or the git command fails — + * in that case the push is still attempted and any real 'workflows' scope rejection will + * be caught and surfaced as the typed error downstream. + * + * Note: `origin/${baseBranch}` and `origin/HEAD` are intentionally different baselines + * for their respective layers. `origin/${baseBranch}` limits detection to commits the + * agent actually introduced (correct for the PR delta). Using `origin/HEAD` here would + * traverse commits on the target branch itself for PRs targeting non-default branches, + * producing false-positive `workflows_scope_required` errors. + * + * @param {{ getExecOutput: Function }} exec - @actions/exec module (or compatible mock) + * @param {Record} gitOptions - Base git exec options (cwd, env, etc.) + * @param {string | undefined} baseBranch - PR base branch name (e.g. "main"); falls back to origin/HEAD when not provided + * @param {typeof core} coreLogger - Actions core logger used for debug output + * @returns {Promise} Unique workflow file paths found in the branch history + */ +async function detectWorkflowFileChanges(exec, gitOptions, baseBranch, coreLogger) { + const baseline = baseBranch && baseBranch.trim() ? `origin/${baseBranch}` : "origin/HEAD"; + try { + const result = await exec.getExecOutput("git", ["log", "--name-only", "--pretty=format:", "HEAD", "--not", baseline, "--", ".github/workflows/"], { ...gitOptions, ignoreReturnCode: true }); + if (result.exitCode !== 0) { + // Non-zero exit means the baseline ref was not resolvable or git failed; + // treat as no workflow changes so the push proceeds and any real scope + // rejection surfaces downstream. + coreLogger.debug(`detectWorkflowFileChanges: git log exited ${result.exitCode} (baseline '${baseline}' may be unavailable); skipping pre-flight`); + return []; + } + return [ + ...new Set( + result.stdout + .split("\n") + .map(f => f.trim()) + .filter(Boolean) + ), + ]; + } catch (err) { + coreLogger.debug(`detectWorkflowFileChanges: git log threw (baseline '${baseline}'); skipping pre-flight: ${err instanceof Error ? err.message : String(err)}`); + return []; + } +} + +/** + * Performs a pre-flight workflow-scope check before pushing a new branch ref. + * Returns the typed error object when the branch history contains workflow file changes + * and `allowWorkflows` is false; returns null when the push may proceed. + * + * Extracts the duplicated guard that appears in both the review-branch and + * fallback-branch push paths so future changes only need to be made in one place. + * + * @param {{ getExecOutput: Function }} exec - @actions/exec module (or compatible mock) + * @param {Record} gitOptions - Base git exec options (cwd, env, etc.) + * @param {boolean} allowWorkflows - Whether the push token has the 'workflows' scope + * @param {string | undefined} baseBranch - PR base branch name passed through to detectWorkflowFileChanges + * @param {string} context - Short label for the push path (e.g. "Review branch", "Fallback branch") + * @param {typeof core} coreLogger - Actions core logger + * @returns {Promise<{ success: false, error_type: string, error: string } | null>} + */ +async function runWorkflowScopePreflightCheck(exec, gitOptions, allowWorkflows, baseBranch, context, coreLogger) { + if (allowWorkflows) return null; + const workflowFiles = await detectWorkflowFileChanges(exec, gitOptions, baseBranch, coreLogger); + if (workflowFiles.length > 0) { + coreLogger.info(`Pre-flight check: branch history contains workflow file changes (${workflowFiles.join(", ")}). Failing before push attempt.`); + return buildWorkflowsScopeError(`${context} pre-flight`, coreLogger); + } + return null; +} + /** * Builds the typed result and logs actionable guidance when a branch push fails * because the token lacks the 'workflows' scope. * * @param {string} context - Short label identifying the push path (e.g. "Review branch", "Fallback branch") - * @param {typeof core} core - Actions core logger + * @param {typeof core} coreLogger - Actions core logger * @returns {{ success: false, error_type: "workflows_scope_required", error: string }} */ -function buildWorkflowsScopeError(context, core) { - core.error(`${context} push rejected: the branch includes changes to workflow files (.github/workflows/**) that require the 'workflows' scope on the push token.`); - core.error("To allow this workflow to push workflow file changes, configure 'push-to-pull-request-branch.allow-workflows: true' together with a GitHub App in 'safe-outputs.github-app'."); +function buildWorkflowsScopeError(context, coreLogger) { + coreLogger.error(`${context} push rejected: the branch includes changes to workflow files (.github/workflows/**) that require the 'workflows' scope on the push token.`); + coreLogger.error("To allow this workflow to push workflow file changes, configure 'push-to-pull-request-branch.allow-workflows: true' together with a GitHub App in 'safe-outputs.github-app'."); return { success: false, error_type: "workflows_scope_required", @@ -170,6 +247,7 @@ async function main(config = {}) { const commitTitleSuffix = config.commit_title_suffix || ""; const maxSizeKb = parsePositiveInteger(config.max_patch_size) ?? 4096; const maxCount = config.max || 0; // 0 means no limit + const allowWorkflows = config.allow_workflows === true; // Cross-repo support: resolve target repository from config // This allows pushing to PRs in a different repository than the workflow @@ -1041,6 +1119,15 @@ async function main(config = {}) { // normalizeBranchName to enforce valid git ref characters + max length. const reviewBranchName = normalizeBranchName(`${branchName}-review`, String(Date.now())); try { + // Pre-flight: check full branch history for workflow file changes. + // GitHub rejects pushes of new branch refs whose commit history contains + // .github/workflows/** changes when the token lacks the 'workflows' scope — + // even if the current changeset itself does not touch workflow files. + // Failing here avoids leaving the local branch in a renamed state after + // a rejected push, and surfaces the error before any side effects. + const preflightError = await runWorkflowScopePreflightCheck(exec, baseGitOpts, allowWorkflows, pullRequest?.base?.ref, "Review branch", core); + if (preflightError) return preflightError; + // Rename current local branch to review branch await exec.exec("git", ["checkout", "-b", reviewBranchName], baseGitOpts); core.info(`Created review branch: ${reviewBranchName}`); @@ -1210,6 +1297,12 @@ async function main(config = {}) { const fallbackBranchName = normalizeBranchName(`${branchName}-fallback`, String(Date.now())); core.warning(`Non-fast-forward push detected; creating fallback pull request from '${fallbackBranchName}' to '${branchName}'`); try { + // Pre-flight: check full branch history for workflow file changes. + // Like the review branch path, creating a new fallback branch ref triggers + // GitHub's scope check on the full commit history, not just the new commits. + const preflightError = await runWorkflowScopePreflightCheck(exec, baseGitOpts, allowWorkflows, pullRequest?.base?.ref, "Fallback branch", core); + if (preflightError) return preflightError; + await exec.exec("git", ["checkout", "-b", fallbackBranchName], baseGitOpts); // Use getExecOutput to capture stderr for 'workflows' scope diagnostics const fallbackPushOutput = await exec.getExecOutput("git", ["push", "origin", fallbackBranchName], { 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 32d2505f9e0..89bdfb44e87 100644 --- a/actions/setup/js/push_to_pull_request_branch.test.cjs +++ b/actions/setup/js/push_to_pull_request_branch.test.cjs @@ -1110,6 +1110,11 @@ index 0000000..abc1234 return { exitCode: 0, stdout: "1111111111111111111111111111111111111111\trefs/heads/feature-branch\n", stderr: "" }; } if (argList[0] === "log") { + // Pre-flight workflow check targets .github/workflows/; return empty to avoid + // short-circuiting the fallback path with a workflows_scope_required error. + if (argList.includes(".github/workflows/")) { + return { exitCode: 0, stdout: "", stderr: "" }; + } return { exitCode: 0, stdout: "Test commit\n", stderr: "" }; } if (argList[0] === "diff-tree") { @@ -1440,6 +1445,72 @@ index 0000000..abc1234 expect(result.error_type).toBeUndefined(); expect(result.error).toContain("Failed to create review PR"); }); + + it("should fail pre-flight with workflows_scope_required when branch history contains workflow files", async () => { + process.env.GH_AW_DETECTION_CONCLUSION = "warning"; + createPatchFile("review-branch-preflight-workflow-files"); + + const originalGetExecOutput = mockExec.getExecOutput; + mockExec.getExecOutput = vi.fn().mockImplementation(async (cmd, args, options) => { + const argList = Array.isArray(args) ? args : []; + // Pre-flight git log targets .github/workflows/ directory — returns a workflow + // file path to simulate branch history containing .github/workflows/** changes. + if (cmd === "git" && argList[0] === "log" && argList.includes(".github/workflows/")) { + return { exitCode: 0, stdout: ".github/workflows/ci.yml\n", stderr: "" }; + } + // The git push should NOT be reached — pre-flight check fires first + if (cmd === "git" && argList[0] === "push" && argList[1] === "origin") { + throw new Error("git push should not be called when pre-flight check fires"); + } + return originalGetExecOutput(cmd, args, options); + }); + + const module = await loadModule(); + // allow_workflows not set (default false) — pre-flight check is active + const handler = await module.main({}); + const result = await handler({ branch: "review-branch-preflight-workflow-files" }, {}); + + expect(result.success).toBe(false); + expect(result.error_type).toBe("workflows_scope_required"); + expect(result.error).toContain("'workflows' scope"); + expect(result.error).toContain("allow-workflows"); + // Pre-flight fires before checkout — no "Failed to create review PR" message + const errorCalls = mockCore.error.mock.calls.map(c => c[0]); + expect(errorCalls.some(msg => msg.includes("Failed to create review PR"))).toBe(false); + expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("Pre-flight check")); + }); + + it("should skip pre-flight check and attempt push when allow_workflows is true", async () => { + process.env.GH_AW_DETECTION_CONCLUSION = "warning"; + createPatchFile("review-branch-allow-workflows-skip-preflight"); + + let preflightCalled = false; + let pushCalled = false; + const originalGetExecOutput = mockExec.getExecOutput; + mockExec.getExecOutput = vi.fn().mockImplementation(async (cmd, args, options) => { + const argList = Array.isArray(args) ? args : []; + if (cmd === "git" && argList[0] === "log" && argList.includes(".github/workflows/")) { + preflightCalled = true; + return { exitCode: 0, stdout: ".github/workflows/ci.yml\n", stderr: "" }; + } + if (cmd === "git" && argList[0] === "push" && argList[1] === "origin") { + pushCalled = true; + return { exitCode: 0, stdout: "", stderr: "" }; + } + return originalGetExecOutput(cmd, args, options); + }); + + const module = await loadModule(); + // allow_workflows: true — skip the pre-flight check + const handler = await module.main({ allow_workflows: true }); + const result = await handler({ branch: "review-branch-allow-workflows-skip-preflight" }, {}); + + // Pre-flight check should NOT have run + expect(preflightCalled).toBe(false); + // Push should have been attempted + expect(pushCalled).toBe(true); + expect(result.success).toBe(true); + }); }); // ────────────────────────────────────────────────────── diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index bd3ae78c668..fe864bfba5a 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -1250,6 +1250,67 @@ function createHandlers(server, appendSafeOutput, config = {}) { pushPinnedSha = null; } + // Full-branch allowed_files check: validate that ALL commits on the PR branch + // (relative to origin/baseBranch) only touch files permitted by allowed_files. + // The incremental patch check at apply-time only inspects the net diff between + // origin/ and the local branch tip; this catches disallowed files that + // appear in earlier commits on the branch (e.g. a Copilot branch that also + // modified .github/workflows/**) and returns an actionable error to the agent + // before any transport artifacts are generated. + if (Array.isArray(pushConfig.allowed_files) && pushConfig.allowed_files.length > 0) { + try { + // Use the pinned SHA as the range head to avoid any TOCTOU window between + // the time the SHA was recorded and the time of the git log query. If no + // pinned SHA is available (e.g. non-bundle path), skip the check so we do + // not race against a mutable ref; the apply-time check still enforces policy. + if (!pushPinnedSha) { + server.debug("Full-branch allowed-files check skipped: branch SHA not pinned (non-bundle path)"); + } else { + const branchHistoryFiles = execGitSync(["log", "--name-only", "--pretty=format:", `origin/${baseBranch}..${pushPinnedSha}`, "--"], { cwd: pushGitCwd }) + .toString() + .split("\n") + .map(s => s.trim()) + .filter(Boolean); + + if (branchHistoryFiles.length > 0) { + const allowedPatterns = pushConfig.allowed_files.map(p => globPatternToRegex(p)); + // Files matching excluded_files are intentionally exempt: they will be stripped + // from the patch at generation time via :(exclude) pathspecs, so they won't be + // present in the final changeset applied to the branch. + const excludedPatterns = Array.isArray(pushConfig.excluded_files) ? pushConfig.excluded_files.map(p => globPatternToRegex(p)) : []; + const uniqueFiles = [...new Set(branchHistoryFiles)]; + const disallowedFiles = uniqueFiles.filter(f => !allowedPatterns.some(re => re.test(f)) && !excludedPatterns.some(re => re.test(f))); + + if (disallowedFiles.length > 0) { + const sample = disallowedFiles.slice(0, 5); + const remaining = disallowedFiles.length - sample.length; + const filesStr = remaining > 0 ? `${sample.join(", ")} (+${remaining} more)` : sample.join(", "); + server.debug(`Full-branch allowed-files check failed: ${filesStr}`); + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: `Cannot push to pull request branch: the branch '${entry.branch}' history contains commits that modify files outside the allowed-files configuration: ${filesStr}. Remove the disallowed file changes from your commits and retry, or update the allowed-files configuration to include these files.`, + disallowed_files: disallowedFiles, + }), + }, + ], + isError: true, + }; + } + } + } + } catch (fullBranchCheckError) { + // Non-fatal: if origin/baseBranch is not available locally or git fails, + // skip the full-branch check and continue. The apply-time policy check in + // push_to_pull_request_branch.cjs will still enforce allowed_files against + // the incremental patch content. + server.debug(`Full-branch allowed-files check skipped (non-fatal): ${getErrorMessage(fullBranchCheckError)}`); + } + } + // Always generate an incremental patch for policy enforcement (allowed-files/protected-files/excluded-files), // even when bundle transport is selected for apply-time commit transport. server.debug(`Generating incremental patch for push_to_pull_request_branch with branch: ${entry.branch}, baseBranch: ${baseBranch}`); diff --git a/actions/setup/js/safe_outputs_handlers.test.cjs b/actions/setup/js/safe_outputs_handlers.test.cjs index df15d908ffa..39bcd7e62f8 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -1800,6 +1800,208 @@ describe("safe_outputs_handlers", () => { process.env.GITHUB_REF_NAME = "feature-branch"; // restore for sibling tests } }); + + describe("full-branch allowed_files check", () => { + /** + * Creates a git repo whose history contains a file that violates `allowed_files` + * (a .js file when only .md is allowed). Sets up `origin/main` as the tracking ref + * so `execGitSync` can compute the range `origin/main..`. + */ + function createRepoWithDisallowedHistoryFile() { + const repoDir = path.join(testWorkspaceDir, "allowed-files-repo"); + fs.mkdirSync(repoDir, { recursive: true }); + + execSync("git init -b main", { cwd: repoDir, stdio: "pipe" }); + execSync("git config user.email 'test@example.com'", { cwd: repoDir, stdio: "pipe" }); + execSync("git config user.name 'Test User'", { cwd: repoDir, stdio: "pipe" }); + + // Base commit on main + fs.writeFileSync(path.join(repoDir, "README.md"), "base\n"); + execSync("git add README.md", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'base commit'", { cwd: repoDir, stdio: "pipe" }); + const mainSha = execSync("git rev-parse HEAD", { cwd: repoDir, stdio: "pipe" }).toString().trim(); + + // Create feature branch + execSync("git checkout -b feature/work", { cwd: repoDir, stdio: "pipe" }); + + // Commit an .md file (allowed) + fs.writeFileSync(path.join(repoDir, "notes.md"), "notes\n"); + execSync("git add notes.md", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'add notes'", { cwd: repoDir, stdio: "pipe" }); + + // Commit a .js file (disallowed when allowed_files is ["*.md"]) + fs.writeFileSync(path.join(repoDir, "script.js"), "console.log('hi');\n"); + execSync("git add script.js", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'add script'", { cwd: repoDir, stdio: "pipe" }); + + // Set up origin/main tracking ref + execSync("git remote add origin https://github.com/test-owner/test-repo.git", { cwd: repoDir, stdio: "pipe" }); + execSync(`git update-ref refs/remotes/origin/main ${mainSha}`, { cwd: repoDir, stdio: "pipe" }); + + return { repoDir }; + } + + it("returns isError when branch history contains a file outside allowed_files", async () => { + let repoDir; + try { + ({ repoDir } = createRepoWithDisallowedHistoryFile()); + } catch { + // Skip if git not available in test environment + return; + } + + process.env.GITHUB_BASE_REF = "main"; + process.env.GITHUB_WORKSPACE = repoDir; + const localHandlers = createHandlers(mockServer, mockAppendSafeOutput, { + push_to_pull_request_branch: { + allowed_files: ["*.md"], + }, + }); + + try { + const result = await localHandlers.pushToPullRequestBranchHandler({ branch: "feature/work" }); + + expect(result.isError).toBe(true); + const data = JSON.parse(result.content[0].text); + expect(data.result).toBe("error"); + expect(data.error).toContain("allowed-files"); + expect(data.disallowed_files).toBeDefined(); + expect(data.disallowed_files).toContain("script.js"); + // Safe output must NOT have been recorded for a disallowed branch + expect(mockAppendSafeOutput).not.toHaveBeenCalled(); + } finally { + delete process.env.GITHUB_BASE_REF; + process.env.GITHUB_WORKSPACE = testWorkspaceDir; + } + }); + + it("does not block when all branch history files are within allowed_files", async () => { + const repoDir = path.join(testWorkspaceDir, "allowed-files-ok-repo"); + fs.mkdirSync(repoDir, { recursive: true }); + try { + execSync("git init -b main", { cwd: repoDir, stdio: "pipe" }); + execSync("git config user.email 'test@example.com'", { cwd: repoDir, stdio: "pipe" }); + execSync("git config user.name 'Test User'", { cwd: repoDir, stdio: "pipe" }); + + fs.writeFileSync(path.join(repoDir, "README.md"), "base\n"); + execSync("git add README.md", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'base commit'", { cwd: repoDir, stdio: "pipe" }); + const mainSha = execSync("git rev-parse HEAD", { cwd: repoDir, stdio: "pipe" }).toString().trim(); + + execSync("git checkout -b feature/docs-only", { cwd: repoDir, stdio: "pipe" }); + fs.writeFileSync(path.join(repoDir, "CHANGELOG.md"), "## v1.0\n"); + execSync("git add CHANGELOG.md", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'add changelog'", { cwd: repoDir, stdio: "pipe" }); + + execSync("git remote add origin https://github.com/test-owner/test-repo.git", { cwd: repoDir, stdio: "pipe" }); + execSync(`git update-ref refs/remotes/origin/main ${mainSha}`, { cwd: repoDir, stdio: "pipe" }); + } catch { + return; // Skip if git not available + } + + process.env.GITHUB_BASE_REF = "main"; + process.env.GITHUB_WORKSPACE = repoDir; + const localHandlers = createHandlers(mockServer, mockAppendSafeOutput, { + push_to_pull_request_branch: { + allowed_files: ["*.md"], + }, + }); + + try { + const result = await localHandlers.pushToPullRequestBranchHandler({ branch: "feature/docs-only" }); + + // The allowed_files check should pass; any subsequent failure is unrelated to it + // (e.g. missing patch file in the test environment is acceptable). + if (result.isError) { + const data = JSON.parse(result.content[0].text); + // Must NOT be an allowed_files error + expect(data.error).not.toContain("allowed-files configuration"); + expect(data.disallowed_files).toBeUndefined(); + } + } finally { + delete process.env.GITHUB_BASE_REF; + process.env.GITHUB_WORKSPACE = testWorkspaceDir; + } + }); + + it("exempts files matching excluded_files from the allowed_files check", async () => { + let repoDir; + try { + ({ repoDir } = createRepoWithDisallowedHistoryFile()); + } catch { + return; + } + + process.env.GITHUB_BASE_REF = "main"; + process.env.GITHUB_WORKSPACE = repoDir; + // excluded_files exempts .js files, so script.js should not trigger the error + const localHandlers = createHandlers(mockServer, mockAppendSafeOutput, { + push_to_pull_request_branch: { + allowed_files: ["*.md"], + excluded_files: ["*.js"], + }, + }); + + try { + const result = await localHandlers.pushToPullRequestBranchHandler({ branch: "feature/work" }); + + // .js file is exempted by excluded_files; the check should not block on it + if (result.isError) { + const data = JSON.parse(result.content[0].text); + expect(data.error).not.toContain("allowed-files configuration"); + expect(data.disallowed_files).toBeUndefined(); + } + } finally { + delete process.env.GITHUB_BASE_REF; + process.env.GITHUB_WORKSPACE = testWorkspaceDir; + } + }); + + it("is non-fatal when origin/baseBranch is not available (continues without blocking)", async () => { + const repoDir = path.join(testWorkspaceDir, "no-origin-repo"); + fs.mkdirSync(repoDir, { recursive: true }); + try { + execSync("git init -b main", { cwd: repoDir, stdio: "pipe" }); + execSync("git config user.email 'test@example.com'", { cwd: repoDir, stdio: "pipe" }); + execSync("git config user.name 'Test User'", { cwd: repoDir, stdio: "pipe" }); + + fs.writeFileSync(path.join(repoDir, "README.md"), "base\n"); + execSync("git add README.md", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'base'", { cwd: repoDir, stdio: "pipe" }); + + execSync("git checkout -b feature/work", { cwd: repoDir, stdio: "pipe" }); + // Add a disallowed file — but since origin/main is absent the check must be skipped + fs.writeFileSync(path.join(repoDir, "script.js"), "// disallowed\n"); + execSync("git add script.js", { cwd: repoDir, stdio: "pipe" }); + execSync("git commit -m 'disallowed file'", { cwd: repoDir, stdio: "pipe" }); + // No remote / no origin/main tracking ref + } catch { + return; + } + + process.env.GITHUB_BASE_REF = "main"; + process.env.GITHUB_WORKSPACE = repoDir; + const localHandlers = createHandlers(mockServer, mockAppendSafeOutput, { + push_to_pull_request_branch: { + allowed_files: ["*.md"], + }, + }); + + try { + const result = await localHandlers.pushToPullRequestBranchHandler({ branch: "feature/work" }); + + // The non-fatal catch must not have blocked execution with an allowed-files error + if (result.isError) { + const data = JSON.parse(result.content[0].text); + expect(data.error).not.toContain("allowed-files configuration"); + expect(data.disallowed_files).toBeUndefined(); + } + } finally { + delete process.env.GITHUB_BASE_REF; + process.env.GITHUB_WORKSPACE = testWorkspaceDir; + } + }); + }); }); describe("handler structure", () => { diff --git a/pkg/workflow/safe_outputs_handler_registry.go b/pkg/workflow/safe_outputs_handler_registry.go index c4c76781d04..8bbbffc89e0 100644 --- a/pkg/workflow/safe_outputs_handler_registry.go +++ b/pkg/workflow/safe_outputs_handler_registry.go @@ -557,6 +557,7 @@ var handlerRegistry = map[string]handlerBuilder{ AddBoolPtr("fallback_as_pull_request", c.FallbackAsPullRequest). AddBoolPtr("signed_commits", c.SignedCommits). AddBoolPtr("check_branch_protection", c.CheckBranchProtection). + AddIfTrue("allow_workflows", c.AllowWorkflows). Build() }, "update_pull_request": func(cfg *SafeOutputsConfig) map[string]any { diff --git a/pkg/workflow/skills_frontmatter.go b/pkg/workflow/skills_frontmatter.go index 7a6b5e86e38..237c4fc57b1 100644 --- a/pkg/workflow/skills_frontmatter.go +++ b/pkg/workflow/skills_frontmatter.go @@ -26,16 +26,17 @@ type SkillReference struct { func validateSkillSpecValue(skillSpec string, idx int) error { if strings.TrimSpace(skillSpec) == "" { - return fmt.Errorf("skills[%d] must be a non-empty string", idx) + return fmt.Errorf("skills[%d] must be a non-empty string. Example: skills[%d]: \"owner/repo@abc1234...\"", idx, idx) } if githubActionsExpressionRegexp.MatchString(skillSpec) || skillSpecExpressionRefRegexp.MatchString(skillSpec) { return nil } if !skillSpecRegexp.MatchString(skillSpec) { return fmt.Errorf( - "skills[%d] must use owner/repo@<40-char-sha>, owner/repo/skill/path@<40-char-sha>, or a GitHub Actions expression: %q", + "skills[%d] must use owner/repo@<40-char-sha>, owner/repo/skill/path@<40-char-sha>, or a GitHub Actions expression (got %q). Example: skills[%d]: \"owner/repo@abcdef1234567890abcdef1234567890abcdef12\"", idx, skillSpec, + idx, ) } return nil @@ -49,7 +50,7 @@ func validateFrontmatterSkills(frontmatter map[string]any) error { skills, ok := rawSkills.([]any) if !ok { - return errors.New("skills must be an array of skill references") + return errors.New("skills must be an array of skill references. Example: skills: [\"owner/repo@sha\"]") } skillsFrontmatterLog.Printf("validateFrontmatterSkills: validating %d skill entr(ies)", len(skills)) @@ -62,18 +63,18 @@ func validateFrontmatterSkills(frontmatter map[string]any) error { } case map[string]any: if len(typed) == 0 { - return fmt.Errorf("skills[%d] must include a non-empty skill field", i) + return fmt.Errorf("skills[%d] must include a non-empty skill field. Example: skills[%d]: {skill: \"owner/repo@sha\"}", i, i) } skillValue, hasSkill := typed["skill"] if !hasSkill { - return fmt.Errorf("skills[%d].skill is required", i) + return fmt.Errorf("skills[%d].skill is required. Example: skills[%d].skill: \"owner/repo@sha\"", i, i) } skillSpec, ok := skillValue.(string) if !ok { - return fmt.Errorf("skills[%d].skill must be a string", i) + return fmt.Errorf("skills[%d].skill must be a string. Example: skills[%d].skill: \"owner/repo@sha\"", i, i) } if strings.TrimSpace(skillSpec) == "" { - return fmt.Errorf("skills[%d].skill must be a non-empty string", i) + return fmt.Errorf("skills[%d].skill must be a non-empty string. Example: skills[%d].skill: \"owner/repo@sha\"", i, i) } if err := validateSkillSpecValue(skillSpec, i); err != nil { return err @@ -94,11 +95,12 @@ func validateFrontmatterSkills(frontmatter map[string]any) error { if tokenValue, hasToken := typed["github-token"]; hasToken { token, ok := tokenValue.(string) if !ok { - return fmt.Errorf("skills[%d].github-token must be a string", i) + return fmt.Errorf("skills[%d].github-token must be a string. Example: skills[%d].github-token: \"${{ secrets.MY_TOKEN }}\"", i, i) } if !githubTokenExpressionRegexp.MatchString(token) { return fmt.Errorf( - "skills[%d].github-token must be a valid GitHub token expression (e.g., '${{ secrets.NAME }}' or '${{ needs.auth.outputs.token }}')", + "skills[%d].github-token must be a valid GitHub token expression. Example: skills[%d].github-token: \"${{ secrets.NAME }}\" or \"${{ needs.auth.outputs.token }}\"", + i, i, ) } @@ -106,15 +108,15 @@ func validateFrontmatterSkills(frontmatter map[string]any) error { if app, hasApp := typed["github-app"]; hasApp { appMap, ok := app.(map[string]any) if !ok { - return fmt.Errorf("skills[%d].github-app must be an object", i) + return fmt.Errorf("skills[%d].github-app must be an object. Example: skills[%d].github-app: {client-id: \"Iv1.abc\", private-key: \"...\"}", i, i) } parsed := parseAppConfig(appMap) if !parsed.hasRequiredCredentials() { - return fmt.Errorf("skills[%d].github-app must include non-empty client-id/app-id and private-key", i) + return fmt.Errorf("skills[%d].github-app must include non-empty client-id/app-id and private-key. Example: skills[%d].github-app: {client-id: \"Iv1.abc\", private-key: \"...\"}", i, i) } } default: - return fmt.Errorf("skills[%d] must be a string or object", i) + return fmt.Errorf("skills[%d] must be a string or object. Example: skills[%d]: \"owner/repo@sha\" or {skill: \"owner/repo@sha\"}", i, i) } }