diff --git a/actions/setup/js/create_agent_session.cjs b/actions/setup/js/create_agent_session.cjs index ddec23a8fd..b7a32ccec0 100644 --- a/actions/setup/js/create_agent_session.cjs +++ b/actions/setup/js/create_agent_session.cjs @@ -65,16 +65,27 @@ async function main() { let summaryContent = "## 🎭 Staged Mode: Create Agent Sessions Preview\n\n"; summaryContent += "The following agent sessions would be created if staged mode was disabled:\n\n"; - // Resolve base branch: use custom config if set, otherwise resolve dynamically - const baseBranch = process.env.GITHUB_AW_AGENT_SESSION_BASE || (await getBaseBranch()); - for (const [index, item] of createAgentSessionItems.entries()) { + // Resolve and validate target repository for this item using the standardized helper + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "agent session"); + if (!repoResult.success) { + summaryContent += `### Task ${index + 1}\n\n`; + summaryContent += `**Error:** ${repoResult.error}\n\n`; + summaryContent += "---\n\n"; + continue; + } + const { repo: effectiveRepo, repoParts } = repoResult; + + // Resolve base branch: use custom config if set, otherwise resolve dynamically + // Pass target repo for cross-repo scenarios + const baseBranch = process.env.GITHUB_AW_AGENT_SESSION_BASE || (await getBaseBranch(repoParts)); + summaryContent += `### Task ${index + 1}\n\n`; summaryContent += `**Description:**\n${item.body || "No description provided"}\n\n`; summaryContent += `**Base Branch:** ${baseBranch}\n\n`; - summaryContent += `**Target Repository:** ${defaultTargetRepo}\n\n`; + summaryContent += `**Target Repository:** ${effectiveRepo}\n\n`; summaryContent += "---\n\n"; } @@ -85,11 +96,6 @@ async function main() { return; } - // Resolve base branch: use custom config if set, otherwise resolve dynamically - // Dynamic resolution is needed for issue_comment events on PRs where the base branch - // is not available in GitHub Actions expressions and requires an API call - const baseBranch = process.env.GITHUB_AW_AGENT_SESSION_BASE || (await getBaseBranch()); - // Process all agent session items const createdTasks = []; let summaryContent = "## ✅ Agent Sessions Created\n\n"; @@ -109,7 +115,13 @@ async function main() { core.error(`E004: ${repoResult.error}`); continue; } - const effectiveRepo = repoResult.repo; + const { repo: effectiveRepo, repoParts } = repoResult; + + // Resolve base branch: use custom config if set, otherwise resolve dynamically + // Dynamic resolution is needed for issue_comment events on PRs where the base branch + // is not available in GitHub Actions expressions and requires an API call + // Pass target repo for cross-repo scenarios + const baseBranch = process.env.GITHUB_AW_AGENT_SESSION_BASE || (await getBaseBranch(repoParts)); try { // Write task description to a temporary file diff --git a/actions/setup/js/create_pull_request.cjs b/actions/setup/js/create_pull_request.cjs index da36f45b27..96840a47e1 100644 --- a/actions/setup/js/create_pull_request.cjs +++ b/actions/setup/js/create_pull_request.cjs @@ -100,12 +100,24 @@ async function main(config = {}) { const autoMerge = parseBoolTemplatable(config.auto_merge, false); const expiresHours = config.expires ? parseInt(String(config.expires), 10) : 0; const maxCount = config.max || 1; // PRs are typically limited to 1 - // Resolve base branch: use config value if set, otherwise resolve dynamically - // Dynamic resolution is needed for issue_comment events on PRs where the base branch - // is not available in GitHub Actions expressions and requires an API call - let baseBranch = config.base_branch || (await getBaseBranch()); const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + + // Base branch from config (if set) - validated at factory level if explicit + // Dynamic base branch resolution happens per-message after resolving the actual target repo + const configBaseBranch = config.base_branch || null; + + // SECURITY: If base branch is explicitly configured, validate it at factory level + if (configBaseBranch) { + const normalizedConfigBase = normalizeBranchName(configBaseBranch); + if (!normalizedConfigBase) { + throw new Error(`Invalid baseBranch: sanitization resulted in empty string (original: "${configBaseBranch}")`); + } + if (configBaseBranch !== normalizedConfigBase) { + throw new Error(`Invalid baseBranch: contains invalid characters (original: "${configBaseBranch}", normalized: "${normalizedConfigBase}")`); + } + } + const includeFooter = parseBoolTemplatable(config.footer, true); const fallbackAsIssue = config.fallback_as_issue !== false; // Default to true (fallback enabled) @@ -115,25 +127,13 @@ async function main(config = {}) { throw new Error("GH_AW_WORKFLOW_ID environment variable is required"); } - // SECURITY: Sanitize base branch name to prevent shell injection (defense in depth) - // Even though baseBranch comes from workflow config, normalize it for safety - const originalBaseBranch = baseBranch; - baseBranch = normalizeBranchName(baseBranch); - if (!baseBranch) { - throw new Error(`Invalid baseBranch: sanitization resulted in empty string (original: "${originalBaseBranch}")`); - } - // Fail if base branch name changes during normalization (indicates invalid config) - if (originalBaseBranch !== baseBranch) { - throw new Error(`Invalid baseBranch: contains invalid characters (original: "${originalBaseBranch}", normalized: "${baseBranch}")`); - } - // Extract triggering issue number from context (for auto-linking PRs to issues) const triggeringIssueNumber = context.payload?.issue?.number && !context.payload?.issue?.pull_request ? context.payload.issue.number : undefined; // Check if we're in staged mode const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true"; - core.info(`Base branch: ${baseBranch}`); + core.info(`Base branch: ${configBaseBranch || "(dynamic - resolved per target repo)"}`); core.info(`Default target repo: ${defaultTargetRepo}`); if (allowedRepos.size > 0) { core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`); @@ -221,6 +221,29 @@ async function main(config = {}) { const { repo: itemRepo, repoParts } = repoResult; core.info(`Target repository: ${itemRepo}`); + // Resolve base branch for this target repository + // Use config value if set, otherwise resolve dynamically for the specific target repo + // Dynamic resolution is needed for issue_comment events on PRs where the base branch + // is not available in GitHub Actions expressions and requires an API call + let baseBranch = configBaseBranch || (await getBaseBranch(repoParts)); + + // SECURITY: Sanitize dynamically resolved base branch to prevent shell injection + const originalBaseBranch = baseBranch; + baseBranch = normalizeBranchName(baseBranch); + if (!baseBranch) { + return { + success: false, + error: `Invalid base branch: sanitization resulted in empty string (original: "${originalBaseBranch}")`, + }; + } + if (originalBaseBranch !== baseBranch) { + return { + success: false, + error: `Invalid base branch: contains invalid characters (original: "${originalBaseBranch}", normalized: "${baseBranch}")`, + }; + } + core.info(`Base branch for ${itemRepo}: ${baseBranch}`); + // Check if patch file exists and has valid content if (!patchFilePath || !fs.existsSync(patchFilePath)) { // If allow-empty is enabled, we can proceed without a patch file diff --git a/actions/setup/js/generate_git_patch.cjs b/actions/setup/js/generate_git_patch.cjs index 384f1213c4..d485f8ecc5 100644 --- a/actions/setup/js/generate_git_patch.cjs +++ b/actions/setup/js/generate_git_patch.cjs @@ -4,7 +4,6 @@ const fs = require("fs"); const path = require("path"); -const { getBaseBranch } = require("./get_base_branch.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { execGitSync } = require("./git_helpers.cjs"); @@ -47,6 +46,7 @@ function getPatchPath(branchName) { /** * Generates a git patch file for the current changes * @param {string} branchName - The branch name to generate patch for + * @param {string} baseBranch - The base branch to diff against (e.g., "main", "master") * @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) @@ -54,15 +54,23 @@ function getPatchPath(branchName) { * In incremental mode, origin/branchName is fetched explicitly and merge-base fallback is disabled. * @returns {Promise} Object with patch info or error */ -async function generateGitPatch(branchName, options = {}) { - const mode = options.mode || "full"; +async function generateGitPatch(branchName, baseBranch, options = {}) { const patchPath = getPatchPath(branchName); + + // Validate baseBranch early to avoid confusing git errors (e.g., origin/undefined) + if (typeof baseBranch !== "string" || baseBranch.trim() === "") { + const errorMessage = "baseBranch is required and must be a non-empty string (received: " + String(baseBranch) + ")"; + debugLog(`Invalid baseBranch: ${errorMessage}`); + return { + patchPath, + patchGenerated: false, + errorMessage, + }; + } + + const mode = options.mode || "full"; 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 repository. - // If the checked-out 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 || (await getBaseBranch()); + const defaultBranch = baseBranch; const githubSha = process.env.GITHUB_SHA; debugLog(`Starting patch generation: mode=${mode}, branch=${branchName}, defaultBranch=${defaultBranch}`); diff --git a/actions/setup/js/generate_git_patch.test.cjs b/actions/setup/js/generate_git_patch.test.cjs index 46a564f428..01f9b2af5c 100644 --- a/actions/setup/js/generate_git_patch.test.cjs +++ b/actions/setup/js/generate_git_patch.test.cjs @@ -30,7 +30,7 @@ describe("generateGitPatch", () => { const { generateGitPatch } = await import("./generate_git_patch.cjs"); - const result = await generateGitPatch(null); + const result = await generateGitPatch(null, "main"); expect(result.success).toBe(false); expect(result).toHaveProperty("error"); @@ -43,7 +43,7 @@ describe("generateGitPatch", () => { process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "abc123"; - const result = await generateGitPatch("nonexistent-branch"); + const result = await generateGitPatch("nonexistent-branch", "main"); expect(result.success).toBe(false); expect(result).toHaveProperty("error"); @@ -57,7 +57,7 @@ describe("generateGitPatch", () => { process.env.GITHUB_SHA = "abc123"; // Even if it fails, it should try to create the directory - const result = await generateGitPatch("test-branch"); + const result = await generateGitPatch("test-branch", "main"); expect(result).toHaveProperty("patchPath"); // Patch path includes sanitized branch name @@ -70,7 +70,7 @@ describe("generateGitPatch", () => { process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "abc123"; - const result = await generateGitPatch("test-branch"); + const result = await generateGitPatch("test-branch", "main"); expect(result).toHaveProperty("success"); expect(result).toHaveProperty("patchPath"); @@ -83,7 +83,7 @@ describe("generateGitPatch", () => { process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "abc123"; - const result = await generateGitPatch(null); + const result = await generateGitPatch(null, "main"); expect(result).toHaveProperty("success"); expect(result).toHaveProperty("patchPath"); @@ -95,37 +95,34 @@ describe("generateGitPatch", () => { process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "abc123"; - const result = await generateGitPatch(""); + const result = await generateGitPatch("", "main"); expect(result).toHaveProperty("success"); expect(result).toHaveProperty("patchPath"); }); - it("should use default branch from environment", async () => { + it("should use provided base branch", async () => { const { generateGitPatch } = await import("./generate_git_patch.cjs"); process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "abc123"; - process.env.DEFAULT_BRANCH = "develop"; - const result = await generateGitPatch("feature-branch"); + const result = await generateGitPatch("feature-branch", "develop"); expect(result).toHaveProperty("success"); - // Should attempt to use develop as default branch + // Should use develop as base branch }); - it("should fall back to GH_AW_CUSTOM_BASE_BRANCH if DEFAULT_BRANCH not set", async () => { + it("should use provided master branch as base", async () => { const { generateGitPatch } = await import("./generate_git_patch.cjs"); process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "abc123"; - delete process.env.DEFAULT_BRANCH; - process.env.GH_AW_CUSTOM_BASE_BRANCH = "master"; - const result = await generateGitPatch("feature-branch"); + const result = await generateGitPatch("feature-branch", "master"); expect(result).toHaveProperty("success"); - // Should attempt to use master as base branch + // Should use master as base branch }); it("should safely handle branch names with special characters", async () => { @@ -138,7 +135,7 @@ describe("generateGitPatch", () => { const maliciousBranchNames = ["feature; rm -rf /", "feature && echo hacked", "feature | cat /etc/passwd", "feature$(whoami)", "feature`whoami`", "feature\nrm -rf /"]; for (const branchName of maliciousBranchNames) { - const result = await generateGitPatch(branchName); + const result = await generateGitPatch(branchName, "main"); // Should not throw an error and should handle safely expect(result).toHaveProperty("success"); @@ -155,7 +152,7 @@ describe("generateGitPatch", () => { // Test with malicious SHA that could cause shell injection process.env.GITHUB_SHA = "abc123; echo hacked"; - const result = await generateGitPatch("test-branch"); + const result = await generateGitPatch("test-branch", "main"); // Should not throw an error and should handle safely expect(result).toHaveProperty("success"); @@ -195,7 +192,7 @@ describe("generateGitPatch - cross-repo checkout scenarios", () => { process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "deadbeef123456789"; // SHA that doesn't exist in target repo - const result = await generateGitPatch("feature-branch"); + const result = await generateGitPatch("feature-branch", "main"); // Should fail gracefully, not crash expect(result).toHaveProperty("success"); @@ -209,9 +206,8 @@ describe("generateGitPatch - cross-repo checkout scenarios", () => { // 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 = await generateGitPatch("feature-branch"); + const result = await generateGitPatch("feature-branch", "main"); // Should try multiple strategies and fail gracefully expect(result).toHaveProperty("success"); @@ -225,9 +221,8 @@ describe("generateGitPatch - cross-repo checkout scenarios", () => { // 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 = await generateGitPatch("feature-branch"); + const result = await generateGitPatch("feature-branch", "main"); // Should complete without hanging or crashing due to fetch attempts expect(result).toHaveProperty("success"); @@ -239,9 +234,8 @@ describe("generateGitPatch - cross-repo checkout scenarios", () => { process.env.GITHUB_WORKSPACE = "/tmp/nonexistent-repo"; process.env.GITHUB_SHA = "sha-from-workflow-repo"; - process.env.DEFAULT_BRANCH = "main"; - const result = await generateGitPatch("agent-created-branch"); + const result = await generateGitPatch("agent-created-branch", "main"); expect(result.success).toBe(false); expect(result).toHaveProperty("error"); @@ -254,10 +248,9 @@ describe("generateGitPatch - cross-repo checkout scenarios", () => { 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 = await generateGitPatch("feature-branch", { mode: "incremental" }); + const result = await generateGitPatch("feature-branch", "main", { mode: "incremental" }); expect(result.success).toBe(false); expect(result).toHaveProperty("error"); @@ -272,9 +265,8 @@ describe("generateGitPatch - cross-repo checkout scenarios", () => { // 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 = await generateGitPatch("agent-changes"); + const result = await generateGitPatch("agent-changes", "main"); // Should not crash, should return failure with helpful error expect(result).toHaveProperty("success"); diff --git a/actions/setup/js/get_base_branch.cjs b/actions/setup/js/get_base_branch.cjs index f6b28645a8..1c5e220d0d 100644 --- a/actions/setup/js/get_base_branch.cjs +++ b/actions/setup/js/get_base_branch.cjs @@ -11,9 +11,13 @@ * 4. API lookup for issue_comment events on PRs (the PR's base ref is not in the payload) * 5. Fallback to DEFAULT_BRANCH env var or "main" * + * @param {{owner: string, repo: string}|null} [targetRepo] - Optional target repository. + * If provided, API calls (step 4) use this instead of context.repo, + * which is needed for cross-repo scenarios where the target repo differs + * from the workflow repository. * @returns {Promise} The base branch name */ -async function getBaseBranch() { +async function getBaseBranch(targetRepo = null) { // 1. Custom base branch from workflow configuration if (process.env.GH_AW_CUSTOM_BASE_BRANCH) { return process.env.GH_AW_CUSTOM_BASE_BRANCH; @@ -30,12 +34,15 @@ async function getBaseBranch() { } // 4. For issue_comment events on PRs - must call API since base ref not in payload + // Use targetRepo if provided (cross-repo scenarios), otherwise fall back to context.repo if (typeof context !== "undefined" && context.eventName === "issue_comment" && context.payload?.issue?.pull_request) { try { if (typeof github !== "undefined") { + const repoOwner = targetRepo?.owner ?? context.repo.owner; + const repoName = targetRepo?.repo ?? context.repo.repo; const { data: pr } = await github.rest.pulls.get({ - owner: context.repo.owner, - repo: context.repo.repo, + owner: repoOwner, + repo: repoName, pull_number: context.payload.issue.number, }); return pr.base.ref; diff --git a/actions/setup/js/get_base_branch.test.cjs b/actions/setup/js/get_base_branch.test.cjs index 4540363009..e83701c4f9 100644 --- a/actions/setup/js/get_base_branch.test.cjs +++ b/actions/setup/js/get_base_branch.test.cjs @@ -80,4 +80,23 @@ describe("getBaseBranch", () => { process.env.GH_AW_CUSTOM_BASE_BRANCH = "feature/new-feature"; expect(await getBaseBranch()).toBe("feature/new-feature"); }); + + it("should accept optional targetRepo parameter without affecting simple cases", async () => { + // When env vars are set, targetRepo doesn't change the result + process.env.GH_AW_CUSTOM_BASE_BRANCH = "custom-branch"; + + const { getBaseBranch } = await import("./get_base_branch.cjs"); + + // With targetRepo parameter + const result = await getBaseBranch({ owner: "other-owner", repo: "other-repo" }); + expect(result).toBe("custom-branch"); + + // Without targetRepo parameter (null) + const result2 = await getBaseBranch(null); + expect(result2).toBe("custom-branch"); + + // Without targetRepo parameter (undefined) + const result3 = await getBaseBranch(); + expect(result3).toBe("custom-branch"); + }); }); diff --git a/actions/setup/js/git_patch_integration.test.cjs b/actions/setup/js/git_patch_integration.test.cjs index 5606dfe733..d7e0518244 100644 --- a/actions/setup/js/git_patch_integration.test.cjs +++ b/actions/setup/js/git_patch_integration.test.cjs @@ -554,7 +554,7 @@ describe("git patch integration tests", () => { process.env.DEFAULT_BRANCH = "main"; try { - const result = await generateGitPatch("feature-branch", { mode: "incremental" }); + const result = await generateGitPatch("feature-branch", "main", { mode: "incremental" }); expect(result.success).toBe(true); expect(result.patchPath).toBeDefined(); @@ -598,7 +598,7 @@ describe("git patch integration tests", () => { process.env.DEFAULT_BRANCH = "main"; try { - const result = await generateGitPatch("local-only-branch", { mode: "incremental" }); + const result = await generateGitPatch("local-only-branch", "main", { mode: "incremental" }); // Should fail with a clear error message expect(result.success).toBe(false); @@ -638,7 +638,7 @@ describe("git patch integration tests", () => { try { // Full mode (default) - should fall back to merge-base and include all commits - const result = await generateGitPatch("full-mode-branch", { mode: "full" }); + const result = await generateGitPatch("full-mode-branch", "main", { mode: "full" }); // Debug output if test fails if (!result.success) { diff --git a/actions/setup/js/push_to_pull_request_branch.cjs b/actions/setup/js/push_to_pull_request_branch.cjs index 880f16cd48..0510ed1622 100644 --- a/actions/setup/js/push_to_pull_request_branch.cjs +++ b/actions/setup/js/push_to_pull_request_branch.cjs @@ -10,7 +10,6 @@ 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"); -const { getBaseBranch } = require("./get_base_branch.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -32,22 +31,22 @@ async function main(config = {}) { const ifNoChanges = config.if_no_changes || "warn"; const commitTitleSuffix = config.commit_title_suffix || ""; const maxSizeKb = config.max_patch_size ? parseInt(String(config.max_patch_size), 10) : 1024; - // Resolve base branch: use config value if set, otherwise resolve dynamically - // Dynamic resolution is needed for issue_comment events on PRs where the base branch - // is not available in GitHub Actions expressions and requires an API call - const baseBranch = config.base_branch || (await getBaseBranch()); 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); + // Base branch from config (if set) - used only for logging at factory level + // Dynamic base branch resolution happens per-message after resolving the actual target repo + const configBaseBranch = config.base_branch || null; + // Check if we're in staged mode const isStaged = process.env.GH_AW_SAFE_OUTPUTS_STAGED === "true"; core.info(`Target: ${target}`); - if (baseBranch) { - core.info(`Base branch: ${baseBranch}`); + if (configBaseBranch) { + core.info(`Base branch (from config): ${configBaseBranch}`); } if (titlePrefix) { core.info(`Title prefix: ${titlePrefix}`); diff --git a/actions/setup/js/repo_helpers.cjs b/actions/setup/js/repo_helpers.cjs index 9b2a94e96b..4f9e2d3c68 100644 --- a/actions/setup/js/repo_helpers.cjs +++ b/actions/setup/js/repo_helpers.cjs @@ -9,6 +9,69 @@ const { globPatternToRegex } = require("./glob_pattern_helpers.cjs"); const { ERR_VALIDATION } = require("./error_codes.cjs"); +// ============================================================================ +// Type Definitions +// ============================================================================ + +/** + * Parsed repository owner and name + * @typedef {Object} RepoParts + * @property {string} owner - Repository owner (organization or user) + * @property {string} repo - Repository name + */ + +/** + * Result of repository validation + * @typedef {Object} RepoValidationResult + * @property {boolean} valid - Whether the repository is allowed + * @property {string|null} error - Error message if validation failed, null otherwise + * @property {string} qualifiedRepo - Fully qualified repository slug (owner/repo) + */ + +/** + * Successful result from resolveAndValidateRepo + * @typedef {Object} RepoResolutionSuccess + * @property {true} success - Always true for success + * @property {string} repo - Fully qualified repository slug (owner/repo) + * @property {RepoParts} repoParts - Parsed owner and repo components + */ + +/** + * Failed result from resolveAndValidateRepo + * @typedef {Object} RepoResolutionError + * @property {false} success - Always false for error + * @property {string} error - Error message describing why resolution failed + */ + +/** + * Union type for resolveAndValidateRepo result + * @typedef {RepoResolutionSuccess | RepoResolutionError} RepoResolutionResult + */ + +/** + * Result of resolveTargetRepoConfig + * @typedef {Object} TargetRepoConfig + * @property {string} defaultTargetRepo - Default target repository slug + * @property {Set} allowedRepos - Set of allowed repository patterns + */ + +/** + * Handler configuration object with repository settings + * @typedef {Object} HandlerRepoConfig + * @property {string} [target-repo] - Configured target repository + * @property {string[]|string} [allowed_repos] - Allowed repositories (array or comma-separated) + */ + +/** + * Message item that may contain a repo field + * @typedef {Object} MessageItemWithRepo + * @property {string} [repo] - Optional repository slug override + */ + +// ============================================================================ +// Functions +// ============================================================================ + /** * Parse the allowed repos from config value (array or comma-separated string) * @param {string[]|string|undefined} allowedReposValue - Allowed repos from config (array or comma-separated string) @@ -33,7 +96,7 @@ function parseAllowedRepos(allowedReposValue) { /** * Get the default target repository - * @param {Object} [config] - Optional config object with target-repo field + * @param {HandlerRepoConfig | import('./types/handler-factory').HandlerConfig} [config] - Optional config object with target-repo field * @returns {string} Repository slug in "owner/repo" format */ function getDefaultTargetRepo(config) { @@ -86,7 +149,7 @@ function isRepoAllowed(qualifiedRepo, allowedRepos) { * @param {string} repo - Repository slug to validate (can be "owner/repo" or just "repo") * @param {string} defaultRepo - Default target repository * @param {Set} allowedRepos - Set of explicitly allowed repo patterns - * @returns {{valid: boolean, error: string|null, qualifiedRepo: string}} + * @returns {RepoValidationResult} */ function validateRepo(repo, defaultRepo, allowedRepos) { // If repo is a bare name (no slash), qualify it with the default repo's org @@ -116,7 +179,7 @@ function validateRepo(repo, defaultRepo, allowedRepos) { /** * Parse owner and repo from a repository slug * @param {string} repoSlug - Repository slug in "owner/repo" format - * @returns {{owner: string, repo: string}|null} + * @returns {RepoParts|null} */ function parseRepoSlug(repoSlug) { const parts = repoSlug.split("/"); @@ -129,8 +192,8 @@ function parseRepoSlug(repoSlug) { /** * Resolve target repository configuration from handler config * Combines parsing of allowed-repos and resolution of default target repo - * @param {Object} config - Handler configuration object - * @returns {{defaultTargetRepo: string, allowedRepos: Set}} + * @param {HandlerRepoConfig | import('./types/handler-factory').HandlerConfig} config - Handler configuration object + * @returns {TargetRepoConfig} */ function resolveTargetRepoConfig(config) { const defaultTargetRepo = getDefaultTargetRepo(config); @@ -144,11 +207,11 @@ function resolveTargetRepoConfig(config) { /** * Resolve and validate target repository from a message item * Combines repo resolution, validation, and parsing into a single function - * @param {Object} item - Message item that may contain a repo field + * @param {MessageItemWithRepo} item - Message item that may contain a repo field * @param {string} defaultTargetRepo - Default target repository slug * @param {Set} allowedRepos - Set of allowed repository slugs * @param {string} operationType - Type of operation (e.g., "comment", "pull request", "issue") for error messages - * @returns {{success: true, repo: string, repoParts: {owner: string, repo: string}}|{success: false, error: string}} + * @returns {RepoResolutionResult} */ function resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, operationType) { // Determine target repository for this operation @@ -196,7 +259,7 @@ function resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, operation * @param {string} repo - Repository slug to validate (can be "owner/repo" or just "repo") * @param {string} defaultRepo - Default (always-allowed) target repository * @param {Set} allowedRepos - Set of explicitly allowed repo patterns - * @returns {{valid: boolean, error: string|null, qualifiedRepo: string}} + * @returns {RepoValidationResult} */ function validateTargetRepo(repo, defaultRepo, allowedRepos) { return validateRepo(repo, defaultRepo, allowedRepos); diff --git a/actions/setup/js/safe_outputs_handlers.cjs b/actions/setup/js/safe_outputs_handlers.cjs index ad25fa930b..5527b168c6 100644 --- a/actions/setup/js/safe_outputs_handlers.cjs +++ b/actions/setup/js/safe_outputs_handlers.cjs @@ -13,6 +13,7 @@ const { generateGitPatch } = require("./generate_git_patch.cjs"); const { enforceCommentLimits } = require("./comment_limit_helpers.cjs"); const { getErrorMessage } = require("./error_helpers.cjs"); const { ERR_CONFIG, ERR_SYSTEM, ERR_VALIDATION } = require("./error_codes.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** * Create handlers for safe output tools @@ -190,7 +191,33 @@ function createHandlers(server, appendSafeOutput, config = {}) { */ const createPullRequestHandler = async args => { const entry = { ...args, type: "create_pull_request" }; - const baseBranch = await getBaseBranch(); + + // Resolve target repo configuration and validate the target repo early + // This is needed before getBaseBranch to ensure we resolve the base branch + // for the correct repository (especially in cross-repo scenarios) + const prConfig = config.create_pull_request || {}; + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(prConfig); + + // Resolve and validate the target repository from the entry + const repoResult = resolveAndValidateRepo(entry, defaultTargetRepo, allowedRepos, "pull request"); + if (!repoResult.success) { + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: repoResult.error, + }), + }, + ], + isError: true, + }; + } + const { repoParts } = repoResult; + + // Get base branch for the resolved target repository + const baseBranch = await getBaseBranch(repoParts); // If branch is not provided, is empty, or equals the base branch, use the current branch from git // This handles cases where the agent incorrectly passes the base branch instead of the working branch @@ -228,8 +255,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { } // Generate git patch - server.debug(`Generating patch for create_pull_request with branch: ${entry.branch}`); - const patchResult = await generateGitPatch(entry.branch); + server.debug(`Generating patch for create_pull_request with branch: ${entry.branch}, baseBranch: ${baseBranch}`); + const patchResult = await generateGitPatch(entry.branch, baseBranch); if (!patchResult.success) { // Patch generation failed or patch is empty @@ -287,7 +314,33 @@ function createHandlers(server, appendSafeOutput, config = {}) { */ const pushToPullRequestBranchHandler = async args => { const entry = { ...args, type: "push_to_pull_request_branch" }; - const baseBranch = await getBaseBranch(); + + // Resolve target repo configuration and validate the target repo early + // This is needed before getBaseBranch to ensure we resolve the base branch + // for the correct repository (especially in cross-repo scenarios) + const pushConfig = config.push_to_pull_request_branch || {}; + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(pushConfig); + + // Resolve and validate the target repository from the entry + const repoResult = resolveAndValidateRepo(entry, defaultTargetRepo, allowedRepos, "push to PR branch"); + if (!repoResult.success) { + return { + content: [ + { + type: "text", + text: JSON.stringify({ + result: "error", + error: repoResult.error, + }), + }, + ], + isError: true, + }; + } + const { repoParts } = repoResult; + + // Get base branch for the resolved target repository + const baseBranch = await getBaseBranch(repoParts); // If branch is not provided, is empty, or equals the base branch, use the current branch from git // This handles cases where the agent incorrectly passes the base branch instead of the working branch @@ -306,8 +359,8 @@ function createHandlers(server, appendSafeOutput, config = {}) { // 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 = await generateGitPatch(entry.branch, { mode: "incremental" }); + server.debug(`Generating incremental patch for push_to_pull_request_branch with branch: ${entry.branch}, baseBranch: ${baseBranch}`); + const patchResult = await generateGitPatch(entry.branch, baseBranch, { 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 d5c0a3a64e..3534aaebcc 100644 --- a/actions/setup/js/safe_outputs_handlers.test.cjs +++ b/actions/setup/js/safe_outputs_handlers.test.cjs @@ -14,8 +14,19 @@ const mockCore = { setOutput: vi.fn(), }; +// Mock context object used by repo_helpers.cjs +const mockContext = { + repo: { + owner: "test-owner", + repo: "test-repo", + }, + eventName: "push", + payload: {}, +}; + // Set up global mocks before importing the module global.core = mockCore; +global.context = mockContext; describe("safe_outputs_handlers", () => { let mockServer;