Python: Add windows junction detection for skills - #7507
Conversation
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
agent-framework-automation
Bot
added
the
python
github-actions
Bot
changed the title
There was a problem hiding this comment.
🟡 Not ready to approve
A newly added test (test_link_probe_detects_windows_reparse_attribute) will fail on non-Windows platforms because it asserts a Windows-only stat constant without skipping.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
This PR hardens Python skill and file-access path handling on Windows by treating directory junctions (and other reparse points) as disallowed “link-like” path segments, closing an escape vector similar to symlinks.
Changes:
- Added a shared private helper (
is_link_or_reparse_point) for detecting symlinks, junctions, and Windows reparse points. - Updated file-based skills discovery/validation to reject any path containing a symlink or reparse point segment (fail-closed on probe errors).
- Consolidated junction creation helpers in tests and added coverage for junction/reparse-point detection.
File summaries
| File | Description |
|---|---|
| python/packages/core/agent_framework/_filesystem.py | Introduces shared link/reparse-point detection helper used by security checks. |
| python/packages/core/agent_framework/_skills.py | Extends skill resource/script scanning and resource validation to treat reparse points like symlinks (and fail closed on probe errors). |
| python/packages/core/agent_framework/_harness/_file_access.py | Reuses the shared helper to consistently filter/deny link-like filesystem entries. |
| python/packages/core/tests/core/conftest.py | Adds a reusable create_junction_or_skip test helper for Windows junction creation. |
| python/packages/core/tests/core/test_skills.py | Updates symlink tests to new API name and adds a junction guard test. |
| python/packages/core/tests/core/test_harness_file_access.py | Switches to shared junction helper and adds a unit test for the Windows reparse-attribute fallback. |
Review details
- Files reviewed: 6/6 changed files
- Comments generated: 1
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Python Test Coverage Report •
Python Unit Test Overview
|
|||||||||||||||||||||||||||||||||||||||||||||
| return | ||
|
|
||
| if FileSkillsSource._has_symlink_in_path(resolved_target, root_directory_path): | ||
| if FileSkillsSource._has_link_or_reparse_point_in_path(resolved_target, root_directory_path): |
There was a problem hiding this comment.
Could we reject a junction before _discover_skill_directories adopts it as the skill_dir? Path.is_dir() follows a child junction, and every check here starts from that junction as root_directory_path, so the junction itself is never inspected and its external SKILL.md, resources, and scripts are loaded. I think this breaks the configured-root trust boundary and can lead to prompt injection or arbitrary script execution when skill approvals are disabled.
Would it make sense to probe each discovery entry with is_link_or_reparse_point before descending, failing closed on OSError?
Motivation & Context
Description & Review Guide
Related Issue
Fixes #
Contribution Checklist
breaking changelabel (or add "[BREAKING]" to the title prefix, before or after any language prefix) — a workflow keeps the label and title prefix in sync automatically.