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"]