Fix module-level mutable state in create_agent_session.cjs handler#52696
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
PR Triage\n\n- Category: bug\n- Risk: medium\n- Priority: medium\n- Score: 52/100 (impact 35 + urgency 10 + quality 7)\n- Recommended action:
|
There was a problem hiding this comment.
Pull request overview
Isolates agent-session results per handler invocation, preventing cross-run contamination.
Changes:
- Moves mutable results into
main()closure scope. - Exposes result accessors on each handler instance.
- Updates manager integration and adds regression coverage.
Show a summary per file
| File | Description |
|---|---|
actions/setup/js/create_agent_session.cjs |
Introduces per-instance result state and accessors. |
actions/setup/js/safe_output_handler_manager.cjs |
Reads outputs from the loaded handler instance. |
actions/setup/js/create_agent_session.test.cjs |
Updates accessor tests and verifies state isolation. |
Review details
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Balanced
|
@copilot Branch refresh has been requested for this PR via the update-branch action. Please run the
|
Ran the pr-finisher pass:
No code changes were needed in this pass — the concurrency fix and its regression test remain intact and passing. Still needed: maintainer re-trigger of CI and review/approval from |
PR Triage
Automated triage by PR Triage Agent.
|
|
@copilot Please run the
|
Ran another pr-finisher pass:
No code changes were needed — the concurrency fix and its regression test remain intact and passing after the merge with Still needed: a maintainer to approve/re-run the gated CI workflows and provide review approval. |
create_agent_session.cjsstored per-run results in a module-levellet _allResults = [], shared across everymain()invocation for the lifetime of the process (due to Node'srequire()caching). Sequential processing masked the bug today, but any future parallel dispatch or repeatedmain()call in the same process could cross-contaminate results between runs, corruptingsession_number/session_urloutputs and step summaries.State isolation
_allResultsarray entirely.allResultsis now aconstdeclared insidemain(), scoped to that invocation's closure.Accessor relocation
getSessionNumber,getSessionUrl, andwriteSummaryare now attached as properties on thehandleMessagefunction returned bymain(), each closing over its ownallResults, instead of being exported as module-level functions reading shared state.module.exportsnow only exposesmain.Caller update
safe_output_handler_manager.cjsretrieves the specific handler instance frommessageHandlersand invokes the accessors on it directly:Tests
main()invocations and interleaving their message processing to verify results never mix between handler instances.