fix(micro-fix): posix only permission check for skill#6889
fix(micro-fix): posix only permission check for skill#6889TimothyZhang7 wants to merge 1 commit intomainfrom
Conversation
📝 WalkthroughWalkthroughA platform-specific conditional guard is added to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@core/framework/skills/validator.py`:
- Around line 137-141: The POSIX check in the scripts existence branch uses a
broad test (os.name != "nt"); update the conditional to explicitly check for
POSIX by replacing that test with os.name == "posix" so the permission-bit logic
only runs on POSIX platforms; locate the block using base_dir and scripts_dir
(the if that currently reads "if scripts_dir.is_dir() and os.name != 'nt':") and
change the os.name check accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: efa05fd2-0c9b-415e-a752-59d53eece65d
📒 Files selected for processing (1)
core/framework/skills/validator.py
| # 11. Scripts in scripts/ exist and are executable (POSIX only — | ||
| # Windows does not use POSIX permission bits) | ||
| base_dir = path.parent | ||
| scripts_dir = base_dir / "scripts" | ||
| if scripts_dir.is_dir(): | ||
| if scripts_dir.is_dir() and os.name != "nt": |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify platform-gate usage and whether tests explicitly cover platform branching.
rg -n -C2 'os\.name\s*(==|!=)\s*"(nt|posix)"' --type=py
rg -n -C3 'TestCheck11Scripts|test_error_on_non_executable_script|test_passes_with_executable_script|monkeypatch|os\.name' core/tests/test_skill_validator.pyRepository: aden-hive/hive
Length of output: 3358
🏁 Script executed:
# Get the full validator check implementation around lines 137-150
sed -n '137,160p' core/framework/skills/validator.py
# Get the full test class implementation
grep -n "class TestCheck11Scripts" core/tests/test_skill_validator.py
# Then read the full test class
sed -n '274,320p' core/tests/test_skill_validator.pyRepository: aden-hive/hive
Length of output: 2722
Use an explicit POSIX check at line 141.
os.name != "nt" is broader than POSIX and can evaluate true on non-POSIX runtimes. For a POSIX-only permission-bit check, gate on os.name == "posix".
Suggested patch
- if scripts_dir.is_dir() and os.name != "nt":
+ if scripts_dir.is_dir() and os.name == "posix":📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # 11. Scripts in scripts/ exist and are executable (POSIX only — | |
| # Windows does not use POSIX permission bits) | |
| base_dir = path.parent | |
| scripts_dir = base_dir / "scripts" | |
| if scripts_dir.is_dir(): | |
| if scripts_dir.is_dir() and os.name != "nt": | |
| # 11. Scripts in scripts/ exist and are executable (POSIX only — | |
| # Windows does not use POSIX permission bits) | |
| base_dir = path.parent | |
| scripts_dir = base_dir / "scripts" | |
| if scripts_dir.is_dir() and os.name == "posix": |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@core/framework/skills/validator.py` around lines 137 - 141, The POSIX check
in the scripts existence branch uses a broad test (os.name != "nt"); update the
conditional to explicitly check for POSIX by replacing that test with os.name ==
"posix" so the permission-bit logic only runs on POSIX platforms; locate the
block using base_dir and scripts_dir (the if that currently reads "if
scripts_dir.is_dir() and os.name != 'nt':") and change the os.name check
accordingly.
1 participant
Description
Brief description of the changes in this PR.
Type of Change
Related Issues
Fixes #(issue number)
Changes Made
Testing
Describe the tests you ran to verify your changes:
cd core && pytest tests/)cd core && ruff check .)Checklist
Screenshots (if applicable)
Add screenshots to demonstrate UI changes.
Summary by CodeRabbit