Harden assign_to_agent concurrency: isolate handler state and serialize MCP stdin dispatch#52034
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Hardens assign_to_agent and MCP stdio concurrency, addressing #51997.
Changes:
- Isolates assignment results per handler instance.
- Atomically reserves assignment slots and serializes MCP dispatch.
- Adds concurrency and state-isolation regression tests.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/assign_to_agent.cjs |
Isolates state and changes max enforcement. |
actions/setup/js/assign_to_agent.test.cjs |
Adds concurrency and isolation tests. |
actions/setup/js/mcp_server_core.cjs |
Queues stdin processing. |
actions/setup/js/mcp_server_core.test.cjs |
Tests serialized dispatch. |
actions/setup/js/safe_output_handler_manager.cjs |
Reads results from handler instances. |
Review details
Tip
Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 5/5 changed files
- Comments generated: 1
- Review effort level: Balanced
| allResults.push({ issue_number: message.issue_number || null, pull_number: message.pull_number || null, agent: agentNameForSkip, owner: null, repo: null, success: false, skipped: true }); | ||
| return { success: false, skipped: true }; | ||
| } | ||
| processedCount++; |
|
@copilot Please refresh this branch, verify checks, and use the pr-finisher skill before handing back. No failed checks were reported in the compact candidate set for this run.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed the branch with current |
|
/matt |
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs and /tdd — requesting changes on correctness of the deferral-rollback logic and test coverage gaps.
📋 Key Themes & Highlights
Key Issues
- Deferral-rollback race (correctness): The
processedCount--on deferral creates a window where two concurrent deferred messages can both decrement back below the max gate and both complete on retry — violatingmax. - Test doesn't exercise the real race: The
Promise.allconcurrent test uses instant mocks, so the gate is already respected before anyawait; the pre-fix race is never actually reproduced. - Delay guard boundary test missing: The
processedCount > 1change to the delay guard lacks a dedicated test for the 1→2 transition.
Positive Highlights
- ✅ Moving
allResultsto closure scope is a clean, correct fix for state isolation — no more module-level bleed between invocations. - ✅ Attaching accessors directly to the handler function is an elegant pattern that avoids threading an extra argument through call sites.
- ✅ Promise-chaining for MCP stdio serialization is the right approach and the new test correctly verifies sequential dispatch.
- ✅ Extraction of
formatAssignedOutput,formatErrorsOutput,countHardFailures, andwriteAssignSummaryas pure functions improves testability.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · sonnet46 · 42.7 AIC · ⌖ 7.4 AIC · ⊞ 7.1K
Comment /matt to run again
|
@copilot Please address the open review feedback, refresh the branch if needed, and use the pr-finisher skill before handing back. Open review threads (newest reviewer feedback first):
No failed checks were reported in the compact candidate set for this run.
|
Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Addressed the max-slot review feedback in |
|
📰 BREAKING: Smoke Copilot - AOAI (Entra) is now investigating this pull request. Sources say the story is developing... |
|
📰 BREAKING: Smoke Copilot - AOAI (apikey) is now investigating this pull request. Sources say the story is developing... |
|
✅ Smoke OTEL completed successfully!
|
|
Race conditions hide, Inspired by this PR's fix for shared state races and serialized stdin dispatch in assign_to_agent.
|
|
Caution agentic threat detected DetailsPotential security threats were detected in the agent output. Review the workflow run logs for details. Smoke test FAIL
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "proxy.golang.org"See Network Configuration for more information.
|
|
Smoke test summary: recent commits focus on security hardening, policy validation, and workflow robustness, with additional docs and lint-rule updates.
|
Agent Container Tool Check
Result: 12/12 tools available ✅ — Overall status: PASS
|
|
Commit pushed:
|
Smoke Test Codex: FAILMerged PRs:
Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
💨 Smoke Test Run 31732491833Core Tests #1–12: ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅ ✅
Overall: PARTIAL Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
💥 Automated smoke test review - all systems nominal! (Run 31732491833)
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comsafebrowsingohttpgateway.googleapis.comwww.google.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "safebrowsingohttpgateway.googleapis.com"
- "www.google.com"See Network Configuration for more information.
💥 [THE END] — Illustrated by Smoke Claude · sonnet46 · 64.1 AIC · ⌖ 8.64 AIC · ⊞ 6.3K
Comment /smoke-claude to run again
Comments that could not be inline-anchored
actions/setup/js/assign_to_agent.cjs:28
🔍 The token precedence logic here looks good — using a dedicated per-handler PAT before falling back to step-level token is a solid security pattern. Smoke test agent approves! (Run 31732491833)
actions/setup/js/mcp_server_core.cjs:9
✅ Logging at module load time is a great debugging aid. This helps trace initialization order in complex pipelines. Smoke test agent confirms! (Run 31732491833)
|
Smoke Test Copilot - PR Summary PR: Harden assign_to_agent concurrency: isolate handler state and serialize MCP stdin dispatch Results: 15✅ / 1❌ (test #3 Serena timeout) Overall: FAIL Author: app/copilot-swe-agent | Assignees: Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 6 domainsThe following domains were blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
|
There was a problem hiding this comment.
Smoke test review: build good. Me approve as comment only.
Warning
Firewall blocked 6 domains
The following domains were blocked by the firewall during workflow execution:
accounts.google.comandroid.clients.google.comclients2.google.comcontentautofill.googleapis.comwww.google.comwww.gstatic.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "accounts.google.com"
- "android.clients.google.com"
- "clients2.google.com"
- "contentautofill.googleapis.com"
- "www.google.com"
- "www.gstatic.com"See Network Configuration for more information.
📰 BREAKING: Report filed by Smoke Copilot · auto · 46.6 AIC · ⌖ 2.41 AIC · ⊞ 8.5K
Comment /smoke-copilot to run again
Add label smoke to run again
|
Smoke test 31732550397 results:
|
|
@copilot Triage nudge from sous-chef. Please inspect the latest smoke failures, refresh the branch if needed, and run the Failed checks from the candidate set:
Branch update was requested automatically for this run.
|
assign_to_agentrelied on module-global mutable results and a check-then-await max gate, which allowed cross-invocation state bleed and a TOCTOU race under concurrent dispatch. In parallel, MCP stdio processing could interleaveprocessReadBufferexecutions when clients pipelined requests.State isolation in
assign_to_agentmain()closure state.getAssignToAgentAssigned,getAssignToAgentErrors,getAssignToAgentErrorCount,writeAssignToAgentSummary) so data is scoped to a single handler lifecycle.TOCTOU fix for
maxenforcementprocessedCount++before anyawait).Serialized MCP stdio dispatch
mcp_server_core.start()to chainprocessReadBuffercalls through a promise queue, preventing overlapping reads/dispatch from concurrentstdindata events.Handler-manager integration update
safe_output_handler_managernow reads assign-to-agent outputs/summaries from the loaded handler instance instead of module-level exported state.Regression coverage
assign_to_agentinvocation withmax: 1.main()handlers do not share results.Run: https://github.com/github/gh-aw/actions/runs/31511130973> Generated by 👨🍳 PR Sous Chef · gpt54 · 8.27 AIC · ⌖ 5.37 AIC · ⊞ 8.5K · ◷
Requested branch update from https://github.com/github/gh-aw/actions/runs/31535473039.> Generated by 👨🍳 PR Sous Chef · gpt54 · 8.59 AIC · ⌖ 5.18 AIC · ⊞ 8.5K · ◷
Run report: https://github.com/github/gh-aw/actions/runs/31554759452> Generated by 👨🍳 PR Sous Chef · gpt54 · 13.6 AIC · ⌖ 7.61 AIC · ⊞ 8.5K · ◷
Run context: https://github.com/github/gh-aw/actions/runs/31616336935> Generated by 👨🍳 PR Sous Chef · gpt54 · 12.9 AIC · ⌖ 5.25 AIC · ⊞ 8.5K · ◷
✨ PR Review Safe Output Test - Run 31732491833> [!WARNING]