fix: use os.access for cross-platform executable check in skill validator

[Hundao]

Description

Replace Unix-only permission bit check with os.access(path, os.X_OK) in skill validator. Fixes Windows CI failure on main.

Type of Change

• Bug fix (non-breaking change that fixes an issue)

Related Issues

Fixes #6893

Changes Made

• Replaced stat.S_IXUSR | S_IXGRP | S_IXOTH bit check with os.access(script_path, os.X_OK) in validator.py
• Removed unused import stat

Testing

• Unit tests pass (cd core && pytest tests/test_skill_validator.py)
• Lint passes (cd core && ruff check .)

Checklist

• My code follows the project's style guidelines
• I have performed a self-review of my code
• New and existing unit tests pass locally with my changes

Summary by CodeRabbit

• Chores
  • Adjusted script-existence and executability checks to avoid incorrect failures on Windows, improving cross-platform reliability.
• Tests
  • Updated tests to skip permission-dependent checks on Windows to prevent spurious test failures.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 22bbf21b-d921-46fb-a379-90f909c3555d

📥 Commits

Reviewing files that changed from the base of the PR and between d9f5ee5 and 571c28b.

📒 Files selected for processing (2)

• core/framework/skills/validator.py
• core/tests/test_skill_validator.py

---

📝 Walkthrough

Walkthrough

The validator's script-executability check was modified to skip POSIX executable-bit validation on Windows by gating the per-script st_mode test behind a platform check (sys.platform != "win32"); tests that assert POSIX executable-bit behavior were marked to skip on Windows.

Changes

Cohort / File(s)	Summary
Validator change core/framework/skills/validator.py	Added sys import and wrapped the per-script POSIX st_mode executable-bit test so it is only performed when sys.platform != "win32". If on Windows, the per-file loop is effectively skipped even if scripts_dir exists.
Test adjustments core/tests/test_skill_validator.py	Added sys and pytest imports and annotated two TestCheck11Scripts test methods with pytest.mark.skipif(sys.platform == "win32", reason=...) to skip POSIX-executable-bit assertions on Windows.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Poem

🐰 I hopped through code with careful paws,

Skipped a Unix check for Windows' laws,
Tests now rest when platforms disagree,
One small tweak, and builds run free! 🥕

🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (3 warnings)

Check name	Status	Explanation	Resolution
Linked Issues check	⚠️ Warning	The PR addresses the core requirement from issue #6893 by replacing Unix permission bit checks with os.access(). However, the actual implementation in the code changes uses sys.platform != 'win32' to gate validation instead of os.access() as specified in the issue.	Replace the sys.platform-based conditional with os.access(script_path, os.X_OK) to achieve true cross-platform compatibility as required by issue #6893.
Out of Scope Changes check	⚠️ Warning	Changes include skipping the entire validation loop on Windows rather than using os.access() per the issue, and test methods are decorated to skip on Windows. These diverge from the intended cross-platform approach.	Implement the fix using os.access() to check file executability cross-platform, rather than platform-specific branching logic that skips validation on Windows.
Docstring Coverage	⚠️ Warning	Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately describes the main change: replacing Unix-only permission bit check with os.access() for cross-platform executable validation in the skill validator.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

core/framework/skills/validator.py (1)

142-145: Add platform-aware executability check for POSIX systems.

The os.access(script_path, os.X_OK) call diverges from strict execute-bit validation on POSIX for privileged users: POSIX permits implementations to return True for X_OK even without execute bits set when the process is privileged (root), and Linux/Solaris/AIX implementations differ. For strict validation, use stat mode bits on POSIX and keep os.access on Windows.

♻️ Suggested cross-platform adjustment

+import stat
...
-                if not os.access(script_path, os.X_OK):
+                is_executable = (
+                    os.access(script_path, os.X_OK)
+                    if os.name == "nt"
+                    else bool(script_path.stat().st_mode & (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH))
+                )
+                if not is_executable:
                     errors.append(
                         f"Script not executable: {script_path.name}. Run: chmod +x {script_path}"
                     )

🤖 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 142 - 145, The current
executability check using os.access(script_path, os.X_OK) should be made
platform-aware: on POSIX (os.name != 'nt') inspect the file mode bits via
script_path.stat().st_mode and test for any execute bit (stat.S_IXUSR |
stat.S_IXGRP | stat.S_IXOTH) to enforce strict execute-bit validation, while on
Windows keep using os.access(script_path, os.X_OK); update the conditional in
the validator (around the block that appends "Script not executable" in
core/framework/skills/validator.py) to use the stat-based check for POSIX and
fall back to os.access for Windows, leaving the errors.append message unchanged.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@core/framework/skills/validator.py`:
- Around line 142-145: The current executability check using
os.access(script_path, os.X_OK) should be made platform-aware: on POSIX (os.name
!= 'nt') inspect the file mode bits via script_path.stat().st_mode and test for
any execute bit (stat.S_IXUSR | stat.S_IXGRP | stat.S_IXOTH) to enforce strict
execute-bit validation, while on Windows keep using os.access(script_path,
os.X_OK); update the conditional in the validator (around the block that appends
"Script not executable" in core/framework/skills/validator.py) to use the
stat-based check for POSIX and fall back to os.access for Windows, leaving the
errors.append message unchanged.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 1669718c-abbd-4050-8888-4d5c3800cdd6

📥 Commits

Reviewing files that changed from the base of the PR and between 5e628c7 and d9f5ee5.

📒 Files selected for processing (1)

• core/framework/skills/validator.py