From 0537e2600df1c78d5855ec7e873644fe2b2fcd80 Mon Sep 17 00:00:00 2001
From: kshitijk4poor <82637225+kshitijk4poor@users.noreply.github.com>
Date: Thu, 28 May 2026 02:09:15 +0530
Subject: [PATCH] fix(skills): atomic lock write + drop dead
 _validate_category_name
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

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.
---
 tools/skills_hub.py  |  4 ----
 tools/skills_sync.py | 24 +++++++++++++++++++++++-
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/tools/skills_hub.py b/tools/skills_hub.py
index 657a455cf4a..01b53b68691 100644
--- a/tools/skills_hub.py
+++ b/tools/skills_hub.py
@@ -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)
 
diff --git a/tools/skills_sync.py b/tools/skills_sync.py
index 91c1408c432..81710a7b870 100644
--- a/tools/skills_sync.py
+++ b/tools/skills_sync.py
@@ -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