diff --git a/actions/setup/js/assign_milestone.cjs b/actions/setup/js/assign_milestone.cjs index 73d230ea7a6..dc13f6d3d82 100644 --- a/actions/setup/js/assign_milestone.cjs +++ b/actions/setup/js/assign_milestone.cjs @@ -10,6 +10,7 @@ const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { loadTemporaryIdMapFromResolved, resolveRepoIssueTarget } = require("./temporary_id.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** @type {string} Safe output type handled by this module */ const HANDLER_TYPE = "assign_milestone"; @@ -54,6 +55,10 @@ async function main(config = {}) { if (requiredLabels.length > 0) core.info(`Required labels (all): ${requiredLabels.join(", ")}`); if (requiredTitlePrefix) core.info(`Required title prefix: ${requiredTitlePrefix}`); + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + if (defaultTargetRepo) core.info(`Target repository: ${defaultTargetRepo}`); + if (allowedRepos.size > 0) core.info(`Allowed repositories: ${Array.from(allowedRepos).join(", ")}`); + core.info(`Assign milestone configuration: max=${maxCount}, auto_create=${autoCreate}`); if (allowedMilestones.length > 0) { core.info(`Allowed milestones: ${allowedMilestones.join(", ")}`); @@ -73,9 +78,11 @@ async function main(config = {}) { * Find a milestone by title using lazy paginated search with early exit. * Results are cached so repeated lookups don't re-paginate. * @param {string} title + * @param {string} owner + * @param {string} repo * @returns {Promise} */ - async function findMilestoneByTitle(title) { + async function findMilestoneByTitle(title, owner, repo) { if (milestoneByTitle.has(title)) { return milestoneByTitle.get(title); } @@ -83,7 +90,7 @@ async function main(config = {}) { return null; } let found = false; - await githubClient.paginate(githubClient.rest.issues.listMilestones, { owner: context.repo.owner, repo: context.repo.repo, state: "all", per_page: 100 }, (response, done) => { + await githubClient.paginate(githubClient.rest.issues.listMilestones, { owner, repo, state: "all", per_page: 100 }, (response, done) => { for (const m of response.data) { if (!milestoneByTitle.has(m.title)) { milestoneByTitle.set(m.title, m); @@ -126,8 +133,17 @@ async function main(config = {}) { // Convert resolvedTemporaryIds to a normalized Map for resolveRepoIssueTarget const temporaryIdMap = loadTemporaryIdMapFromResolved(resolvedTemporaryIds); + // Resolve target repo from item or default target-repo config + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "milestone assignment"); + if (!repoResult.success) { + core.warning(`assign_milestone: ${repoResult.error}`); + return { success: false, error: repoResult.error }; + } + const milestoneOwner = repoResult.repoParts.owner; + const milestoneRepo = repoResult.repoParts.repo; + // Resolve issue_number, which may be a temporary ID (e.g. "aw_abc123") or a plain number - const resolvedIssueTarget = resolveRepoIssueTarget(item.issue_number, temporaryIdMap, context.repo.owner, context.repo.repo); + const resolvedIssueTarget = resolveRepoIssueTarget(item.issue_number, temporaryIdMap, milestoneOwner, milestoneRepo); // If the issue_number is a temporary ID that hasn't been resolved yet, defer processing if (resolvedIssueTarget.wasTemporaryId && !resolvedIssueTarget.resolved) { @@ -149,7 +165,7 @@ async function main(config = {}) { const issueNumber = resolvedIssueTarget.resolved.number; - const repoParts = { owner: context.repo.owner, repo: context.repo.repo }; + const repoParts = { owner: milestoneOwner, repo: milestoneRepo }; const filterResult = await checkRequiredFilter(githubClient, repoParts, issueNumber, requiredLabels, requiredTitlePrefix, "assign_milestone"); if (filterResult) return filterResult; @@ -174,15 +190,15 @@ async function main(config = {}) { // Resolve milestone by title if milestone_number is not valid if (!hasMilestoneNumber && milestoneTitle !== null) { try { - const match = await findMilestoneByTitle(milestoneTitle); + const match = await findMilestoneByTitle(milestoneTitle, milestoneOwner, milestoneRepo); if (match) { milestoneNumber = match.number; core.info(`Resolved milestone title "${milestoneTitle}" to #${milestoneNumber}`); } else if (autoCreate) { // Create the milestone automatically const created = await githubClient.rest.issues.createMilestone({ - owner: context.repo.owner, - repo: context.repo.repo, + owner: milestoneOwner, + repo: milestoneRepo, title: milestoneTitle, }); milestoneNumber = created.data.number; @@ -211,8 +227,8 @@ async function main(config = {}) { if (allowedMilestones.length > 0) { try { const { data: milestone } = await githubClient.rest.issues.getMilestone({ - owner: context.repo.owner, - repo: context.repo.repo, + owner: milestoneOwner, + repo: milestoneRepo, milestone_number: milestoneNumber, }); @@ -251,8 +267,8 @@ async function main(config = {}) { } await githubClient.rest.issues.update({ - owner: context.repo.owner, - repo: context.repo.repo, + owner: milestoneOwner, + repo: milestoneRepo, issue_number: issueNumber, milestone: milestoneNumber, }); diff --git a/actions/setup/js/assign_milestone.test.cjs b/actions/setup/js/assign_milestone.test.cjs index 7fb8f7f4826..6f2888b1241 100644 --- a/actions/setup/js/assign_milestone.test.cjs +++ b/actions/setup/js/assign_milestone.test.cjs @@ -376,4 +376,69 @@ describe("assign_milestone (Handler Factory Architecture)", () => { expect(result.error).toBe("Either milestone_number or milestone_title must be provided"); expect(mockGithub.rest.issues.update).not.toHaveBeenCalled(); }); + + describe("target-repo support", () => { + it("should use target-repo config for all API calls", async () => { + const { main } = require("./assign_milestone.cjs"); + const handlerWithTarget = await main({ + max: 10, + "target-repo": "external-org/external-repo", + }); + + mockGithub.rest.issues.update.mockResolvedValue({}); + const paginateCalls = []; + mockGithub.paginate.mockImplementation(async (_method, params, callback) => { + paginateCalls.push(params); + if (callback) { + const done = vi.fn(); + callback({ data: [] }, done); + } + return []; + }); + + const result = await handlerWithTarget({ type: "assign_milestone", issue_number: 42, milestone_number: 5 }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.issues.update).toHaveBeenCalledWith(expect.objectContaining({ owner: "external-org", repo: "external-repo" })); + }); + + it("should use context.repo as default when no target-repo configured", async () => { + mockGithub.rest.issues.update.mockResolvedValue({}); + + const result = await handler({ type: "assign_milestone", issue_number: 42, milestone_number: 5 }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.issues.update).toHaveBeenCalledWith(expect.objectContaining({ owner: "test-owner", repo: "test-repo" })); + }); + + it("should use repo from message when allowed_repos is configured", async () => { + const { main } = require("./assign_milestone.cjs"); + const handlerWithAllowedRepos = await main({ + max: 10, + "target-repo": "default-org/default-repo", + allowed_repos: ["cross-org/cross-repo"], + }); + + mockGithub.rest.issues.update.mockResolvedValue({}); + + const result = await handlerWithAllowedRepos({ type: "assign_milestone", issue_number: 42, milestone_number: 5, repo: "cross-org/cross-repo" }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.issues.update).toHaveBeenCalledWith(expect.objectContaining({ owner: "cross-org", repo: "cross-repo" })); + }); + + it("should reject repo not in allowed_repos list", async () => { + const { main } = require("./assign_milestone.cjs"); + const handlerWithAllowedRepos = await main({ + max: 10, + "target-repo": "default-org/default-repo", + allowed_repos: ["allowed-org/allowed-repo"], + }); + + const result = await handlerWithAllowedRepos({ type: "assign_milestone", issue_number: 42, milestone_number: 5, repo: "unauthorized-org/unauthorized-repo" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("not in the allowed-repos list"); + }); + }); }); diff --git a/actions/setup/js/close_discussion.cjs b/actions/setup/js/close_discussion.cjs index 084837555d0..a517607a4ef 100644 --- a/actions/setup/js/close_discussion.cjs +++ b/actions/setup/js/close_discussion.cjs @@ -13,6 +13,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { resolveNumberFromTemporaryId } = require("./temporary_id.cjs"); const { resolveAllowedMentionsFromPayload } = require("./resolve_mentions_from_payload.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** * Get discussion details using GraphQL with pagination for labels @@ -172,6 +173,12 @@ async function main(config = {}) { allowedMentionAliases = await resolveAllowedMentionsFromPayload(context, githubClient, core, config.mentions); } + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + if (defaultTargetRepo) { + core.info(`Target repository: ${defaultTargetRepo}`); + } + if (allowedRepos.size > 0) core.info(`Allowed repositories: ${Array.from(allowedRepos).join(", ")}`); + // Check if we're in staged mode const isStaged = isStagedMode(config); @@ -204,6 +211,15 @@ async function main(config = {}) { processedCount++; + // Resolve and validate target repository for this item + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "discussion"); + if (!repoResult.success) { + core.warning(`Skipping close_discussion: ${repoResult.error}`); + return { success: false, error: repoResult.error }; + } + const discussionOwner = repoResult.repoParts.owner; + const discussionRepo = repoResult.repoParts.repo; + // Determine discussion number let discussionNumber; if (item.discussion_number !== undefined) { @@ -235,7 +251,7 @@ async function main(config = {}) { try { // Fetch discussion details - const discussion = await getDiscussionDetails(githubClient, context.repo.owner, context.repo.repo, discussionNumber); + const discussion = await getDiscussionDetails(githubClient, discussionOwner, discussionRepo, discussionNumber); // Validate required labels if configured if (requiredLabels.length > 0) { diff --git a/actions/setup/js/close_discussion.test.cjs b/actions/setup/js/close_discussion.test.cjs index d04f27db1ab..246e065809e 100644 --- a/actions/setup/js/close_discussion.test.cjs +++ b/actions/setup/js/close_discussion.test.cjs @@ -664,4 +664,131 @@ describe("close_discussion", () => { }); }); }); + + describe("target-repo support", () => { + it("should use target-repo config for getDiscussionDetails query", async () => { + const handler = await main({ + max: 10, + "target-repo": "external-org/external-repo", + }); + + const queryCalls = []; + mockGithub.graphql = async (query, variables) => { + queryCalls.push({ query, variables }); + if (query.includes("closeDiscussion")) { + return { closeDiscussion: { discussion: { id: "D_1", url: "https://github.com/external-org/external-repo/discussions/5" } } }; + } + if (query.includes("addDiscussionComment")) { + return { addDiscussionComment: { comment: { id: "DC_1", url: "url" } } }; + } + return { + repository: { + discussion: { + id: "D_kwDOTest123", + title: "Test Discussion", + closed: false, + category: { name: "General" }, + url: "https://github.com/external-org/external-repo/discussions/5", + labels: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }; + }; + + const result = await handler({ discussion_number: 5, body: "Closing" }, {}); + + expect(result.success).toBe(true); + const detailsQuery = queryCalls.find(c => c.query.includes("repository(owner")); + expect(detailsQuery).toBeDefined(); + expect(detailsQuery.variables.owner).toBe("external-org"); + expect(detailsQuery.variables.repo).toBe("external-repo"); + }); + + it("should use context.repo as default when no target-repo configured", async () => { + const handler = await main({ max: 10 }); + + const queryCalls = []; + mockGithub.graphql = async (query, variables) => { + queryCalls.push({ query, variables }); + if (query.includes("closeDiscussion")) { + return { closeDiscussion: { discussion: { id: "D_1", url: "url" } } }; + } + if (query.includes("addDiscussionComment")) { + return { addDiscussionComment: { comment: { id: "DC_1", url: "url" } } }; + } + return { + repository: { + discussion: { + id: "D_kwDOTest123", + title: "Test Discussion", + closed: false, + category: { name: "General" }, + url: "https://github.com/test-owner/test-repo/discussions/42", + labels: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }; + }; + + const result = await handler({ discussion_number: 42, body: "Closing" }, {}); + + expect(result.success).toBe(true); + const detailsQuery = queryCalls.find(c => c.query.includes("repository(owner")); + expect(detailsQuery).toBeDefined(); + expect(detailsQuery.variables.owner).toBe("test-owner"); + expect(detailsQuery.variables.repo).toBe("test-repo"); + }); + + it("should reject item.repo not in allowed_repos", async () => { + const handler = await main({ + max: 10, + "target-repo": "default-org/default-repo", + allowed_repos: ["default-org/default-repo"], + }); + + const result = await handler({ discussion_number: 5, repo: "evil-org/evil-repo" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toMatch(/not allowed|evil-org\/evil-repo/); + }); + + it("should use item.repo when it is in allowed_repos", async () => { + const handler = await main({ + max: 10, + "target-repo": "default-org/default-repo", + allowed_repos: ["default-org/default-repo", "other-org/other-repo"], + }); + + const queryCalls = []; + mockGithub.graphql = async (query, variables) => { + queryCalls.push({ query, variables }); + if (query.includes("closeDiscussion")) { + return { closeDiscussion: { discussion: { id: "D_1", url: "https://github.com/other-org/other-repo/discussions/9" } } }; + } + if (query.includes("addDiscussionComment")) { + return { addDiscussionComment: { comment: { id: "DC_1", url: "url" } } }; + } + return { + repository: { + discussion: { + id: "D_kwDOOther", + title: "Other Discussion", + closed: false, + category: { name: "General" }, + url: "https://github.com/other-org/other-repo/discussions/9", + labels: { nodes: [], pageInfo: { hasNextPage: false, endCursor: null } }, + }, + }, + }; + }; + + const result = await handler({ discussion_number: 9, repo: "other-org/other-repo" }, {}); + + expect(result.success).toBe(true); + const detailsQuery = queryCalls.find(c => c.query.includes("repository(owner")); + expect(detailsQuery).toBeDefined(); + expect(detailsQuery.variables.owner).toBe("other-org"); + expect(detailsQuery.variables.repo).toBe("other-repo"); + }); + }); }); diff --git a/actions/setup/js/close_pull_request.cjs b/actions/setup/js/close_pull_request.cjs index 6429e5a79ae..a0f2d400040 100644 --- a/actions/setup/js/close_pull_request.cjs +++ b/actions/setup/js/close_pull_request.cjs @@ -4,6 +4,7 @@ const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { createCloseEntityHandler, checkLabelFilter, buildCommentBody, PULL_REQUEST_CONFIG } = require("./close_entity_helpers.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** * @typedef {import('./types/handler-factory').HandlerFactoryFunction} HandlerFactoryFunction @@ -77,9 +78,14 @@ async function closePullRequest(github, owner, repo, prNumber) { async function main(config = {}) { const requiredLabels = config.required_labels || []; const requiredTitlePrefix = config.required_title_prefix || ""; + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); const githubClient = await createAuthenticatedGitHubClient(config); core.info(`Close pull request configuration: max=${config.max || 10}`); + core.info(`Default target repo: ${defaultTargetRepo}`); + if (allowedRepos.size > 0) { + core.info(`Allowed repos: ${Array.from(allowedRepos).join(", ")}`); + } if (requiredLabels.length > 0) { core.info(`Required labels: ${requiredLabels.join(", ")}`); } @@ -92,6 +98,13 @@ async function main(config = {}) { PULL_REQUEST_CONFIG, { resolveTarget(item) { + // Resolve and validate target repository + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "pull request"); + if (!repoResult.success) { + return { success: false, error: repoResult.error }; + } + const { repo: entityRepo, repoParts } = repoResult; + let prNumber; if (item.pull_request_number !== undefined) { prNumber = parseInt(String(item.pull_request_number), 10); @@ -105,7 +118,7 @@ async function main(config = {}) { } prNumber = contextPR; } - return { success: true, entityNumber: prNumber, owner: context.repo.owner, repo: context.repo.repo }; + return { success: true, entityNumber: prNumber, owner: repoParts.owner, repo: repoParts.repo, entityRepo }; }, getDetails: getPullRequestDetails, diff --git a/actions/setup/js/link_sub_issue.cjs b/actions/setup/js/link_sub_issue.cjs index 488d462d54f..eb70ce098e5 100644 --- a/actions/setup/js/link_sub_issue.cjs +++ b/actions/setup/js/link_sub_issue.cjs @@ -6,6 +6,7 @@ const { getErrorMessage } = require("./error_helpers.cjs"); const { logStagedPreviewInfo } = require("./staged_preview.cjs"); const { isStagedMode } = require("./safe_output_helpers.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** * Main handler factory for link_sub_issue @@ -39,6 +40,19 @@ async function main(config = {}) { } core.info(`Max count: ${maxCount}`); + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + let defaultOwner = context.repo.owner; + let defaultRepo = context.repo.repo; + if (defaultTargetRepo) { + const parts = defaultTargetRepo.split("/"); + if (parts.length === 2 && parts[0] && parts[1]) { + defaultOwner = parts[0]; + defaultRepo = parts[1]; + core.info(`Target repository: ${defaultTargetRepo}`); + } + } + if (allowedRepos.size > 0) core.info(`Allowed repositories: ${Array.from(allowedRepos).join(", ")}`); + // Track how many items we've processed for max limit let processedCount = 0; @@ -62,12 +76,26 @@ async function main(config = {}) { const item = message; + // Resolve and validate target repository for this item + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "sub-issue link"); + if (!repoResult.success) { + core.warning(`Skipping link_sub_issue: ${repoResult.error}`); + return { + parent_issue_number: item.parent_issue_number, + sub_issue_number: item.sub_issue_number, + success: false, + error: repoResult.error, + }; + } + const itemOwner = repoResult.repoParts.owner; + const itemRepo = repoResult.repoParts.repo; + // Convert resolvedTemporaryIds to a normalized Map for resolveIssueNumber const temporaryIdMap = loadTemporaryIdMapFromResolved(resolvedTemporaryIds); // Resolve issue numbers, supporting temporary IDs from create_issue job - const parentResolved = resolveRepoIssueTarget(item.parent_issue_number, temporaryIdMap, context.repo.owner, context.repo.repo); - const subResolved = resolveRepoIssueTarget(item.sub_issue_number, temporaryIdMap, context.repo.owner, context.repo.repo); + const parentResolved = resolveRepoIssueTarget(item.parent_issue_number, temporaryIdMap, itemOwner, itemRepo); + const subResolved = resolveRepoIssueTarget(item.sub_issue_number, temporaryIdMap, itemOwner, itemRepo); // Check if either parent or sub issue is an unresolved temporary ID // If so, defer the operation to allow for resolution later @@ -151,8 +179,8 @@ async function main(config = {}) { } } - const owner = parentResolved.resolved?.owner || context.repo.owner; - const repo = parentResolved.resolved?.repo || context.repo.repo; + const owner = parentResolved.resolved?.owner || itemOwner; + const repo = parentResolved.resolved?.repo || itemRepo; // Fetch parent issue to validate filters let parentIssue; diff --git a/actions/setup/js/link_sub_issue.test.cjs b/actions/setup/js/link_sub_issue.test.cjs index f572343312d..7ba753ce2f9 100644 --- a/actions/setup/js/link_sub_issue.test.cjs +++ b/actions/setup/js/link_sub_issue.test.cjs @@ -211,4 +211,48 @@ const mockCore = { expect(mockGithub.rest.issues.get).not.toHaveBeenCalled(); expect(mockGithub.graphql).not.toHaveBeenCalled(); }); + + it("should use target-repo config as default for issue resolution", async () => { + const { main } = require(path.join(process.cwd(), "link_sub_issue.cjs")); + const handlerWithTarget = await main({ + max: 5, + "target-repo": "external-org/external-repo", + }); + + mockGithub.rest.issues.get + .mockResolvedValueOnce({ data: { number: 100, title: "Parent Issue", node_id: "I_parent_100", labels: [] } }) + .mockResolvedValueOnce({ data: { number: 50, title: "Sub Issue", node_id: "I_sub_50", labels: [] } }); + mockGithub.graphql.mockResolvedValueOnce({ repository: { issue: { parent: null } } }).mockResolvedValueOnce({ addSubIssue: { issue: { id: "I_parent_100", number: 100 }, subIssue: { id: "I_sub_50", number: 50 } } }); + + const result = await handlerWithTarget({ type: "link_sub_issue", parent_issue_number: 100, sub_issue_number: 50 }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.issues.get).toHaveBeenCalledWith(expect.objectContaining({ owner: "external-org", repo: "external-repo", issue_number: 100 })); + }); + + it("should use context.repo as default when no target-repo configured", async () => { + mockGithub.rest.issues.get + .mockResolvedValueOnce({ data: { number: 100, title: "Parent Issue", node_id: "I_parent_100", labels: [] } }) + .mockResolvedValueOnce({ data: { number: 50, title: "Sub Issue", node_id: "I_sub_50", labels: [] } }); + mockGithub.graphql.mockResolvedValueOnce({ repository: { issue: { parent: null } } }).mockResolvedValueOnce({ addSubIssue: { issue: { id: "I_parent_100", number: 100 }, subIssue: { id: "I_sub_50", number: 50 } } }); + + const result = await handler({ type: "link_sub_issue", parent_issue_number: 100, sub_issue_number: 50 }, {}); + + expect(result.success).toBe(true); + expect(mockGithub.rest.issues.get).toHaveBeenCalledWith(expect.objectContaining({ owner: "testowner", repo: "testrepo", issue_number: 100 })); + }); + + it("should reject item.repo not in allowed_repos", async () => { + const { main } = require(path.join(process.cwd(), "link_sub_issue.cjs")); + const handlerWithAllowed = await main({ + max: 5, + "target-repo": "default-org/default-repo", + allowed_repos: ["default-org/default-repo"], + }); + + const result = await handlerWithAllowed({ type: "link_sub_issue", parent_issue_number: 100, sub_issue_number: 50, repo: "evil-org/evil-repo" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toMatch(/not allowed|evil-org\/evil-repo/); + }); })); diff --git a/actions/setup/js/mark_pull_request_as_ready_for_review.cjs b/actions/setup/js/mark_pull_request_as_ready_for_review.cjs index fc8589a121b..053c05361a9 100644 --- a/actions/setup/js/mark_pull_request_as_ready_for_review.cjs +++ b/actions/setup/js/mark_pull_request_as_ready_for_review.cjs @@ -13,6 +13,7 @@ const { isStagedMode, checkRequiredFilter } = require("./safe_output_helpers.cjs const { ERR_NOT_FOUND } = require("./error_codes.cjs"); const { createAuthenticatedGitHubClient } = require("./handler_auth.cjs"); const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); +const { resolveTargetRepoConfig, resolveAndValidateRepo } = require("./repo_helpers.cjs"); /** @type {string} Safe output type handled by this module */ const HANDLER_TYPE = "mark_pull_request_as_ready_for_review"; @@ -113,6 +114,10 @@ async function main(config = {}) { core.info(`Mark pull request as ready for review configuration: max=${maxCount}`); + const { defaultTargetRepo, allowedRepos } = resolveTargetRepoConfig(config); + if (defaultTargetRepo) core.info(`Target repository: ${defaultTargetRepo}`); + if (allowedRepos.size > 0) core.info(`Allowed repositories: ${Array.from(allowedRepos).join(", ")}`); + // Track how many items we've processed for max limit let processedCount = 0; @@ -136,6 +141,15 @@ async function main(config = {}) { const item = message; + // Determine PR number and target repo + const repoResult = resolveAndValidateRepo(item, defaultTargetRepo, allowedRepos, "pull request"); + if (!repoResult.success) { + core.warning(`mark_pull_request_as_ready_for_review: ${repoResult.error}`); + return { success: false, error: repoResult.error }; + } + const prOwner = repoResult.repoParts.owner; + const prRepo = repoResult.repoParts.repo; + // Determine PR number let prNumber; if (item.pull_request_number !== undefined) { @@ -160,7 +174,7 @@ async function main(config = {}) { prNumber = contextPR; } - const repoParts = { owner: context.repo.owner, repo: context.repo.repo }; + const repoParts = { owner: prOwner, repo: prRepo }; const filterResult = await checkRequiredFilter(githubClient, repoParts, prNumber, requiredLabels, requiredTitlePrefix, "mark_pull_request_as_ready_for_review"); if (filterResult) return filterResult; @@ -175,7 +189,7 @@ async function main(config = {}) { try { // First, get the current PR to check if it's a draft - const currentPR = await getPullRequestDetails(githubClient, context.repo.owner, context.repo.repo, prNumber); + const currentPR = await getPullRequestDetails(githubClient, prOwner, prRepo, prNumber); // Check if it's already not a draft if (!currentPR.draft) { @@ -234,7 +248,7 @@ async function main(config = {}) { const footer = generateFooterWithMessages(workflowName, runUrl, workflowSource, workflowSourceURL, triggeringIssueNumber, triggeringPRNumber, triggeringDiscussionNumber, undefined, { skipDetectionCaution: true }); const commentBody = `${cautionPrefix}${sanitizedReason}\n\n${footer}`; - await addPullRequestComment(githubClient, context.repo.owner, context.repo.repo, prNumber, commentBody); + await addPullRequestComment(githubClient, prOwner, prRepo, prNumber, commentBody); core.info(`✓ Marked PR #${prNumber} as ready for review and added comment: ${pr.html_url}`); diff --git a/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs b/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs index 3f5a2954e42..d59d1e30a27 100644 --- a/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs +++ b/actions/setup/js/mark_pull_request_as_ready_for_review.test.cjs @@ -326,4 +326,64 @@ describe("mark_pull_request_as_ready_for_review", () => { expect(mockGraphql).not.toHaveBeenCalled(); }); }); + + describe("target-repo support", () => { + it("should use target-repo config for PR fetch and comment", async () => { + const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); + const handler = await main({ + max: 10, + "target-repo": "external-org/external-repo", + }); + + setupDefaultMocks(42); + + const result = await handler({ pull_request_number: 42, reason: "Ready for review" }, {}); + + expect(result.success).toBe(true); + expect(mockRestPullsGet).toHaveBeenCalledWith(expect.objectContaining({ owner: "external-org", repo: "external-repo" })); + expect(mockRestIssuesCreateComment).toHaveBeenCalledWith(expect.objectContaining({ owner: "external-org", repo: "external-repo" })); + }); + + it("should use context.repo as default when no target-repo configured", async () => { + const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); + const handler = await main({ max: 10 }); + + setupDefaultMocks(42); + + const result = await handler({ pull_request_number: 42, reason: "Ready" }, {}); + + expect(result.success).toBe(true); + expect(mockRestPullsGet).toHaveBeenCalledWith(expect.objectContaining({ owner: "test-owner", repo: "test-repo" })); + }); + + it("should use repo from message when allowed_repos is configured", async () => { + const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); + const handler = await main({ + max: 10, + "target-repo": "default-org/default-repo", + allowed_repos: ["cross-org/cross-repo"], + }); + + setupDefaultMocks(42); + + const result = await handler({ pull_request_number: 42, reason: "Ready", repo: "cross-org/cross-repo" }, {}); + + expect(result.success).toBe(true); + expect(mockRestPullsGet).toHaveBeenCalledWith(expect.objectContaining({ owner: "cross-org", repo: "cross-repo" })); + }); + + it("should reject repo not in allowed_repos list", async () => { + const { main } = require("./mark_pull_request_as_ready_for_review.cjs"); + const handler = await main({ + max: 10, + "target-repo": "default-org/default-repo", + allowed_repos: ["allowed-org/allowed-repo"], + }); + + const result = await handler({ pull_request_number: 42, reason: "Ready", repo: "unauthorized-org/unauthorized-repo" }, {}); + + expect(result.success).toBe(false); + expect(result.error).toContain("not in the allowed-repos list"); + }); + }); }); diff --git a/pkg/cli/add_package_manifest_test.go b/pkg/cli/add_package_manifest_test.go index eab6c058e0a..795f4a5561f 100644 --- a/pkg/cli/add_package_manifest_test.go +++ b/pkg/cli/add_package_manifest_test.go @@ -1329,6 +1329,8 @@ files: `), nil case "README.md": return []byte("# Full Package\n"), nil + case "skills/my-skill/SKILL.md": + return []byte("# My Skill\nThis is a skill.\n"), nil default: return nil, createRepositoryPackageNotFoundError(filePath) } diff --git a/pkg/cli/copilot_agents.go b/pkg/cli/copilot_agents.go index 1fdb136ceea..f965a182e0f 100644 --- a/pkg/cli/copilot_agents.go +++ b/pkg/cli/copilot_agents.go @@ -158,6 +158,10 @@ func buildAgenticWorkflowsSkillContent(gitRoot string) (string, error) { awRoot := filepath.Join(gitRoot, ".github", "aw") entries, err := os.ReadDir(awRoot) if err != nil { + if os.IsNotExist(err) { + // No .github/aw directory yet — emit a minimal skill without the file list. + return agenticWorkflowsSkillHeader + agenticWorkflowsSkillIntro + agenticWorkflowsSkillOutro, nil + } return "", fmt.Errorf("failed to read .github/aw directory for skill generation (%s): %w", awRoot, err) } diff --git a/pkg/cli/logs_json_stderr_order_test.go b/pkg/cli/logs_json_stderr_order_test.go index e7c8a96ca34..c6ade42d0a7 100644 --- a/pkg/cli/logs_json_stderr_order_test.go +++ b/pkg/cli/logs_json_stderr_order_test.go @@ -54,12 +54,13 @@ func TestLogsJSONOutputBeforeStderr(t *testing.T) { os.Stdout = oldStdout os.Stderr = oldStderr - // Skip test if GitHub API is not accessible + // Skip test if GitHub API is not accessible or workflow not found if err != nil { if strings.Contains(err.Error(), "no auth token found") || strings.Contains(err.Error(), "GitHub CLI authentication required") || - strings.Contains(err.Error(), "HTTP 403") { - t.Skip("Skipping test: GitHub authentication not available") + strings.Contains(err.Error(), "HTTP 403") || + strings.Contains(err.Error(), "failed to list workflow runs") { + t.Skip("Skipping test: GitHub API not available or workflow not found in this environment") } // For other errors, we still want to verify the output format } @@ -163,12 +164,13 @@ func TestLogsJSONAndStderrRedirected(t *testing.T) { os.Stdout = oldStdout os.Stderr = oldStderr - // Skip test if GitHub API is not accessible + // Skip test if GitHub API is not accessible or workflow not found if err != nil { if strings.Contains(err.Error(), "no auth token found") || strings.Contains(err.Error(), "GitHub CLI authentication required") || - strings.Contains(err.Error(), "HTTP 403") { - t.Skip("Skipping test: GitHub authentication not available") + strings.Contains(err.Error(), "HTTP 403") || + strings.Contains(err.Error(), "failed to list workflow runs") { + t.Skip("Skipping test: GitHub API not available or workflow not found in this environment") } } diff --git a/pkg/cli/update_manifest_test.go b/pkg/cli/update_manifest_test.go index dafca2df5c6..2fd51e11ad3 100644 --- a/pkg/cli/update_manifest_test.go +++ b/pkg/cli/update_manifest_test.go @@ -21,12 +21,16 @@ func TestUpdateManifestWorkflowGroup_AddsUpdatesRemoves(t *testing.T) { originalListPackage := listPackageWorkflowFilesForHost originalDefaultBranch := getRepositoryPackageDefaultBranch originalDownloadWorkflow := downloadWorkflowContentFn + originalDirSubdirs := listPackageDirSubdirsForHost + originalDirFiles := listPackageDirFilesForHost t.Cleanup(func() { resolveLatestRefFn = originalResolveLatestRef downloadPackageFileFromGitHubForHost = originalDownloadPackage listPackageWorkflowFilesForHost = originalListPackage getRepositoryPackageDefaultBranch = originalDefaultBranch downloadWorkflowContentFn = originalDownloadWorkflow + listPackageDirSubdirsForHost = originalDirSubdirs + listPackageDirFilesForHost = originalDirFiles }) resolveLatestRefFn = func(ctx context.Context, repo, currentRef string, allowMajor, verbose bool, coolDown time.Duration) (string, error) { @@ -52,6 +56,13 @@ func TestUpdateManifestWorkflowGroup_AddsUpdatesRemoves(t *testing.T) { listPackageWorkflowFilesForHost = func(owner, repo, ref, workflowPath, host string) ([]string, error) { return nil, errors.New("unexpected scan") } + // Return not-found so skill/agent auto-scan skips gracefully (no real network needed) + listPackageDirSubdirsForHost = func(owner, repo, ref, dirPath, host string) ([]string, error) { + return nil, createRepositoryPackageNotFoundError(dirPath) + } + listPackageDirFilesForHost = func(owner, repo, ref, dirPath, host string) ([]string, error) { + return nil, createRepositoryPackageNotFoundError(dirPath) + } downloadWorkflowContentFn = func(_ context.Context, repo, path, ref string, _ bool) ([]byte, error) { if repo != "owner/repo" { diff --git a/pkg/parser/remote_fetch.go b/pkg/parser/remote_fetch.go index 35708fb302c..4df65a3e3c0 100644 --- a/pkg/parser/remote_fetch.go +++ b/pkg/parser/remote_fetch.go @@ -53,15 +53,19 @@ func getOrCreateListRepoClone(owner, repo, ref, host string) (string, error) { repoURL := fmt.Sprintf("%s/%s/%s.git", githubHost, owner, repo) cacheKey := fmt.Sprintf("%s|%s|%s|%s", githubHost, owner, repo, ref) - gitListCloneCache.mu.Lock() - if cloneDir, ok := gitListCloneCache.dirs[cacheKey]; ok { - if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() { - gitListCloneCache.mu.Unlock() - return cloneDir, nil + if cloneDir, found := func() (string, bool) { + gitListCloneCache.mu.Lock() + defer gitListCloneCache.mu.Unlock() + if cloneDir, ok := gitListCloneCache.dirs[cacheKey]; ok { + if stat, err := os.Stat(filepath.Join(cloneDir, ".git")); err == nil && stat.IsDir() { + return cloneDir, true + } + delete(gitListCloneCache.dirs, cacheKey) } - delete(gitListCloneCache.dirs, cacheKey) + return "", false + }(); found { + return cloneDir, nil } - gitListCloneCache.mu.Unlock() tmpDir, err := os.MkdirTemp("", "gh-aw-list-*") if err != nil { @@ -78,18 +82,23 @@ func getOrCreateListRepoClone(owner, repo, ref, host string) (string, error) { return "", fmt.Errorf("failed to clone repository for %s/%s@%s: %w", owner, repo, ref, err) } - gitListCloneCache.mu.Lock() - if existingDir, ok := gitListCloneCache.dirs[cacheKey]; ok { - if stat, statErr := os.Stat(filepath.Join(existingDir, ".git")); statErr == nil && stat.IsDir() { - gitListCloneCache.mu.Unlock() - if cleanupErr := os.RemoveAll(tmpDir); cleanupErr != nil { - remoteLog.Printf("Failed to clean up duplicate clone %q: %v", tmpDir, cleanupErr) + existingDir, found := func() (string, bool) { + gitListCloneCache.mu.Lock() + defer gitListCloneCache.mu.Unlock() + if existingDir, ok := gitListCloneCache.dirs[cacheKey]; ok { + if stat, statErr := os.Stat(filepath.Join(existingDir, ".git")); statErr == nil && stat.IsDir() { + return existingDir, true } - return existingDir, nil } + gitListCloneCache.dirs[cacheKey] = tmpDir + return "", false + }() + if found { + if cleanupErr := os.RemoveAll(tmpDir); cleanupErr != nil { + remoteLog.Printf("Failed to clean up duplicate clone %q: %v", tmpDir, cleanupErr) + } + return existingDir, nil } - gitListCloneCache.dirs[cacheKey] = tmpDir - gitListCloneCache.mu.Unlock() return tmpDir, nil } diff --git a/pkg/workflow/schemas/awf-config.schema.json b/pkg/workflow/schemas/awf-config.schema.json index 7fbf4ba858a..0869201e268 100644 --- a/pkg/workflow/schemas/awf-config.schema.json +++ b/pkg/workflow/schemas/awf-config.schema.json @@ -61,10 +61,7 @@ }, "anthropicCacheTailTtl": { "type": "string", - "enum": [ - "5m", - "1h" - ], + "enum": ["5m", "1h"], "description": "TTL for Anthropic cache tail optimization. Only applies when anthropicAutoCache is enabled. Allowed values: \"5m\" or \"1h\"." }, "maxEffectiveTokens": { @@ -105,9 +102,7 @@ }, "strategy": { "type": "string", - "enum": [ - "middle_power" - ], + "enum": ["middle_power"], "description": "Fallback selection strategy. Currently only 'middle_power' is supported." }, "excludeEngines": { @@ -163,19 +158,12 @@ "properties": { "type": { "type": "string", - "enum": [ - "github-oidc" - ], + "enum": ["github-oidc"], "description": "Authentication type. Currently only 'github-oidc' is supported. Maps to AWF_AUTH_TYPE." }, "provider": { "type": "string", - "enum": [ - "azure", - "aws", - "gcp", - "anthropic" - ], + "enum": ["azure", "aws", "gcp", "anthropic"], "description": "Cloud provider for OIDC token exchange. Determines which token exchange protocol is used. Maps to AWF_AUTH_PROVIDER.", "default": "azure" }, @@ -198,11 +186,7 @@ }, "azureCloud": { "type": "string", - "enum": [ - "public", - "usgovernment", - "china" - ], + "enum": ["public", "usgovernment", "china"], "description": "Azure cloud environment. Maps to AWF_AUTH_AZURE_CLOUD.", "default": "public" }, @@ -249,24 +233,17 @@ "description": "Anthropic workspace ID. Required when the federation rule covers multiple workspaces. Maps to AWF_AUTH_ANTHROPIC_WORKSPACE_ID." } }, - "required": [ - "type" - ], + "required": ["type"], "if": { "properties": { "provider": { "const": "aws" } }, - "required": [ - "provider" - ] + "required": ["provider"] }, "then": { - "required": [ - "awsRoleArn", - "awsRegion" - ] + "required": ["awsRoleArn", "awsRegion"] }, "else": { "if": { @@ -275,14 +252,10 @@ "const": "gcp" } }, - "required": [ - "provider" - ] + "required": ["provider"] }, "then": { - "required": [ - "gcpWorkloadIdentityProvider" - ] + "required": ["gcpWorkloadIdentityProvider"] }, "else": { "if": { @@ -291,22 +264,13 @@ "const": "anthropic" } }, - "required": [ - "provider" - ] + "required": ["provider"] }, "then": { - "required": [ - "anthropicFederationRuleId", - "anthropicOrganizationId", - "anthropicServiceAccountId" - ] + "required": ["anthropicFederationRuleId", "anthropicOrganizationId", "anthropicServiceAccountId"] }, "else": { - "required": [ - "azureTenantId", - "azureClientId" - ] + "required": ["azureTenantId", "azureClientId"] } } } @@ -482,12 +446,7 @@ "properties": { "logLevel": { "type": "string", - "enum": [ - "debug", - "info", - "warn", - "error" - ], + "enum": ["debug", "info", "warn", "error"], "description": "Log verbosity level. Defaults to \"info\"." }, "diagnosticLogs": { diff --git a/pkg/workflow/tool_description_enhancer.go b/pkg/workflow/tool_description_enhancer.go index 7150db07f76..e7a85cc7257 100644 --- a/pkg/workflow/tool_description_enhancer.go +++ b/pkg/workflow/tool_description_enhancer.go @@ -124,6 +124,9 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO if config.Target != "" { constraints = append(constraints, fmt.Sprintf("Target: %s.", config.Target)) } + if config.TargetRepoSlug != "" { + constraints = append(constraints, fmt.Sprintf("Discussions will be closed in repository %q.", config.TargetRepoSlug)) + } if config.RequiredTitlePrefix != "" { constraints = append(constraints, fmt.Sprintf("Only discussions with title prefix %q can be closed.", config.RequiredTitlePrefix)) } @@ -179,6 +182,9 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO if config.Target != "" { constraints = append(constraints, fmt.Sprintf("Target: %s.", config.Target)) } + if config.TargetRepoSlug != "" { + constraints = append(constraints, fmt.Sprintf("Pull requests will be closed in repository %q.", config.TargetRepoSlug)) + } if len(config.RequiredLabels) > 0 { constraints = append(constraints, fmt.Sprintf("Only PRs with labels %v can be closed.", config.RequiredLabels)) } @@ -187,6 +193,16 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO } } + case "mark_pull_request_as_ready_for_review": + if config := safeOutputs.MarkPullRequestAsReadyForReview; config != nil { + if templatableIntValue(config.Max) > 0 { + constraints = append(constraints, fmt.Sprintf("Maximum %d pull request(s) can be marked as ready for review.", templatableIntValue(config.Max))) + } + if config.TargetRepoSlug != "" { + constraints = append(constraints, fmt.Sprintf("Pull requests will be marked as ready in repository %q.", config.TargetRepoSlug)) + } + } + case "add_comment": if config := safeOutputs.AddComments; config != nil { if templatableIntValue(config.Max) > 0 { @@ -389,6 +405,9 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO if config.SubTitlePrefix != "" { constraints = append(constraints, fmt.Sprintf("The sub-issue title must start with %q.", config.SubTitlePrefix)) } + if config.TargetRepoSlug != "" { + constraints = append(constraints, fmt.Sprintf("Sub-issues will be linked in repository %q.", config.TargetRepoSlug)) + } } case "assign_milestone": @@ -396,6 +415,9 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO if templatableIntValue(config.Max) > 0 { constraints = append(constraints, fmt.Sprintf("Maximum %d milestone assignment(s) can be made.", templatableIntValue(config.Max))) } + if config.TargetRepoSlug != "" { + constraints = append(constraints, fmt.Sprintf("Milestones will be assigned in repository %q.", config.TargetRepoSlug)) + } } case "assign_to_agent": @@ -413,7 +435,6 @@ func enhanceToolDescription(toolName, baseDescription string, safeOutputs *SafeO constraints = append(constraints, fmt.Sprintf("Agent assignment can target these repositories: %v.", config.AllowedRepos)) } } - case "update_project": if config := safeOutputs.UpdateProjects; config != nil { if templatableIntValue(config.Max) > 0 { diff --git a/pkg/workflow/tool_description_enhancer_test.go b/pkg/workflow/tool_description_enhancer_test.go index 9c7085a8af1..054a040f291 100644 --- a/pkg/workflow/tool_description_enhancer_test.go +++ b/pkg/workflow/tool_description_enhancer_test.go @@ -112,3 +112,67 @@ func TestEnhanceToolDescriptionCloseIssueAllowBodyTrue(t *testing.T) { t.Fatalf("did not expect body-not-allowed constraint when allow-body is true, got: %s", description) } } + +func TestEnhanceToolDescriptionCloseDiscussionTargetRepo(t *testing.T) { + description := enhanceToolDescription("close_discussion", "Close a discussion.", &SafeOutputsConfig{ + CloseDiscussions: &CloseDiscussionsConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(5)}, + SafeOutputTargetConfig: SafeOutputTargetConfig{TargetRepoSlug: "myorg/myrepo"}, + }, + }) + + if !strings.Contains(description, `"myorg/myrepo"`) { + t.Fatalf("expected target repo constraint in description, got: %s", description) + } +} + +func TestEnhanceToolDescriptionAssignMilestoneTargetRepo(t *testing.T) { + description := enhanceToolDescription("assign_milestone", "Assign a milestone.", &SafeOutputsConfig{ + AssignMilestone: &AssignMilestoneConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(5)}, + SafeOutputTargetConfig: SafeOutputTargetConfig{TargetRepoSlug: "myorg/myrepo"}, + }, + }) + + if !strings.Contains(description, `"myorg/myrepo"`) { + t.Fatalf("expected target repo constraint in description, got: %s", description) + } +} + +func TestEnhanceToolDescriptionLinkSubIssueTargetRepo(t *testing.T) { + description := enhanceToolDescription("link_sub_issue", "Link a sub-issue.", &SafeOutputsConfig{ + LinkSubIssue: &LinkSubIssueConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(5)}, + SafeOutputTargetConfig: SafeOutputTargetConfig{TargetRepoSlug: "myorg/myrepo"}, + }, + }) + + if !strings.Contains(description, `"myorg/myrepo"`) { + t.Fatalf("expected target repo constraint in description, got: %s", description) + } +} + +func TestEnhanceToolDescriptionMarkPRReadyForReviewTargetRepo(t *testing.T) { + description := enhanceToolDescription("mark_pull_request_as_ready_for_review", "Mark PR as ready.", &SafeOutputsConfig{ + MarkPullRequestAsReadyForReview: &MarkPullRequestAsReadyForReviewConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(10)}, + SafeOutputTargetConfig: SafeOutputTargetConfig{TargetRepoSlug: "myorg/myrepo"}, + }, + }) + + if !strings.Contains(description, `"myorg/myrepo"`) { + t.Fatalf("expected target repo constraint in description, got: %s", description) + } +} + +func TestEnhanceToolDescriptionMarkPRReadyForReviewMaxCount(t *testing.T) { + description := enhanceToolDescription("mark_pull_request_as_ready_for_review", "Mark PR as ready.", &SafeOutputsConfig{ + MarkPullRequestAsReadyForReview: &MarkPullRequestAsReadyForReviewConfig{ + BaseSafeOutputConfig: BaseSafeOutputConfig{Max: defaultIntStr(3)}, + }, + }) + + if !strings.Contains(description, "Maximum 3 pull request(s) can be marked as ready for review.") { + t.Fatalf("expected max count constraint in description, got: %s", description) + } +}