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. ✅
|
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 |
PR Triage: #52034
Automated triage — see labels for machine-readable classification.
|
|
@copilot Please refresh this branch, verify checks, and use the pr-finisher skill before handing back. Unresolved review threads to address (newest first):
No failed checks were reported in the compact candidate set for this run. Branch update was requested by sous-chef in run https://github.com/github/gh-aw/actions/runs/31531364207.
|
…ix-assign-to-agent Co-authored-by: gh-aw-bot <259018956+gh-aw-bot@users.noreply.github.com>
Refreshed with current |
|
@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. Branch update was requested by sous-chef in run https://github.com/github/gh-aw/actions/runs/31535473039.
|
The branch already contains current |
|
@copilot This PR looks ready for another finishing pass. Please refresh the branch context, address any remaining reviewer feedback, and run the
|
Finishing pass complete. |
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 · ◷