fix(skills): apply review feedback on unlock action

Code review caught a race + a missing None guard; designer review
caught a window.confirm regression and a button-hierarchy issue.

Backend:
- Race fix (bug-2): replace set_skill_readonly+create_skill_version
  with a single atomic unlock_skill(template_id, snapshot, changed_by)
  -> int|None on the storage protocol (sqlite + postgres). Snapshot
  insert + readonly flip happen in one transaction; the next version
  number is computed via SELECT MAX(version)+1 inside the txn rather
  than len(list)+1 outside, closing the (skill_id, version)
  collision window where two concurrent admin actions could both pick
  the same version.
- None guard (bug-3): check the post-flip get_prompt_template re-read;
  return 404 instead of letting _skill_to_response(None) raise.
- Audit body: also record snapshot_version, and harden None-vs-empty
  with `or ""` on the existing.get(...) calls.

Frontend:
- D-1: replace window.confirm with the existing showConfirmModal
  (admin.js:2350) — themed dialog, focus-trap, can render the source
  URL with consistent typography. The native dialog could collapse
  the multi-paragraph copy depending on browser.
- D-2: mutate-in-place on success rather than hide → reload → reopen.
  loadGovSkills now returns its fetch promise so unlockSkill can
  chain showEditTemplateModal after the cache refresh — no flicker,
  no focus bounce, and it kills bug-1 (the reopen was reading stale
  _govSkills before loadGovSkills resolved). showEditTemplateModal
  is idempotent when already open: it skips the trigger-element
  capture and the focus-trap reinstall.
- D-3: button hierarchy. Drop flex:1 from .modal-secondary so the
  Save button keeps a stable width whether or not Customize is
  rendered; insert a flex-spacer between Customize and Save so the
  destructive-ish detach groups left next to Cancel and the primary
  action floats right.
- D-4: NBSP normalized to match the existing   escape pattern
  on the sibling badge line (was an actual NBSP byte).
- D-5: success toast now reads "Skill unlocked — fields are now
  editable" so the operator gets a positive affirmation that the
  edit affordance is live.
- D-10: aria-describedby="etm-origin-badge" on disabled spec inputs
  so screen-reader users get the same "this came from upstream"
  context that sighted users see in the cyan badge.

Tests: + test_unlock_skill_versions_after_existing_history seeds an
out-of-order version (3) and asserts unlock picks 4, defending
against the len()-based version computation regressing.

tests/test_skills.py

@@ -1108,6 +1108,28 @@ class TestSkillAPI:
         assert len(versions) == 1
         assert versions[0]["version"] == 1
 
+    def test_unlock_skill_versions_after_existing_history(self, api_client, api_storage):
+        """Unlock picks the next version number based on max(version), not list length.
+
+        Defends against regressions where len(list)+1 racing with a concurrent
+        update could produce a (skill_id, version) collision. Seeds an
+        out-of-order version (3) and verifies unlock chooses 4, not 2.
+        """
+        import json as _json
+
+        _create_template(
+            api_storage, "s1", "installed", "v0 content", origin="source", readonly=True
+        )
+        api_storage.create_skill_version(
+            skill_id="s1", version=3, snapshot=_json.dumps({"v": 3}), changed_by="prior"
+        )
+
+        resp = api_client.post("/v1/api/admin/skills/s1/unlock")
+        assert resp.status_code == 200
+
+        versions = sorted(api_storage.list_skill_versions("s1"), key=lambda v: v["version"])
+        assert [v["version"] for v in versions] == [3, 4]
+
     def test_unlock_skill_already_unlocked(self, api_client, api_storage):
         """Unlocking an already-unlocked skill returns 400."""
         _create_template(api_storage, "s1", "local", "content", origin="manual", readonly=False)

turnstone/console/server.py

@@ -7052,16 +7052,12 @@ async def admin_unlock_skill(request: Request) -> JSONResponse:
 
     audit_uid, ip = _audit_context(request)
 
