fix(skills): keep one unreadable skill directory from failing the whole request

[he-yufeng]

Fixes #10589.

SkillManager.list_skills() iterated data/skills/ and normalized each directory with no error isolation: one unreadable skill directory (ACL-denied, transient I/O) raised PermissionError out of the listing, through the system-prompt build in astr_main_agent.py, and killed the whole Agent/LLM request even when the broken skill was unrelated to it. The workspace sibling list_workspace_skills() already guards the same iterdir() with try/except OSError and continues.

This brings the local path to parity: the skills-root listing and both per-directory normalizations (local and plugin-provided) now log a warning naming the offending path and skip the broken entry, so the remaining skills still load and the request completes.

Tests: new tests/test_skill_manager_listing.py pins that an unreadable skill directory is skipped while its siblings still list, and that an unreadable skills root yields an empty list instead of raising. Red-green verified against master (de898af): both fail with the raw PermissionError before the fix and pass after; the 40 neighboring skill-manager tests still pass. The astrbot imports during the runs resolved to the patched tree.

Summary by Sourcery

Make skill discovery resilient to filesystem access errors so unrelated unreadable skills cannot fail an agent request.

Bug Fixes:

• Prevent unreadable local or plugin skill directories, and unreadable skill roots, from aborting skill discovery and the associated agent request.
• Allow LLM request preparation to continue without skills when skill listing fails unexpectedly.

Enhancements:

• Add warning logs identifying skill paths that are skipped due to filesystem errors.

Tests:

• Add regression coverage confirming unreadable skill directories are skipped while readable siblings remain available and unreadable roots return an empty list.

[sourcery-ai]

Hey - I've found 3 issues

Prompt for AI Agents

Please address the comments from this code review:

## Individual Comments

### Comment 1
<location path="astrbot/core/skills/skill_manager.py" line_range="622-624" />
<code_context>
             sandbox_cached_paths[name] = path

-        for entry in sorted(Path(self.skills_root).iterdir()):
+        try:
+            local_skill_dirs = sorted(Path(self.skills_root).iterdir())
+        except OSError as exc:
+            # An unreadable skills root must not kill the whole agent request;
+            # mirror list_workspace_skills and continue with the other sources.
</code_context>
<issue_to_address>
**Plugin discovery still aborts listings**

When plugin skill discovery encounters an `OSError` while normalizing a skill directory or accessing its `skills/` root, `_iter_plugin_skill_dirs()` raises before `list_skills()` reaches the per-directory `try`, so the listing aborts and the request fails instead of skipping the unreadable plugin entry.

Catch `OSError` inside `_iter_plugin_skill_dirs()` during discovery and skip the affected plugin entry.
</issue_to_address>

### Comment 2
<location path="astrbot/core/skills/skill_manager.py" line_range="571-575" />
<code_context>
             if not entry.is_dir():
                 continue
             skill_name = entry.name
-            skill_md = _normalize_skill_markdown_path(entry)
+            try:
+                skill_md = _normalize_skill_markdown_path(entry)
+            except OSError as exc:
+                # One broken or permission-denied skill directory is not a
+                # reason to fail the entire request (#10589).
+                logger.warning(f"Skip unreadable skill directory {entry}: {exc}")
+                continue
             if skill_md is None:
</code_context>
<issue_to_address>
**Local entry checks still abort listings**

When checking a local skill entry with `Path.is_dir()` raises `OSError`, `entry.is_dir()` runs before the per-entry `try`, so the exception escapes `list_skills()` and the request fails instead of skipping that entry.

Move the `entry.is_dir()` check inside the per-entry `try` so its `OSError` is caught and the entry is skipped.
</issue_to_address>

### Comment 3
<location path="astrbot/core/skills/skill_manager.py" line_range="567" />
<code_context>
+            local_skill_dirs = []
+
+        for entry in local_skill_dirs:
             if not entry.is_dir():
                 continue
             skill_name = entry.name
</code_context>
<issue_to_address>
**Unreadable entries still abort discovery**

When filesystem metadata access fails for a local entry, or plugin skill discovery raises while iterating or normalizing a child directory, `entry.is_dir()` runs before the local `try`, and `_iter_plugin_skill_dirs()` normalizes plugin children before the plugin `try` can run; either `OSError` escapes `list_skills()` and aborts the request.

Guard the full per-entry checks and plugin discovery against `OSError`, and add regression coverage for the plugin path.

Also at `astrbot/core/skills/skill_manager.py:623`.
</issue_to_address>

Sourcery assessment

Approval pending. 3 findings to address first.

Blocking findings: astrbot/core/skills/skill_manager.py:624, astrbot/core/skills/skill_manager.py:575, astrbot/core/skills/skill_manager.py:567

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

[sourcery-ai]

Sourcery assessment

Approved.