From 24d51407f91cb4333a57d939db1fd7b0b173b1ac Mon Sep 17 00:00:00 2001 From: Devika Verma Date: Wed, 23 Sep 2026 17:26:03 +0530 Subject: [PATCH] Never let an unreadable skills dir abort a run MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The live skill menu is rebuilt every turn (agent.py calls rescan() while assembling per-turn context), so every filesystem call in SkillLoader._discover sat on the hot path unguarded. pathlib ignores ENOENT/ENOTDIR/EBADF/ELOOP but propagates EACCES, so an unreadable /.coworker/skills — an OPTIONAL dir, usually absent — raised straight out of the turn and terminated the run. Found while evaluating against benchmark datasets, where the agent runs headless as an unprivileged user: a root-owned .coworker/ appeared in the workspace mid-session and long runs died outright, several around 100 turns deep, after the work was already done. At startup the dir does not exist, ENOENT is ignored and the session begins normally, which is why it struck at arbitrary points rather than immediately. _discover now degrades to "no skills here" on OSError, and guards each skill's parse separately so one unreadable or malformed folder costs only that skill. Also drop "permission denied" from the provider no-access markers. Every OS PermissionError stringifies as "[Errno 13] Permission denied: ", so that substring turned any unreadable file into "Your account doesn't have access to " — a billing failure that did not exist, and hours spent chasing a provider outage that never happened. Anthropic's genuine marker is the underscored error type permission_error, which was already listed and still matches. Three regression tests; all three fail without these changes. --- coworker/providers/errors.py | 8 ++++- coworker/skills/base.py | 31 +++++++++++++++--- tests/test_model_errors.py | 18 +++++++++++ tests/test_skills.py | 61 ++++++++++++++++++++++++++++++++++++ 4 files changed, 112 insertions(+), 6 deletions(-) diff --git a/coworker/providers/errors.py b/coworker/providers/errors.py index baeebef3c3..92c92d6974 100644 --- a/coworker/providers/errors.py +++ b/coworker/providers/errors.py @@ -20,12 +20,18 @@ # you do not have access to it."}} (404/403) and {"code": "insufficient_quota"} (429). # Anthropic: {"type": "not_found_error", "message": "model: X"} (404), # {"type": "permission_error"} (403), and "credit balance is too low" (400). +# NOTE: markers must be specific to a PROVIDER error body. "permission denied" (with a +# space) was listed here once and was far too broad: every OS PermissionError stringifies +# as "[Errno 13] Permission denied: ", so an unreadable file anywhere in the turn was +# reported to the user as "your account doesn't have access to " — a billing problem +# that did not exist, sending debugging after a provider outage that never happened while +# the real cause sat in the raw traceback. Anthropic's genuine marker is the underscored +# error TYPE, which is already listed. _NO_ACCESS = ( "model_not_found", "does not exist or you do not have access", "does not have access to model", "permission_error", - "permission denied", ) _NO_QUOTA = ( "insufficient_quota", diff --git a/coworker/skills/base.py b/coworker/skills/base.py index a24617c7a7..f163280adc 100644 --- a/coworker/skills/base.py +++ b/coworker/skills/base.py @@ -40,13 +40,34 @@ def rescan(self) -> None: self._discover(directory) def _discover(self, directory: Path) -> None: - if not directory.is_dir(): + """Read one skill dir, best-effort. + + Every filesystem call here is guarded, because `rescan()` runs on EVERY TURN to + rebuild the live skill menu: a dir that is merely unreadable must degrade to "no + skills here", never abort the run. Seen in the wild with an agent running headless + as an unprivileged user — a root-owned `.coworker/` appeared in the workspace + mid-session and `is_dir()` raised EACCES (pathlib ignores ENOENT/ENOTDIR/EBADF/ + ELOOP but propagates EACCES). Long sessions died outright, some ~100 turns deep, + over an OPTIONAL dir that was empty anyway. + + Per-skill parsing is guarded separately so one unreadable or malformed folder + costs only that skill, not the rest of the dir. + """ + try: + if not directory.is_dir(): + return + entries = sorted(directory.iterdir()) + except OSError: return - for sub in sorted(directory.iterdir()): - md = sub / "SKILL.md" - if md.is_file(): + for sub in entries: + try: + md = sub / "SKILL.md" + if not md.is_file(): + continue skill = _parse_skill(md) - self._skills[skill.name] = skill + except (OSError, UnicodeDecodeError, ValueError): + continue + self._skills[skill.name] = skill def names(self) -> list[str]: return list(self._skills) diff --git a/tests/test_model_errors.py b/tests/test_model_errors.py index 10ea4ff507..79e1f5ca00 100644 --- a/tests/test_model_errors.py +++ b/tests/test_model_errors.py @@ -84,3 +84,21 @@ def test_unrelated_errors_pass_through_raw(): friendly_model_error("gpt-5.6-sol", RuntimeError("connection reset by peer")) is None ) + + +def test_os_permission_error_is_not_a_model_access_error(): + """An unreadable FILE is not a billing problem. + + Every OS PermissionError stringifies as "[Errno 13] Permission denied: ". The + marker list used to contain "permission denied" with a space, so any such error was + reported as "your account doesn't have access to ", sending debugging after a + provider outage that never happened. Unrecognized errors must return None so the + caller surfaces the raw message. + """ + exc = PermissionError(13, "Permission denied", "/src/.coworker/skills") + assert friendly_model_error("openrouter:moonshotai/kimi-k3", exc) is None + + # The genuine provider marker (Anthropic's underscored error type) still translates. + real = RuntimeError("Error code: 403 - {'type': 'permission_error', 'message': '...'}") + msg = friendly_model_error("anthropic:claude-fable-5", real) + assert msg and "doesn't have access to anthropic:claude-fable-5" in msg diff --git a/tests/test_skills.py b/tests/test_skills.py index 84c4bdb36c..c8884f9bc7 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -102,3 +102,64 @@ def test_build_engine_code_has_agents_md_and_skills(tmp_path): assert engine.agent_name == "code" finally: engine.executor.close() + + +def test_unreadable_skill_dir_does_not_abort_the_scan(tmp_path, monkeypatch): + """An unreadable skills dir degrades to "no skills", it never raises. + + `rescan()` runs every turn to rebuild the live skill menu, so an EACCES from + `is_dir()` on the workspace's optional `.coworker/skills` propagated out of the turn + and killed long headless sessions outright, some ~100 turns deep. + """ + from pathlib import Path + + from coworker.skills.base import SkillLoader + + good = tmp_path / "good" + (good / "helper").mkdir(parents=True) + (good / "helper" / "SKILL.md").write_text( + "---\nname: helper\ndescription: works\n---\nbody\n", encoding="utf-8" + ) + denied = tmp_path / "denied" + denied.mkdir() + + real_is_dir = Path.is_dir + + def fake_is_dir(self): + if self == denied: + raise PermissionError(13, "Permission denied", str(self)) + return real_is_dir(self) + + monkeypatch.setattr(Path, "is_dir", fake_is_dir) + + # The unreadable dir is skipped; the readable one is still loaded. + loader = SkillLoader([denied, good]) + assert loader.names() == ["helper"] + loader.rescan() # the per-turn call must stay quiet too + assert loader.get("helper") is not None + + +def test_unreadable_single_skill_folder_is_skipped(tmp_path, monkeypatch): + """One bad folder costs only that skill, not the whole dir.""" + from pathlib import Path + + from coworker.skills.base import SkillLoader + + root = tmp_path / "skills" + for name in ("alpha", "beta"): + (root / name).mkdir(parents=True) + (root / name / "SKILL.md").write_text( + "---\nname: %s\ndescription: d\n---\nbody\n" % name, encoding="utf-8" + ) + + real_read_text = Path.read_text + + def fake_read_text(self, *a, **kw): + if self.parent.name == "alpha": + raise PermissionError(13, "Permission denied", str(self)) + return real_read_text(self, *a, **kw) + + monkeypatch.setattr(Path, "read_text", fake_read_text) + + loader = SkillLoader([root]) + assert loader.names() == ["beta"]