-    existing_versions = storage.list_skill_versions(skill_id)
+    snapshot = _json.dumps(existing, default=str)
-    version_int = len(existing_versions) + 1
+    version_int = storage.unlock_skill(skill_id, snapshot, audit_uid)
-    storage.create_skill_version(
+    if version_int is None:
-        skill_id=skill_id,
+        # Row vanished between the existence check and the atomic unlock —
-        version=version_int,
+        # treat as 404 rather than 500.
-        snapshot=_json.dumps(existing, default=str),
+        return JSONResponse({"error": "Skill not found"}, status_code=404)
-        changed_by=audit_uid,
-    )
-
-    storage.set_skill_readonly(skill_id, False)
 
     record_audit(
         storage,
@@ -7070,14 +7066,17 @@ async def admin_unlock_skill(request: Request) -> JSONResponse:
         "skill",
         skill_id,
         {
-            "name": existing.get("name", ""),
+            "name": existing.get("name") or "",
-            "source_url": existing.get("source_url", ""),
+            "source_url": existing.get("source_url") or "",
-            "origin": existing.get("origin", ""),
+            "origin": existing.get("origin") or "",
+            "snapshot_version": version_int,
         },
         ip,
     )
 
     updated = storage.get_prompt_template(skill_id)
+    if updated is None:
+        return JSONResponse({"error": "Skill not found"}, status_code=404)
     rc_map = storage.count_skill_resources_bulk([skill_id])
     return JSONResponse(_skill_to_response(updated, resource_count=rc_map.get(skill_id, 0)))
 

turnstone/console/static/governance.js

@@ -664,7 +664,7 @@ function submitEditPolicy() {
 // ---------------------------------------------------------------------------
 
 function loadGovSkills() {
-  authFetch("/v1/api/admin/skills")
+  return authFetch("/v1/api/admin/skills")
     .then(function (r) {
       if (!r.ok) throw new Error("Failed");
       return r.json();
@@ -1248,7 +1248,14 @@ function submitCreateTemplate() {
 }
 
 function showEditTemplateModal(tmplId) {
-  _etmTriggerEl = document.activeElement;
+  var ov = document.getElementById("edit-template-overlay");
+  // When called against an already-open modal (e.g. mutate-in-place after
+  // unlock), preserve the original trigger so focus restores to the row
+  // launcher on close, and don't reinstall the focus trap.
+  var alreadyOpen = ov && ov.style.display === "flex";
+  if (!alreadyOpen) {
+    _etmTriggerEl = document.activeElement;
+  }
   var tmpl = null;
   for (var i = 0; i < _govSkills.length; i++) {
     if (_govSkills[i].template_id === tmplId) {
@@ -1257,7 +1264,6 @@ function showEditTemplateModal(tmplId) {
     }
   }
   if (!tmpl) return;
-  var ov = document.getElementById("edit-template-overlay");
   ov.style.display = "flex";
   document.getElementById("etm-id").value = tmplId;
   document.getElementById("etm-name").value = tmpl.name;
@@ -1431,7 +1437,7 @@ function showEditTemplateModal(tmplId) {
       originBadge.textContent = "Installed skill";
       originBadge.style.display = "inline-flex";
     } else if (isUnlockedInstall && tmpl.source_url) {
-      originBadge.textContent = "Customized from  " + tmpl.source_url;
+      originBadge.textContent = "Customized from \u00a0" + tmpl.source_url;
       originBadge.style.display = "inline-flex";
     } else if (isUnlockedInstall) {
       originBadge.textContent = "Customized from upstream";
@@ -1451,7 +1457,9 @@ function showEditTemplateModal(tmplId) {
     submitBtn.style.display = "";
     submitBtn.textContent = isReadonly ? "Save Config" : "Save";
   }
-  // Spec/content fields: locked for installed skills (preserve source fidelity)
+  // Spec/content fields: locked for installed skills (preserve source fidelity).
+  // Point screen readers at the origin badge so the "why is this disabled?"
+  // affordance sighted users see is also announced.
   [
     "etm-name",
     "etm-category",
@@ -1466,7 +1474,13 @@ function showEditTemplateModal(tmplId) {
     "etm-default",
   ].forEach(function (id) {
     var el = document.getElementById(id);
-    if (el) el.disabled = isReadonly;
+    if (!el) return;
+    el.disabled = isReadonly;
+    if (isReadonly) {
+      el.setAttribute("aria-describedby", "etm-origin-badge");
+    } else {
+      el.removeAttribute("aria-describedby");
+    }
   });
   // Runtime config fields: always editable (local settings, not part of SKILL.md spec)
   [
@@ -1499,12 +1513,18 @@ function showEditTemplateModal(tmplId) {
   if (resSection) {
     _loadSkillResources(tmplId, isReadonly);
   }
-  _etmTrapHandler = _installTrap("edit-template-overlay", "edit-template-box");
+  if (!alreadyOpen) {
-  // Focus management
+    _etmTrapHandler = _installTrap(
-  if (isReadonly) {
+      "edit-template-overlay",
-    if (cancelBtn) cancelBtn.focus();
+      "edit-template-box",
-  } else {
+    );
-    document.getElementById("etm-name").focus();
+    // Focus management — only on the initial open. Re-renders preserve
+    // wherever focus was so a screen reader doesn't get a transition.
+    if (isReadonly) {
+      if (cancelBtn) cancelBtn.focus();
+    } else {
+      document.getElementById("etm-name").focus();
+    }
   }
 }
 
@@ -1523,15 +1543,25 @@ function unlockSkill() {
   var skillId = btn.dataset.skillId;
   var skillName = btn.dataset.skillName || "this skill";
   if (!skillId) return;
-  var ok = window.confirm(
+  showConfirmModal(
-    "Customize " +
+    "Customize " + skillName + "?",
-      skillName +
+    "This detaches the skill from its upstream source so you can edit " +
-      "?\n\nThis detaches the skill from its upstream source so you can edit content, description, and resources locally. The pre-customization state is saved to the skill's version history — you can review it from the History tab.\n\nUpstream updates will not flow into this skill after customization.",
+      "content, description, and resources locally. The current version is " +
+      "saved to History — you can revert from there. Future updates from " +
+      "the upstream source will not be applied.",
+    "Customize",
+    function () {
+      _performUnlockSkill(skillId);
+    },
   );
-  if (!ok) return;
+}
-  btn.disabled = true;
+
-  var prevText = btn.textContent;
+function _performUnlockSkill(skillId) {
-  btn.textContent = "Customizing…";
+  var btn = document.getElementById("etm-unlock");
+  if (btn) {
+    btn.disabled = true;
+    btn.textContent = "Customizing…";
+  }
   authFetch("/v1/api/admin/skills/" + skillId + "/unlock", {
     method: "POST",
   })
@@ -1544,17 +1574,21 @@ function unlockSkill() {
       return r.json();
     })
     .then(function () {
-      showToast("Skill unlocked");
+      showToast("Skill unlocked — fields are now editable");
-      hideEditTemplateModal();
+      // Refresh the cached list, then re-render the open modal in place from
-      loadGovSkills();
+      // the fresh row. No close/reopen → no flicker, no focus bounce.
-      showEditTemplateModal(skillId);
+      return loadGovSkills().then(function () {
+        showEditTemplateModal(skillId);
+      });
     })
     .catch(function (e) {
       showToast(e.message || "Unlock failed");
     })
     .finally(function () {
-      btn.disabled = false;
+      if (btn) {
-      btn.textContent = prevText;
+        btn.disabled = false;
+        btn.textContent = "Customize…";
+      }
     });
 }
 

turnstone/console/static/index.html

@@ -3587,6 +3587,7 @@
           >
             Customize…
           </button>
+          <span class="modal-buttons-spacer" aria-hidden="true"></span>
           <button
             id="etm-submit"
             class="modal-submit"

turnstone/console/static/style.css

@@ -2074,6 +2074,9 @@ textarea.skill-content-area {
   filter: brightness(1.1);
 }
 .modal-submit:focus-visible {
+  /* fg-bright (not accent) — modal-submit has an accent background, so
+     accent-on-accent would be invisible. Cancel/secondary use --accent
+     because their backgrounds are transparent / highlight. */
   outline: 2px solid var(--fg-bright);
   outline-offset: 2px;
 }
@@ -2083,8 +2086,9 @@ textarea.skill-content-area {
   filter: none;
 }
 .modal-secondary {
-  flex: 1;
+  /* Sized to content so the primary action (Save) keeps a stable footprint
-  padding: 9px;
+     whether or not Customize is rendered. The spacer below pushes Save right. */
+  padding: 9px 14px;
   background: transparent;
   color: var(--fg);
   border: 1px solid var(--border-strong);
@@ -2108,6 +2112,10 @@ textarea.skill-content-area {
   cursor: not-allowed;
   pointer-events: none;
 }
+.modal-buttons-spacer {
+  flex: 1;
+  min-width: 0;
+}
 
 #create-user-overlay,
 #create-token-overlay,

turnstone/core/storage/_postgresql.py

@@ -2564,16 +2564,39 @@ class PostgreSQLBackend:
             conn.commit()
             return result.rowcount > 0
 
-    def set_skill_readonly(self, template_id: str, readonly: bool) -> bool:
+    def unlock_skill(self, template_id: str, snapshot: str, changed_by: str) -> int | None:
         now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S")
         with self._conn() as conn:
-            result = conn.execute(
+            row = conn.execute(
+                sa.select(prompt_templates.c.template_id).where(
+                    prompt_templates.c.template_id == template_id
+                )
+            ).first()
+            if row is None:
+                return None
+            current_max = conn.execute(
+                sa.select(sa.func.coalesce(sa.func.max(skill_versions.c.version), 0)).where(
+                    skill_versions.c.skill_id == template_id
+                )
+            ).scalar()
+            next_version = int(current_max or 0) + 1
+            conn.execute(
+                sa.insert(skill_versions),
+                {
+                    "skill_id": template_id,
+                    "version": next_version,
+                    "snapshot": snapshot,
+                    "changed_by": changed_by,
+                    "created": now,
+                },
+            )
+            conn.execute(
                 sa.update(prompt_templates)
                 .where(prompt_templates.c.template_id == template_id)
-                .values(readonly=bool(readonly), updated=now)
+                .values(readonly=False, updated=now)
             )
             conn.commit()
-            return result.rowcount > 0
+            return next_version
 
     def delete_prompt_template(self, template_id: str) -> bool:
         with self._conn() as conn:

turnstone/core/storage/_protocol.py

@@ -1254,13 +1254,19 @@ class StorageBackend(Protocol):
         """Update specified fields on a prompt template. Returns True if found."""
         ...
 
-    def set_skill_readonly(self, template_id: str, readonly: bool) -> bool:
+    def unlock_skill(self, template_id: str, snapshot: str, changed_by: str) -> int | None:
-        """Toggle the readonly flag on a skill. Returns True if the row exists.
+        """Atomically snapshot a readonly skill and flip ``readonly=False``.
 
-        Dedicated writer because ``readonly`` is intentionally absent from
+        Writes ``snapshot`` into ``skill_versions`` with the next sequential
-        :data:`SKILL_MUTABLE` — flipping provenance state is a deliberate
+        version number, then sets ``readonly=False`` on the template row, all
-        admin action (skill.unlock) that should not piggyback on the generic
+        in a single transaction so concurrent updates can't produce
-        update path.
+        ``(skill_id, version)`` collisions or a snapshot whose state is out of
+        sync with the row at the moment readonly is flipped.
+
+        Returns the assigned version number, or ``None`` if the template row
+        does not exist. ``readonly`` is intentionally absent from
+        :data:`SKILL_MUTABLE` — this dedicated writer is the only path for
+        flipping it (matching the ``set_mcp_oauth_client_secret_ct`` pattern).
         """
         ...
 

turnstone/core/storage/_sqlite.py

@@ -2717,16 +2717,39 @@ class SQLiteBackend:
             conn.commit()
             return result.rowcount > 0
 
-    def set_skill_readonly(self, template_id: str, readonly: bool) -> bool:
+    def unlock_skill(self, template_id: str, snapshot: str, changed_by: str) -> int | None:
         now = datetime.now(UTC).strftime("%Y-%m-%dT%H:%M:%S")
         with self._conn() as conn:
-            result = conn.execute(
+            row = conn.execute(
+                sa.select(prompt_templates.c.template_id).where(
+                    prompt_templates.c.template_id == template_id
+                )
+            ).first()
+            if row is None:
+                return None
+            current_max = conn.execute(
+                sa.select(sa.func.coalesce(sa.func.max(skill_versions.c.version), 0)).where(
+                    skill_versions.c.skill_id == template_id
+                )
+            ).scalar()
+            next_version = int(current_max or 0) + 1
+            conn.execute(
+                sa.insert(skill_versions),
+                {
+                    "skill_id": template_id,
+                    "version": next_version,
+                    "snapshot": snapshot,
+                    "changed_by": changed_by,
+                    "created": now,
+                },
+            )
+            conn.execute(
                 sa.update(prompt_templates)
                 .where(prompt_templates.c.template_id == template_id)
-                .values(readonly=int(bool(readonly)), updated=now)
+                .values(readonly=0, updated=now)
             )
             conn.commit()
-            return result.rowcount > 0
+            return next_version
 
     def delete_prompt_template(self, template_id: str) -> bool:
         with self._conn() as conn: