fix: validate the skill name before opening its lock; key lock files on a digest

Review finding on #112218 (major): `_skill_lock_path` opened `<skills>/.locks/<name>.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.
This commit is contained in:
teknium1
2026-09-15 14:39:49 -07:00
committed by Teknium
parent 273986f88f
commit 54ed7cbb7b
7 changed files with 38 additions and 10 deletions
+1 -1
View File
@@ -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(
+9 -4
View File
@@ -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 ``<skills>/.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.