Skip to content

.NET: Fix SearchDirectoriesForSkills to stop recursing after finding SKILL.md - #6686

Merged
SergeyMenshykh merged 2 commits into
microsoft:mainfrom
SergeyMenshykh:sergeymenshykh-fix-search-directories-stop-recurse-on-s
Jun 24, 2026
Merged

.NET: Fix SearchDirectoriesForSkills to stop recursing after finding SKILL.md#6686
SergeyMenshykh merged 2 commits into
microsoft:mainfrom
SergeyMenshykh:sergeymenshykh-fix-search-directories-stop-recurse-on-s

Conversation

@SergeyMenshykh

@SergeyMenshykh SergeyMenshykh commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Motivation & Context

SearchDirectoriesForSkills finds a SKILL.md in a directory and adds it as a skill, but then keeps recursing into subdirectories. This causes subdirectories beneath a skill to be incorrectly treated as independent skill roots.

Description & Review Guide

  • What are the major changes? One-line fix: add a return after results.Add(...) so recursion stops once a SKILL.md is found.
  • What is the impact of these changes? Subdirectories of a skill are no longer discovered as separate skills. The standard flat layout is unaffected (the root directory has no SKILL.md). Script/resource scanning is unaffected — those use their own separate recursion.
  • What do you want reviewers to focus on? Whether there's a legitimate use case for nested independent skills within a parent skill boundary.

Related Issue

Fixes #6683

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 (see Related Issue above).
  • This is not a breaking change.

When a directory contains SKILL.md, subdirectories are part of that skill
and should not be treated as independent skill roots. Add a return after
adding the directory to results to prevent incorrect recursion.

Also adds a regression test verifying nested SKILL.md files are not
discovered as separate skills.

Fixes microsoft#6683

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings June 23, 2026 15:40
@moonbox3 moonbox3 added the .NET Usage: [Issues, PRs], Target: .Net label Jun 23, 2026
@SergeyMenshykh SergeyMenshykh self-assigned this Jun 23, 2026
@SergeyMenshykh SergeyMenshykh moved this to In Review in Agent Framework Jun 23, 2026

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

This PR adjusts .NET file-based skill discovery so that once a directory containing SKILL.md is identified as a skill root, discovery stops recursing into that directory’s subfolders—preventing nested directories from being incorrectly treated as independent skill roots.

Changes:

  • Add an early return in SearchDirectoriesForSkills after finding SKILL.md, establishing a skill boundary.
  • Add a unit test intended to verify nested SKILL.md files are not discovered as separate skills.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs Stops recursion into subdirectories once a SKILL.md skill root is found.
dotnet/tests/Microsoft.Agents.AI.UnitTests/AgentSkills/FileAgentSkillLoaderTests.cs Adds coverage for the nested-skill-root scenario (but currently needs a fix to actually validate the behavior).

@github-actions github-actions Bot 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.

Automated Code Review

Reviewers: 5 | Confidence: 93% | Result: All clear

Reviewed: Correctness, Security Reliability, Test Coverage, Failure Modes, Design Approach


Automated review by SergeyMenshykh's agents

…alidation

The child skill's frontmatter name must match its directory name,
otherwise it gets rejected by validation regardless of the recursion fix.
This ensures the test actually validates the stop-recursing behavior.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SergeyMenshykh
SergeyMenshykh marked this pull request as ready for review June 23, 2026 15:47

@github-actions github-actions Bot 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.

Automated Code Review

Reviewers: 5 | Confidence: 94%

✓ Correctness

The one-line return fix is correct. It only exits the current recursion frame when a SKILL.md is found, so sibling directories at the same level are still discovered normally. The test properly validates the behavior: the child directory is named child and its SKILL.md declares name: child, so it would pass the directory-name validation (line 287) if discovered—confirming the test actually exercises the new early-return logic rather than relying on a pre-existing name-mismatch rejection.

✓ Security Reliability

This is a clean, minimal fix that adds early return after discovering a SKILL.md to prevent subdirectories from being incorrectly treated as independent skill roots. No security or reliability issues found. The recursive method already has depth-limiting via MaxSkillDirectorySearchDepth, and the paths are properly normalized via Path.GetFullPath. No new trust boundaries, injection vectors, resource leaks, or unhandled failure modes are introduced. The test properly validates the fix by using a child directory name ('child') that matches the frontmatter name, so the child would pass validation if incorrectly discovered.

✓ Test Coverage

The test coverage for this PR is adequate. The new test correctly validates the fix by creating a nested child skill with name: child matching the directory name child, ensuring the child would pass all validation if discovered. This properly addresses the resolved review comment about the earlier version using a non-matching name. The test would fail without the fix (child would be independently discovered) and passes with it (recursion stops at parent). No significant test coverage gaps found.

✓ Failure Modes

The change is a clean, minimal fix that adds an early return after discovering a SKILL.md to prevent recursion into subdirectories. The logic is straightforward with no silent failure paths, exception swallowing, partial writes, or race conditions introduced. The test correctly validates the behavior by using a child directory name ('child') that matches the child SKILL.md frontmatter name, ensuring the child would have been discovered as a valid independent skill without the fix.

✗ Design Approach

The new early return fixes the reported nested-skill case for valid parent skills, but it also changes discovery to treat the mere presence of SKILL.md as a hard traversal boundary before the loader knows whether that file is a valid skill. That introduces a false-negative path where an invalid parent SKILL.md now prevents discovery of valid nested skills beneath it.

Flagged Issues

  • SearchDirectoriesForSkills now stops at any directory containing SKILL.md (AgentFileSkillsSource.cs:163-168), but ParseSkillDirectory can still reject that file later for invalid frontmatter or a name/directory mismatch (lines 187-190, 213-216, 286-295). A tree like parent/SKILL.md (invalid) + parent/child/SKILL.md (valid) silently loses the child skill entirely, because discovery never descends into parent once the invalid marker file exists.

Automated review by SergeyMenshykh's agents

Comment thread dotnet/src/Microsoft.Agents.AI/Skills/File/AgentFileSkillsSource.cs
@github-actions

Copy link
Copy Markdown
Contributor

Flagged issue

SearchDirectoriesForSkills now stops at any directory containing SKILL.md (AgentFileSkillsSource.cs:163-168), but ParseSkillDirectory can still reject that file later for invalid frontmatter or a name/directory mismatch (lines 187-190, 213-216, 286-295). A tree like parent/SKILL.md (invalid) + parent/child/SKILL.md (valid) silently loses the child skill entirely, because discovery never descends into parent once the invalid marker file exists.


Source: automated DevFlow PR review

@SergeyMenshykh
SergeyMenshykh enabled auto-merge June 23, 2026 16:08
@SergeyMenshykh
SergeyMenshykh added this pull request to the merge queue Jun 23, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 23, 2026
@SergeyMenshykh
SergeyMenshykh added this pull request to the merge queue Jun 23, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 23, 2026
@SergeyMenshykh
SergeyMenshykh added this pull request to the merge queue Jun 23, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jun 23, 2026
@SergeyMenshykh
SergeyMenshykh added this pull request to the merge queue Jun 24, 2026
Merged via the queue into microsoft:main with commit dd4b7ff Jun 24, 2026
29 checks passed
@github-project-automation github-project-automation Bot moved this from In Review to Done in Agent Framework Jun 24, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

.NET Usage: [Issues, PRs], Target: .Net

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

.NET: AgentFileSkillsSource.SearchDirectoriesForSkills should stop recursing after finding SKILL.md

6 participants