Repository navigation
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>
Keep both the skill-provider clone tests and upstream extension-provider coverage. Preserve the upstream structured-output and RPC changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Retain transcript recovery with the provider flag and align provider tests with upstream per-request cancellation. Keep upstream transport and generated code unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Closing as superseded by the upstream native skill-provider API, now available in the official GitHub.Copilot.SDK 1.0.17-preview.7 package. Lumi has been migrated from the temporary custom SDK to the official ISkillProvider contract. Validation covered lazy native activation, updated skill bodies, provider rebinding on resume, delegated agents, and coexistence with file-based skills. Windows and Linux builds, targeted/native integration tests, fresh-chat live checks, and single-file publishing/runtime startup with the full CLI sign-in contract all passed. Observed full-suite flakiness also reproduced on unchanged Lumi main and passed in focused isolation; no migration regression was found. This PR no longer needs to merge. Thanks! |
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