From f0e4e477c88cdda07b69ceb0466aa7f78402ba63 Mon Sep 17 00:00:00 2001 From: Mickael Farina Date: Wed, 7 Oct 2026 12:58:09 +0700 Subject: [PATCH 1/3] User skills folder: design note (approved skills to ~/.codec/skills, loaded) Co-Authored-By: Claude Opus 5.5 --- docs/USER-SKILLS-DIR-DESIGN.md | 56 ++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100644 docs/USER-SKILLS-DIR-DESIGN.md diff --git a/docs/USER-SKILLS-DIR-DESIGN.md b/docs/USER-SKILLS-DIR-DESIGN.md new file mode 100644 index 0000000..46d2261 --- /dev/null +++ b/docs/USER-SKILLS-DIR-DESIGN.md @@ -0,0 +1,56 @@ +# User skills folder: approved skills go to ~/.codec/skills, and CODEC loads it + +Asked by Mickael on 2026-10-07 ("point approve at ~/.codec/skills") after the Mac merge pass found that approved +skills land in the repo's `skills/` folder (docs/known-issues.md). + +## Why + +- `POST /api/skill/approve` (the review-and-approve flow, and Settings > Learning's Approve and install) writes to + `codec_config.SKILLS_DIR`. That is `config.json:skills_dir`, or the repo's own `skills/` folder when the key is not + set, as on this Mac. An approved skill becomes an untracked file in the live git tree, next to the hash-pinned + built-ins. +- Every process builds its skill registry with `SkillRegistry(SKILLS_DIR)`, and the registry scans that one folder. + So `~/.codec/skills`, which AGENTS.md calls the user folder and which the marketplace installs into, is never + loaded on this Mac. Writing approved skills there alone would make them dead files. + +## What + +1. `codec_config.USER_SKILLS_DIR`: `config.json:user_skills_dir`, default `~/.codec/skills`. +2. `SkillRegistry(skills_dir, user_dir=None)`: when `user_dir` is given and differs from `skills_dir`, `scan()` reads + the built-in folder first, then the user folder. A user file whose skill name is already a built-in is skipped + (logged), so a user file can never shadow a built-in. User files are never trusted by the manifest: every load + runs the AST gate (`is_dangerous_skill_code`), as unmanifested files do today. Files starting with `_` and + sub-folders are not scanned, as today. +3. The registries in `codec_dispatch`, `codec_mcp`, `codec_voice`, `codec_agents`, `codec_autopilot` and + `codec_slash_commands` pass `USER_SKILLS_DIR`. +4. `/api/skill/approve` writes to `USER_SKILLS_DIR` (folder 0700, file 0600). The built-in-name refusal and the + write-time AST gate stay as they are. The audit line `skill_approved` gains `dir: "user"`. +5. Settings > Learning's "installed" check and `codec_self_improve`'s existing-name scan look in both folders. +6. AGENTS.md §4 and the Learning page text say where approved skills go. + +No change to the manifest, the built-in folder, the AST gate, the security lists or MCP exposure: a user skill's +`SKILL_MCP_EXPOSE` is honoured as before, and over HTTP the blocked and ask-first lists apply. + +## Migration (needs Mickael's OK: it is his folder) + +`~/.codec/skills` holds 6 old files (March-April 2026) that have never been loaded on this Mac. Five fail the AST +gate. `lucy.py` passes, so it would start running. Before the first restart they move to +`~/.codec/skills/_dormant-2026-10-07/` (not scanned; nothing deleted). Any of them can be moved back by hand. + +## Deploy + +Restart `codec-dashboard` and `open-codec`. Until Mickael restarts `codec-mcp-http`, `codec-agent-runner` and +`codec-autopilot`, those keep scanning the repo folder only, so an approved skill is not yet reachable from Claude +over MCP, project agents or autopilot. + +## Test plan + +`tests/test_user_skills_dir.py`: the registry finds built-in and user skills; a built-in wins a name clash; a user +file is never trusted by hash and a dangerous one is refused at load; `_` files and sub-folders are skipped; approve +writes to the user folder with 0600 and still refuses a built-in name; the Learning installed flag and the +self-improve name scan see user skills. Full suite with an empty HOME (CI has no ~/.codec). + +## Rollback + +Revert the PR. Skills approved meanwhile stay in `~/.codec/skills` and are no longer loaded; move them to the repo +folder by hand if needed. From 298b52a4e1a276305b981c290d4d01ec64864269 Mon Sep 17 00:00:00 2001 From: Mickael Farina Date: Wed, 7 Oct 2026 13:48:12 +0700 Subject: [PATCH 2/3] Load the user skills folder and write approved skills there Every skill registry reads the built-in folder (SKILLS_DIR, the repo's skills/ on this Mac) and then the user folder (USER_SKILLS_DIR, ~/.codec/skills): a built-in name always wins, user files are never trusted by the manifest so every load runs the AST gate, and _ files and sub-folders are skipped. /api/skill/approve (review-and-approve, Settings > Learning) writes to the user folder (0700 folder, 0600 file, atomic) instead of the repo's built-in folder; the built-in-name refusal and the AST gate are unchanged. Learning's installed check and self-improve's name scan see user skills. The suite points CODEC_USER_SKILLS_DIR at a temp folder and no longer imports the owner's user skills. docs/USER-SKILLS-DIR-DESIGN.md. Co-Authored-By: Claude Opus 5.5 --- AGENTS.md | 2 + codec_agents.py | 5 +- codec_autopilot.py | 6 +- codec_config.py | 4 + codec_dashboard.html | 4 +- codec_dispatch.py | 4 +- codec_mcp.py | 4 +- codec_self_improve.py | 5 +- codec_skill_registry.py | 58 +++++++--- codec_slash_commands.py | 4 +- codec_voice.py | 5 +- routes/learning.py | 5 + routes/skills.py | 19 ++- tests/conftest.py | 6 + tests/test_learning.py | 1 + tests/test_reaudit_low.py | 1 + tests/test_skill_overwrite.py | 2 + tests/test_skill_reviews_persistence.py | 1 + tests/test_skill_routes.py | 1 + tests/test_skills.py | 6 +- tests/test_user_skills_dir.py | 147 ++++++++++++++++++++++++ 21 files changed, 255 insertions(+), 35 deletions(-) create mode 100644 tests/test_user_skills_dir.py diff --git a/AGENTS.md b/AGENTS.md index 8aedf80..5f7930c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -299,6 +299,8 @@ Required: `SKILL_NAME`, `SKILL_TRIGGERS`, `SKILL_DESCRIPTION`. Optional: `SKILL_ ### Adding a new skill Drop a `.py` file in `~/.codec/skills/` (user) or `skills/` (built-in). Restart `codec-dashboard` and the main `open-codec` process. Hot-reload is not currently supported. +**Two folders (2026-10-07, docs/USER-SKILLS-DIR-DESIGN.md).** Every registry reads the built-in folder (`codec_config.SKILLS_DIR`, the repo's `skills/` unless `config.json:skills_dir` says otherwise) and then the user folder (`codec_config.USER_SKILLS_DIR`: `config.json:user_skills_dir`, default `~/.codec/skills`). A built-in name always wins; a user file is never trusted by the manifest, so every load runs the AST gate; files starting with `_` and sub-folders are skipped. `/api/skill/approve` (review-and-approve, Settings > Learning) writes approved skills to the user folder (0700 folder, 0600 file), never the repo. The test suite points `CODEC_USER_SKILLS_DIR` at a temp folder (tests/conftest.py). + ## 5. Memory contract Single SQLite database at `~/.codec/memory.db`. Wrapper: `codec_memory.CodecMemory`. diff --git a/codec_agents.py b/codec_agents.py index 36560e5..b0e6448 100644 --- a/codec_agents.py +++ b/codec_agents.py @@ -39,9 +39,10 @@ # ── CONFIG ── CONFIG_PATH = os.path.expanduser("~/.codec/config.json") try: - from codec_config import SKILLS_DIR + from codec_config import SKILLS_DIR, USER_SKILLS_DIR except ImportError: SKILLS_DIR = os.path.expanduser("~/.codec/skills") + USER_SKILLS_DIR = None DB_PATH = os.path.expanduser("~/.codec/memory.db") def _cfg(): @@ -408,7 +409,7 @@ def _shell_execute(cmd: str) -> str: from codec_skill_registry import SkillRegistry -_agents_registry = SkillRegistry(SKILLS_DIR) +_agents_registry = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) def _make_lazy_fn(registry: "SkillRegistry", skill_name: str): diff --git a/codec_autopilot.py b/codec_autopilot.py index 6874f9e..e547806 100644 --- a/codec_autopilot.py +++ b/codec_autopilot.py @@ -39,7 +39,7 @@ sys.path.insert(0, str(_REPO / "skills")) from codec_skill_registry import SkillRegistry -from codec_config import SKILLS_DIR +from codec_config import SKILLS_DIR, USER_SKILLS_DIR from codec_audit import audit log = logging.getLogger("codec-autopilot") @@ -116,7 +116,7 @@ def _day_matches(days_spec: str, weekday: int) -> bool: def _speak(text: str): """Best-effort TTS via tts_say skill; falls back to macOS `say`.""" try: - registry = SkillRegistry(SKILLS_DIR) + registry = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) registry.scan() mod = registry.load("tts_say") if mod and hasattr(mod, "run"): @@ -225,7 +225,7 @@ def main(): codec_lifecycle.install_handlers( lambda: log.info("CODEC Autopilot graceful shutdown"), name="codec-autopilot") - registry = SkillRegistry(SKILLS_DIR) + registry = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) registry.scan() while True: try: diff --git a/codec_config.py b/codec_config.py index 07b4413..fd35ea2 100644 --- a/codec_config.py +++ b/codec_config.py @@ -361,6 +361,10 @@ def get_telegram_bot_token() -> str: # Skills load from the repo directly — single source of truth _REPO_DIR = os.path.dirname(os.path.abspath(__file__)) SKILLS_DIR = cfg.get("skills_dir", os.path.join(_REPO_DIR, "skills")) +# The owner's skills (approved reviews, marketplace installs): read after the built-ins, +# never trusted by the manifest. CODEC_USER_SKILLS_DIR lets the test suite point it away. +USER_SKILLS_DIR = os.path.expanduser(os.environ.get("CODEC_USER_SKILLS_DIR") + or cfg.get("user_skills_dir") or "~/.codec/skills") AUDIT_LOG = os.path.expanduser("~/.codec/audit.log") # Features diff --git a/codec_dashboard.html b/codec_dashboard.html index 8c792cc..059390d 100644 --- a/codec_dashboard.html +++ b/codec_dashboard.html @@ -1032,7 +1032,7 @@

