Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Tighten types
Co-Authored-By: SteveSandersonMS <1101362+SteveSandersonMS@users.noreply.github.com>
  • Loading branch information
SteveSandersonMS and SteveSandersonMS committed Apr 7, 2026
commit 5a14441c4dfb7fcbd4fdf1bcd4fd1dc61b6ddacf
10 changes: 3 additions & 7 deletions dotnet/src/Types.cs
Original file line number Diff line number Diff line change
Expand Up @@ -234,22 +234,18 @@ public sealed class SessionFsConfig
/// <summary>
/// Initial working directory for sessions (user's project directory).
/// </summary>
public string InitialCwd { get; set; } = string.Empty;
public required string InitialCwd { get; init; }

/// <summary>
/// Path within each session's SessionFs where the runtime stores
/// session-scoped files (events, workspace, checkpoints, and temp files).
/// </summary>
public string SessionStatePath { get; set; } = string.Empty;
public required string SessionStatePath { get; init; }

/// <summary>
/// Path conventions used by this filesystem provider.
/// Defaults to the conventions of the current operating system.
/// </summary>
public SessionFsSetProviderRequestConventions Conventions { get; set; }
= OperatingSystem.IsWindows()
? SessionFsSetProviderRequestConventions.Windows
: SessionFsSetProviderRequestConventions.Posix;
public required SessionFsSetProviderRequestConventions Conventions { get; init; }
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (in SessionFsConfig)
  • Python: SessionFsConventions = Literal["posix", "windows"]
  • Go: rpc.Conventions (the type is named simply Conventions, not SessionFsSetProviderRequestConventions)

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>
Expand Down
19 changes: 19 additions & 0 deletions go/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,22 @@ import (

const noResultPermissionV2Error = "permission handlers cannot return 'no-result' when connected to a protocol v2 server"

func validateSessionFsConfig(config *SessionFsConfig) error {
if config == nil {
return nil
}
if config.InitialCwd == "" {
return errors.New("SessionFs.InitialCwd is required")
}
if config.SessionStatePath == "" {
return errors.New("SessionFs.SessionStatePath is required")
}
if config.Conventions != rpc.ConventionsPosix && config.Conventions != rpc.ConventionsWindows {
return errors.New("SessionFs.Conventions must be either 'posix' or 'windows'")
}
return nil
}

// Client manages the connection to the Copilot CLI server and provides session management.
//
// The Client can either spawn a CLI server process or connect to an existing server.
Expand Down Expand Up @@ -193,6 +209,9 @@ func NewClient(options *ClientOptions) *Client {
client.onListModels = options.OnListModels
}
if options.SessionFs != nil {
if err := validateSessionFsConfig(options.SessionFs); err != nil {
panic(err.Error())
}
sessionFs := *options.SessionFs
opts.SessionFs = &sessionFs
}
Expand Down
44 changes: 44 additions & 0 deletions go/client_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (
"regexp"
"sync"
"testing"

"github.com/github/copilot-sdk/go/rpc"
)

// This file is for unit tests. Where relevant, prefer to add e2e tests in e2e/*.test.go instead
Expand Down Expand Up @@ -223,6 +225,48 @@ func TestClient_URLParsing(t *testing.T) {
})
}

func TestClient_SessionFsConfig(t *testing.T) {
t.Run("should throw error when InitialCwd is missing", func(t *testing.T) {
defer func() {
if r := recover(); r == nil {
t.Error("Expected panic for missing SessionFs.InitialCwd")
} else {
matched, _ := regexp.MatchString("SessionFs.InitialCwd is required", r.(string))
if !matched {
t.Errorf("Expected panic message to contain 'SessionFs.InitialCwd is required', got: %v", r)
}
}
}()

NewClient(&ClientOptions{
SessionFs: &SessionFsConfig{
SessionStatePath: "/session-state",
Conventions: rpc.ConventionsPosix,
},
})
})

t.Run("should throw error when SessionStatePath is missing", func(t *testing.T) {
defer func() {
if r := recover(); r == nil {
t.Error("Expected panic for missing SessionFs.SessionStatePath")
} else {
matched, _ := regexp.MatchString("SessionFs.SessionStatePath is required", r.(string))
if !matched {
t.Errorf("Expected panic message to contain 'SessionFs.SessionStatePath is required', got: %v", r)
}
}
}()

NewClient(&ClientOptions{
SessionFs: &SessionFsConfig{
InitialCwd: "/",
Conventions: rpc.ConventionsPosix,
},
})
})
}

