-
Notifications
You must be signed in to change notification settings - Fork 822
Expose session context in listSessions and add filtering #427
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Adds session “working directory / git repo” context to Node SDK SessionMetadata returned by listSessions(), and introduces an optional filter parameter to support server-side session filtering.
Changes:
- Added new public types
SessionContextandSessionListFilter, and extendedSessionMetadatawithcontext?: SessionContext. - Updated
CopilotClient.listSessions()to accept an optional filter and to return the newcontextfield. - Updated Node docs/cookbook examples and added an E2E assertion for the new
contextfield.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| nodejs/src/types.ts | Introduces SessionContext / SessionListFilter and adds context onto SessionMetadata. |
| nodejs/src/client.ts | Extends listSessions() to accept a filter and map context from the JSON-RPC response. |
| nodejs/src/index.ts | Re-exports SessionListFilter from the package entrypoint (but currently not SessionContext). |
| nodejs/test/e2e/session.test.ts | Adds an E2E test asserting listSessions() returns a context.cwd. |
| nodejs/README.md | Documents SessionMetadata.context and the SessionContext fields (but doesn’t yet document filtering). |
| cookbook/nodejs/persisting-sessions.md | Adds example printing session context information. |
| cookbook/nodejs/multiple-sessions.md | Adds example showing context?.cwd when listing sessions. |
| * } | ||
| * ``` | ||
| */ | ||
| async listSessions(): Promise<SessionMetadata[]> { | ||
| async listSessions(filter?: SessionListFilter): Promise<SessionMetadata[]> { | ||
| if (!this.connection) { | ||
| throw new Error("Client not connected"); | ||
| } | ||
|
|
||
| const response = await this.connection.sendRequest("session.list", {}); | ||
| const response = await this.connection.sendRequest("session.list", { filter }); |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Now that listSessions accepts an optional filter, the method-level docs should be updated to document that parameter and include a filtered example (e.g. by repository or cwd). Otherwise IDE docs and generated docs will be misleading.
| SessionEventHandler, | ||
| SessionEventPayload, | ||
| SessionEventType, | ||
| SessionListFilter, |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SessionMetadata.context is typed as SessionContext, but SessionContext itself isn’t re-exported from the package entrypoint. This makes it awkward for consumers to reference the type directly (and the README now documents it). Re-export SessionContext alongside SessionListFilter in this file.
| SessionListFilter, | |
| SessionListFilter, | |
| SessionContext, |
| ##### `listSessions(): Promise<SessionMetadata[]>` | ||
|
|
||
| List all available sessions. | ||
|
|
||
| **SessionMetadata:** |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The README still documents listSessions(): Promise<SessionMetadata[]>, but the implementation now accepts an optional filter. Update the method signature docs and add a short description/example for the SessionListFilter parameter so the docs match the API.
| const { sessions } = response as { | ||
| sessions: Array<{ | ||
| sessionId: string; | ||
| startTime: string; | ||
| modifiedTime: string; | ||
| summary?: string; | ||
| isRemote: boolean; | ||
| context?: { | ||
| cwd: string; | ||
| gitRoot?: string; | ||
| repository?: string; | ||
| branch?: string; | ||
| }; |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SessionContext is re-declared inline in the response cast, which can drift from the exported SessionContext interface over time. Reuse SessionContext in the response typing (e.g., context?: SessionContext) to keep a single source of truth.
| async listSessions(filter?: SessionListFilter): Promise<SessionMetadata[]> { | ||
| if (!this.connection) { | ||
| throw new Error("Client not connected"); | ||
| } | ||
|
|
||
| const response = await this.connection.sendRequest("session.list", {}); | ||
| const response = await this.connection.sendRequest("session.list", { filter }); |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Filtering support was added to listSessions(filter?: SessionListFilter), but there’s no test exercising the filter behavior (only the presence of context). Add an E2E test that creates at least two sessions with different workingDirectory/repo context (or uses a filterable field) and asserts listSessions({ ... }) returns the expected subset.
| console.log(` Repository: ${session.context.repository}`); | ||
| console.log(` Branch: ${session.context.branch}`); |
Copilot
AI
Feb 10, 2026
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This example prints Repository: / Branch: even when those fields are undefined, which will produce noisy output like Repository: undefined. Consider printing those lines only when the value is present (or using a fallback like "(none)").
| console.log(` Repository: ${session.context.repository}`); | |
| console.log(` Branch: ${session.context.branch}`); | |
| if (session.context.repository) { | |
| console.log(` Repository: ${session.context.repository}`); | |
| } | |
| if (session.context.branch) { | |
| console.log(` Branch: ${session.context.branch}`); | |
| } |
Adds SessionContext to SessionMetadata so SDK consumers can see the working directory and repository information for each session. Also adds optional filter parameter to listSessions() for filtering by context fields (cwd, gitRoot, repository, branch). Implemented in all SDK clients: - Node.js - Python - Go - .NET Fixes #413 Fixes #200
Cross-SDK Consistency Review: ✅ Excellent ConsistencyI've reviewed this PR for cross-language SDK consistency. This PR adds session context information ( ✅ What's Consistent1. Type DefinitionsAll SDKs define the same two new types with equivalent fields: SessionContext:
SessionListFilter:
All field names use consistent casing (camelCase for TypeScript, snake_case for Python, PascalCase for .NET/Go). 2. API ChangesAll SDKs updated their
The filter parameter is optional/nullable in all implementations. 3. SessionMetadata UpdatesAll SDKs added the 4. Code ChangesAll SDKs properly:
5. Tests
📝 Minor Documentation Gap (Non-blocking)The following READMEs document the new
These are documentation-only gaps and don't affect API consistency. The in-code documentation (docstrings/JSDoc/XML comments) is complete in all languages. 🎯 RecommendationApprove - This PR maintains excellent cross-SDK consistency. The API design is parallel across all four languages, accounting for language-specific idioms. The minor documentation gaps can be addressed in a follow-up if desired, but they don't affect the functionality or API design consistency. Great work maintaining feature parity across all SDKs! 🚀
|
✅ Cross-SDK Consistency ReviewI've reviewed PR #427 for consistency across all four SDK implementations (Node.js, Python, Go, .NET), and I'm pleased to report that this PR maintains excellent cross-SDK consistency! What was added:
Consistency verification:✅ All four SDKs updated - Node.js, Python, Go, and .NET all include the changes
✅ Naming conventions respected - camelCase (Node/Python JSON), PascalCase (Go/C#) as appropriate Language-specific differences (appropriate):
No consistency issues found. This PR successfully maintains feature parity across all SDK implementations while respecting each language's conventions. Nice work! 🎉
|
|
@jmoseley does the SDK track if the git branch changes? if it does could it expose an event for it? |
Summary
Exposes the working directory and repository context for sessions via
listSessions(), and adds optional filtering support.New Types
SessionContext
SessionListFilter
API Changes
SessionMetadata
Added
context?: SessionContextfield.listSessions()
Now accepts optional filter parameter:
Changes
SessionContexttype to types.tsSessionListFiltertype to types.tsSessionMetadatato includecontextfieldlistSessions()to accept optional filter and return contextFixes #413
Fixes #200