.NET: [BREAKING] Extract caching from AgentSkillsProvider into CachingAgentSkillsSource

[SergeyMenshykh]

Motivation & Context

AgentSkillsProvider mixes context construction with caching. This extracts caching into a composable CachingAgentSkillsSource decorator, consistent with the existing Deduplicating/Filtering/DelegatingAgentSkillsSource patterns.

Description & Review Guide

• What are the major changes?

  • New internal CachingAgentSkillsSource decorator (lock-free, thread-safe; clears on failure).
  • Builder applies caching after aggregation, before filter/dedup; adds DisableCaching() opt-out.
  • Convenience constructors wrap with CachingAgentSkillsSource before dedup.
  • Removed DisableCaching from AgentSkillsProviderOptions and caching logic from the provider.

• What is the impact of these changes?

  • Caching is a composable pipeline layer instead of provider-internal logic.
  • The custom-source constructor no longer caches implicitly - use the builder or wrap explicitly.

Closes #6765

Contribution Checklist

• The code builds clean without any errors or warnings
• All unit tests pass, and I have added new tests where possible
• The PR follows the Contribution Guidelines
• This PR is linked to an issue and there is no other open PR for this issue
• This is not a breaking change.

[github-actions]

Automated Code Review

Reviewers: 5 | Confidence: 92%

✓ Correctness

The CachingAgentSkillsSource implementation is correct and thread-safe, using the same lock-free pattern as the removed provider-level caching. The builder pipeline (aggregate → cache → filter → dedup) is correctly ordered. The DisableCaching property removal from AgentSkillsProviderOptions is safe given the [Experimental] attribute. The only gap is the new builder DisableCaching() method has no test coverage — the old test was removed but not replaced with one exercising the new API.

✓ Security Reliability

The PR cleanly extracts caching into a composable CachingAgentSkillsSource decorator using a well-known lock-free pattern (Interlocked.CompareExchange + TaskCompletionSource). The thread-safety, error-clearing, and pipeline ordering are correct. No security vulnerabilities, resource leaks, or unhandled failure modes were identified. The only minor observation is an unnecessary TCS allocation on every cache-hit path, but this is a performance nit rather than a reliability concern.

✓ Test Coverage

The new CachingAgentSkillsSource has good unit test coverage (sequential caching, concurrency, error recovery, correctness). However, the new AgentSkillsProviderBuilder.DisableCaching() method — which controls whether CachingAgentSkillsSource is applied in the Build() pipeline — has no test coverage. The old test Build_WithCacheDisabled_ReloadsOnEachCallAsync was removed but not replaced with one exercising the new method.

✓ Failure Modes

The extraction of caching into CachingAgentSkillsSource is clean. The lock-free pattern (CompareExchange + TCS) correctly ensures single-flight fetch, cache-on-success, and clear-on-failure. The error path properly resets _cachedTask before setting the TCS exception, allowing subsequent callers to retry. The builder correctly applies caching after aggregation but before filter/dedup. No silent failures, lost errors, or dangerous race conditions identified.

✓ Design Approach

The extracted cache mostly matches the old provider behavior, but it introduces a concrete concurrency/cancellation bug in the new decorator: whichever caller wins the CAS at CachingAgentSkillsSource.GetSkillsAsync donates its cancellation token to the shared fetch, so other concurrent callers can be canceled even when they passed CancellationToken.None. The repo already has a better pattern for shared refreshes in AgentMcpSkillsSource that preserves per-caller cancellation without letting one caller abort the shared work.

---

Automated review by SergeyMenshykh's agents

[Copilot]

Pull request overview

This PR refactors .NET skills caching so it becomes a composable decorator (CachingAgentSkillsSource) in the AgentSkillsSource pipeline rather than being implemented inside AgentSkillsProvider, enabling callers (via the builder) to control where caching sits relative to aggregation/filtering/deduplication.

Changes:

