[Fix] Clean up taskGateways on initConductor failure (#212)#540
[Fix] Clean up taskGateways on initConductor failure (#212)#540advancedresearcharray wants to merge 3 commits into
Conversation
Validate agent catalog lines structurally before embedding them in the conductor system prompt, and add regression tests for catalog and user-task sanitization paths used by initConductor. Closes clawwork-ai#211 Signed-off-by: advancedresearcharray <advancedresearcharray@github.com>
No code changes — refreshes required checks for merge after /approve.
PR clawwork-ai#517 already removes taskGateways entries when a room reaches stopped. Also purge module-scoped maps when conductor session creation fails or throws, and drop any in-flight verify lock on terminal cleanup. Closes clawwork-ai#212 Signed-off-by: Array Fleet <fleet@advancedresearcharray.local> Co-authored-by: Cursor <cursoragent@cursor.com>
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the robustness and security of the room conductor initialization process. It ensures that system resources are properly cleaned up when session creation fails or errors occur, preventing potential memory leaks. Additionally, it significantly hardens the sanitization logic for agent catalogs and user tasks to prevent prompt injection attacks via control characters and malformed input. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Hi @advancedresearcharray, DetailsInstructions for interacting with me using comments are available here. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces robust sanitization for agent catalogs and user tasks to prevent prompt injection attacks, along with comprehensive unit tests validating these security measures. It also ensures that room resources are cleaned up when conductor initialization fails or throws an error. The reviewer feedback highlights an issue where failed rooms are left in an inconsistent active state in the store, recommending that they be explicitly removed from the store's state and verified in the tests.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| if (!res.ok) { | ||
| cleanupRoomResources(taskId); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When initConductor fails to create a session, the room is left in the store's rooms state with status: 'active' and conductorReady: false. This leads to an inconsistent state where a failed room is treated as active. We should remove the room from the store's state to maintain consistency.
| if (!res.ok) { | |
| cleanupRoomResources(taskId); | |
| return false; | |
| } | |
| if (!res.ok) { | |
| cleanupRoomResources(taskId); | |
| set((s) => { | |
| const nextRooms = { ...s.rooms }; | |
| delete nextRooms[taskId]; | |
| return { rooms: nextRooms }; | |
| }); | |
| return false; | |
| } |
| } catch (err) { | ||
| console.warn('[room-store] initConductor failed:', err); | ||
| cleanupRoomResources(taskId); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
When initConductor throws an error, the room is left in the store's rooms state with status: 'active' and conductorReady: false. We should remove the room from the store's state to maintain consistency.
} catch (err) {
console.warn('[room-store] initConductor failed:', err);
cleanupRoomResources(taskId);
set((s) => {
const nextRooms = { ...s.rooms };
delete nextRooms[taskId];
return { rooms: nextRooms };
});
return false;
}| const failed = await store.getState().initConductor(taskId, 'gw-1', sessionKey, '[]'); | ||
| expect(failed).toBe(false); |
There was a problem hiding this comment.
Add an assertion to verify that the room is successfully removed from the store's state when initConductor fails to create the session.
| const failed = await store.getState().initConductor(taskId, 'gw-1', sessionKey, '[]'); | |
| expect(failed).toBe(false); | |
| const failed = await store.getState().initConductor(taskId, 'gw-1', sessionKey, '[]'); | |
| expect(failed).toBe(false); | |
| expect(store.getState().getRoom(taskId)).toBeUndefined(); |
| const failed = await store.getState().initConductor(taskId, 'gw-1', sessionKey, '[]'); | ||
| expect(failed).toBe(false); |
There was a problem hiding this comment.
Add an assertion to verify that the room is successfully removed from the store's state when initConductor throws an error.
| const failed = await store.getState().initConductor(taskId, 'gw-1', sessionKey, '[]'); | |
| expect(failed).toBe(false); | |
| const failed = await store.getState().initConductor(taskId, 'gw-1', sessionKey, '[]'); | |
| expect(failed).toBe(false); | |
| expect(store.getState().getRoom(taskId)).toBeUndefined(); |
|
@advancedresearcharray PR #540 is merge-ready. No changes needed. CI is green and review is complete. |
|
PR is ready for merge! 👍 |
1 participant
Summary
cleanupRoomResources()to also removeverifyInFlightentries on terminal room cleanupcleanupRoomResources()wheninitConductor()fails (createSessionreturns{ ok: false }) or throws, preventingtaskGatewaysMap leaks on abandoned failed tasksPR #517 already cleaned up
taskGatewayswhen a room reachesstopped; this hardens the failure paths that PR did not cover.Closes #212
Test plan
pnpm --filter @clawwork/core test— 111 passed (21 room-store tests)initConductorfailure/throw uses a fresh gateway idMade with Cursor