diff --git a/.github/workflows/agentic-auto-upgrade.yml b/.github/workflows/agentic-auto-upgrade.yml index 7035101ee35..e169cea32b6 100644 --- a/.github/workflows/agentic-auto-upgrade.yml +++ b/.github/workflows/agentic-auto-upgrade.yml @@ -34,7 +34,7 @@ name: Agentic Auto-Upgrade on: schedule: - - cron: "21 3 * * 5" # Weekly (auto-upgrade) + - cron: "11 4 * * 6" # Weekly (auto-upgrade) workflow_dispatch: permissions: diff --git a/actions/setup/js/add_reaction.cjs b/actions/setup/js/add_reaction.cjs index df3a561c6de..723b2a75520 100644 --- a/actions/setup/js/add_reaction.cjs +++ b/actions/setup/js/add_reaction.cjs @@ -41,7 +41,7 @@ async function main() { const { owner, repo } = invocationContext.eventRepo; const payload = invocationContext.eventPayload; - /** @type {string | null} */ + /** @type {{ route: string, params: Record } | null} */ const reactionEndpoint = resolveRestEndpoint(eventName, owner, repo, payload); if (reactionEndpoint === null) { @@ -52,22 +52,22 @@ async function main() { return; } - core.info(`Adding reaction to: ${reactionEndpoint}`); + core.info(`Adding reaction to: ${reactionEndpoint.route}`); try { - await addReaction(reactionEndpoint, reaction); + await addReaction(reactionEndpoint.route, reactionEndpoint.params, reaction); } catch (error) { handleReactionError(error); } } /** - * Resolve the REST API endpoint for non-discussion events. + * Resolve the REST API route and params for non-discussion events. * Returns null for discussion/discussion_comment/pull_request_review/unsupported events (handled separately). * @param {string} eventName * @param {string} owner * @param {string} repo * @param {Record} payload - * @returns {string | null} + * @returns {{ route: string, params: Record } | null} */ function resolveRestEndpoint(eventName, owner, repo, payload) { switch (eventName) { @@ -77,7 +77,7 @@ function resolveRestEndpoint(eventName, owner, repo, payload) { core.setFailed(`${ERR_NOT_FOUND}: Issue number not found in event payload`); return null; } - return `/repos/${owner}/${repo}/issues/${issueNumber}/reactions`; + return { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner, repo, issue_number: issueNumber } }; } case "issue_comment": { @@ -86,7 +86,7 @@ function resolveRestEndpoint(eventName, owner, repo, payload) { core.setFailed(`${ERR_VALIDATION}: Comment ID not found in event payload`); return null; } - return `/repos/${owner}/${repo}/issues/comments/${commentId}/reactions`; + return { route: "POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", params: { owner, repo, comment_id: commentId } }; } case "pull_request": { @@ -96,7 +96,7 @@ function resolveRestEndpoint(eventName, owner, repo, payload) { return null; } // PRs are "issues" for the reactions endpoint - return `/repos/${owner}/${repo}/issues/${prNumber}/reactions`; + return { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner, repo, issue_number: prNumber } }; } case "pull_request_review_comment": { @@ -105,7 +105,7 @@ function resolveRestEndpoint(eventName, owner, repo, payload) { core.setFailed(`${ERR_VALIDATION}: Review comment ID not found in event payload`); return null; } - return `/repos/${owner}/${repo}/pulls/comments/${reviewCommentId}/reactions`; + return { route: "POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", params: { owner, repo, comment_id: reviewCommentId } }; } case "pull_request_review": @@ -186,11 +186,13 @@ function handleReactionError(error) { /** * Add a reaction to a GitHub issue, PR, or comment using REST API - * @param {string} endpoint - The GitHub API endpoint to add the reaction to + * @param {string} route - The typed GitHub API route string (e.g. "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions") + * @param {Record} params - The route parameters (owner, repo, issue_number, etc.) * @param {string} reaction - The reaction type to add */ -async function addReaction(endpoint, reaction) { - const response = await github.request(`POST ${endpoint}`, { +async function addReaction(route, params, reaction) { + const response = await github.request(route, { + ...params, content: reaction, headers: { Accept: "application/vnd.github+json", diff --git a/actions/setup/js/add_reaction.test.cjs b/actions/setup/js/add_reaction.test.cjs index 63402966f95..c6678b2be62 100644 --- a/actions/setup/js/add_reaction.test.cjs +++ b/actions/setup/js/add_reaction.test.cjs @@ -151,7 +151,7 @@ describe("add_reaction", () => { await runScript(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/456/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", issue_number: 456 })); expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", "12345"); }); @@ -180,7 +180,7 @@ describe("add_reaction", () => { await runScript(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/comments/789/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", comment_id: 789 })); }); it("should fail when comment ID is missing", async () => { @@ -206,7 +206,7 @@ describe("add_reaction", () => { await runScript(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/999/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", issue_number: 999 })); }); it("should fail when PR number is missing", async () => { @@ -232,7 +232,7 @@ describe("add_reaction", () => { await runScript(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/pulls/comments/555/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", comment_id: 555 })); }); it("should fail when review comment ID is missing", async () => { @@ -464,7 +464,7 @@ describe("add_reaction", () => { await runScript(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/target-owner/target-repo/issues/comments/456/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", expect.objectContaining({ content: "eyes", owner: "target-owner", repo: "target-repo", comment_id: 456 })); }); it("should fail for unsupported event types", async () => { @@ -590,9 +590,12 @@ describe("add_reaction", () => { const { addReaction } = await importHelpers(); mockGithub.request.mockResolvedValueOnce({ data: { id: 111 } }); - await addReaction("/repos/owner/repo/issues/1/reactions", "+1"); + await addReaction("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { owner: "owner", repo: "repo", issue_number: 1 }, "+1"); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/owner/repo/issues/1/reactions", { + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { + owner: "owner", + repo: "repo", + issue_number: 1, content: "+1", headers: { Accept: "application/vnd.github+json" }, }); @@ -603,7 +606,7 @@ describe("add_reaction", () => { const { addReaction } = await importHelpers(); mockGithub.request.mockResolvedValueOnce({ data: {} }); - await addReaction("/repos/owner/repo/issues/1/reactions", "eyes"); + await addReaction("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { owner: "owner", repo: "repo", issue_number: 1 }, "eyes"); expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", ""); }); @@ -612,7 +615,7 @@ describe("add_reaction", () => { const { addReaction } = await importHelpers(); mockGithub.request.mockResolvedValueOnce({ data: { id: 777 } }); - await addReaction("/repos/owner/repo/issues/1/reactions", "rocket"); + await addReaction("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { owner: "owner", repo: "repo", issue_number: 1 }, "rocket"); expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("rocket")); expect(mockCore.info).toHaveBeenCalledWith(expect.stringContaining("777")); @@ -694,7 +697,7 @@ describe("add_reaction", () => { it("should return issues endpoint", async () => { global.context = { eventName: "issues", repo: { owner: "o", repo: "r" }, payload: { issue: { number: 1 } } }; const { resolveRestEndpoint } = await importHelpers(); - expect(resolveRestEndpoint("issues", "o", "r", global.context.payload)).toBe("/repos/o/r/issues/1/reactions"); + expect(resolveRestEndpoint("issues", "o", "r", global.context.payload)).toEqual({ route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner: "o", repo: "r", issue_number: 1 } }); }); it("should return null and setFailed when issue number is missing", async () => { @@ -707,7 +710,7 @@ describe("add_reaction", () => { it("should return issue_comment endpoint", async () => { global.context = { eventName: "issue_comment", repo: { owner: "o", repo: "r" }, payload: { comment: { id: 42 } } }; const { resolveRestEndpoint } = await importHelpers(); - expect(resolveRestEndpoint("issue_comment", "o", "r", global.context.payload)).toBe("/repos/o/r/issues/comments/42/reactions"); + expect(resolveRestEndpoint("issue_comment", "o", "r", global.context.payload)).toEqual({ route: "POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", params: { owner: "o", repo: "r", comment_id: 42 } }); }); it("should return null and setFailed when comment id is missing", async () => { @@ -720,7 +723,7 @@ describe("add_reaction", () => { it("should return pull_request endpoint using issues path", async () => { global.context = { eventName: "pull_request", repo: { owner: "o", repo: "r" }, payload: { pull_request: { number: 7 } } }; const { resolveRestEndpoint } = await importHelpers(); - expect(resolveRestEndpoint("pull_request", "o", "r", global.context.payload)).toBe("/repos/o/r/issues/7/reactions"); + expect(resolveRestEndpoint("pull_request", "o", "r", global.context.payload)).toEqual({ route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner: "o", repo: "r", issue_number: 7 } }); }); it("should return null and setFailed when PR number is missing", async () => { @@ -733,7 +736,7 @@ describe("add_reaction", () => { it("should return pull_request_review_comment endpoint", async () => { global.context = { eventName: "pull_request_review_comment", repo: { owner: "o", repo: "r" }, payload: { comment: { id: 55 } } }; const { resolveRestEndpoint } = await importHelpers(); - expect(resolveRestEndpoint("pull_request_review_comment", "o", "r", global.context.payload)).toBe("/repos/o/r/pulls/comments/55/reactions"); + expect(resolveRestEndpoint("pull_request_review_comment", "o", "r", global.context.payload)).toEqual({ route: "POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", params: { owner: "o", repo: "r", comment_id: 55 } }); }); it("should return null without failure for pull_request_review events", async () => { diff --git a/actions/setup/js/add_reaction_and_edit_comment.cjs b/actions/setup/js/add_reaction_and_edit_comment.cjs index b4ab19bc419..2a2f1a29819 100644 --- a/actions/setup/js/add_reaction_and_edit_comment.cjs +++ b/actions/setup/js/add_reaction_and_edit_comment.cjs @@ -27,6 +27,31 @@ const EVENT_TYPE_DESCRIPTIONS = { /** Valid GitHub reaction types */ const VALID_REACTIONS = Object.freeze(Object.keys(REACTION_MAP)); +/** + * @typedef {{ route: string, params: Record }} RestEndpoint + */ + +/** + * @param {unknown} endpoint + * @returns {endpoint is RestEndpoint} + */ +function isRestEndpoint(endpoint) { + return typeof endpoint === "object" && endpoint !== null && "route" in endpoint && "params" in endpoint; +} + +/** + * @param {unknown} endpoint + * @param {string} endpointName + * @param {string} eventName + * @returns {RestEndpoint} + */ +function expectRestEndpoint(endpoint, endpointName, eventName) { + if (!isRestEndpoint(endpoint)) { + throw new Error(`${ERR_VALIDATION}: Unexpected ${endpointName} endpoint shape for event: ${eventName}`); + } + return endpoint; +} + /** * Resolve the reaction and comment API endpoints for a given event. * Returns null (after calling core.setFailed) when the event or payload is invalid. @@ -34,7 +59,7 @@ const VALID_REACTIONS = Object.freeze(Object.keys(REACTION_MAP)); * @param {string} owner - Repository owner * @param {string} repo - Repository name * @param {Record} payload - The event payload - * @returns {Promise<{reactionEndpoint: string, commentUpdateEndpoint: string} | null>} + * @returns {Promise<{reactionEndpoint: { route: string, params: Record } | string, commentUpdateEndpoint: { route: string, params: Record } | string} | null>} */ async function resolveEventEndpoints(eventName, owner, repo, payload) { switch (eventName) { @@ -45,8 +70,8 @@ async function resolveEventEndpoints(eventName, owner, repo, payload) { return null; } return { - reactionEndpoint: `/repos/${owner}/${repo}/issues/${issueNumber}/reactions`, - commentUpdateEndpoint: `/repos/${owner}/${repo}/issues/${issueNumber}/comments`, + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner, repo, issue_number: issueNumber } }, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: issueNumber } }, }; } @@ -62,9 +87,9 @@ async function resolveEventEndpoints(eventName, owner, repo, payload) { return null; } return { - reactionEndpoint: `/repos/${owner}/${repo}/issues/comments/${commentId}/reactions`, + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", params: { owner, repo, comment_id: commentId } }, // Create new comment on the issue itself, not on the comment - commentUpdateEndpoint: `/repos/${owner}/${repo}/issues/${issueNumber}/comments`, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: issueNumber } }, }; } @@ -76,8 +101,8 @@ async function resolveEventEndpoints(eventName, owner, repo, payload) { } // PRs are "issues" for the reactions endpoint return { - reactionEndpoint: `/repos/${owner}/${repo}/issues/${prNumber}/reactions`, - commentUpdateEndpoint: `/repos/${owner}/${repo}/issues/${prNumber}/comments`, + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner, repo, issue_number: prNumber } }, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: prNumber } }, }; } @@ -93,9 +118,9 @@ async function resolveEventEndpoints(eventName, owner, repo, payload) { return null; } return { - reactionEndpoint: `/repos/${owner}/${repo}/pulls/comments/${reviewCommentId}/reactions`, + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", params: { owner, repo, comment_id: reviewCommentId } }, // Create new comment on the PR itself (using issues endpoint since PRs are issues) - commentUpdateEndpoint: `/repos/${owner}/${repo}/issues/${prNumber}/comments`, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: prNumber } }, }; } @@ -171,16 +196,20 @@ async function main() { const { reactionEndpoint, commentUpdateEndpoint } = endpoints; - core.info(`Reaction API endpoint: ${reactionEndpoint}`); + core.info(`Reaction API endpoint: ${typeof reactionEndpoint === "object" ? reactionEndpoint.route : reactionEndpoint}`); - // For discussions, reactionEndpoint is a node ID (GraphQL), otherwise it's a REST API path + // For discussions, reactionEndpoint is a node ID (GraphQL), otherwise it's a typed REST route if (eventName === "discussion" || eventName === "discussion_comment") { + if (typeof reactionEndpoint !== "string") { + throw new Error(`${ERR_VALIDATION}: Unexpected reaction endpoint shape for event: ${eventName}`); + } await addDiscussionReaction(reactionEndpoint, reaction); } else { - await addReaction(reactionEndpoint, reaction); + const { route, params } = expectRestEndpoint(reactionEndpoint, "reaction", eventName); + await addReaction(route, params, reaction); } - core.info(`Comment endpoint: ${commentUpdateEndpoint}`); + core.info(`Comment endpoint: ${typeof commentUpdateEndpoint === "object" ? commentUpdateEndpoint.route : commentUpdateEndpoint}`); await addCommentWithWorkflowLink(commentUpdateEndpoint, runUrl, eventName, invocationContext); } catch (error) { if (isLockedError(error)) { @@ -211,7 +240,7 @@ function setCommentOutputs(commentId, commentUrl, eventRepo = context.repo) { /** * Add a comment with a workflow run link - * @param {string} endpoint - The GitHub API endpoint to create the comment (or special format for discussions) + * @param {{ route: string, params: Record } | string} endpoint - Typed route info for REST events, or special format string for discussions * @param {string} runUrl - The URL of the workflow run * @param {string} eventName - The event type (to determine the comment text) * @param {{ @@ -251,6 +280,9 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName, invocatio const commentBody = commentParts.join("\n\n"); if (eventName === "discussion" || eventName === "discussion_comment") { + if (typeof endpoint !== "string") { + throw new Error(`${ERR_VALIDATION}: Unexpected comment endpoint shape for event: ${eventName}`); + } // Parse discussion number from special format: "discussion:NUMBER" or "discussion_comment:NUMBER:COMMENT_ID" const discussionNumber = parseInt(endpoint.split(":")[1], 10); const discussionId = await getDiscussionNodeId(eventRepo.owner, eventRepo.repo, discussionNumber); @@ -262,8 +294,10 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName, invocatio return; } - // Create a new comment for non-discussion events - const createResponse = await github.request(`POST ${endpoint}`, { + // Create a new comment for non-discussion events using typed route + const { route, params } = expectRestEndpoint(endpoint, "comment", eventName); + const createResponse = await github.request(route, { + ...params, body: commentBody, headers: { Accept: "application/vnd.github+json", @@ -278,4 +312,4 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName, invocatio } } -module.exports = { main, addCommentWithWorkflowLink, resolveEventEndpoints, VALID_REACTIONS, addReaction, addDiscussionReaction }; +module.exports = { main, addCommentWithWorkflowLink, resolveEventEndpoints, VALID_REACTIONS, addReaction, addDiscussionReaction, expectRestEndpoint }; diff --git a/actions/setup/js/add_reaction_and_edit_comment.test.cjs b/actions/setup/js/add_reaction_and_edit_comment.test.cjs index 2e967171a7c..3051bf2629c 100644 --- a/actions/setup/js/add_reaction_and_edit_comment.test.cjs +++ b/actions/setup/js/add_reaction_and_edit_comment.test.cjs @@ -38,8 +38,8 @@ global.context = mockContext; // Helper to import the module fresh (bust module cache) async function loadModule() { - const { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction, resolveEventEndpoints, VALID_REACTIONS } = await import("./add_reaction_and_edit_comment.cjs?" + Date.now()); - return { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction, resolveEventEndpoints, VALID_REACTIONS }; + const { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction, resolveEventEndpoints, VALID_REACTIONS, expectRestEndpoint } = await import("./add_reaction_and_edit_comment.cjs?" + Date.now()); + return { main, addCommentWithWorkflowLink, addReaction, addDiscussionReaction, resolveEventEndpoints, VALID_REACTIONS, expectRestEndpoint }; } describe("add_reaction_and_edit_comment.cjs", () => { @@ -81,10 +81,18 @@ describe("add_reaction_and_edit_comment.cjs", () => { const { main } = await loadModule(); await main(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/123/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", issue_number: 123 })); expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", "456"); }); + describe("Endpoint validation", () => { + it("should throw a validation error when a REST endpoint is unexpectedly a string", async () => { + const { expectRestEndpoint } = await loadModule(); + + expect(() => expectRestEndpoint("discussion:12", "reaction", "issues")).toThrow(`${ERR_VALIDATION}: Unexpected reaction endpoint shape for event: issues`); + }); + }); + it("should default to 'eyes' reaction when GH_AW_REACTION is not set", async () => { // GH_AW_REACTION is deleted in beforeEach global.context.eventName = "issues"; @@ -94,7 +102,7 @@ describe("add_reaction_and_edit_comment.cjs", () => { const { main } = await loadModule(); await main(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/123/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", issue_number: 123 })); }); it("should reject invalid reaction type", async () => { @@ -135,8 +143,8 @@ describe("add_reaction_and_edit_comment.cjs", () => { const { main } = await loadModule(); await main(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/456/reactions", expect.objectContaining({ content: "heart" })); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/456/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this pull request") })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", expect.objectContaining({ content: "heart", owner: "testowner", repo: "testrepo", issue_number: 456 })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this pull request") })); expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", "789"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-id", "999"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-url", "https://github.com/testowner/testrepo/pull/456#issuecomment-999"); @@ -169,7 +177,7 @@ describe("add_reaction_and_edit_comment.cjs", () => { const { main } = await loadModule(); await main(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/123/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this issue comment") })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this issue comment") })); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-id", "789"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-url", "https://github.com/testowner/testrepo/issues/123#issuecomment-789"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-repo", "testowner/testrepo"); @@ -208,8 +216,8 @@ describe("add_reaction_and_edit_comment.cjs", () => { const { main } = await loadModule(); await main(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/targetowner/targetrepo/issues/comments/456/reactions", expect.objectContaining({ content: "eyes" })); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/targetowner/targetrepo/issues/123/comments", expect.objectContaining({ body: expect.stringContaining("https://github.com/sideowner/siderepo/actions/runs/12345") })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", expect.objectContaining({ content: "eyes", owner: "targetowner", repo: "targetrepo", comment_id: 456 })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("https://github.com/sideowner/siderepo/actions/runs/12345") })); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-repo", "targetowner/targetrepo"); }); }); @@ -229,7 +237,7 @@ describe("add_reaction_and_edit_comment.cjs", () => { const { main } = await loadModule(); await main(); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/456/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this pull request review comment") })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this pull request review comment") })); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-id", "999"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-url", "https://github.com/testowner/testrepo/pull/456#discussion_r999"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-repo", "testowner/testrepo"); @@ -472,7 +480,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 123, html_url: "https://example.com" } }); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "issues" + ); expect(mockGithub.request).toHaveBeenCalledWith(expect.stringContaining("POST"), expect.objectContaining({ body: expect.stringContaining("") })); }); @@ -482,7 +494,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 123, html_url: "https://example.com" } }); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "issues" + ); expect(mockGithub.request).toHaveBeenCalledWith(expect.stringContaining("POST"), expect.objectContaining({ body: expect.stringContaining("") })); }); @@ -491,7 +507,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 123, html_url: "https://example.com" } }); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "issues" + ); expect(mockGithub.request).toHaveBeenCalledWith(expect.stringContaining("POST"), expect.objectContaining({ body: expect.stringContaining("") })); }); @@ -501,7 +521,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 123, html_url: "https://example.com" } }); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "issues" + ); expect(mockGithub.request).toHaveBeenCalledWith(expect.stringContaining("POST"), expect.objectContaining({ body: expect.stringContaining("🔒 This issue has been locked") })); }); @@ -511,7 +535,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 123, html_url: "https://example.com" } }); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issue_comment"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "issue_comment" + ); expect(mockGithub.request).toHaveBeenCalledWith(expect.stringContaining("POST"), expect.objectContaining({ body: expect.stringContaining("🔒 This issue has been locked") })); }); @@ -535,7 +563,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 123, html_url: "https://example.com" } }); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "pull_request"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "pull_request" + ); expect(mockGithub.request).toHaveBeenCalledWith(expect.stringContaining("POST"), expect.objectContaining({ body: expect.not.stringContaining("🔒 This issue has been locked") })); }); @@ -582,7 +614,11 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockRejectedValueOnce(new Error("Network failure")); const { addCommentWithWorkflowLink } = await loadModule(); - await addCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await addCommentWithWorkflowLink( + { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }, + "https://github.com/testowner/testrepo/actions/runs/12345", + "issues" + ); expect(mockCore.warning).toHaveBeenCalledWith(expect.stringContaining("Failed to create comment with workflow link")); expect(mockCore.setFailed).not.toHaveBeenCalled(); @@ -594,9 +630,9 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: { id: 789 } }); const { addReaction } = await loadModule(); - await addReaction("/repos/testowner/testrepo/issues/123/reactions", "eyes"); + await addReaction("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { owner: "testowner", repo: "testrepo", issue_number: 123 }, "eyes"); - expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/testowner/testrepo/issues/123/reactions", expect.objectContaining({ content: "eyes" })); + expect(mockGithub.request).toHaveBeenCalledWith("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", expect.objectContaining({ content: "eyes", owner: "testowner", repo: "testrepo", issue_number: 123 })); expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", "789"); }); @@ -604,7 +640,7 @@ describe("add_reaction_and_edit_comment.cjs", () => { mockGithub.request.mockResolvedValueOnce({ data: {} }); const { addReaction } = await loadModule(); - await addReaction("/repos/testowner/testrepo/issues/123/reactions", "eyes"); + await addReaction("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { owner: "testowner", repo: "testrepo", issue_number: 123 }, "eyes"); expect(mockCore.setOutput).toHaveBeenCalledWith("reaction-id", ""); }); @@ -623,8 +659,8 @@ describe("add_reaction_and_edit_comment.cjs", () => { const payload = { issue: { number: 42 } }; const result = await resolveEventEndpoints("issues", "owner", "repo", payload); expect(result).toEqual({ - reactionEndpoint: "/repos/owner/repo/issues/42/reactions", - commentUpdateEndpoint: "/repos/owner/repo/issues/42/comments", + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner: "owner", repo: "repo", issue_number: 42 } }, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "owner", repo: "repo", issue_number: 42 } }, }); }); @@ -640,8 +676,8 @@ describe("add_reaction_and_edit_comment.cjs", () => { const payload = { pull_request: { number: 7 } }; const result = await resolveEventEndpoints("pull_request", "owner", "repo", payload); expect(result).toEqual({ - reactionEndpoint: "/repos/owner/repo/issues/7/reactions", - commentUpdateEndpoint: "/repos/owner/repo/issues/7/comments", + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", params: { owner: "owner", repo: "repo", issue_number: 7 } }, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "owner", repo: "repo", issue_number: 7 } }, }); }); @@ -650,8 +686,8 @@ describe("add_reaction_and_edit_comment.cjs", () => { const payload = { comment: { id: 55 }, issue: { number: 10 } }; const result = await resolveEventEndpoints("issue_comment", "owner", "repo", payload); expect(result).toEqual({ - reactionEndpoint: "/repos/owner/repo/issues/comments/55/reactions", - commentUpdateEndpoint: "/repos/owner/repo/issues/10/comments", + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", params: { owner: "owner", repo: "repo", comment_id: 55 } }, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "owner", repo: "repo", issue_number: 10 } }, }); }); @@ -660,8 +696,8 @@ describe("add_reaction_and_edit_comment.cjs", () => { const payload = { comment: { id: 99 }, pull_request: { number: 3 } }; const result = await resolveEventEndpoints("pull_request_review_comment", "owner", "repo", payload); expect(result).toEqual({ - reactionEndpoint: "/repos/owner/repo/pulls/comments/99/reactions", - commentUpdateEndpoint: "/repos/owner/repo/issues/3/comments", + reactionEndpoint: { route: "POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", params: { owner: "owner", repo: "repo", comment_id: 99 } }, + commentUpdateEndpoint: { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "owner", repo: "repo", issue_number: 3 } }, }); }); diff --git a/actions/setup/js/add_workflow_run_comment.cjs b/actions/setup/js/add_workflow_run_comment.cjs index b1e46637e81..7152ef36a8f 100644 --- a/actions/setup/js/add_workflow_run_comment.cjs +++ b/actions/setup/js/add_workflow_run_comment.cjs @@ -12,6 +12,14 @@ const { buildWorkflowRunUrl } = require("./workflow_metadata_helpers.cjs"); const { resolveTopLevelDiscussionCommentId } = require("./github_api_helpers.cjs"); const { resolveInvocationContext } = require("./invocation_context_helpers.cjs"); +/** + * @param {unknown} endpoint + * @returns {endpoint is { route: string, params: Record }} + */ +function isRestEndpoint(endpoint) { + return typeof endpoint === "object" && endpoint !== null && "route" in endpoint && "params" in endpoint; +} + /** * @typedef {{ owner: string, repo: string }} RepoRef * @typedef {{ id: string, url: string, repo: RepoRef }} CommentMetadata @@ -285,7 +293,7 @@ async function createOrReuseStatusComment(rawContext = context) { reportCommentError(rawContext, `${ERR_NOT_FOUND}: Issue number not found in event payload`); return null; } - commentEndpoint = `/repos/${owner}/${repo}/issues/${number}/comments`; + commentEndpoint = { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: number } }; break; } @@ -298,7 +306,7 @@ async function createOrReuseStatusComment(rawContext = context) { reportCommentError(rawContext, `${ERR_NOT_FOUND}: Pull request number not found in event payload`); return null; } - commentEndpoint = `/repos/${owner}/${repo}/issues/${number}/comments`; + commentEndpoint = { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner, repo, issue_number: number } }; break; } @@ -328,7 +336,7 @@ async function createOrReuseStatusComment(rawContext = context) { return null; } - core.info(`Creating comment on: ${commentEndpoint}`); + core.info(`Creating comment on: ${typeof commentEndpoint === "object" ? commentEndpoint.route : commentEndpoint}`); return addCommentWithWorkflowLink(commentEndpoint, runUrl, eventName, invocationContext); } @@ -409,7 +417,7 @@ async function postDiscussionComment(discussionNumber, commentBody, replyToNodeI /** * Add a comment with a workflow run link - * @param {string} endpoint - The GitHub API endpoint to create the comment (or special format for discussions) + * @param {{ route: string, params: Record } | string} endpoint - Typed route info for REST events, or special format string for discussions ("discussion:NUMBER" or "discussion_comment:NUMBER:COMMENT_ID") * @param {string} runUrl - The URL of the workflow run * @param {string} eventName - The event type (to determine the comment text) * @param {{ @@ -426,12 +434,18 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName, invocatio const commentBody = buildCommentBody(eventName, runUrl); if (eventName === "discussion") { + if (typeof endpoint !== "string") { + throw new Error(`${ERR_VALIDATION}: Unexpected comment endpoint shape for event: ${eventName}`); + } // Parse discussion number from special format: "discussion:NUMBER" const discussionNumber = parseInt(endpoint.split(":")[1], 10); return postDiscussionComment(discussionNumber, commentBody, null, eventRepo); } if (eventName === "discussion_comment") { + if (typeof endpoint !== "string") { + throw new Error(`${ERR_VALIDATION}: Unexpected comment endpoint shape for event: ${eventName}`); + } // Parse discussion number from special format: "discussion_comment:NUMBER:COMMENT_ID" const discussionNumber = parseInt(endpoint.split(":")[1], 10); @@ -440,8 +454,13 @@ async function addCommentWithWorkflowLink(endpoint, runUrl, eventName, invocatio return postDiscussionComment(discussionNumber, commentBody, commentNodeId, eventRepo); } - // Create a new comment for non-discussion events - const createResponse = await github.request("POST " + endpoint, { + // Create a new comment for non-discussion events using typed route + if (!isRestEndpoint(endpoint)) { + throw new Error(`${ERR_VALIDATION}: Unexpected comment endpoint shape for event: ${eventName}`); + } + const { route, params } = endpoint; + const createResponse = await github.request(route, { + ...params, body: commentBody, headers: { Accept: "application/vnd.github+json" }, }); diff --git a/actions/setup/js/add_workflow_run_comment.test.cjs b/actions/setup/js/add_workflow_run_comment.test.cjs index de9f68cc070..43874447d31 100644 --- a/actions/setup/js/add_workflow_run_comment.test.cjs +++ b/actions/setup/js/add_workflow_run_comment.test.cjs @@ -118,6 +118,10 @@ describe("add_workflow_run_comment", () => { await addCommentWithWorkflowLink(endpoint, runUrl, eventName); } + // Typed route endpoint used across addCommentWithWorkflowLink tests + const issuesCommentEndpoint123 = { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 123 } }; + const issuesCommentEndpoint101 = { route: "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", params: { owner: "testowner", repo: "testrepo", issue_number: 101 } }; + describe("main() - issues event", () => { it("should create comment on an issue", async () => { global.context = { @@ -133,7 +137,7 @@ describe("add_workflow_run_comment", () => { await runScript(); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/testowner/testrepo/issues/456/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("https://github.com/testowner/testrepo/actions/runs/12345"), }) @@ -174,7 +178,7 @@ describe("add_workflow_run_comment", () => { await runScript(); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/testowner/testrepo/issues/789/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.any(String), }) @@ -201,7 +205,7 @@ describe("add_workflow_run_comment", () => { await runScript(); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/targetowner/targetrepo/issues/789/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("https://github.com/sideowner/siderepo/actions/runs/12345"), }) @@ -544,7 +548,7 @@ describe("add_workflow_run_comment", () => { expect(result?.id).toBe("67890"); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/targetowner/targetrepo/issues/101/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this pull request comment"), }) @@ -596,7 +600,7 @@ describe("add_workflow_run_comment", () => { await runScript(); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/testowner/testrepo/issues/101/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.any(String), }) @@ -634,7 +638,7 @@ describe("add_workflow_run_comment", () => { await runScript(); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/testowner/testrepo/issues/202/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.any(String), }) @@ -672,7 +676,7 @@ describe("add_workflow_run_comment", () => { await runScript(); expect(mockGithub.request).toHaveBeenCalledWith( - expect.stringContaining("POST /repos/testowner/testrepo/issues/303/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.any(String), }) @@ -815,7 +819,7 @@ describe("add_workflow_run_comment", () => { it("should include workflow-id marker when GITHUB_WORKFLOW is set", async () => { process.env.GITHUB_WORKFLOW = "test-workflow.yml"; - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), @@ -828,7 +832,7 @@ describe("add_workflow_run_comment", () => { it("should include tracker-id marker when GH_AW_TRACKER_ID is set", async () => { process.env.GH_AW_TRACKER_ID = "tracker-123"; - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), @@ -839,7 +843,7 @@ describe("add_workflow_run_comment", () => { }); it("should always include reaction comment type marker", async () => { - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), @@ -854,7 +858,7 @@ describe("add_workflow_run_comment", () => { it("should add lock notice for issues event when GH_AW_LOCK_FOR_AGENT=true", async () => { process.env.GH_AW_LOCK_FOR_AGENT = "true"; - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), @@ -867,7 +871,7 @@ describe("add_workflow_run_comment", () => { it("should not add lock notice for pull_request events", async () => { process.env.GH_AW_LOCK_FOR_AGENT = "true"; - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/101/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "pull_request"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint101, "https://github.com/testowner/testrepo/actions/runs/12345", "pull_request"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), @@ -880,7 +884,7 @@ describe("add_workflow_run_comment", () => { describe("addCommentWithWorkflowLink() - outputs", () => { it("should set all required outputs (comment-id, comment-url, comment-repo)", async () => { - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-id", "67890"); expect(mockCore.setOutput).toHaveBeenCalledWith("comment-url", expect.stringContaining("issuecomment-67890")); @@ -928,7 +932,7 @@ describe("add_workflow_run_comment", () => { it("should use GH_AW_WORKFLOW_NAME in the comment body", async () => { process.env.GH_AW_WORKFLOW_NAME = "My Custom Workflow"; - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), @@ -941,7 +945,7 @@ describe("add_workflow_run_comment", () => { it("should fall back to GITHUB_WORKFLOW when GH_AW_WORKFLOW_NAME is not set", async () => { process.env.GITHUB_WORKFLOW = "Agentic Commands"; - await runAddCommentWithWorkflowLink("/repos/testowner/testrepo/issues/123/comments", "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); + await runAddCommentWithWorkflowLink(issuesCommentEndpoint123, "https://github.com/testowner/testrepo/actions/runs/12345", "issues"); expect(mockGithub.request).toHaveBeenCalledWith( expect.stringContaining("POST"), diff --git a/actions/setup/js/artifact_client.cjs b/actions/setup/js/artifact_client.cjs index ebeb9dd872d..16c5206d307 100644 --- a/actions/setup/js/artifact_client.cjs +++ b/actions/setup/js/artifact_client.cjs @@ -28,6 +28,14 @@ function sleep(ms) { return new Promise(resolve => setTimeout(resolve, ms)); } +function parseURL(url, base, errorMessage) { + try { + return base === undefined ? new URL(url) : new URL(url, base); + } catch { + throw new Error(errorMessage); + } +} + function decodeJWTPayload(token) { const parts = String(token || "").split("."); if (parts.length < 2 || !parts[1]) { @@ -70,7 +78,7 @@ function getResultsServiceOrigin() { if (!url) { throw new Error("ACTIONS_RESULTS_URL is required for artifact upload"); } - return new URL(url).origin; + return parseURL(url, undefined, `ACTIONS_RESULTS_URL is not a valid URL: ${url}`).origin; } async function twirpRequest(method, body) { @@ -78,7 +86,8 @@ async function twirpRequest(method, body) { if (!runtimeToken) { throw new Error("ACTIONS_RUNTIME_TOKEN is required for artifact upload"); } - const url = new URL(`/twirp/${TWIRP_ARTIFACT_SERVICE}/${method}`, getResultsServiceOrigin()).toString(); + const resultsServiceOrigin = getResultsServiceOrigin(); + const url = parseURL(`/twirp/${TWIRP_ARTIFACT_SERVICE}/${method}`, resultsServiceOrigin, `Failed to construct twirp URL for method: ${method}`).toString(); let lastError; for (let attempt = 1; attempt <= DEFAULT_RETRY_ATTEMPTS; attempt++) { @@ -145,7 +154,7 @@ function isZipResponse(url, contentType) { return true; } try { - return new URL(url).pathname.toLowerCase().endsWith(".zip"); + return parseURL(url, undefined, `Invalid URL for zip detection: ${url}`).pathname.toLowerCase().endsWith(".zip"); } catch { return false; } @@ -249,7 +258,7 @@ class DefaultArtifactClient { let page = 1; const maxPages = Math.ceil(MAX_ARTIFACTS / PAGE_SIZE); for (; page <= maxPages; page++) { - const url = new URL(`/repos/${findBy.repositoryOwner}/${findBy.repositoryName}/actions/runs/${findBy.workflowRunId}/artifacts`, serverUrl); + const url = parseURL(`/repos/${findBy.repositoryOwner}/${findBy.repositoryName}/actions/runs/${findBy.workflowRunId}/artifacts`, serverUrl, `Failed to construct artifacts URL for run ${findBy.workflowRunId}`); url.searchParams.set("per_page", String(PAGE_SIZE)); url.searchParams.set("page", String(page)); const response = await fetch(url.toString(), { @@ -293,7 +302,11 @@ class DefaultArtifactClient { const destination = options.path || process.env.GITHUB_WORKSPACE || process.cwd(); fs.mkdirSync(destination, { recursive: true }); - const apiUrl = new URL(`/repos/${findBy.repositoryOwner}/${findBy.repositoryName}/actions/artifacts/${artifactId}/zip`, process.env.GITHUB_API_URL || "https://api.github.com"); + const apiUrl = parseURL( + `/repos/${findBy.repositoryOwner}/${findBy.repositoryName}/actions/artifacts/${artifactId}/zip`, + process.env.GITHUB_API_URL || "https://api.github.com", + `Failed to construct download URL for artifact ${artifactId}` + ); const redirectResponse = await fetch(apiUrl.toString(), { headers: { Authorization: "Bearer " + findBy.token, diff --git a/actions/setup/js/check_team_member.cjs b/actions/setup/js/check_team_member.cjs index 89e5a91ff61..aa0576b14ab 100644 --- a/actions/setup/js/check_team_member.cjs +++ b/actions/setup/js/check_team_member.cjs @@ -36,8 +36,9 @@ async function main() { // Fail the workflow when team membership check fails (cancellation handled by activation job's if condition) core.warning(`Access denied: Only authorized team members can trigger this workflow. User '${actor}' is not authorized.`); - core.setFailed(`${ERR_PERMISSION}: Access denied: User '${actor}' is not authorized for this workflow`); core.setOutput("is_team_member", "false"); + core.setFailed(`${ERR_PERMISSION}: Access denied: User '${actor}' is not authorized for this workflow`); + return; } module.exports = { main }; diff --git a/actions/setup/js/generate_history_link.cjs b/actions/setup/js/generate_history_link.cjs index 420ebc6fece..eb5700990db 100644 --- a/actions/setup/js/generate_history_link.cjs +++ b/actions/setup/js/generate_history_link.cjs @@ -58,7 +58,13 @@ function generateHistoryUrl({ owner, repo, itemType, workflowCallId, workflowId, queryParts.push(`"${markerId}"`); - const url = new URL(`${server}/search`); + const url = (() => { + try { + return new URL(`${server}/search`); + } catch { + throw new Error(`Invalid server URL: ${server}`); + } + })(); url.searchParams.set("q", queryParts.join(" ")); // Set the type parameter based on itemType for correct GitHub search filtering diff --git a/actions/setup/js/mcp_cli_bridge.cjs b/actions/setup/js/mcp_cli_bridge.cjs index ab5dbf5da6d..9d56d00f17c 100644 --- a/actions/setup/js/mcp_cli_bridge.cjs +++ b/actions/setup/js/mcp_cli_bridge.cjs @@ -156,7 +156,13 @@ function writeStdoutAndFlush(data) { */ function httpPostJSON(urlStr, headers, body, timeoutMs = DEFAULT_HTTP_TIMEOUT_MS) { return new Promise((resolve, reject) => { - const parsedUrl = new URL(urlStr); + let parsedUrl; + try { + parsedUrl = new URL(urlStr); + } catch { + reject(new Error(`Invalid URL: ${urlStr}`)); + return; + } const bodyStr = JSON.stringify(body); const options = { diff --git a/actions/setup/js/mount_mcp_as_cli.cjs b/actions/setup/js/mount_mcp_as_cli.cjs index 39d5a440cac..780a4e44acc 100644 --- a/actions/setup/js/mount_mcp_as_cli.cjs +++ b/actions/setup/js/mount_mcp_as_cli.cjs @@ -153,7 +153,13 @@ function toContainerUrl(rawUrl) { */ function httpPostJSON(urlStr, headers, body, timeoutMs = DEFAULT_HTTP_TIMEOUT_MS) { return new Promise((resolve, reject) => { - const parsedUrl = new URL(urlStr); + let parsedUrl; + try { + parsedUrl = new URL(urlStr); + } catch { + reject(new Error(`Invalid URL: ${urlStr}`)); + return; + } const bodyStr = JSON.stringify(body); const options = { diff --git a/actions/setup/js/route_slash_command.cjs b/actions/setup/js/route_slash_command.cjs index 3e56c59f3ea..49057864a3e 100644 --- a/actions/setup/js/route_slash_command.cjs +++ b/actions/setup/js/route_slash_command.cjs @@ -132,15 +132,29 @@ async function resolveDispatchRef() { return normalizeDispatchRef(defaultBranch); } +/** @typedef {"+1" | "-1" | "confused" | "eyes" | "heart" | "hooray" | "laugh" | "rocket"} ReactionName */ + +/** + * @param {string} value + * @returns {value is ReactionName} + */ +function isReactionName(value) { + return Object.hasOwn(REACTION_MAP, value); +} + +/** + * @param {unknown} reaction + * @returns {ReactionName | ""} + */ function normalizeReaction(reaction) { if (typeof reaction !== "string") { return ""; } const trimmed = reaction.trim(); - if (!trimmed || trimmed === "none" || !Object.hasOwn(REACTION_MAP, trimmed)) { + if (!trimmed || trimmed === "none") { return ""; } - return trimmed; + return isReactionName(trimmed) ? trimmed : ""; } function maintainsStatusComment(route) { @@ -150,7 +164,7 @@ function maintainsStatusComment(route) { /** * Returns the first valid non-"none" ai_reaction configured on matching routes. * @param {Array<{ai_reaction?: unknown}>} routes - * @returns {string} + * @returns {ReactionName | ""} */ function resolveImmediateReaction(routes) { for (const route of routes) { @@ -177,7 +191,10 @@ async function addImmediateReaction(reaction) { core.warning("Skipping immediate reaction: issue number was not found in payload."); return; } - await github.request(`POST /repos/${owner}/${repo}/issues/${issueNumber}/reactions`, { + await github.request("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { + owner, + repo, + issue_number: issueNumber, content: normalized, headers: { Accept: "application/vnd.github+json" }, }); @@ -189,7 +206,10 @@ async function addImmediateReaction(reaction) { core.warning("Skipping immediate reaction: comment id was not found in payload."); return; } - await github.request(`POST /repos/${owner}/${repo}/issues/comments/${commentId}/reactions`, { + await github.request("POST /repos/{owner}/{repo}/issues/comments/{comment_id}/reactions", { + owner, + repo, + comment_id: commentId, content: normalized, headers: { Accept: "application/vnd.github+json" }, }); @@ -201,7 +221,10 @@ async function addImmediateReaction(reaction) { core.warning("Skipping immediate reaction: pull request number was not found in payload."); return; } - await github.request(`POST /repos/${owner}/${repo}/issues/${prNumber}/reactions`, { + await github.request("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions", { + owner, + repo, + issue_number: prNumber, content: normalized, headers: { Accept: "application/vnd.github+json" }, }); @@ -213,7 +236,10 @@ async function addImmediateReaction(reaction) { core.warning("Skipping immediate reaction: review comment id was not found in payload."); return; } - await github.request(`POST /repos/${owner}/${repo}/pulls/comments/${reviewCommentId}/reactions`, { + await github.request("POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions", { + owner, + repo, + comment_id: reviewCommentId, content: normalized, headers: { Accept: "application/vnd.github+json" }, }); diff --git a/actions/setup/js/route_slash_command.test.cjs b/actions/setup/js/route_slash_command.test.cjs index a6c09ccbdbb..8e75a2ca072 100644 --- a/actions/setup/js/route_slash_command.test.cjs +++ b/actions/setup/js/route_slash_command.test.cjs @@ -245,12 +245,12 @@ describe("route_slash_command", () => { expect(awContext.status_comment_repo).toBe("github/gh-aw"); expect(JSON.parse(dispatchCalls[1].inputs.aw_context).status_comment_id).toBe("999"); expect(globals.github.request.mock.calls.filter(([route]) => String(route).includes("/reactions"))).toHaveLength(1); - expect(globals.github.request.mock.calls.filter(([route]) => /\/issues\/77\/comments$/.test(String(route)))).toHaveLength(1); + expect(globals.github.request.mock.calls.filter(([route]) => route === "POST /repos/{owner}/{repo}/issues/{issue_number}/comments")).toHaveLength(1); const statusUpdateCalls = globals.github.request.mock.calls.filter(([route]) => String(route).startsWith("PATCH /repos/{owner}/{repo}/issues/comments/{comment_id}")); expect(statusUpdateCalls.length).toBeGreaterThan(0); expect(statusUpdateCalls[0][1].body).toContain("[archie](https://github.com/github/gh-aw/actions/runs/444)"); expect(globals.github.request).toHaveBeenCalledWith( - expect.stringContaining("/issues/77/comments"), + "POST /repos/{owner}/{repo}/issues/{issue_number}/comments", expect.objectContaining({ body: expect.stringContaining("has started processing this issue comment"), }) @@ -518,7 +518,7 @@ describe("route_slash_command", () => { await main(); expect(dispatchCalls).toHaveLength(1); expect(reactionCalls).toHaveLength(1); - expect(reactionCalls[0][0]).toBe("POST /repos/github/gh-aw/issues/42/reactions"); + expect(reactionCalls[0][0]).toBe("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions"); }); it("adds immediate reaction for pull_request events using PR number", async () => { @@ -530,7 +530,7 @@ describe("route_slash_command", () => { await main(); expect(dispatchCalls).toHaveLength(1); expect(reactionCalls).toHaveLength(1); - expect(reactionCalls[0][0]).toBe("POST /repos/github/gh-aw/issues/7/reactions"); + expect(reactionCalls[0][0]).toBe("POST /repos/{owner}/{repo}/issues/{issue_number}/reactions"); }); it("adds immediate reaction for pull_request_review_comment events using comment id", async () => { @@ -542,7 +542,7 @@ describe("route_slash_command", () => { await main(); expect(dispatchCalls).toHaveLength(1); expect(reactionCalls).toHaveLength(1); - expect(reactionCalls[0][0]).toBe("POST /repos/github/gh-aw/pulls/comments/99/reactions"); + expect(reactionCalls[0][0]).toBe("POST /repos/{owner}/{repo}/pulls/comments/{comment_id}/reactions"); }); it("adds immediate reaction for discussion_comment events using node_id", async () => { diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 719cd9e10b3..3164ca7ab1f 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -1457,6 +1457,8 @@ async function main() { // Detect staged mode before try/finally so it's accessible in the finally block. // In staged mode (🎭 Staged Mode Preview) no real items are created in GitHub so no manifest should be emitted. const isStaged = isStagedMode(); + /** @type {string | null} */ + let failedOutputsMessage = null; try { core.info("Safe Output Handler Manager starting..."); @@ -1641,7 +1643,7 @@ async function main() { core.warning(`${failureCount} message(s) failed to process`); const failedItemLines = fatalFailures.map(r => ` - ${r.type}: ${r.error || "Unknown error"}`); const failedItems = failedItemLines.join("\n"); - core.setFailed(`${failureCount} safe output(s) failed:\n${failedItems}`); + failedOutputsMessage = `${failureCount} safe output(s) failed:\n${failedItems}`; } if (reportOnlyFailureCount > 0) { const reportOnlyTypes = [...new Set(reportOnlyFailures.map(r => r.type || "unknown"))]; @@ -1739,9 +1741,18 @@ async function main() { // so this is a safety net for cases where we never reached the logger creation. if (!isStaged) ensureManifestExists(); + if (failedOutputsMessage !== null) { + core.setFailed(failedOutputsMessage); + return; + } core.info("Safe Output Handler Manager completed"); } catch (error) { - core.setFailed(`${ERR_VALIDATION}: Handler manager failed: ${getErrorMessage(error)}`); + const handlerError = `${ERR_VALIDATION}: Handler manager failed: ${getErrorMessage(error)}`; + if (failedOutputsMessage !== null) { + core.setFailed(`${failedOutputsMessage}\n${handlerError}`); + return; + } + core.setFailed(handlerError); } finally { // Guarantee the manifest file exists for artifact upload even when the handler fails. // This is a no-op if the file was already created by createManifestLogger().