Conversation
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d625376d-a36f-47d0-867d-4070fa06b91d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/d625376d-a36f-47d0-867d-4070fa06b91d Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Copilot
AI
changed the title
[WIP] Fix concurrency safety issue in create_issue
Harden Apr 21, 2026
create_issue concurrency limits and remove dead mutable assignment queue
pelikhan
reviewed
Apr 21, 2026
| * Kept for backward-compatible tests and no-op downstream wiring. | ||
| * @returns {Array<string>} Always empty array | ||
| */ | ||
| function getIssuesToAssignCopilot() { |
Collaborator
There was a problem hiding this comment.
@copilot dead code, remove. No backward compat
Contributor
Author
There was a problem hiding this comment.
Removed the dead compatibility code and dropped backward-compat shims in create_issue and dependent manager/test references in commit f5563aa.
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/16b1e99a-00e9-4025-a027-2a8c120e2315 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Contributor
Author
|
Just as a heads up, I was blocked by some firewall rules while working on your feedback. Expand below for details. Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Copilot
AI
changed the title
Harden
Harden Apr 21, 2026
create_issue concurrency limits and remove dead mutable assignment queuecreate_issue concurrency limits and remove dead copilot assignment queue code
pelikhan
reviewed
Apr 21, 2026
| const issuesToAssignCopilot = getIssuesToAssignCopilot(); | ||
| if (issuesToAssignCopilot.length > 0) { | ||
| const issuesToAssignStr = issuesToAssignCopilot.join(","); | ||
| core.setOutput("issues_to_assign_copilot", issuesToAssignStr); |
Contributor
Author
There was a problem hiding this comment.
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).
Closed
Contributor
🧪 Test Quality Sentinel ReportTest Quality Score: 100/100✅ Excellent test quality
Test Classification Details
Notable Changes (Tests Removed)This PR also removes two implementation tests that were testing internal state of the now-deleted copilot queue:
Removing these is a quality improvement: they were testing implementation details of dead code. New Test Analysis✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
✅ Test Quality Sentinel: 100/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The single new test enforces the concurrency-safety behavioral contract of the create_issue max-count fix, and the PR correctly removes dead implementation-detail tests for the deleted copilot queue code.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens create_issue max-count enforcement under concurrency by reserving a slot before any async work, and removes the previously-dead “issues to assign copilot” queue/output plumbing.
Changes:
- Move
processedCountmax check + reservation ahead of the group-by-day async pre-check to eliminate the TOCTOU window. - Remove the dead Copilot assignment queue helpers/exports from
create_issueand remove corresponding wiring/outputs from the handler manager. - Update tests to drop queue assertions and add a concurrent max-limit test.
Show a summary per file
| File | Description |
|---|---|
| actions/setup/js/safe_output_handler_manager.cjs | Removes create_issue Copilot queue reset/output wiring. |
| actions/setup/js/create_issue.cjs | Removes dead queue API; synchronizes max-count check with slot reservation; releases slot on group-by-day comment path. |
| actions/setup/js/create_issue.test.cjs | Removes deleted helper usage; adds concurrent max-limit coverage. |
| actions/setup/js/create_discussion_fallback.test.cjs | Removes references to deleted create_issue reset helper. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 4/4 changed files
- Comments generated: 2
| @@ -1224,16 +1218,6 @@ async function main() { | |||
| // Export processed count for consistency with project handler | |||
| core.setOutput("processed_count", successCount); | |||
|
|
|||
There was a problem hiding this comment.
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", ""); |
| @@ -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}`); | |||
There was a problem hiding this comment.
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, | |
| }); | |
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
create_issuehad two concurrency hazards: dead module-level copilot assignment queue code and a max-limit TOCTOU window where concurrent invocations could both pass the limit check before incrementing. This updates the handler so max enforcement is synchronized with slot reservation before async work, and fully removes the dead queue path.Max-count race fix (
processedCount)await.Dead copilot queue code removal (no backward compatibility)
create_issuequeue helpers and exports:getIssuesToAssignCopilot()resetIssuesToAssignCopilot()safe_output_handler_manager.cjs.Focused concurrency coverage
max: 1) that invokeshandleCreateIssueviaPromise.all(...)through the group-by-day pre-check path and asserts only one create succeeds.