Python: [BREAKING] Treat nested SKILL.md content as part of the parent skill#6849
Conversation
File-based skill discovery kept descending after finding a SKILL.md, which treated content nested beneath a skill boundary as an independent skill root. Return immediately after recording a directory that contains SKILL.md so everything below it stays part of that skill, and add a regression test with a nested SKILL.md. Fixes microsoft#6682
There was a problem hiding this comment.
Pull request overview
This PR updates Python file-based skill discovery to treat a directory containing SKILL.md as a hard “skill boundary” and stop recursing into its subdirectories, preventing nested directories from being discovered as separate skill roots.
Changes:
- Update
FileSkillsSource._discover_skill_directoriesto return immediately after finding aSKILL.mdin the current directory. - Add a regression test ensuring discovery does not descend into a nested directory that also contains a
SKILL.md.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| python/packages/core/agent_framework/_skills.py | Stops recursion once a SKILL.md is found, enforcing a skill boundary during discovery. |
| python/packages/core/tests/core/test_skills.py | Adds a regression test for “do not discover nested skills under an existing skill boundary”. |
|
Flagged issue The PR changes nested Source: automated DevFlow PR review |
There was a problem hiding this comment.
Automated Code Review
Reviewers: 5 | Confidence: 93%
✓ Correctness
The change adds a single
returnstatement in_searchafter recording a directory that containsSKILL.md. This correctly stops recursion into subdirectories of an already-discovered skill boundary while preserving discovery of sibling skill directories. The logic is sound: thereturnexits only the current recursive call, so the parent's iteration over other entries continues normally. The new test properly validates the boundary behavior.
✓ Security Reliability
Clean, minimal change that adds a single
returnto stop recursive skill discovery below an already-found SKILL.md boundary. No security or reliability concerns: the existing OSError handling, depth limit, and input validation remain intact. The change actually improves reliability by preventing unintended discovery of nested skill content as independent skills.
✓ Test Coverage
The PR adds a single
returnstatement to stop recursion after discovering a skill boundary, with a well-structured regression test that directly validates the new behavior. Test coverage is adequate: the new test verifies that nested SKILL.md directories below an already-discovered skill boundary are not treated as independent skills. Existing tests cover sibling skill discovery (via the parent loop), root-level skills, depth limits, and edge cases. One minor suggestion for additional coverage.
✓ Failure Modes
The change is a minimal, correct one-line addition of
returnafter recording a skill directory in_discover_skill_directories._search. This stops recursion below skill boundaries, aligning discovery behavior with the sibling_scan_directory_for_scriptsmethod (which already skips subdirectories containing their own SKILL.md at line 3112). No silent failure modes, lost errors, or operational issues are introduced. The existingtest_finds_nested_skilltest remains valid because it tests a case where only the child has SKILL.md (not the parent). The new regression test correctly validates the boundary behavior.
✗ Design Approach
The new boundary rule changes more than file attachment: it suppresses discovery of nested skills entirely. Existing tests already establish that a nested
SKILL.mddefines a separate child skill whose files must not be attached to the parent, so the new test is asserting behavior that contradicts the current contract and would hide a real regression inget_skills().
Flagged Issues
- The PR changes nested
SKILL.mdhandling from 'discover both skills, but keep their files separate' to 'ignore the nested skill entirely'. That contradicts the existing contract inpython/packages/core/tests/core/test_skills.py:1810-1845, wheretest_nested_skill_directory_not_crossedexplicitly asserts both parent and child skills are discovered, andFileSkillsSource.get_skills()still sources its candidates from_discover_skill_directories()atpython/packages/core/agent_framework/_skills.py:2706.
Automated review by giles17's agents
Removing the SKILL.md subdirectory skip in resource and script scanning so that content beneath a skill boundary is attached to that skill, and update the discovery docstring and the nested-skill test to match. Complements the discovery early-return so a nested SKILL.md is never treated as an independent skill root.
Python Test Coverage Report •
Python Unit Test Overview
|
||||||||||||||||||||||||||||||
Motivation & Context
File-based skill discovery kept descending past a
SKILL.md, so aSKILL.mdnested inside another skill's directory was treated as an independent skill root, and content beneath a skill boundary was split away from its skill. Discovery should stop at the first skill boundary, and everything beneath that boundary should belong to that skill.Description & Review Guide
_discover_skill_directories: stop recursing once aSKILL.mdis found, so a nestedSKILL.mdis no longer discovered as a separate skill._scan_directory_for_resources/_scan_directory_for_scripts: no longer skip subdirectories that contain their ownSKILL.md, so resources and scripts beneath a skill boundary are attached to that skill (theSKILL.mdfile itself is still excluded as a resource).[BREAKING — experimental]change to the skills feature (ExperimentalFeature.SKILLS)._discover_skill_directoriesand the recursion in the resource/script scans.Related Issue
Fixes #6682
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and the title prefix in sync automatically.