From 54ed7cbb7b9518e1e957b62849c3d9aae3d9f6d3 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Tue, 15 Sep 2026 14:39:49 -0700 Subject: [PATCH] fix: validate the skill name before opening its lock; key lock files on a digest Review finding on #112218 (major): `_skill_lock_path` opened `/.locks/.lock` before the name was validated, so `skill_manage(action='create', name='a'*300)` raised OSError (File name too long) and a NUL name raised ValueError instead of the handler's JSON error, and every rejected name ('../../etc', '') left a residue lock file. - tools/skill_manager_tool.py: lock filename is sha256(basename).lock (fixed width, no filesystem limit reachable; `foo` and `category/foo` still share one lock), the redundant `_find_skill` rglob is gone, and `skill_manage` runs `_validate_name` on the name (create) / basename (other actions) before the lock is opened. - '.locks' joins the skills-dir exclusion sets (EXCLUDED_SKILL_DIRS, ledger _NON_PACKAGE_TOPS, learning-graph/skill-commands skip parts, curator backup excludes). - tests: 2 invariants in TestSkillMutationLock (rejected names -> JSON + no .locks residue; digest-keyed lock shared across name forms), red on the old head. --- agent/curator_backup.py | 5 +++-- agent/learning_graph.py | 2 +- agent/skill_commands.py | 2 +- agent/skill_utils.py | 2 +- tests/tools/test_skill_manager_tool.py | 22 ++++++++++++++++++++++ tools/skill_ledger.py | 2 +- tools/skill_manager_tool.py | 13 +++++++++---- 7 files changed, 38 insertions(+), 10 deletions(-) diff --git a/agent/curator_backup.py b/agent/curator_backup.py index 3078f6d03b..842a67bbfc 100644 --- a/agent/curator_backup.py +++ b/agent/curator_backup.py @@ -32,9 +32,10 @@ DEFAULT_KEEP = 5 # Never rolled into a snapshot: .hub/ is owned by the skills hub (rolling it back breaks lockfile invariants); .curator_backups # is the backup dir itself; .git is repository metadata — rolling it back breaks git tracking, and snapshots that include it grow # with the full history (once backups are committed back, each snapshot contains the prior ones: 38MB of skills inflated to 24GB -# in weeks). The tar filter in ``snapshot_skills`` applies the same set to nested paths, so a nested ``.git`` is skipped too. +# in weeks); .locks holds skill_manage's per-skill lock files — restoring them would swap a lock out from under a waiting +# writer. The tar filter in ``snapshot_skills`` applies the same set to nested paths, so a nested ``.git`` is skipped too. # See #91449. -_EXCLUDE_TOP_LEVEL = {".curator_backups", ".hub", ".git"} +_EXCLUDE_TOP_LEVEL = {".curator_backups", ".hub", ".locks", ".git"} # Snapshot id: UTC ISO with colons replaced by dashes (Windows-safe filename); optional ``-NN`` suffix for same-second snapshots. _ID_RE = re.compile(r"^\d{4}-\d{2}-\d{2}T\d{2}-\d{2}-\d{2}Z(-\d{2})?$") diff --git a/agent/learning_graph.py b/agent/learning_graph.py index 29311cd3a7..394cd2eb31 100644 --- a/agent/learning_graph.py +++ b/agent/learning_graph.py @@ -18,7 +18,7 @@ from typing import Any, Optional from hermes_constants import get_hermes_home -_SKIP_PARTS = {".archive", ".hub", "node_modules", ".git"} +_SKIP_PARTS = {".archive", ".hub", ".locks", "node_modules", ".git"} _USAGE_TS_KEYS = ("last_activity_at", "last_used_at", "last_viewed_at", "last_patched_at", "created_at") diff --git a/agent/skill_commands.py b/agent/skill_commands.py index 04ef794e74..ae64a88189 100644 --- a/agent/skill_commands.py +++ b/agent/skill_commands.py @@ -323,7 +323,7 @@ def _scaffold_header( return "\n".join(lines) -_SCAN_SKIP_PARTS = {'.git', '.github', '.hub', '.archive'} +_SCAN_SKIP_PARTS = {'.git', '.github', '.hub', '.archive', '.locks'} def _scan_skill_md(skill_md: Path, disabled: set, seen_names: set, commands: Dict[str, Dict[str, Any]], resolve_command) -> None: diff --git a/agent/skill_utils.py b/agent/skill_utils.py index 17b83186a1..1b0b002ab4 100644 --- a/agent/skill_utils.py +++ b/agent/skill_utils.py @@ -21,7 +21,7 @@ logger = logging.getLogger(__name__) PLATFORM_MAP = {"macos": "darwin", "linux": "linux", "windows": "win32"} EXCLUDED_SKILL_DIRS = frozenset(( - ".git", ".github", ".hub", ".archive", ".curator_backups", + ".git", ".github", ".hub", ".archive", ".curator_backups", ".locks", ".venv", "venv", "node_modules", "site-packages", "__pycache__", ".tox", ".nox", ".pytest_cache", ".mypy_cache", ".ruff_cache", )) diff --git a/tests/tools/test_skill_manager_tool.py b/tests/tools/test_skill_manager_tool.py index 3fa02e9a32..720169fb11 100644 --- a/tests/tools/test_skill_manager_tool.py +++ b/tests/tools/test_skill_manager_tool.py @@ -1,5 +1,6 @@ """Tests for tools/skill_manager_tool.py — skill creation, editing, and deletion.""" +import hashlib import json import threading from contextlib import contextmanager @@ -21,6 +22,7 @@ from tools.skill_manager_tool import ( _write_file, _remove_file, _find_skill, + _skill_lock_path, skill_manage, ) from agent.skill_utils import ( @@ -359,6 +361,26 @@ class TestSkillMutationLock: assert "# Test Skill Updated" in content assert "Step 1 Updated:" in content + @pytest.mark.parametrize("name", ["a" * 300, "bad\x00name", "../../etc", ""]) + def test_rejected_name_returns_json_and_leaves_no_lock_file(self, tmp_path, name): + """Name validation runs before the lock is opened: an over-long / NUL / traversal / empty + name yields the create handler's JSON error (never OSError/ValueError from the lock + path) and leaves nothing behind in ``/.locks/``.""" + with _skill_dir(tmp_path): + result = json.loads(skill_manage(action="create", name=name, content=VALID_SKILL_CONTENT)) + assert result["success"] is False + assert result["error"] == _validate_name(name) + assert not (tmp_path / ".locks").exists() + + def test_lock_path_is_digest_keyed_and_shared_across_name_forms(self, tmp_path): + """``foo`` and ``category/foo`` share one lock, keyed on a fixed-width digest of the + basename so the filename never depends on the skill name's length or characters.""" + with _skill_dir(tmp_path): + lock = _skill_lock_path("mlops/foo") + assert lock == _skill_lock_path("foo") + assert lock.parent == tmp_path / ".locks" + assert lock.name == hashlib.sha256(b"foo").hexdigest() + ".lock" + class TestDeleteSkill: def test_delete_cleans_empty_category_dir(self, tmp_path): diff --git a/tools/skill_ledger.py b/tools/skill_ledger.py index cbe041ddff..5a2a58dc77 100644 --- a/tools/skill_ledger.py +++ b/tools/skill_ledger.py @@ -35,7 +35,7 @@ _ARCHIVE_TS_SUFFIX_RE = re.compile(r"^(.+)-\d{14}$") # support files first, so a disk-only capture would restore a hollow skill. _PACKAGE_RESTORE_ACTIONS = frozenset({"delete", "archive", "purge"}) _VALID_ACTORS = {"curator", "agent", "user"} -_NON_PACKAGE_TOPS = {".curator_backups", ".hub", ".archive"} +_NON_PACKAGE_TOPS = {".curator_backups", ".hub", ".archive", ".locks"} # Explicit actor override: the CLI sets "user", the curator walk sets "curator". _actor_override: contextvars.ContextVar[Optional[str]] = contextvars.ContextVar( diff --git a/tools/skill_manager_tool.py b/tools/skill_manager_tool.py index 29c027e08e..db87bee81a 100644 --- a/tools/skill_manager_tool.py +++ b/tools/skill_manager_tool.py @@ -8,6 +8,7 @@ existing skills (bundled, hub, user) are modified in place. Layout: """ import contextvars as _ctxvars +import hashlib import json from contextlib import ExitStack, suppress import logging @@ -83,10 +84,10 @@ def _skills_dir() -> Path: def _skill_lock_path(name: str) -> Path: """Per-skill lock file under ``/.locks/`` (same idiom as the usage ledger's ``.usage.json.lock``), never inside the skill dir so delete/recreate cannot unlink it under a - waiting writer. Keyed by the resolved skill dir so ``foo`` and ``category/foo`` share one lock.""" - existing = _find_skill(name) - skill_dir = Path(existing["path"]) if existing else _resolve_skill_dir(name) - return _skills_dir() / ".locks" / f"{skill_dir.name}.lock" + waiting writer. Keyed by a digest of the basename so ``foo`` and ``category/foo`` share one + lock and no name can hit a filesystem limit (callers validate the basename first).""" + digest = hashlib.sha256(Path(name).name.encode("utf-8", "surrogatepass")).hexdigest() + return _skills_dir() / ".locks" / f"{digest}.lock" def _skill_mutation_lock(name: str): @@ -785,6 +786,10 @@ def skill_manage( for arg, missing, message in _REQUIRED_ARGS.get(action, ()): if missing(args[arg]): return tool_error(message, success=False) + # Validate before the lock is keyed on the name, so a rejected name never touches .locks/ + # (create takes a bare name; the other actions also accept ``category/name``). + if (name_err := _validate_name(name if action == "create" or not name else Path(name).name)) is not None: + return json.dumps(_err(name_err), ensure_ascii=False) # A mutation is read-modify-write even when its action eventually delegates # to a helper: guards, ledger capture, patch matching, validation, rollback, # and the atomic replacement all belong to the same ownership window.