feat: better async processes#52
Conversation
rupertsworld
commented
Aug 14, 2025
rupertsworld
commented
- Async processes working properly
- Adjusted message renderer to add tool calls/results where there are async delays
- improved testing
There was a problem hiding this comment.
Pull Request Overview
This PR enhances async process handling in the kernel system by refactoring the tool execution architecture and improving message rendering. The changes enable proper handling of async processes through a cleaner separation between execute and process-based tools, while ensuring tool calls and results are properly rendered even with async delays.
Key changes:
- Refactored tool architecture to support both
executeandprocessproperties - Enhanced message renderer to handle async tool calls with pending states
- Improved testing infrastructure with timeout utilities and better fixtures
Reviewed Changes
Copilot reviewed 24 out of 24 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| test/utils.ts | Adds timeout utility for async test handling |
| test/tools.test.ts | New test suite for tool execution and process instantiation |
| test/fixtures.ts | Consolidated test fixtures with process classes and mock model |
| src/tools.ts | Simplified tool interface to support both execute and process functions |
| src/stream.ts | Enhanced message renderer with async tool call handling |
| src/kernel.ts | Refactored tool call handling to support async processes properly |
| src/processes/*.ts | Updated process architecture with snapshot-based serialization |
| src/runtime.ts | Improved process lifecycle management and event handling |
| cli/src/tools/*.ts | Updated tool implementations to use new process-based architecture |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| resolve(); | ||
| } | ||
| }); | ||
| }, 1); |
There was a problem hiding this comment.
The timeout value of 1ms is too short and will likely cause the test to fail before the kernel can process the message and emit the expected event. Consider using a more reasonable timeout like 1000ms.
| }, 1); | |
| }, 1000); |
| resolve(); | ||
| } | ||
| }); | ||
| }, 1); |
There was a problem hiding this comment.
The timeout value of 1ms is too short and will likely cause the test to fail before the kernel can process the message and emit the expected event. Consider using a more reasonable timeout like 1000ms.
| }, 1); | |
| }, 1000); |
| }) | ||
| ); | ||
|
|
||
| withTimeout((resolve) => { |
There was a problem hiding this comment.
The withTimeout function returns a Promise but this test doesn't await it or return it, which means the test will complete before the async operation finishes. Add 'await' or 'return' before withTimeout.
| withTimeout((resolve) => { | |
| it('calls a simple execute tool', async () => { | |
| kernel.send( | |
| createMessage('tool-call', { | |
| name: 'simple_execute', | |
| }) | |
| ); | |
| await withTimeout((resolve) => { |
| }) | ||
| ); | ||
|
|
||
| withTimeout((resolve) => { |
There was a problem hiding this comment.
The withTimeout function returns a Promise but this test doesn't await it or return it, which means the test will complete before the async operation finishes. Add 'await' or 'return' before withTimeout.
| withTimeout((resolve) => { | |
| it('instantiates a process & returns the call output', async () => { | |
| kernel.send( | |
| createMessage('tool-call', { | |
| name: 'simple_process', | |
| }) | |
| ); | |
| await withTimeout((resolve) => { |
| * Called with a snapshot to load the process. Replace this with logic to restore your snapshot data. | ||
| */ | ||
| static fromSnapshot(snapshot: unknown): Process { | ||
| const ctor = this.constructor as ProcessConstructor; |
There was a problem hiding this comment.
The method tries to access this.constructor but this refers to the class constructor, not an instance. It should be this directly since this is a static method. Change to const ctor = this;
| const ctor = this.constructor as ProcessConstructor; | |
| const ctor = this as ProcessConstructor; |
| if (!this.process) | ||
| throw new Error('Tried to unmount, but process not running.'); | ||
| if (!this.process.unmount) | ||
| throw new Error('Cannot unmound process, no unmount function supplied.'); |
There was a problem hiding this comment.
Typo in error message: 'unmound' should be 'unmount'.
| throw new Error('Cannot unmound process, no unmount function supplied.'); | |
| throw new Error('Cannot unmount process, no unmount function supplied.'); |
| // If the next message is not a valid tool response, add pending tool | ||
| // results message. | ||
| const nextMsg = msgs.at(index + 1); | ||
| if (nextMsg && !isValidToolResponse(msg, nextMsg)) { |
There was a problem hiding this comment.
Using msgs.at(index + 1) when the next message doesn't exist will return undefined, but the code doesn't handle this case properly. The check nextMsg && !isValidToolResponse(msg, nextMsg) could pass when nextMsg is undefined, which might not be the intended behavior.
| if (nextMsg && !isValidToolResponse(msg, nextMsg)) { | |
| if (!nextMsg || !isValidToolResponse(msg, nextMsg)) { |
| toolName: result.name, | ||
| args: {}, | ||
| })), | ||
| }); |
There was a problem hiding this comment.
Using msgs.at(index - 1) when the previous message doesn't exist will return undefined, but the check !prevMsg || !isValidToolResponse(prevMsg, msg) assumes prevMsg can be falsy. However, this could lead to unexpected behavior when index is 0.
| }); | |
| if (index > 0) { | |
| const prevMsg = msgs.at(index - 1); | |
| if (!prevMsg || !isValidToolResponse(prevMsg, msg)) { | |
| renderedMsgs.push({ | |
| role: 'assistant', | |
| content: msg.results.map((result) => ({ | |
| type: 'tool-call', | |
| toolCallId: result.callId, | |
| toolName: result.name, | |
| args: {}, | |
| })), | |
| }); | |
| } |
