Skip to content

Strongly type expAssignments session config across all SDKs#2033

Open
devm33 wants to merge 5 commits into
mainfrom
devm33/exp
Open

Strongly type expAssignments session config across all SDKs#2033
devm33 wants to merge 5 commits into
mainfrom
devm33/exp

Conversation

@devm33

@devm33 devm33 commented Jul 19, 2026

Copy link
Copy Markdown
Member

Summary

Strongly types the internal expAssignments session-config field across all six SDKs. Previously each SDK typed it as opaque JSON (Record<string, unknown> / any / dict / JsonElement / JsonNode / serde_json::Value). This replaces those with a concrete CopilotExpAssignmentResponse type (plus ExpConfigEntry) that mirrors the runtime contract from copilot-agent-runtime — the same shape the Copilot CLI fetches from the experimentation service.

This closes a gap from #1750, which forwarded expAssignments but left it loosely typed even though the runtime's in-process type (sessionOptions.ts) is the concrete CopilotExpAssignmentResponse.

Type shape

ExpConfigEntry { Id: string; Parameters: Record<string, string|number|boolean|null> }
CopilotExpAssignmentResponse {
  Features: string[]; Flights: Record<string,string>; Configs: ExpConfigEntry[];
  ParameterGroups?; FlightingVersion?: number; ImpressionId?: string;
  AssignmentContext: string;
}

Wire JSON keys stay PascalCase (Features, Flights, Configs, Id, Parameters, ...); optional fields are omitted when null; and the field keeps its @internal/hidden posture (not part of the broadly advertised public surface) in every language.

Changes per SDK

Each SDK updates the type, the create/resume config field, the wire path, clone/copy logic, and tests.

