diff --git a/actions/setup/js/create_discussion_fallback.test.cjs b/actions/setup/js/create_discussion_fallback.test.cjs index 81cb74af8da..1f6a181b1b5 100644 --- a/actions/setup/js/create_discussion_fallback.test.cjs +++ b/actions/setup/js/create_discussion_fallback.test.cjs @@ -4,7 +4,6 @@ import { createRequire } from "module"; const require = createRequire(import.meta.url); const { main: createDiscussionMain } = require("./create_discussion.cjs"); -const { resetIssuesToAssignCopilot } = require("./create_issue.cjs"); describe("create_discussion fallback with close_older_discussions", () => { let mockGithub; @@ -17,9 +16,6 @@ describe("create_discussion fallback with close_older_discussions", () => { // Save original environment originalEnv = { ...process.env }; - // Reset copilot assignment tracking - resetIssuesToAssignCopilot(); - // Mock GitHub API mockGithub = { rest: { @@ -259,8 +255,6 @@ describe("create_discussion double-posting prevention", () => { beforeEach(() => { originalEnv = { ...process.env }; - resetIssuesToAssignCopilot(); - // Base mock — each test overrides graphql as needed. mockGithub = { rest: { diff --git a/actions/setup/js/create_issue.cjs b/actions/setup/js/create_issue.cjs index ed66c1f0250..637b41a989a 100644 --- a/actions/setup/js/create_issue.cjs +++ b/actions/setup/js/create_issue.cjs @@ -1,36 +1,6 @@ // @ts-check /// -/** - * Module-level storage retained for backward compatibility with the - * `assign_copilot_to_created_issues` step. The create_issue handler now assigns - * copilot inline immediately after issue creation (via assign_agent_helpers.cjs), - * so this list is never populated during normal operation and the downstream step - * is a no-op. It is exposed via getIssuesToAssignCopilot/resetIssuesToAssignCopilot - * for unit tests. - * @type {Array} - */ -let issuesToAssignCopilotGlobal = []; - -/** - * Get the list of issues that need copilot assignment. - * Returns a defensive copy so callers cannot accidentally mutate global state. - * In practice this list is always empty because assignment is now done inline. - * @returns {Array} Copy of the "repo:number" strings array - */ -function getIssuesToAssignCopilot() { - return [...issuesToAssignCopilotGlobal]; -} - -/** - * Reset the list of issues that need copilot assignment. - * Clears the internal array in-place. Previously returned snapshots (copies) - * are not affected. Used for testing. - */ -function resetIssuesToAssignCopilot() { - issuesToAssignCopilotGlobal.length = 0; -} - const { sanitizeLabelContent } = require("./sanitize_label_content.cjs"); const { sanitizeTitle, applyTitlePrefix } = require("./sanitize_title.cjs"); const { sanitizeContent } = require("./sanitize_content.cjs"); @@ -322,15 +292,6 @@ async function main(config = {}) { * @returns {Promise} Result with success/error status and issue details */ return async function handleCreateIssue(message, resolvedTemporaryIds) { - // Check if we've hit the max limit - if (processedCount >= maxCount) { - core.warning(`Skipping create_issue: max count of ${maxCount} reached`); - return { - success: false, - error: `Max count of ${maxCount} reached`, - }; - } - // Merge external resolved temp IDs with our local map if (resolvedTemporaryIds) { for (const [tempId, resolved] of Object.entries(resolvedTemporaryIds)) { @@ -529,11 +490,22 @@ async function main(config = {}) { bodyLines.push(""); const body = bodyLines.join("\n").trim(); + // Reserve a max-count slot synchronously before any async pre-creation work. + // There is no await between check and increment, so concurrent invocations + // cannot interleave between these two operations. + if (processedCount >= maxCount) { + core.warning(`Skipping create_issue: max count of ${maxCount} reached`); + return { + success: false, + error: `Max count of ${maxCount} reached`, + }; + } + processedCount++; + // Group-by-day check: if enabled, search for an existing open issue created today. // When found, post the new content as a comment on the existing issue instead of // creating a duplicate. This groups multiple same-day runs into a single issue. - // The max-count slot is NOT consumed when posting as a comment (processedCount is - // only incremented below, just before actual issue creation). + // The reserved max-count slot is released when posting as a comment. if (groupByDayEnabled && (closeOlderKey || workflowId)) { const today = new Date().toISOString().split("T")[0]; // YYYY-MM-DD (UTC) try { @@ -554,6 +526,9 @@ async function main(config = {}) { core.info(`Group-by-day: found open issue #${todayIssue.number} created today (${today}) — posting new content as a comment`); const comment = await addIssueComment(githubClient, repoParts.owner, repoParts.repo, todayIssue.number, body); core.info(`Posted content as comment ${comment.html_url} on issue #${todayIssue.number}`); + // No issue was created (content was grouped into a comment), so free + // the reserved slot for subsequent create_issue calls. + processedCount--; return { success: true, grouped: true, @@ -568,10 +543,6 @@ async function main(config = {}) { } } - // Increment processed count only when we are about to create an issue - // (group-by-day comment paths return above without consuming a slot) - processedCount++; - core.info(`Creating issue in ${qualifiedItemRepo} with title: ${title}`); core.info(`Labels: ${labels.join(", ")}`); if (assignees.length > 0) { @@ -815,4 +786,4 @@ async function main(config = {}) { }; } -module.exports = { main, createParentIssueTemplate, searchForExistingParent, getSubIssueCount, getIssuesToAssignCopilot, resetIssuesToAssignCopilot }; +module.exports = { main, createParentIssueTemplate, searchForExistingParent, getSubIssueCount }; diff --git a/actions/setup/js/create_issue.test.cjs b/actions/setup/js/create_issue.test.cjs index d936961fb19..fb46e536c80 100644 --- a/actions/setup/js/create_issue.test.cjs +++ b/actions/setup/js/create_issue.test.cjs @@ -2,7 +2,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from "vitest"; import { createRequire } from "module"; const require = createRequire(import.meta.url); -const { main, getIssuesToAssignCopilot, resetIssuesToAssignCopilot } = require("./create_issue.cjs"); +const { main } = require("./create_issue.cjs"); describe("create_issue", () => { let mockGithub; @@ -15,9 +15,6 @@ describe("create_issue", () => { // Save original environment originalEnv = { ...process.env }; - // Reset copilot assignment tracking - resetIssuesToAssignCopilot(); - // Mock GitHub API mockGithub = { rest: { @@ -257,33 +254,6 @@ describe("create_issue", () => { // Verify graphql was called three times (findAgent, getIssueDetails, assignAgentToIssue) expect(mockGithub.graphql).toHaveBeenCalledTimes(3); - // Global queue should remain empty (assignment done directly, not queued) - expect(getIssuesToAssignCopilot()).toHaveLength(0); - }); - }); - - describe("global state safety", () => { - it("getIssuesToAssignCopilot returns a copy, not the live reference", () => { - const snapshot = getIssuesToAssignCopilot(); - const lengthBefore = snapshot.length; - - // Mutating the returned copy must NOT affect internal state - snapshot.push("external:999"); - expect(getIssuesToAssignCopilot().length).toBe(lengthBefore); - }); - - it("resetIssuesToAssignCopilot clears internal state but does not affect held snapshots", () => { - const snapshot = getIssuesToAssignCopilot(); - - // Make the held copy observably different from internal state - snapshot.push("external:999"); - - resetIssuesToAssignCopilot(); - - // Fresh reads reflect cleared internal state - expect(getIssuesToAssignCopilot()).toHaveLength(0); - // Previously returned snapshots remain unchanged because getter returns a copy - expect(snapshot).toEqual(expect.arrayContaining(["external:999"])); }); }); @@ -300,6 +270,24 @@ describe("create_issue", () => { expect(result3.success).toBe(false); expect(result3.error).toContain("Max count of 2 reached"); }); + + it("should respect max count limit under concurrent calls with group-by-day pre-check", async () => { + const handler = await main({ + max: 1, + group_by_day: true, + close_older_key: "concurrency-key", + }); + + const [result1, result2] = await Promise.all([handler({ title: "Issue 1" }), handler({ title: "Issue 2" })]); + const results = [result1, result2]; + const successes = results.filter(result => result.success); + const failures = results.filter(result => !result.success); + + expect(successes).toHaveLength(1); + expect(failures).toHaveLength(1); + expect(failures[0].error).toContain("Max count of 1 reached"); + expect(mockGithub.rest.issues.create).toHaveBeenCalledTimes(1); + }); }); describe("title prefix", () => { diff --git a/actions/setup/js/safe_output_handler_manager.cjs b/actions/setup/js/safe_output_handler_manager.cjs index 1aa5cd9c504..93f376c9bc7 100644 --- a/actions/setup/js/safe_output_handler_manager.cjs +++ b/actions/setup/js/safe_output_handler_manager.cjs @@ -16,7 +16,6 @@ const { hasUnresolvedTemporaryIds, replaceTemporaryIdReferences, replaceArtifact const { generateMissingInfoSections } = require("./missing_info_formatter.cjs"); const { setCollectedMissings } = require("./missing_messages_helper.cjs"); const { writeSafeOutputSummaries } = require("./safe_output_summary.cjs"); -const { getIssuesToAssignCopilot } = require("./create_issue.cjs"); const { getAssignToAgentAssigned, getAssignToAgentErrors, getAssignToAgentErrorCount, writeAssignToAgentSummary } = require("./assign_to_agent.cjs"); const { getCreateAgentSessionNumber, getCreateAgentSessionUrl, writeCreateAgentSessionSummary } = require("./create_agent_session.cjs"); const { createReviewBuffer } = require("./pr_review_buffer.cjs"); @@ -1048,11 +1047,6 @@ async function main() { try { core.info("Safe Output Handler Manager starting..."); - // Reset create_issue handler's global state to ensure clean state for this run - // This prevents stale data accumulation if the module is reused - const { resetIssuesToAssignCopilot } = require("./create_issue.cjs"); - resetIssuesToAssignCopilot(); - // Load configuration const config = loadConfig(); core.debug(`Configuration: ${JSON.stringify(Object.keys(config))}`); @@ -1224,16 +1218,6 @@ async function main() { // Export processed count for consistency with project handler core.setOutput("processed_count", successCount); - // Export issues that need copilot assignment (if any) - const issuesToAssignCopilot = getIssuesToAssignCopilot(); - if (issuesToAssignCopilot.length > 0) { - const issuesToAssignStr = issuesToAssignCopilot.join(","); - core.setOutput("issues_to_assign_copilot", issuesToAssignStr); - core.info(`Exported ${issuesToAssignCopilot.length} issue(s) for copilot assignment: ${issuesToAssignStr}`); - } else { - core.setOutput("issues_to_assign_copilot", ""); - } - // Export assign_to_agent outputs when the handler was loaded if (messageHandlers.has("assign_to_agent")) { const assignToAgentAssigned = getAssignToAgentAssigned();