• Introduces CachingAgentSkillsSource and wires it into AgentSkillsProviderBuilder (default on) with a new opt-out DisableCaching() method.
• Updates AgentSkillsProvider convenience constructors to wrap file/in-memory sources with CachingAgentSkillsSource, and removes provider-internal AIContext caching.
• Removes DisableCaching from AgentSkillsProviderOptions and updates unit tests to reflect source-level caching.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
dotnet/src/Microsoft.Agents.AI/Skills/Decorators/CachingAgentSkillsSource.cs	Adds a new lock-free caching decorator for AgentSkillsSource.
dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProviderBuilder.cs	Adds builder-level caching control and places caching after aggregation, before filter/dedup.
dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProvider.cs	Removes provider-level context caching and applies caching via source decorators in convenience constructors.
dotnet/src/Microsoft.Agents.AI/Skills/AgentSkillsProviderOptions.cs	Removes the public DisableCaching option from provider options.
dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/CachingAgentSkillsSourceTests.cs	Adds unit tests covering caching behavior (single, concurrent, failure retry).
dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentSkillsProviderTests.cs	Updates tests to validate source-level caching semantics.
dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/AgentSkillsProviderBuilderTests.cs	Removes tests that were specific to the old options-based caching disable mechanism.

[github-actions]

Automated Code Review

Reviewers: 5 | Confidence: 92%

✓ Correctness

The PR cleanly extracts caching from AgentSkillsProvider into a composable CachingAgentSkillsSource decorator. The lock-free CAS caching pattern is correct, Volatile.Write is used for cache clearing on failure, OperationCanceledException is handled separately from general exceptions, and the builder pipeline order (aggregate → cache → filter → dedup) is sound since filter/dedup create new lists rather than mutating the cached result. The convenience constructors properly wrap with CachingAgentSkillsSource before DeduplicatingAgentSkillsSource. Compatibility suppressions are added for all target frameworks. Tests are properly updated to match the new source-level caching semantics. No correctness issues found.

✓ Security Reliability

The PR cleanly extracts caching into a composable CachingAgentSkillsSource decorator. The lock-free CAS pattern is correctly implemented with Volatile.Write for cache clearing, separate OperationCanceledException handling, and RunContinuationsAsynchronously on the TCS. The breaking API removal (AgentSkillsProviderOptions.DisableCaching) is properly suppressed via CompatibilitySupressions.xml across all target frameworks. The [Experimental] attribute removal from internal decorators is consistent. Test coverage for the new DisableCaching() builder method and the CachingAgentSkillsSource class is adequate. No new security or reliability issues were identified beyond what was already discussed and resolved in the prior review thread.

✓ Test Coverage

The PR has good test coverage for the new CachingAgentSkillsSource (basic caching, concurrency, error recovery, result passthrough) and for the builder's DisableCaching() method. However, the OperationCanceledException catch branch in CachingAgentSkillsSource (lines 48-52) has distinct behavior from the general exception path (TrySetCanceled vs TrySetException, cache clearing) and is not exercised by any test.

✓ Failure Modes

The PR cleanly extracts caching from AgentSkillsProvider into a composable CachingAgentSkillsSource decorator. The lock-free caching pattern is correctly implemented with Volatile.Write for cache clearing on failure and proper separation of OperationCanceledException vs general Exception handling. The builder pipeline ordering (source → cache → filter → dedup) is correct. The behavioral change where the custom-source constructor no longer caches implicitly is intentional and documented. All previously-reviewed issues (memory barriers, cancellation handling, experimental attribute, missing tests, breaking-change suppression) have been addressed. No new failure modes were identified.

✓ Design Approach

The extraction to a composable caching decorator is directionally good, but the new cache layer still gets cancellation ownership wrong for shared in-flight loads. That leaves one real correctness issue in the design: a single caller’s token cancel the shared fetch for everyone else, while followers cannot cancel their own wait independently.

---

Automated review by SergeyMenshykh's agents