fix(skills): atomic lock write + drop dead _validate_category_name

Self-review follow-ups on the salvage of #33177 + #33188 + #33209:

W3 (real, lock_path.write_text was non-atomic AND the read path silently
resets data to an empty installed dict on JSONDecodeError — a crash mid-
write could nuke ALL hub provenance, not just official-optional). Switch
to the same mkstemp + fsync + atomic_replace pattern that _write_manifest
already uses in this module.

W5 (dead code) — _validate_category_name had one caller on origin/main
(install_from_quarantine), swapped to _validate_install_parent_path by
#33177. Remove the now-unused definition to avoid the attractive-nuisance
of contributors picking the wrong validator.

Behavior preserved on the happy path; verified all 200 skills/hub tests
plus the three E2E scenarios (destructive restore, backfill idempotency,
adversarial nonexistent skill) still pass after both fixes.
This commit is contained in:
kshitijk4poor 2026-05-28 02:09:15 +05:30 committed by kshitij
parent ee80dfdea0
commit 0537e2600d
2 changed files with 23 additions and 5 deletions

View file

@ -120,10 +120,6 @@ def _validate_skill_name(name: str) -> str:
return _normalize_bundle_path(name, field_name="skill name", allow_nested=False)
def _validate_category_name(category: str) -> str:
return _normalize_bundle_path(category, field_name="category", allow_nested=False)
def _validate_install_parent_path(category: str) -> str:
return _normalize_bundle_path(category, field_name="install parent path", allow_nested=True)

View file

@ -390,7 +390,29 @@ def _backfill_optional_provenance(quiet: bool = False) -> List[str]:
if changed:
lock_path.parent.mkdir(parents=True, exist_ok=True)
lock_path.write_text(json.dumps(data, indent=2, ensure_ascii=False) + "\n")
# Atomic write so a crash mid-write can't silently wipe all provenance
# via the JSONDecodeError fallback above (which resets `installed` to
# an empty dict).
import tempfile
payload = json.dumps(data, indent=2, ensure_ascii=False) + "\n"
fd, tmp_path = tempfile.mkstemp(
dir=str(lock_path.parent),
prefix=".lock_",
suffix=".tmp",
)
try:
with os.fdopen(fd, "w", encoding="utf-8") as f:
f.write(payload)
f.flush()
os.fsync(f.fileno())
atomic_replace(tmp_path, lock_path)
except BaseException:
try:
os.unlink(tmp_path)
except OSError:
pass
raise
return backfilled