macOS permissions

Proposed skills
-

Drafted by the self-improvement run when CODEC lacked a skill. Review shows the code; Approve and install writes it to your skills folder after the safety check, and CODEC loads it after the next restart.

+

Drafted by the self-improvement run when CODEC lacked a skill. Review shows the code; Approve and install writes it to your skills folder (~/.codec/skills) after the safety check, and CODEC loads it after the next restart.

Loading...
@@ -3002,7 +3002,7 @@

macOS permissions

LRN.review = rid; LRN.name = p.name; _learnDlg(); document.getElementById('learnTitle').textContent = 'Review ' + p.name; - document.getElementById('learnWhat').textContent = 'Approve and install writes ' + fname + ' to your skills folder after the safety check; CODEC loads it after the next restart.' + + document.getElementById('learnWhat').textContent = 'Approve and install writes ' + fname + ' to your skills folder (~/.codec/skills) after the safety check; CODEC loads it after the next restart.' + (p.truncated ? ' The code is longer than shown here.' : ''); document.getElementById('learnCode').textContent = p.code; document.getElementById('learnErr').textContent = ''; diff --git a/codec_dispatch.py b/codec_dispatch.py index 9bc1a3a..6d382b7 100644 --- a/codec_dispatch.py +++ b/codec_dispatch.py @@ -11,7 +11,7 @@ # Audit emits route through the unified log_event adapter (real, not no-op) # per docs/PHASE1-STEP1-DESIGN.md. from codec_audit import log_event -from codec_config import SKILLS_DIR +from codec_config import SKILLS_DIR, USER_SKILLS_DIR from codec_hooks import HookVeto, run_with_hooks from codec_skill_registry import SkillRegistry from codec_skill_switches import is_off as _skill_off, off_message as _off_message @@ -19,7 +19,7 @@ log = logging.getLogger('codec') # Global registry instance shared across codec.py -registry = SkillRegistry(SKILLS_DIR) +registry = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) def load_skills(): diff --git a/codec_mcp.py b/codec_mcp.py index bc7a88f..3f7d1c2 100644 --- a/codec_mcp.py +++ b/codec_mcp.py @@ -61,7 +61,7 @@ def _validate_mcp_input(tool_name: str, task: str, context: str = "") -> str | N if _REPO_DIR not in sys.path: sys.path.insert(0, _REPO_DIR) -from codec_config import MCP_DEFAULT_ALLOW, MCP_ALLOWED_TOOLS, MCP_BLOCKED_TOOLS, SKILLS_DIR +from codec_config import MCP_DEFAULT_ALLOW, MCP_ALLOWED_TOOLS, MCP_BLOCKED_TOOLS, SKILLS_DIR, USER_SKILLS_DIR if SKILLS_DIR not in sys.path: sys.path.insert(0, SKILLS_DIR) from codec_skill_registry import SkillRegistry @@ -78,7 +78,7 @@ def __iter__(self): # Global registry for MCP skill tools -_mcp_registry = SkillRegistry(SKILLS_DIR) +_mcp_registry = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) def build_mcp(auth=None): diff --git a/codec_self_improve.py b/codec_self_improve.py index c1c3434..5b9224f 100644 --- a/codec_self_improve.py +++ b/codec_self_improve.py @@ -81,7 +81,7 @@ _REPO = Path(__file__).resolve().parent sys.path.insert(0, str(_REPO)) -from codec_config import QWEN_BASE_URL, QWEN_MODEL, SKILLS_DIR, is_dangerous_skill_code +from codec_config import QWEN_BASE_URL, QWEN_MODEL, SKILLS_DIR, USER_SKILLS_DIR, is_dangerous_skill_code try: from codec_audit import log_event as _si_log_event @@ -106,7 +106,8 @@ def _si_log_event(*a, **kw): # type: ignore[no-redef] def _existing_skill_names() -> set[str]: out = set() - for f in Path(SKILLS_DIR).glob("*.py"): + # Built-ins and the owner's folder (approved skills): never propose what is already there. + for f in [*Path(SKILLS_DIR).glob("*.py"), *Path(USER_SKILLS_DIR).glob("*.py")]: if f.name.startswith("_") or f.name == "codec.py": continue out.add(f.stem) diff --git a/codec_skill_registry.py b/codec_skill_registry.py index b4b1e72..df47d52 100644 --- a/codec_skill_registry.py +++ b/codec_skill_registry.py @@ -105,8 +105,15 @@ class SkillRegistry: mod = registry.load("weather") # first call imports; subsequent calls return cache """ - def __init__(self, skills_dir: str): + def __init__(self, skills_dir: str, user_dir: Optional[str] = None): self.skills_dir = skills_dir + # The owner's folder (~/.codec/skills): read after the built-ins, a built-in + # name always wins, and its files are never trusted by the manifest (every + # load runs the AST gate). None, or the built-in folder itself, means off. + same = user_dir and os.path.realpath(user_dir) == os.path.realpath(skills_dir) + self.user_dir = None if (not user_dir or same) else user_dir + # names whose file is in user_dir (set at scan() time) + self._user_names: Set[str] = set() # name -> metadata dict (always populated after scan) self._meta: Dict[str, Dict[str, Any]] = {} # name -> file path @@ -129,9 +136,11 @@ def scan(self) -> int: Returns the number of skills discovered.""" self._meta.clear() self._paths.clear() + self._user_names.clear() # Do NOT clear _modules — keep any already-loaded modules cached - if not os.path.isdir(self.skills_dir): + has_user = bool(self.user_dir) and os.path.isdir(self.user_dir) + if not os.path.isdir(self.skills_dir) and not has_user: return 0 # Reload the trusted-skill manifest on every scan so manifest edits @@ -140,21 +149,42 @@ def scan(self) -> int: # Reload user trigger overrides too (A-4) — same refresh-on-scan policy. self._custom_triggers = self._load_custom_triggers() - for fname in sorted(os.listdir(self.skills_dir)): - if not fname.endswith(".py") or fname.startswith("_"): - continue - filepath = os.path.join(self.skills_dir, fname) - meta = _extract_metadata(filepath) - if meta is None: - continue - name = meta.get("SKILL_NAME", fname[:-3]) - self._meta[name] = meta - self._paths[name] = filepath + if os.path.isdir(self.skills_dir): + for fname in sorted(os.listdir(self.skills_dir)): + if not fname.endswith(".py") or fname.startswith("_"): + continue + filepath = os.path.join(self.skills_dir, fname) + meta = _extract_metadata(filepath) + if meta is None: + continue + name = meta.get("SKILL_NAME", fname[:-3]) + self._meta[name] = meta + self._paths[name] = filepath + + if has_user: + for fname in sorted(os.listdir(self.user_dir)): + if not fname.endswith(".py") or fname.startswith("_"): + continue + filepath = os.path.join(self.user_dir, fname) + if not os.path.isfile(filepath): + continue + meta = _extract_metadata(filepath) + if meta is None: + continue + name = meta.get("SKILL_NAME", fname[:-3]) + if name in self._meta and name not in self._user_names: + log.warning("User skill %s skipped: the built-in skill %r has that name", fname, name) + continue + self._meta[name] = meta + self._paths[name] = filepath + self._user_names.add(name) log.info( "Skill registry: %d skills discovered (metadata only), %d in trusted manifest", len(self._meta), len(self._trusted_hashes), ) + if self._user_names: + log.info("Skill registry: %d of them from the user folder %s", len(self._user_names), self.user_dir) return len(self._meta) def _load_trusted_manifest(self) -> Set[str]: @@ -283,7 +313,9 @@ def load(self, name: str) -> Optional[Any]: return None file_hash = hashlib.sha256(raw).hexdigest() - trusted = file_hash in self._trusted_hashes + # A file in the user folder is never trusted by the manifest, even if its + # bytes equal a pinned built-in's: it always goes through the AST gate. + trusted = file_hash in self._trusted_hashes and name not in self._user_names if not trusted: # Decode for AST check; fail-safe on UnicodeDecodeError. diff --git a/codec_slash_commands.py b/codec_slash_commands.py index 6680253..e532989 100644 --- a/codec_slash_commands.py +++ b/codec_slash_commands.py @@ -162,11 +162,11 @@ def _cmd_skills(args: list[str]) -> str: # Lazy import to avoid pulling skill registry at module load try: from codec_skill_registry import SkillRegistry - from codec_config import SKILLS_DIR + from codec_config import SKILLS_DIR, USER_SKILLS_DIR except Exception as e: return f"Error: skill registry unavailable: {e}" - reg = SkillRegistry(SKILLS_DIR) + reg = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) reg.scan() # P3.6: the deny list the Skills page writes and every path enforces (codec_skill_switches). import codec_skill_switches as switches diff --git a/codec_voice.py b/codec_voice.py index 7bc8fbc..35d5c62 100644 --- a/codec_voice.py +++ b/codec_voice.py @@ -198,9 +198,10 @@ def _clear_voice_session_marker() -> None: KOKORO_VOICE = "am_adam" KOKORO_SPEED = 1.15 # config:tts_speed overrides (normal mode; flash has its own) try: - from codec_config import SKILLS_DIR + from codec_config import SKILLS_DIR, USER_SKILLS_DIR except ImportError: SKILLS_DIR = os.path.expanduser("~/.codec/skills") + USER_SKILLS_DIR = None _CONFIG_PATH = os.path.expanduser("~/.codec/config.json") try: @@ -511,7 +512,7 @@ def _save_for_resume(self): def _load_skills(self): from codec_skill_registry import SkillRegistry - self._skill_registry = SkillRegistry(SKILLS_DIR) + self._skill_registry = SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR) self._skill_registry.scan() # Build a lightweight dict with triggers only (no module imports) for name in self._skill_registry.names(): diff --git a/routes/learning.py b/routes/learning.py index 15db9f7..e4945c2 100644 --- a/routes/learning.py +++ b/routes/learning.py @@ -64,6 +64,11 @@ def _installed() -> set: names |= {f[:-3] for f in os.listdir(_get_skills_dir()) if f.endswith(".py")} except OSError: pass + try: # approved skills go to the user folder; one approved before a restart is not in the registry yet + from codec_config import USER_SKILLS_DIR + names |= {f[:-3] for f in os.listdir(USER_SKILLS_DIR) if f.endswith(".py") and not f.startswith("_")} + except OSError: + pass return names diff --git a/routes/skills.py b/routes/skills.py index d2a070b..6e11086 100644 --- a/routes/skills.py +++ b/routes/skills.py @@ -187,6 +187,14 @@ async def skill_review(request: Request): } +def _approved_skills_dir() -> str: + """Where an approved skill is written: the owner's folder (codec_config.USER_SKILLS_DIR, + ~/.codec/skills by default), never the repo's built-in folder. The registry reads it + after the built-ins (docs/USER-SKILLS-DIR-DESIGN.md).""" + from codec_config import USER_SKILLS_DIR + return USER_SKILLS_DIR + + @router.post("/api/skill/approve") async def skill_approve(request: Request): """Approve a pending skill review -- writes to disk and removes from pending.""" @@ -233,11 +241,14 @@ async def skill_approve(request: Request): dangerous, reason = is_dangerous_skill_code(code) if dangerous: return JSONResponse({"error": f"Blocked: {reason}"}, status_code=400) - skill_dir = _get_skills_dir() - os.makedirs(skill_dir, exist_ok=True) + skill_dir = _approved_skills_dir() + os.makedirs(skill_dir, mode=0o700, exist_ok=True) path = os.path.join(skill_dir, filename) - with open(path, "w") as f: + tmp = path + ".tmp" # not a .py, so a scan never reads a half-written file + fd = os.open(tmp, os.O_WRONLY | os.O_CREAT | os.O_TRUNC, 0o600) + with os.fdopen(fd, "w", encoding="utf-8") as f: f.write(code) + os.replace(tmp, path) # Only now that the skill is on disk do we clear the staged review (from both # disk and the in-memory cache) so a crash mid-write leaves it re-approvable. _delete_review(review_id) @@ -247,7 +258,7 @@ async def skill_approve(request: Request): "skill_approved", source="codec-routes-skills", message=f"approved skill {filename} ({len(code)} bytes) → {path}", level="info", outcome="ok", - extra={"filename": filename, "review_id": review_id, "size": len(code)}, + extra={"filename": filename, "review_id": review_id, "size": len(code), "dir": "user"}, ) except Exception: pass diff --git a/tests/conftest.py b/tests/conftest.py index 4aa329b..dd63be3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -35,6 +35,12 @@ # this variable when the module is imported. os.environ.setdefault("CODEC_DASHBOARD_EXTRA_HOSTS", "testserver") +# The owner's skills folder (~/.codec/skills) is read by every skill registry +# and written by /api/skill/approve. Point it at an empty temp folder before +# anything imports codec_config, so the suite neither loads nor writes the +# owner's approved skills (tests that need user skills use their own tmp dirs). +os.environ.setdefault("CODEC_USER_SKILLS_DIR", tempfile.mkdtemp(prefix="codec-test-user-skills-")) + def _isolate_audit_log() -> None: """Point the audit log at a throwaway file for the whole test session. diff --git a/tests/test_learning.py b/tests/test_learning.py index b8e3116..697c043 100644 --- a/tests/test_learning.py +++ b/tests/test_learning.py @@ -159,6 +159,7 @@ def test_review_then_approve_go_through_the_existing_gates(tmp_path, monkeypatch skills_dir = tmp_path / "skills" monkeypatch.setattr(rs, "_reviews_dir", lambda: str(tmp_path / "reviews")) monkeypatch.setattr(rs, "_get_skills_dir", lambda: str(skills_dir)) + monkeypatch.setattr(rs, "_approved_skills_dir", lambda: str(skills_dir)) monkeypatch.setattr(rs, "_pending_skills", {}) app = FastAPI() app.include_router(rs.router) diff --git a/tests/test_reaudit_low.py b/tests/test_reaudit_low.py index cf8907a..07cba4e 100644 --- a/tests/test_reaudit_low.py +++ b/tests/test_reaudit_low.py @@ -40,6 +40,7 @@ def test_ast_gate_still_allows_safe_code(): # ── N20: skill_approve must FAIL CLOSED when the pinned-builtin manifest can't be read ── def test_skill_approve_fails_closed_when_manifest_unreadable(tmp_path, monkeypatch): monkeypatch.setattr(sr, "_get_skills_dir", lambda: str(tmp_path)) + monkeypatch.setattr(sr, "_approved_skills_dir", lambda: str(tmp_path)) monkeypatch.setattr(sr, "_pinned_builtin_names", lambda: None) # simulate read failure rid = "rev_failclosed" sr._pending_skills[rid] = {"code": _BENIGN, "filename": "my_helper.py"} diff --git a/tests/test_skill_overwrite.py b/tests/test_skill_overwrite.py index f8433d1..c1d0371 100644 --- a/tests/test_skill_overwrite.py +++ b/tests/test_skill_overwrite.py @@ -34,6 +34,7 @@ def _approve(payload): def test_skill_approve_refuses_pinned_builtin_name(tmp_path, monkeypatch): monkeypatch.setattr(skills_routes, "_get_skills_dir", lambda: str(tmp_path)) + monkeypatch.setattr(skills_routes, "_approved_skills_dir", lambda: str(tmp_path)) rid = "rev_pinned" skills_routes._pending_skills[rid] = {"code": _BENIGN, "filename": "calculator.py"} resp = _approve({"review_id": rid}) @@ -44,6 +45,7 @@ def test_skill_approve_refuses_pinned_builtin_name(tmp_path, monkeypatch): def test_skill_approve_allows_non_pinned_name(tmp_path, monkeypatch): monkeypatch.setattr(skills_routes, "_get_skills_dir", lambda: str(tmp_path)) + monkeypatch.setattr(skills_routes, "_approved_skills_dir", lambda: str(tmp_path)) rid = "rev_ok" skills_routes._pending_skills[rid] = {"code": _BENIGN, "filename": "my_unique_helper.py"} resp = _approve({"review_id": rid}) diff --git a/tests/test_skill_reviews_persistence.py b/tests/test_skill_reviews_persistence.py index e600f5e..9ac2cca 100644 --- a/tests/test_skill_reviews_persistence.py +++ b/tests/test_skill_reviews_persistence.py @@ -43,6 +43,7 @@ def client(tmp_path, monkeypatch): reviews_dir = tmp_path / "skill_reviews" skills_dir.mkdir() monkeypatch.setattr(skills_routes, "_get_skills_dir", lambda: str(skills_dir)) + monkeypatch.setattr(skills_routes, "_approved_skills_dir", lambda: str(skills_dir)) monkeypatch.setattr(skills_routes, "_reviews_dir", lambda: str(reviews_dir)) skills_routes._pending_skills.clear() app = FastAPI() diff --git a/tests/test_skill_routes.py b/tests/test_skill_routes.py index 95aa8cd..a27a436 100644 --- a/tests/test_skill_routes.py +++ b/tests/test_skill_routes.py @@ -137,6 +137,7 @@ def test_skill_approve_writes_only_after_review(tmp_path, monkeypatch): skills_dir = tmp_path / "skills" skills_dir.mkdir() monkeypatch.setattr(skills_routes, "_get_skills_dir", lambda: str(skills_dir)) + monkeypatch.setattr(skills_routes, "_approved_skills_dir", lambda: str(skills_dir)) # Isolate the staged-review store to a tmp dir so the review→approve round # trip doesn't touch the real ~/.codec/skill_reviews/. reviews_dir = tmp_path / "skill_reviews" diff --git a/tests/test_skills.py b/tests/test_skills.py index 356337d..b303b78 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -4,8 +4,12 @@ import os import sys -SKILLS_DIR = os.path.expanduser("~/.codec/skills") +# The user skills folder. conftest points CODEC_USER_SKILLS_DIR at an empty temp folder, so the +# suite never imports (runs module code of) the owner's approved skills. The smoke tests below +# import built-ins from the repo's skills/ folder. +SKILLS_DIR = os.environ.get("CODEC_USER_SKILLS_DIR") or os.path.expanduser("~/.codec/skills") sys.path.insert(0, SKILLS_DIR) +sys.path.insert(0, os.path.join(os.path.dirname(os.path.dirname(os.path.abspath(__file__))), "skills")) def get_skill_files(): diff --git a/tests/test_user_skills_dir.py b/tests/test_user_skills_dir.py new file mode 100644 index 0000000..d7b4693 --- /dev/null +++ b/tests/test_user_skills_dir.py @@ -0,0 +1,147 @@ +"""User skills folder (docs/USER-SKILLS-DIR-DESIGN.md). + +Every skill registry reads the built-in folder, then the owner's folder +(~/.codec/skills): a built-in name always wins, a user file is never trusted +by the manifest, and /api/skill/approve writes approved skills there. Nothing +here touches ~/.codec: tmp folders only. +""" +from __future__ import annotations + +import asyncio +import hashlib +import json +import os +import stat +from pathlib import Path + +import pytest + +REPO = Path(__file__).resolve().parent.parent + +SAFE = 'SKILL_TRIGGERS = ["say {word}"]\nSKILL_DESCRIPTION = "x"\n\ndef run(task, app="", ctx=""):\n return "said {word}"\n' +DANGER = ('import subprocess\nSKILL_TRIGGERS = ["run it"]\nSKILL_DESCRIPTION = "x"\n\n' + 'def run(task, app="", ctx=""):\n return subprocess.run(["true"]).returncode\n') + + +def _skill(folder: Path, fname: str, src: str) -> Path: + folder.mkdir(parents=True, exist_ok=True) + p = folder / fname + p.write_text(src, encoding="utf-8") + return p + + +@pytest.fixture +def dirs(tmp_path): + return tmp_path / "builtin", tmp_path / "user" + + +def test_the_registry_reads_the_builtins_then_the_user_folder(dirs): + from codec_skill_registry import SkillRegistry + builtin, user = dirs + _skill(builtin, "alpha.py", SAFE.format(word="builtin")) + _skill(user, "beta.py", SAFE.format(word="beta")) + _skill(user, "alpha.py", SAFE.format(word="shadow")) # same name as a built-in + reg = SkillRegistry(str(builtin), str(user)) + assert reg.scan() == 2 and sorted(reg.names()) == ["alpha", "beta"] + assert reg.load("alpha").run("") == "said builtin", "a built-in name always wins" + assert reg.load("beta").run("") == "said beta" + + +def test_a_user_file_is_never_trusted_by_the_manifest(dirs): + from codec_skill_registry import SkillRegistry + builtin, user = dirs + _skill(builtin, "danger_tool.py", DANGER) + (builtin / ".manifest.json").write_text(json.dumps( + {"skills": {"danger_tool.py": hashlib.sha256(DANGER.encode()).hexdigest()}})) + _skill(user, "copycat.py", DANGER) # the same bytes, under another name + reg = SkillRegistry(str(builtin), str(user)) + reg.scan() + assert reg.load("danger_tool") is not None, "the pinned built-in still loads" + assert reg.load("copycat") is None, "the user copy goes through the AST gate and is refused" + + +def test_the_ast_gate_decides_for_user_files(dirs): + from codec_skill_registry import SkillRegistry + builtin, user = dirs + builtin.mkdir() + _skill(user, "ok_skill.py", SAFE.format(word="ok")) + _skill(user, "bad_skill.py", DANGER) + reg = SkillRegistry(str(builtin), str(user)) + reg.scan() + assert reg.load("ok_skill").run("") == "said ok" + assert reg.load("bad_skill") is None + + +def test_underscore_files_and_subfolders_are_not_read(dirs): + from codec_skill_registry import SkillRegistry + builtin, user = dirs + builtin.mkdir() + _skill(user, "_private.py", SAFE.format(word="p")) + _skill(user / "_dormant-2026-10-07", "old.py", SAFE.format(word="old")) + _skill(user / "sub", "nested.py", SAFE.format(word="n")) + reg = SkillRegistry(str(builtin), str(user)) + assert reg.scan() == 0 and reg.names() == [] + + +def test_the_user_folder_is_off_when_it_is_the_builtin_folder_or_missing(dirs): + from codec_skill_registry import SkillRegistry + builtin, user = dirs + _skill(builtin, "alpha.py", SAFE.format(word="a")) + assert SkillRegistry(str(builtin), str(builtin)).user_dir is None + reg = SkillRegistry(str(builtin), str(user)) # user folder does not exist + assert reg.scan() == 1 and reg.names() == ["alpha"] + + +class _Req: + def __init__(self, payload): + self._p = payload + + async def json(self): + return self._p + + +def test_approve_writes_to_the_user_folder_owner_only(tmp_path, monkeypatch): + import codec_config + import routes.skills as rs + user = tmp_path / "user_skills" + monkeypatch.setattr(codec_config, "USER_SKILLS_DIR", str(user)) + monkeypatch.setattr(rs, "_reviews_dir", lambda: str(tmp_path / "reviews")) + code = 'SKILL_NAME = "moon"\nSKILL_DESCRIPTION = "x"\nSKILL_TRIGGERS = []\ndef run(task, app="", ctx=""):\n return "ok"\n' + rs._pending_skills["rev_user"] = {"code": code, "filename": "moon_helper.py"} + resp = asyncio.run(rs.skill_approve(_Req({"review_id": "rev_user"}))) + assert getattr(resp, "status_code", 200) == 200, resp + path = user / "moon_helper.py" + assert path.read_text() == code and resp["path"] == str(path) + assert stat.S_IMODE(path.stat().st_mode) == 0o600 and stat.S_IMODE(user.stat().st_mode) == 0o700 + assert not list(user.glob("*.tmp")), "no half-written file left behind" + assert not (REPO / "skills" / "moon_helper.py").exists(), "never the repo's built-in folder" + rs._pending_skills["rev_pinned"] = {"code": code, "filename": "calculator.py"} + resp = asyncio.run(rs.skill_approve(_Req({"review_id": "rev_pinned"}))) + assert resp.status_code == 400 and not (user / "calculator.py").exists(), "a built-in name is still refused" + + +def test_learning_and_self_improve_see_user_skills(tmp_path, monkeypatch): + import codec_config + import codec_self_improve + import routes.learning as rl + user = tmp_path / "user_skills" + _skill(user, "moon_helper.py", 'SKILL_NAME = "moon"\n' + SAFE.format(word="m")) + monkeypatch.setattr(codec_config, "USER_SKILLS_DIR", str(user)) + monkeypatch.setattr(codec_self_improve, "USER_SKILLS_DIR", str(user)) + assert "moon_helper" in rl._installed(), "an approved skill shows as installed before a restart" + names = codec_self_improve._existing_skill_names() + assert {"moon_helper", "moon"} <= names, "self-improve does not propose it again" + + +def test_every_registry_gets_the_user_folder(): + for mod in ("codec_dispatch.py", "codec_mcp.py", "codec_voice.py", "codec_agents.py", "codec_autopilot.py", + "codec_slash_commands.py"): + src = (REPO / mod).read_text(encoding="utf-8") + assert "SkillRegistry(SKILLS_DIR, USER_SKILLS_DIR)" in src and "SkillRegistry(SKILLS_DIR)" not in src, mod + import codec_config + import codec_dispatch + # The module-level registry keeps the folder it was built with (import order decides + # which test's value that is), so check what matters: it has a user folder, not the repo's. + assert codec_dispatch.registry.user_dir + assert os.path.realpath(codec_dispatch.registry.user_dir) != os.path.realpath(REPO / "skills") + assert codec_config.USER_SKILLS_DIR == os.environ["CODEC_USER_SKILLS_DIR"], "the suite never reads the owner's folder" From 5f71af234a1136ac8ccab757d2fac7bc63c13385 Mon Sep 17 00:00:00 2001 From: Mickael Farina Date: Wed, 7 Oct 2026 13:48:28 +0700 Subject: [PATCH 3/3] docs: approved-skills folder known issue fixed Co-Authored-By: Claude Opus 5.5 --- docs/known-issues.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/known-issues.md b/docs/known-issues.md index c2dacb3..fba080f 100644 --- a/docs/known-issues.md +++ b/docs/known-issues.md @@ -443,7 +443,7 @@ one waiting for approval or paused, where it is the time since it started. Fix: "started N min ago" unless the project is running. Found in the Mac merge pass. Revisit: the next item that touches the Activity board. -## Approved skills are written into the repo's `skills/` folder (2026-10-07) +## Approved skills are written into the repo's `skills/` folder (2026-10-07) — FIXED (user skills folder, docs/USER-SKILLS-DIR-DESIGN.md: approve writes to ~/.codec/skills and every registry reads it) `POST /api/skill/approve` (the review-and-approve flow, and Settings > Learning's Approve and install since P3.12) writes to `routes._shared._get_skills_dir()`,