func TestClient_AuthOptions(t *testing.T) {
t.Run("should accept GitHubToken option", func(t *testing.T) {
client := NewClient(&ClientOptions{
Expand Down
18 changes: 18 additions & 0 deletions nodejs/src/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -297,6 +297,10 @@ export class CopilotClient {
);
}

if (options.sessionFs) {
this.validateSessionFsConfig(options.sessionFs);
}

// Parse cliUrl if provided
if (options.cliUrl) {
const { host, port } = this.parseCliUrl(options.cliUrl);
Expand Down Expand Up @@ -367,6 +371,20 @@ export class CopilotClient {
return { host, port };
}

private validateSessionFsConfig(config: SessionFsConfig): void {
if (!config.initialCwd) {
throw new Error("sessionFs.initialCwd is required");
}

if (!config.sessionStatePath) {
throw new Error("sessionFs.sessionStatePath is required");
}

if (config.conventions !== "windows" && config.conventions !== "posix") {
throw new Error("sessionFs.conventions must be either 'windows' or 'posix'");
}
}

/**
* Starts the CLI server and establishes a connection.
*
Expand Down
28 changes: 28 additions & 0 deletions nodejs/test/client.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -278,6 +278,34 @@ describe("CopilotClient", () => {
});
});

describe("SessionFs config", () => {
it("throws when initialCwd is missing", () => {
expect(() => {
new CopilotClient({
sessionFs: {
initialCwd: "",
sessionStatePath: "/session-state",
conventions: "posix",
},
logLevel: "error",
});
}).toThrow(/sessionFs\.initialCwd is required/);
});

it("throws when sessionStatePath is missing", () => {
expect(() => {
new CopilotClient({
sessionFs: {
initialCwd: "/",
sessionStatePath: "",
conventions: "posix",
},
logLevel: "error",
});
}).toThrow(/sessionFs\.sessionStatePath is required/);
});
});

describe("Auth options", () => {
it("should accept githubToken option", () => {
const client = new CopilotClient({
Expand Down
11 changes: 11 additions & 0 deletions python/copilot/client.py
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,15 @@
LogLevel = Literal["none", "error", "warning", "info", "debug", "all"]


def _validate_session_fs_config(config: SessionFsConfig) -> None:
if not config.get("initial_cwd"):
raise ValueError("session_fs.initial_cwd is required")
if not config.get("session_state_path"):
raise ValueError("session_fs.session_state_path is required")
if config.get("conventions") not in ("posix", "windows"):
raise ValueError("session_fs.conventions must be either 'posix' or 'windows'")


class TelemetryConfig(TypedDict, total=False):
"""Configuration for OpenTelemetry integration with the Copilot CLI."""

Expand Down Expand Up @@ -903,6 +912,8 @@ def __init__(
self._lifecycle_handlers_lock = threading.Lock()
self._rpc: ServerRpc | None = None
self._negotiated_protocol_version: int | None = None
if config.session_fs is not None:
_validate_session_fs_config(config.session_fs)
self._session_fs_config = config.session_fs

@property
Expand Down
30 changes: 30 additions & 0 deletions python/test_client.py
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,36 @@ def test_is_external_server_true(self):
assert client._is_external_server


class TestSessionFsConfig:
def test_missing_initial_cwd(self):
with pytest.raises(ValueError, match="session_fs.initial_cwd is required"):
CopilotClient(
SubprocessConfig(
cli_path=CLI_PATH,
log_level="error",
session_fs={
"initial_cwd": "",
"session_state_path": "/session-state",
"conventions": "posix",
},
)
)

def test_missing_session_state_path(self):
with pytest.raises(ValueError, match="session_fs.session_state_path is required"):
CopilotClient(
SubprocessConfig(
cli_path=CLI_PATH,
log_level="error",
session_fs={
"initial_cwd": "/",
"session_state_path": "",
"conventions": "posix",
},
)
)


class TestAuthOptions:
def test_accepts_github_token(self):
client = CopilotClient(
Expand Down
Loading