ewilliams28/turnstone
1
0
Fork 0
mirror of https://github.com/turnstonelabs/turnstone.git synced 2026-08-13 15:32:24 -06:00
Code Issues Packages Projects Releases Wiki Activity

fix(skills): apply review feedback on unlock action

Browse Source 
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.
This commit is contained in:
Patrick Buckley
2026-05-08 14:14:44 -07:00
parent ee06e4961f
commit eea795d34a
8 changed files with 171 additions and 55 deletions
Download Patch File Download Diff File Expand all files Collapse all files
tests/test_skills.py
+22
View File
@@ -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
+12 -13
View File
@@ -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)
version_int = len(existing_versions) + 1
storage.create_skill_version(
skill_id=skill_id,
version=version_int,
snapshot=_json.dumps(existing, default=str),
changed_by=audit_uid,
)
storage.set_skill_readonly(skill_id, False)
snapshot = _json.dumps(existing, default=str)
version_int = storage.unlock_skill(skill_id, snapshot, audit_uid)
if version_int is None:
# Row vanished between the existence check and the atomic unlock —
# treat as 404 rather than 500.
return JSONResponse({"error": "Skill not found"}, status_code=404)
record_audit(
storage,
@@ -7070,14 +7066,17 @@ async def admin_unlock_skill(request: Request) -> JSONResponse:
"skill",
skill_id,
{
"name": existing.get("name", ""),
"source_url": existing.get("source_url", ""),
"origin": existing.get("origin", ""),
"name": existing.get("name") or "",
"source_url": existing.get("source_url") or "",
"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
+60 -26
View File
@@ -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");
// Focus management
if (isReadonly) {
if (cancelBtn) cancelBtn.focus();
} else {
document.getElementById("etm-name").focus();
if (!alreadyOpen) {
_etmTrapHandler = _installTrap(
"edit-template-overlay",
"edit-template-box",
);
// 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(
"Customize " +
skillName +
"?\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.",
showConfirmModal(
"Customize " + skillName + "?",
"This detaches the skill from its upstream source so you can edit " +
"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;
btn.textContent = "Customizing…";
}
function _performUnlockSkill(skillId) {
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");
hideEditTemplateModal();
loadGovSkills();
showEditTemplateModal(skillId);
showToast("Skill unlocked — fields are now editable");
// Refresh the cached list, then re-render the open modal in place from
// the fresh row. No close/reopen → no flicker, no focus bounce.
return loadGovSkills().then(function () {
showEditTemplateModal(skillId);
});
})
.catch(function (e) {
showToast(e.message || "Unlock failed");
})
.finally(function () {
btn.disabled = false;
btn.textContent = prevText;
if (btn) {
btn.disabled = false;
btn.textContent = "Customize…";
}
});
}
turnstone/console/static/index.html
+1
View File
@@ -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
+10 -2
View File
@@ -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;
padding: 9px;
/* Sized to content so the primary action (Save) keeps a stable footprint
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
+27 -4
View File
@@ -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
+12 -6
View File
@@ -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:
"""Toggle the readonly flag on a skill. Returns True if the row exists.
def unlock_skill(self, template_id: str, snapshot: str, changed_by: str) -> int | None:
"""Atomically snapshot a readonly skill and flip ``readonly=False``.
Dedicated writer because ``readonly`` is intentionally absent from
:data:`SKILL_MUTABLE` — flipping provenance state is a deliberate
admin action (skill.unlock) that should not piggyback on the generic
update path.
Writes ``snapshot`` into ``skill_versions`` with the next sequential
version number, then sets ``readonly=False`` on the template row, all
in a single transaction so concurrent updates can't produce
``(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
+27 -4
View File
@@ -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: