Skip to content

.NET: add experimental session-scoped skill providers - #2672

Open
adirh3 wants to merge 2 commits into
github:mainfrom
adirh3:feat/dotnet-native-skill-provider
Open

adirh3 wants to merge 2 commits into
github:mainfrom
adirh3:feat/dotnet-native-skill-provider

Conversation

@adirh3

@adirh3 adirh3 commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

Proposal and API alignment

Related to #2651.

This implementation is ready for maintainer review, not a claim that the feature or a .NET-only rollout has been agreed. The contribution guide asks for maintainer alignment and cross-language consistency; the issue describes the use case, and this PR offers a concrete .NET implementation for that discussion.

Motivation

SDK hosts may store reusable skills in application-managed storage rather than project files. Today, such a host must materialize skill files or expose a separate custom loading tool. A session-scoped provider lets these skills participate in the existing native skill tool without either workaround.

The published CLI 1.0.83 protocol already contains internal/experimental skillProvider.list, skillProvider.read, and hasSkillProvider support. This change exposes an idiomatic experimental .NET facade over that existing mechanism. It does not make the raw transport handlers public or change generated protocol code.

Changes

  • Add [Experimental] SkillProvider with asynchronous metadata listing and lazy Markdown reads, plus a typed descriptor.
  • Add non-serialized SessionConfigBase.SkillProvider and preserve it in create/resume config clones.
  • Register typed reverse-RPC handlers before listening and bind each provider to its owning session before the legacy create/resume requests.
  • Send hasSkillProvider only when a provider is configured.
  • Resolve callbacks by the existing session registry, clear provider references during cleanup, and use the existing RPC error/cancellation and source-generated JSON infrastructure.
  • Add focused public-API unit tests and a .NET usage example.

Example API:

#pragma warning disable GHCP001
await using var session = await client.CreateSessionAsync(new SessionConfig
{
    SkillProvider = new ApplicationSkills(),
    EnableSkills = true,
});

// ApplicationSkills implements:
// Task<IReadOnlyList<SkillProviderDescriptor>> ListAsync(CancellationToken)
// Task<string> ReadAsync(string name, CancellationToken)

Scope and behavior

  • Opt-in: sessions without a provider keep their existing wire configuration.
  • Text-only and pathless; no supporting-file/asset provider, virtual filesystem, or storage policy.
  • Native activation is unchanged: no extra loading tool, tool override, or transport proxy.
  • The binding is ephemeral and must be supplied again on resume.
  • EnableSkills = false keeps the provider bound but dormant. CopilotClientMode.Empty callers should explicitly enable skills.
  • Catalog metadata and Markdown frontmatter must agree; the native runtime retains validation responsibility.
  • This does not add cloud-session support; all cloud configurations are rejected before connecting, including callers supplying an explicit session ID.
  • The public contract documents the native limits: at most 1,024 catalog descriptors, 1 MiB of aggregate metadata, and 1 MiB of UTF-8 text per Markdown response.
  • No CLI version bumps, dependency changes, workflow modifications, or application-specific behavior.

Validation

On the current SDK main base (a675b55):

  • 296 targeted .NET 8 tests passed across provider/lifecycle, cloning, JSON-RPC, serialization, public DTO, and forward-compatibility coverage, including both cloud session-ID cases.
  • The existing .NET 8 test project disables reflection-based System.Text.Json serialization.
  • Release builds passed for net8.0, net10.0, and netstandard2.0, with zero warnings or errors.
  • git diff --check passed.

Commands (from dotnet):

dotnet test test/GitHub.Copilot.SDK.Test.csproj -f net8.0 -c Release -p:CopilotSkipCliDownload=true --filter "FullyQualifiedName~ClientSessionLifetimeTests|FullyQualifiedName~CloneTests|FullyQualifiedName~JsonRpcTests|FullyQualifiedName~SerializationTests|FullyQualifiedName~PublicDtoTests|FullyQualifiedName~ForwardCompatibilityTests"
dotnet build src/GitHub.Copilot.SDK.csproj -c Release --no-restore -p:CopilotSkipCliDownload=true

These committed tests use the SDK's existing fake server and public APIs. Separately, a host prototype against the real CLI 1.0.83 runtime exercised native tool loading, file-skill coexistence, reload, resume, and subagent skill preloading. That prior prototype evidence is not a claim that runtime E2E coverage is included in this PR.

