Skip to content

[dotnet-port] Align fsskills nested discovery with .NET - #348

Merged
Quim Muntal (qmuntal) merged 3 commits into
mainfrom
dotnet-port-fsskills-stop-recursing-7ef700e8ce62f08d
Jun 25, 2026
Merged

[dotnet-port] Align fsskills nested discovery with .NET#348
Quim Muntal (qmuntal) merged 3 commits into
mainfrom
dotnet-port-fsskills-stop-recursing-7ef700e8ce62f08d

Conversation

@michelle-clayton-work

Copy link
Copy Markdown
Contributor

Stop recursive skill root discovery once a SKILL.md is found so nested files remain part of the parent skill instead of becoming independent skill roots.

Stop recursive skill root discovery once a SKILL.md is found so nested files remain part of the parent skill instead of becoming independent skill roots.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

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 updates the file-system skill discovery logic to match the .NET behavior by treating a directory containing SKILL.md as a terminal “skill root” (i.e., do not recursively discover nested skill roots beneath it). This prevents nested SKILL.md files from being interpreted as separate skills and instead keeps nested content logically under the parent skill root.

Changes:

  • Stop recursive skill-root discovery once a SKILL.md is found in a directory.
  • Add a unit test asserting that a nested SKILL.md under a skill root does not produce an additional discovered skill.

Reviewed changes

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

File Description
agent/skills/fsskills/source.go Adds an early return to halt further recursive discovery once a skill root is detected.
agent/skills/fsskills/source_test.go Adds coverage ensuring nested SKILL.md does not create an independent skill.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread agent/skills/fsskills/source.go Outdated
@github-actions

This comment has been minimized.

Copilot AI and others added 2 commits June 24, 2026 20:44
Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>
Co-authored-by: michelle-clayton-work <262183035+michelle-clayton-work@users.noreply.github.com>
@github-actions

Copy link
Copy Markdown
Contributor

Cross-repo parity review

The Go change in searchForSkills (stop recursing once a SKILL.md is found) correctly mirrors the .NET SearchDirectoriesForSkills behavior, which also returns immediately after appending the skill directory.

However, the Python equivalent — the inner _search closure in FileSkillsSource._discover_skill_directoriesdoes not return after appending a discovered skill path:

def _search(directory: str, current_depth: int) -> None:
    dir_path = Path(directory)
    if (dir_path / SKILL_FILE_NAME).is_file():
        discovered.append(str(dir_path.absolute()))
        # ← no return; recursion continues into subdirectories
    if current_depth >= MAX_SEARCH_DEPTH:
        return
    for entry in entries:
        if entry.is_dir():
            _search(str(entry), current_depth + 1)

This means for a layout like:

skills/
  parent/
    SKILL.md
    child/
      SKILL.md
  • .NET / Go (after this PR): only parent/ is registered as a skill root; child/ is absorbed as part of parent/'s content tree
  • Python: both parent/ and child/ are registered as independent skills

This is a meaningful behavioral divergence for any user who authors nested SKILL.md trees and expects consistent cross-SDK behavior.

Suggested follow-up: open a tracking issue (or add a comment here) and align the Python implementation to add an early return after the discovered.append(...) call in _discover_skill_directories._search, matching .NET and Go semantics. The upstream file to change is:
python/packages/core/agent_framework/_skills.pyFileSkillsSource._discover_skill_directories inner _search function.

Generated by Go API Consistency Review Agent for issue #348 · 233.7 AIC · ⌖ 13 AIC · ⊞ 26.8K ·

@qmuntal
Quim Muntal (qmuntal) added this pull request to the merge queue Jun 25, 2026
@qmuntal
Quim Muntal (qmuntal) removed this pull request from the merge queue due to a manual request Jun 25, 2026
@qmuntal
Quim Muntal (qmuntal) added this pull request to the merge queue Jun 25, 2026
Merged via the queue into main with commit 440448d Jun 25, 2026
17 checks passed
@qmuntal
Quim Muntal (qmuntal) deleted the dotnet-port-fsskills-stop-recursing-7ef700e8ce62f08d branch June 25, 2026 08:32
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.

[dotnet-port] Align fsskills nested discovery with .NET

4 participants