Conversation
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>
There was a problem hiding this comment.
🟡 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.
| 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)); | ||
| } |
There was a problem hiding this comment.
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.
| /// 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. |
There was a problem hiding this comment.
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.
| /// 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"/>. |
There was a problem hiding this comment.
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.
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>
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
skilltool without either workaround.The published CLI 1.0.83 protocol already contains internal/experimental
skillProvider.list,skillProvider.read, andhasSkillProvidersupport. 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
[Experimental]SkillProviderwith asynchronous metadata listing and lazy Markdown reads, plus a typed descriptor.SessionConfigBase.SkillProviderand preserve it in create/resume config clones.hasSkillProvideronly when a provider is configured.Example API:
Scope and behavior
EnableSkills = falsekeeps the provider bound but dormant.CopilotClientMode.Emptycallers should explicitly enable skills.Validation
On the current SDK main base (
a675b55):git diff --checkpassed.Commands (from
dotnet):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