A further isolated probe of this exact PR against CLI 1.0.84-8 passed genuinely asynchronous list/read callbacks, native/file skill coexistence, session isolation, reload/resume rebinding, read-error recovery, and disabled/no-provider controls. It used locally mocked inference; the probe is separate from this PR's committed tests. Callback cancellation follows the existing RPC connection lifetime, not turn-abort lifetime.

Maintainer questions

  • Is exposing this native provider capability aligned with the intended public SDK roadmap?
  • Is an abstract provider with list/read methods the preferred .NET shape, or should it use a different facade?
  • Should corresponding language implementations land together before the API is offered, even experimentally?

Expose session-scoped metadata listing and lazy Markdown callbacks through the existing native skill provider protocol.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@adirh3
adirh3 marked this pull request as ready for review September 15, 2026 18:47
@adirh3
adirh3 requested a review from a team as a code owner September 15, 2026 18:47
Copilot AI balanced review requested due to automatic review settings September 15, 2026 18:47

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.

🟡 Changes recommended

Explicit-ID cloud sessions bypass validation even though the protocol rejects skill providers for all cloud creation flows.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds an experimental .NET facade for session-scoped, in-memory skill providers proposed in #2651.

Changes:

  • Adds provider APIs, descriptors, configuration, and documentation.
  • Implements session-scoped reverse-RPC routing and cleanup.
  • Adds lifecycle, serialization, cloning, and isolation tests.
File summaries
File Description
dotnet/src/SkillProvider.cs Defines the experimental provider API.
dotnet/src/Types.cs Adds provider configuration and cloning.
dotnet/src/Session.cs Stores and invokes session providers.
dotnet/src/Client.cs Registers callbacks and wire flags.
dotnet/README.md Documents provider usage.
dotnet/test/Unit/CloneTests.cs Tests configuration cloning.
dotnet/test/Unit/ClientSessionLifetimeTests.cs Enables split lifetime tests.
dotnet/test/Unit/ClientSessionLifetimeTests.SkillProvider.cs Tests provider lifecycle and RPC behavior.
Review details

Suppressed comments (1)

dotnet/src/SkillProvider.cs:44

  • The protocol enforces a 1 MiB UTF-8 byte limit on each Markdown response (nodejs/src/generated/rpc.ts:21508-21512). Include that bound in this public contract so implementations can enforce it before returning content that the runtime will reject.
    /// The complete Markdown, including YAML frontmatter matching the metadata returned by
    /// <see cref="ListAsync"/>. The native runtime rejects inconsistent metadata.
  • Files reviewed: 8/8 changed files
  • Comments generated: 3
  • Review effort level: Balanced (auto)

Note

Copilot is running an experiment and ran this review at Balanced.


💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread dotnet/src/Client.cs Outdated
Comment on lines +1174 to +1179
if (config.SkillProvider is not null && config.Cloud is not null && string.IsNullOrEmpty(config.SessionId))
{
throw new ArgumentException(
"SkillProvider requires a session ID before creation; server-assigned cloud session IDs are not supported.",
nameof(config));
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 82b8aa8: every Cloud + SkillProvider configuration now fails before connecting, including explicit session IDs. The parameterized regression test covers both generated and explicit IDs. The 296 targeted tests and all three SDK target-framework builds pass.

Comment thread dotnet/src/Types.cs Outdated
/// The provider is registered before the create/resume request and is not serialized or persisted.
/// Supply it again on resume. Setting <see cref="EnableSkills"/> to <see langword="false"/>
/// keeps the provider bound but prevents the runtime from loading skills. File-based skills may
/// coexist with provider skills. Cloud creation with a server-assigned session ID is not supported.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 82b8aa8: the configuration remarks, README, and PR description now consistently state that cloud sessions do not support skill providers regardless of who supplies the session ID.

Comment on lines +32 to +33
/// The skill catalog, or an empty list when no skills are available. Names must be unique
/// using a case-insensitive comparison and satisfy <see cref="SkillProviderDescriptor.Name"/>.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 82b8aa8: ListAsync documents the 1,024-descriptor and 1 MiB aggregate metadata limits; ReadAsync documents the 1 MiB UTF-8 response limit. The README includes the same bounds. Validation remains with the native runtime.

adirh3 added a commit to adirh3/Lumi that referenced this pull request Sep 16, 2026
Use the unofficial package built from github/copilot-sdk#2672 through a credential-free local feed, and remove SDK source/patch workflow coupling.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Reject explicit-ID cloud sessions before connecting and document the existing native catalog/Markdown bounds.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
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.

2 participants