Pull request overview

Refactors Python file-based skill discovery to match the newer .NET behavior by replacing fixed subdirectory whitelists with depth-based recursive scanning and optional predicate filters, while keeping existing containment/symlink safety checks.

Changes:

• Replace resource_directories / script_directories with search_depth (default 2) for recursive discovery within each skill directory.
• Add script_filter / resource_filter predicates using the new exported FileSkillFilterContext to allow per-file inclusion/exclusion decisions.
• Rewrite/extend unit tests to validate depth behavior, filtering, and existing safety guarantees (path traversal + symlink skipping).

Reviewed changes

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

Automated Code Review

Reviewers: 5 | Confidence: 89%

✓ Correctness

The refactoring from directory-whitelist to depth-based scanning with filter predicates is well-implemented and correct. The depth logic correctly maps search_depth=1 to root-only, search_depth=2 to root+one-level, etc. Security checks (path containment, symlink detection) are preserved at all recursion levels. Filter predicates are applied after security checks, and the FileSkillFilterContext validation is sound. The from_paths factory method correctly forwards new parameters. No correctness bugs found.

✓ Security Reliability

The refactoring correctly preserves all security invariants (path containment, symlink detection at both directory and file levels). The depth-based recursion applies directory-level security checks before descending and file-level checks before including any resource. One minor reliability concern: unhandled exceptions from user-provided filter predicates would crash the entire skill discovery process rather than gracefully skipping the affected file.

✓ Test Coverage

Test coverage for the refactored FileSkillsSource is generally strong. Depth-based discovery is well-tested for resources at both unit and integration levels. Filter predicates are tested for both resources and scripts. However, there's an asymmetry: resource_filter has a dedicated context-verification test (test_resource_filter_receives_correct_context) confirming that skill_name and relative_file_path are passed correctly, but script_filter lacks an equivalent test. Similarly, from_paths has a passthrough test for resource_filter and search_depth but not for script_filter. These are low-severity gaps since the code paths are structurally identical.

✓ Failure Modes

The refactoring from directory-whitelists to depth-based recursive scanning is well-implemented from a failure-mode perspective. Error handling for OS-level failures (iterdir OSError), security checks (symlinks, path traversal), and input validation (search_depth >= 1) are all preserved or improved. User-provided filter predicates follow standard Python fail-fast callback semantics. No silent failures, lost errors, or partial-write scenarios were identified.

✗ Design Approach

The refactor mostly hangs together, but the new recursive file discovery no longer preserves skill boundaries when one skill directory contains another nested skill directory. Because nested SKILL.md folders are still discovered as separate skills elsewhere, the current approach can silently attach a child skill’s resources/scripts to its parent as well, which is a design regression worth fixing before merge.

Flagged Issues

• Nested skill directories are treated as separate skills by _discover_skill_directories (lines 3161-3195), but the new recursive resource/script scanners descend into every subdirectory (lines 2796-2807 and 2945-2956) without checking for a nested SKILL.md. With parent/SKILL.md and parent/child/SKILL.md, the child's files will be surfaced under both skills. The scaners should stop recursing into subdirectories that contain their own SKILL.md.

Automated review by giles17's agents