Conversation
Adds a query() function that wraps CopilotClient + session creation into a simple async generator, similar to the Claude Agent SDK's query() API. - New QueryOptions type in types.ts - New query() async generator in query.ts (auto-creates client/session, yields SessionEvent, supports maxTurns, auto-reads COPILOT_CLI_URL) - Exported from index.ts - New todo-tracker.ts sample demonstrating the pattern Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Cross-SDK Consistency ReviewThis PR adds a useful Summary of FindingsCurrent state:
What the Node.js
|
|
@SteveSandersonMS would be good to talk about this. We might want to make deeper change. |
There was a problem hiding this comment.
Pull request overview
This PR adds a convenience query() function to the Node.js/TypeScript SDK, providing a simpler async-iterator API pattern inspired by the Claude Agent SDK. The implementation wraps the existing CopilotClient and session APIs to allow users to consume Copilot responses as async iterators without manually managing client/session lifecycle.
Changes:
- New
query()async generator function that creates a client, sends a prompt, and yields session events - New
QueryOptionsinterface combining essential client and session configuration options - Sample demonstrating the pattern with a TodoTracker tool
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| nodejs/src/types.ts | Adds QueryOptions interface defining configuration for the convenience API |
| nodejs/src/query.ts | Implements the query() async generator with event bridging, maxTurns support, and auto-cleanup |
| nodejs/src/index.ts | Exports the new query function and QueryOptions type |
| nodejs/samples/todo-tracker.ts | Demonstrates query() usage with a custom tool for tracking todos |
Comments suppressed due to low confidence (4)
nodejs/src/query.ts:107
- The buffering mechanism has a potential race condition: if events arrive faster than they're consumed, the buffer will grow unboundedly. The implementation doesn't have any backpressure mechanism or buffer size limit, which could lead to memory exhaustion in scenarios where events are produced faster than consumed or if the consumer stops iterating but doesn't break out of the loop.
Consider adding a buffer size limit with backpressure handling, or document this as a known limitation.
// Bridge the event-driven API to an async iterator via a simple queue.
let resolve: ((value: IteratorResult<SessionEvent>) => void) | null = null;
const buffer: SessionEvent[] = [];
let done = false;
let turns = 0;
const finish = () => {
done = true;
if (resolve) {
resolve({ value: undefined as unknown as SessionEvent, done: true });
resolve = null;
}
};
session.on((event: SessionEvent) => {
if (done) return;
// Count tool-calling turns for maxTurns support.
if (
options.maxTurns &&
event.type === "assistant.message" &&
event.data.toolRequests?.length
) {
turns++;
if (turns >= options.maxTurns) {
if (resolve) {
resolve({ value: event, done: false });
resolve = null;
} else {
buffer.push(event);
}
finish();
return;
}
}
if (event.type === "session.idle") {
if (resolve) {
resolve({ value: event, done: false });
resolve = null;
} else {
buffer.push(event);
}
finish();
return;
}
if (resolve) {
resolve({ value: event, done: false });
resolve = null;
} else {
buffer.push(event);
}
nodejs/src/query.ts:127
- When
doneis true and the promise resolver tries to returnundefined as unknown as SessionEvent(line 121), this yieldedundefinedvalue will be processed by the calling code as a SessionEvent. However, this is problematic because:
- Line 114 uses
yield buffer.shift()!which could also be undefined if the buffer is somehow empty - Line 121 explicitly yields undefined when done=true, but the consumer expects SessionEvent types
The while loop condition on line 112 should prevent yielding when done is true and buffer is empty, but there's a subtle issue: when a promise is waiting (line 118) and finish() is called, the promise resolves with undefined as unknown as SessionEvent, which gets yielded. The loop should check done again after the await to avoid this.
Consider restructuring the logic to ensure undefined is never yielded.
yield await new Promise<SessionEvent>((r) => {
resolve = (result) => {
if (result.done) {
r(undefined as unknown as SessionEvent);
} else {
r(result.value);
}
};
});
}
nodejs/src/query.ts:110
- The event handler is registered on line 69 after the session is created, but before
session.send()is called on line 110. However, there's a potential race condition: if the session immediately starts emitting events (e.g.,session.start) beforesend()is called, these events will be captured. While this might be intentional, it differs from the typical pattern where users callsend()and then start listening for events.
Additionally, if an error occurs during session.send() (line 110), the event handler will remain registered and could cause issues. While the finally block will clean up the client, consider whether the event handler should be unsubscribed on error.
session.on((event: SessionEvent) => {
if (done) return;
// Count tool-calling turns for maxTurns support.
if (
options.maxTurns &&
event.type === "assistant.message" &&
event.data.toolRequests?.length
) {
turns++;
if (turns >= options.maxTurns) {
if (resolve) {
resolve({ value: event, done: false });
resolve = null;
} else {
buffer.push(event);
}
finish();
return;
}
}
if (event.type === "session.idle") {
if (resolve) {
resolve({ value: event, done: false });
resolve = null;
} else {
buffer.push(event);
}
finish();
return;
}
if (resolve) {
resolve({ value: event, done: false });
resolve = null;
} else {
buffer.push(event);
}
});
await session.send({ prompt: options.prompt });
nodejs/src/query.ts:44
- The spread operators on lines 41-43 are using conditional spreading, which means that when a value is falsy (including empty string, 0, false, null, undefined), the property is not spread into the object. However, for
cliPath, an empty string might be a valid value that the user intends to pass (though unlikely). Similarly, an empty string forgithubTokenwould be treated as falsy and not passed.
While this is likely the intended behavior, consider using explicit checks for undefined or !== undefined to be more precise about which values should be omitted, especially if empty strings could be valid inputs in the future.
const client = new CopilotClient({
...(cliUrl ? { cliUrl } : {}),
...(options.cliPath ? { cliPath: options.cliPath } : {}),
...(options.githubToken ? { githubToken: options.githubToken } : {}),
});
| session.on((event: SessionEvent) => { | ||
| if (done) return; | ||
|
|
||
| // Count tool-calling turns for maxTurns support. | ||
| if ( | ||
| options.maxTurns && | ||
| event.type === "assistant.message" && | ||
| event.data.toolRequests?.length | ||
| ) { | ||
| turns++; | ||
| if (turns >= options.maxTurns) { | ||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| finish(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (event.type === "session.idle") { | ||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| finish(); | ||
| return; | ||
| } | ||
|
|
||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| }); |
There was a problem hiding this comment.
The event handler does not handle session.error events, which means errors that occur during session execution will not terminate the async iterator or propagate to the caller. According to the SessionEvent types and the sendAndWait pattern in session.ts (lines 175-178), session.error events should be caught and handled appropriately.
Consider adding a handler for session.error that calls finish() to terminate the iterator and potentially throws or yields the error event so the caller can handle it appropriately.
This issue also appears on line 69 of the same file.
| if ( | ||
| options.maxTurns && | ||
| event.type === "assistant.message" && | ||
| event.data.toolRequests?.length | ||
| ) { | ||
| turns++; | ||
| if (turns >= options.maxTurns) { | ||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| finish(); | ||
| return; | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling maxTurns has a potential issue: it yields the final assistant.message event before calling finish(), but it doesn't handle subsequent events that might arrive before the session is fully terminated. This could lead to buffered events being lost or the iterator continuing to process events after the maxTurns limit is reached.
Additionally, when maxTurns is reached on line 79, the iterator finishes immediately after yielding that event. However, other events (like tool.execution_start, tool.execution_end) that are already in flight or buffered might not be yielded, potentially leaving the caller without complete information about the final turn.
This issue also appears in the following locations of the same file:
- line 55
- line 118
|
|
||
| /** | ||
| * Maximum number of agentic turns (assistant responses that include tool calls). | ||
| * The generator will end after this many tool-calling turns. |
There was a problem hiding this comment.
The documentation for maxTurns describes it as "Maximum number of agentic turns (assistant responses that include tool calls)", but the implementation only counts assistant.message events that have toolRequests. This means that if the assistant sends a message without tool calls, it won't count toward the limit. However, according to the description, turns should be counted as "assistant responses that include tool calls", which is technically correct but could be clearer.
Consider clarifying the documentation to explicitly state that only assistant messages with tool calls count toward the limit, and that final messages without tool calls will still be yielded even if they occur after maxTurns is reached.
| * The generator will end after this many tool-calling turns. | |
| * Only assistant messages that include tool calls count toward this limit. | |
| * After this many tool-calling turns, no further tool calls will be made, | |
| * but a final assistant message without tool calls may still be returned. |
| class TodoTracker { | ||
| private todos: any[] = []; |
There was a problem hiding this comment.
The sample uses any[] type for the todos array. Following TypeScript best practices and conventions visible in the codebase (which uses explicit types throughout), consider defining a proper type for todo items to provide better type safety.
For example: private todos: Array<{ content: string; status: 'completed' | 'in_progress' | 'pending'; activeForm?: string }> = [];
| class TodoTracker { | |
| private todos: any[] = []; | |
| type TodoItem = { | |
| content: string; | |
| status: "completed" | "in_progress" | "pending"; | |
| activeForm?: string; | |
| }; | |
| class TodoTracker { | |
| private todos: TodoItem[] = []; |
| } finally { | ||
| await client.stop(); | ||
| } |
There was a problem hiding this comment.
The query() function creates a new session but never explicitly destroys it. While client.stop() is called in the finally block (line 130), which stops the client and closes all sessions, it would be more explicit and follow better resource management practices to call session.destroy() before client.stop(). This is the pattern used in the codebase examples (see nodejs/src/client.ts lines 105-106 and nodejs/samples/chat.ts).
| export async function* query(options: QueryOptions): AsyncGenerator<SessionEvent> { | ||
| const cliUrl = options.cliUrl ?? process.env.COPILOT_CLI_URL; | ||
| const client = new CopilotClient({ | ||
| ...(cliUrl ? { cliUrl } : {}), | ||
| ...(options.cliPath ? { cliPath: options.cliPath } : {}), | ||
| ...(options.githubToken ? { githubToken: options.githubToken } : {}), | ||
| }); | ||
|
|
||
| try { | ||
| const session = await client.createSession({ | ||
| model: options.model, | ||
| tools: options.tools ?? [], | ||
| streaming: options.streaming ?? true, | ||
| systemMessage: options.systemMessage, | ||
| onPermissionRequest: options.onPermissionRequest ?? approveAll, | ||
| }); | ||
|
|
||
| // Bridge the event-driven API to an async iterator via a simple queue. | ||
| let resolve: ((value: IteratorResult<SessionEvent>) => void) | null = null; | ||
| const buffer: SessionEvent[] = []; | ||
| let done = false; | ||
| let turns = 0; | ||
|
|
||
| const finish = () => { | ||
| done = true; | ||
| if (resolve) { | ||
| resolve({ value: undefined as unknown as SessionEvent, done: true }); | ||
| resolve = null; | ||
| } | ||
| }; | ||
|
|
||
| session.on((event: SessionEvent) => { | ||
| if (done) return; | ||
|
|
||
| // Count tool-calling turns for maxTurns support. | ||
| if ( | ||
| options.maxTurns && | ||
| event.type === "assistant.message" && | ||
| event.data.toolRequests?.length | ||
| ) { | ||
| turns++; | ||
| if (turns >= options.maxTurns) { | ||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| finish(); | ||
| return; | ||
| } | ||
| } | ||
|
|
||
| if (event.type === "session.idle") { | ||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| finish(); | ||
| return; | ||
| } | ||
|
|
||
| if (resolve) { | ||
| resolve({ value: event, done: false }); | ||
| resolve = null; | ||
| } else { | ||
| buffer.push(event); | ||
| } | ||
| }); | ||
|
|
||
| await session.send({ prompt: options.prompt }); | ||
|
|
||
| while (!done || buffer.length > 0) { | ||
| if (buffer.length > 0) { | ||
| yield buffer.shift()!; | ||
| } else if (done) { | ||
| break; | ||
| } else { | ||
| yield await new Promise<SessionEvent>((r) => { | ||
| resolve = (result) => { | ||
| if (result.done) { | ||
| r(undefined as unknown as SessionEvent); | ||
| } else { | ||
| r(result.value); | ||
| } | ||
| }; | ||
| }); | ||
| } | ||
| } | ||
| } finally { | ||
| await client.stop(); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new query() convenience API lacks test coverage. Given that the codebase has comprehensive E2E tests for other SDK features (see nodejs/test/e2e/ with tests for client lifecycle, session handling, tools, etc.), this new feature should have corresponding tests to verify:
- Basic query functionality with streaming events
- maxTurns limiting behavior
- Environment variable handling (COPILOT_CLI_URL)
- Cleanup on early termination (breaking out of the iterator)
- Error propagation from session errors
| const client = new CopilotClient({ | ||
| ...(cliUrl ? { cliUrl } : {}), | ||
| ...(options.cliPath ? { cliPath: options.cliPath } : {}), | ||
| ...(options.githubToken ? { githubToken: options.githubToken } : {}), |
There was a problem hiding this comment.
When cliUrl is provided (either via options or COPILOT_CLI_URL environment variable), the CopilotClient constructor will throw an error if githubToken is also provided, as per the validation on lines 200-204 of client.ts. However, the query function on line 39 reads COPILOT_CLI_URL from the environment, and on line 43 conditionally passes githubToken if provided.
This creates a problematic scenario: if a user sets COPILOT_CLI_URL environment variable and also provides githubToken in QueryOptions, the client constructor will throw an error. The error message is clear ("githubToken and useLoggedInUser cannot be used with cliUrl"), but it would be better to either:
- Document this limitation clearly in QueryOptions
- Validate and provide a clearer error message in the query function itself
- Handle this case by not passing githubToken when cliUrl is present
This issue also appears on line 40 of the same file.
| ...(options.githubToken ? { githubToken: options.githubToken } : {}), | |
| // When using cliUrl (including via COPILOT_CLI_URL), CopilotClient does not allow githubToken. | |
| ...(!cliUrl && options.githubToken ? { githubToken: options.githubToken } : {}), |
Summary
Adds a
query()convenience function that provides a simple async-iterator API over the Copilot SDK, inspired by the Claude Agent SDK'squery()pattern.Motivation
The current SDK requires creating a
CopilotClient, a session, sending a message, and subscribing to events — several steps for simple use cases. The Claude Agent SDK offers a singlequery()function that returns an async iterator. This PR brings the same ergonomics to the Copilot SDK.Before (existing API)
After (new convenience API)
Changes
nodejs/src/types.ts— NewQueryOptionsinterfacenodejs/src/query.ts— Newquery()async generator that wraps client/session lifecycle, bridges events to an async iterator, supportsmaxTurns, and auto-readsCOPILOT_CLI_URLenv varnodejs/src/index.ts— ExportsqueryandQueryOptionsnodejs/samples/todo-tracker.ts— Sample demonstrating the pattern with a custom TodoWrite toolNotes
streaming: trueandonPermissionRequest: approveAllfor conveniencebreak