SDK Key files Validation
Node nodejs/src/types.ts, index.ts tsc + vitest
Go go/types.go build / vet / gofmt / test
Python python/copilot/client.py, __init__.py ruff + pytest
Rust rust/src/types.rs, wire.rs fmt + clippy -D warnings + test
.NET dotnet/src/Types.cs, Client.cs build + serialization tests
Java new CopilotExpAssignmentResponse.java, ExpConfigEntry.java + rpc/* spotless + checkstyle + tests

Testing

  • Nodetsc --noEmit clean; vitest passing
  • Gogo build/vet/gofmt/test passing
  • Pythonruff + pytest passing
  • Rustcargo fmt --check, clippy --all-features --all-targets -- -D warnings, cargo test passing
  • .NETdotnet build clean; 46 serialization tests passing
  • Javamvn spotless:check + checkstyle clean; 85 SessionRequestBuilderTest tests passing (JDK 25)

Replace the opaque JSON typing of the internal `expAssignments`
session-config field with a strongly-typed `CopilotExpAssignmentResponse`
(plus `ExpConfigEntry`) in every SDK, mirroring the runtime contract.

Wire keys remain PascalCase (Features, Flights, Configs, Id, Parameters,
...), optional fields are omitted when null, and the field keeps its
internal/hidden posture in each language.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43d7a02b-08ff-4e7d-a8c0-afa6dfbe94b0
@devm33
devm33 requested a review from a team as a code owner July 19, 2026 15:02
Copilot AI review requested due to automatic review settings July 19, 2026 15:02

Copilot AI left a comment

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.

Pull request overview

Strongly types ExP assignment session configuration across all six SDKs while preserving PascalCase wire serialization.

Changes:

  • Adds shared ExP response, configuration-entry, and flag-value types.
  • Updates create/resume APIs and wire requests to use the new types.
  • Revises serialization, clone, unit, and E2E tests.
Show a summary per file
File Description
rust/tests/e2e/client_options.rs Updates ExP forwarding tests.
rust/src/wire.rs Types create/resume wire fields.
rust/src/types.rs Adds ExP types, builders, and tests.
python/test_client.py Tests typed ExP serialization.
python/copilot/client.py Adds dataclasses and wire conversion.
python/copilot/__init__.py Exports ExP types.
nodejs/test/client.test.ts Updates typed payload fixture.
nodejs/src/types.ts Defines ExP TypeScript types.
nodejs/src/index.ts Exports ExP types.
java/src/test/java/com/github/copilot/SessionRequestBuilderTest.java Updates request and clone tests.
java/src/main/java/com/github/copilot/rpc/SessionConfig.java Types create configuration.
java/src/main/java/com/github/copilot/rpc/ResumeSessionRequest.java Types resume wire request.
java/src/main/java/com/github/copilot/rpc/ResumeSessionConfig.java Types resume configuration.
java/src/main/java/com/github/copilot/rpc/ExpConfigEntry.java Adds ExP configuration entries.
java/src/main/java/com/github/copilot/rpc/CreateSessionRequest.java Types create wire request.
java/src/main/java/com/github/copilot/rpc/CopilotExpAssignmentResponse.java Adds ExP response model.
go/types.go Adds ExP types and request fields.
go/internal/e2e/client_options_e2e_test.go Updates forwarding scenarios.
go/client_test.go Updates request serialization tests.
dotnet/test/Unit/SerializationTests.cs Updates serialization and clone tests.
dotnet/src/Types.cs Adds ExP models and serializer metadata.
dotnet/src/Client.cs Types create/resume request records.

Review details

  • Files reviewed: 22/22 changed files
  • Comments generated: 6
  • Review effort level: Medium

Comment thread go/types.go
Comment thread go/types.go
Comment thread go/types.go
Comment thread java/src/main/java/com/github/copilot/rpc/CopilotExpAssignmentResponse.java Outdated
Comment thread java/src/main/java/com/github/copilot/rpc/ExpConfigEntry.java
Comment thread dotnet/src/Types.cs Outdated
@github-actions

This comment has been minimized.

- Go: add MarshalJSON to CopilotExpAssignmentResponse/ExpConfigEntry so
  nil Features/Flights/Configs/Parameters serialize as []/{} instead of
  null, matching the Python/Rust/.NET reference behavior; add a test.
- Java: default AssignmentContext to "" so the required field is not
  dropped by NON_NULL when unset.
- .NET: tighten ExpConfigEntry.Parameters value type from JsonNode? to
  JsonValue? to constrain values to JSON scalars.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43d7a02b-08ff-4e7d-a8c0-afa6dfbe94b0
@github-actions

This comment has been minimized.

Copilot AI left a comment

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.

Review details

  • Files reviewed: 22/22 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread java/src/main/java/com/github/copilot/rpc/ExpConfigEntry.java Outdated
The Id field is required by the wire contract, but defaulting to null let
class-level NON_NULL drop the key for a zero-value entry. Default it to ""
so the required key is always emitted, matching the Go and .NET defaults.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43d7a02b-08ff-4e7d-a8c0-afa6dfbe94b0

Copilot AI left a comment

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.

Review details

Comments suppressed due to low confidence (1)

python/copilot/client.py:3018

  • The adjacent comment still describes this as “opaque JSON,” but this path now accepts and serializes a concrete CopilotExpAssignmentResponse. Update the comment so it does not contradict the new API.
            payload["expAssignments"] = _exp_assignment_response_to_dict(exp_assignments)
  • Files reviewed: 22/22 changed files
  • Comments generated: 1
  • Review effort level: Medium

Comment thread python/copilot/client.py
The expAssignments path now serializes a concrete CopilotExpAssignmentResponse,
so drop the outdated "opaque JSON" wording from the adjacent comments.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43d7a02b-08ff-4e7d-a8c0-afa6dfbe94b0

Copilot AI left a comment

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.

Review details

  • Files reviewed: 22/22 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

Move `use std::collections::HashMap;` into the std import block so
`cargo fmt --check` with the nightly group_imports config passes in CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 43d7a02b-08ff-4e7d-a8c0-afa6dfbe94b0

Copilot AI left a comment

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.

Review details

  • Files reviewed: 22/22 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@vijayrathore5983-dev

Copy link
Copy Markdown

Vijay

Rathor

@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review

The PR successfully adds strongly-typed CopilotExpAssignmentResponse, ExpConfigEntry, and ExpFlagValue across all six SDKs (Node.js, Python, Go, .NET, Java, Rust) in a consistent and well-documented way. The wire contract (PascalCase JSON keys, required collection fields default to empty arrays/objects rather than null) is faithfully mirrored in every language.

One minor inconsistency found: Python ExpConfigEntry.id has no default

All other SDKs default ExpConfigEntry.Id / id to an empty string:

  • Go: ID string — zero-value is ""
  • .NET: public string Id { get; set; } = string.Empty;
  • Java: private String id = ""; (explicitly fixed in a follow-up commit in this very PR — commit 69ab40f)
  • Rust: #[derive(Default)] pub id: String — zero value is ""
  • Node.js/TypeScript: interface Id: string — no default needed at declaration

But in Python (python/copilot/client.py line 160), id has no default value:

`@dataclass`
class ExpConfigEntry:
    id: str          # no default — required positional argument
    parameters: dict[str, ExpFlagValue] = field(default_factory=dict)

Because id has no default but parameters does, Python dataclass rules force callers to always supply idExpConfigEntry() raises TypeError. The Java id field was explicitly defaulted to "" for exactly this reason. Suggest the same fix in Python:

    id: str = ""
    """Identifier of the configuration entry."""

This is minor since ExpConfigEntry is an internal/trusted-integrator type, but keeping zero-value construction consistent across all SDKs matches the stated goal of this PR.

Warning

Firewall blocked 1 domain

The following domain was blocked by the firewall during workflow execution:

  • awmgmcpg

To allow these domains, add them to the network.allowed list in your workflow frontmatter:

network:
  allowed:
    - defaults
    - "awmgmcpg"

See Network Configuration for more information.

Generated by SDK Consistency Review Agent for #2033 · 49.2 AIC · ⌖ 7.66 AIC · ⊞ 5K ·

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants