Add session fs support across SDKs#1036
Conversation
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
This comment has been minimized.
This comment has been minimized.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1036
| ElicitationResult, | ||
| InputOptions, | ||
| SessionCapabilities, | ||
| SessionFsConfig, |
There was a problem hiding this comment.
Suggestion: also export CreateSessionFsHandler (and SessionFsHandler) for parity with other SDKs
SessionFsConfig is now exported here so users can configure the feature, but to actually implement a session-fs handler users also need the handler types. Currently those aren't accessible through the public API:
SessionFsHandler— theProtocolclass ingenerated/rpc.pythat users implementCreateSessionFsHandler— the type aliasCallable[["CopilotSession"], SessionFsHandler]
Compare with the other SDKs:
- Node.js
index.tsexportsSessionFsHandleralongsideSessionFsConfig - .NET
ISessionFsHandlerispublicinGenerated/Rpc.cs - Go
rpc.SessionFsHandleris accessible via therpcsub-package
The existing precedent in Python is that analogous callback types like ElicitationHandler are exported from __init__.py. Without CreateSessionFsHandler (or at minimum SessionFsHandler) being exported, users would need to reach into copilot.session or copilot.generated.rpc to type their handler, which isn't great for a public API.
Suggested addition to the from .session import (...) block and __all__:
CreateSessionFsHandler,
SessionFsHandler, # re-exported from generated/rpc via session.py(Or export SessionFsHandler directly from generated/rpc if that's the preferred pattern.)
There was a problem hiding this comment.
Good catch. I exported both CreateSessionFsHandler and SessionFsHandler from copilot.__init__ so the public Python API exposes the handler types alongside SessionFsConfig.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds “session fs” support across the SDKs by introducing client-session RPC handler plumbing (server→client calls) and wiring connection/session configuration so the Copilot CLI can delegate session-scoped filesystem operations back to the SDK host.
Changes:
- Extend codegen (Go/Python/.NET) to include
clientSessionRPC surface + registration helpers for server→client calls (e.g.,sessionFs.*). - Add/strengthen session-fs configuration plumbing + validation (notably Node/Python/Go) and require per-session handler factories when enabled.
- Add cross-SDK E2E and unit coverage for session-fs behavior (route I/O, resume, large output temp files, compaction, and setProvider rejection).
Show a summary per file
| File | Description |
|---|---|
| scripts/codegen/python.ts | Includes clientSession methods in Python RPC generation and emits client-session handler registration code. |
| scripts/codegen/go.ts | Includes clientSession methods in Go RPC generation and emits client-session handler registration code. |
| scripts/codegen/csharp.ts | Adds client-session handler generation/registration and improves RPC type naming helpers. |
| python/test_client.py | Adds unit tests asserting session-fs config validation errors. |
| python/e2e/test_session_fs.py | Adds Python E2E coverage for session-fs behaviors. |
| python/copilot/session.py | Introduces SessionFsConfig/handler types and session config fields for session-fs. |
| python/copilot/generated/rpc.py | Adds generated SessionFs RPC param/result types + client-session handler protocol/registration. |
| python/copilot/client.py | Wires session-fs config into client start + session creation/resume with required handler factory. |
| python/copilot/init.py | Re-exports SessionFsConfig. |
| nodejs/test/client.test.ts | Adds Node unit tests for session-fs config validation. |
| nodejs/src/client.ts | Adds session-fs config validation in the Node client constructor. |
| go/types.go | Adds Go SessionFsConfig and per-session handler factory options. |
| go/session.go | Adds per-session client-session handler container. |
| go/rpc/generated_rpc.go | Adds generated SessionFs RPC types + client-session handler registration support. |
| go/internal/e2e/testharness/context.go | Adjusts CI fake-token injection to avoid affecting external-server clients. |
| go/internal/e2e/session_fs_test.go | Adds Go E2E coverage for session-fs behaviors. |
| go/client.go | Adds session-fs config validation, provider registration on connect, and per-session handler wiring. |
| go/client_test.go | Adds Go unit tests for session-fs config validation. |
| dotnet/test/SessionFsTests.cs | Adds .NET E2E coverage for session-fs behaviors. |
| dotnet/test/Harness/E2ETestContext.cs | Allows passing options into harness client creation; refines CI fake-token behavior. |
| dotnet/src/Types.cs | Adds SessionFsConfig to options and per-session handler factory hooks to session configs. |
| dotnet/src/Session.cs | Adds per-session client-session handler container. |
| dotnet/src/Generated/Rpc.cs | Adds generated SessionFs RPC types + client-session handler registration types. |
| dotnet/src/Client.cs | Registers session-fs provider on connect, wires per-session session-fs handlers, and registers client-session RPC handlers. |
Copilot's findings
- Files reviewed: 22/24 changed files
- Comments generated: 3
| await Rpc.SessionFs.SetProviderAsync( | ||
| _options.SessionFs.InitialCwd, | ||
| _options.SessionFs.SessionStatePath, | ||
| _options.SessionFs.Conventions, |
There was a problem hiding this comment.
ConfigureSessionFsAsync forwards SessionFs values directly to sessionFs.setProvider without validating required fields (non-empty InitialCwd/SessionStatePath) or guarding invalid enum values. Other SDKs in this PR validate early and throw a clear configuration error; adding equivalent validation here would prevent late/opaque failures from the RPC call.
| await Rpc.SessionFs.SetProviderAsync( | |
| _options.SessionFs.InitialCwd, | |
| _options.SessionFs.SessionStatePath, | |
| _options.SessionFs.Conventions, | |
| var sessionFs = _options.SessionFs; | |
| if (string.IsNullOrWhiteSpace(sessionFs.InitialCwd)) | |
| { | |
| throw new InvalidOperationException( | |
| "CopilotClientOptions.SessionFs.InitialCwd is required when CopilotClientOptions.SessionFs is configured."); | |
| } | |
| if (string.IsNullOrWhiteSpace(sessionFs.SessionStatePath)) | |
| { | |
| throw new InvalidOperationException( | |
| "CopilotClientOptions.SessionFs.SessionStatePath is required when CopilotClientOptions.SessionFs is configured."); | |
| } | |
| object? conventions = sessionFs.Conventions; | |
| if (conventions is null || !System.Enum.IsDefined(conventions.GetType(), conventions)) | |
| { | |
| throw new InvalidOperationException( | |
| $"CopilotClientOptions.SessionFs.Conventions has an invalid value '{sessionFs.Conventions}'."); | |
| } | |
| await Rpc.SessionFs.SetProviderAsync( | |
| sessionFs.InitialCwd, | |
| sessionFs.SessionStatePath, | |
| sessionFs.Conventions, |
|
|
||
|
|
||
| def provider_path(provider_root: Path, session_id: str, path: str) -> Path: | ||
| return provider_root / session_id / path.lstrip("/") |
There was a problem hiding this comment.
provider_path directly concatenates the server-supplied path onto provider_root/session_id without normalizing or preventing .. segments. A malformed path could escape the provider root and read/write outside the test sandbox. Consider resolving the full path and asserting it stays within the expected session root (similar to the .NET test’s ResolvePath guard).
| return provider_root / session_id / path.lstrip("/") | |
| session_root = (provider_root / session_id).resolve() | |
| resolved_path = (session_root / path.lstrip("/")).resolve() | |
| try: | |
| resolved_path.relative_to(session_root) | |
| except ValueError as exc: | |
| raise ValueError(f"Session FS path escapes provider root: {path}") from exc | |
| return resolved_path |
| func providerPath(root string, sessionID string, path string) string { | ||
| trimmed := strings.TrimPrefix(path, "/") | ||
| if trimmed == "" { | ||
| return filepath.Join(root, sessionID) | ||
| } |
There was a problem hiding this comment.
providerPath uses filepath.Join on a server-supplied path without validating for .. segments; filepath.Join will clean the path and can escape root/sessionID (and even root) if path contains traversal. To keep the E2E provider sandboxed, consider cleaning/resolving and asserting the result remains under the expected session directory.
This comment has been minimized.
This comment has been minimized.
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
Cross-SDK Consistency Review ✅ (one minor suggestion)This PR adds session-fs support across all four SDKs (Node.js, Python, Go, .NET) — great feature parity! The overall API design is consistent:
One minor naming inconsistencyI left an inline comment on Everything else looks well-aligned across SDKs. Nice work bringing all four to parity!
|
There was a problem hiding this comment.
Generated by SDK Consistency Review Agent for issue #1036
| /// <summary> | ||
| /// Path conventions used by this filesystem provider. | ||
| /// </summary> | ||
| public required SessionFsSetProviderRequestConventions Conventions { get; init; } |
There was a problem hiding this comment.
Cross-SDK consistency suggestion: The type SessionFsSetProviderRequestConventions comes from the generated RPC layer and leaks the internal RPC method name (sessionFs.setProvider) into the public API surface.
The other SDKs define a dedicated user-facing type for this:
- Node.js:
"windows" | "posix"string union (inSessionFsConfig) - Python:
SessionFsConventions = Literal["posix", "windows"] - Go:
rpc.Conventions(the type is named simplyConventions, notSessionFsSetProviderRequestConventions)
Consider defining a clean public alias in Types.cs, e.g.:
/// <summary>Path-convention options for a session filesystem provider.</summary>
public enum SessionFsConventions
{
[JsonStringEnumMemberName("posix")]
Posix,
[JsonStringEnumMemberName("windows")]
Windows,
}…and using SessionFsConventions in SessionFsConfig.Conventions instead. The mapping to the internal SessionFsSetProviderRequestConventions can live inside ConfigureSessionFsAsync in Client.cs. This keeps the public API surface free of generated-type names.
Summary
Testing