Skip to content

Harden create_issue concurrency limits and remove dead copilot assignment queue code #27533

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
pelikhan merged 4 commits into from
Apr 21, 2026
+36 −99
Merged

Harden create_issue concurrency limits and remove dead copilot assignment queue code #27533

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
02ac899
Initial plan
Copilot Apr 21, 2026
b1ff372
fix: make create_issue max-count reservation concurrency-safe
Copilot Apr 21, 2026
3f1f518
docs: clarify synchronous max-slot reservation wording
Copilot Apr 21, 2026
f5563aa
refactor: remove dead create_issue copilot queue compatibility code
Copilot Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 0 additions & 6 deletions actions/setup/js/create_discussion_fallback.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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: {
Expand Down Expand Up @@ -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: {
Expand Down
63 changes: 17 additions & 46 deletions actions/setup/js/create_issue.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,36 +1,6 @@
// @ts-check
/// <reference types="@actions/github-script" />

/**
* 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<string>}
*/
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<string>} 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");
Expand Down Expand Up @@ -322,15 +292,6 @@ async function main(config = {}) {
* @returns {Promise<Object>} 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)) {
Expand Down Expand Up @@ -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 {
Expand All @@ -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}`);
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When group-by-day finds an existing issue and posts a comment, the handler returns success without recording the temporaryId -> existing issue mapping in temporaryIdMap. If a caller provides an explicit temporary_id and later messages reference it, those references will remain unresolved even though the content was associated with todayIssue. Consider storing a mapping to todayIssue.number (and repo) before returning from the grouped path.

Suggested change
core.info(`Posted content as comment ${comment.html_url} on issue #${todayIssue.number}`);
core.info(`Posted content as comment ${comment.html_url} on issue #${todayIssue.number}`);
if (temporaryId) {
temporaryIdMap.set(normalizeTemporaryId(temporaryId), {
owner: repoParts.owner,
repo: repoParts.repo,
issue_number: todayIssue.number,
});
}

Copilot uses AI. Check for mistakes.
// 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,
Expand All @@ -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) {
Expand Down Expand Up @@ -815,4 +786,4 @@ async function main(config = {}) {
};
}

module.exports = { main, createParentIssueTemplate, searchForExistingParent, getSubIssueCount, getIssuesToAssignCopilot, resetIssuesToAssignCopilot };
module.exports = { main, createParentIssueTemplate, searchForExistingParent, getSubIssueCount };
50 changes: 19 additions & 31 deletions actions/setup/js/create_issue.test.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -15,9 +15,6 @@ describe("create_issue", () => {
// Save original environment
originalEnv = { ...process.env };

// Reset copilot assignment tracking
resetIssuesToAssignCopilot();

// Mock GitHub API
mockGithub = {
rest: {
Expand Down Expand Up @@ -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"]));
});
});

Expand All @@ -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", () => {
Expand Down
16 changes: 0 additions & 16 deletions actions/setup/js/safe_output_handler_manager.cjs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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");
Expand Down Expand Up @@ -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))}`);
Expand Down Expand Up @@ -1224,16 +1218,6 @@ async function main() {
// Export processed count for consistency with project handler
core.setOutput("processed_count", successCount);

Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The workflow output issues_to_assign_copilot is no longer set here. There are still repo workflows referencing steps.process_safe_outputs.outputs.issues_to_assign_copilot (e.g. .github/workflows/*lock.yml), so leaving the output undefined may cause those steps to silently skip (or behave inconsistently across runners). Consider either continuing to set issues_to_assign_copilot to an empty string for compatibility, or updating/removing the downstream workflow steps in the same PR to match the breaking change.

Suggested change
// Preserve the legacy workflow output contract for downstream steps that still
// reference issues_to_assign_copilot, even though this handler no longer populates it.
core.setOutput("issues_to_assign_copilot", "");

Copilot uses AI. Check for mistakes.
// 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);
Copy link
Copy Markdown
Collaborator

@pelikhan pelikhan Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot is this output used anywhere?

Copy link
Copy Markdown
Contributor Author

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes — these assign_to_agent outputs are consumed downstream. They are wired by pkg/workflow/compiler_safe_outputs_job.go into safe_outputs job outputs and then referenced by workflow/error-notification paths (for example in pkg/workflow/notify_comment.go and generated lock workflows such as smoke-agent-public-approved.lock.yml, issue-monster.lock.yml, and workflow-generator.lock.yml).

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();
Expand Down
Loading