From 3f7f8495d630e27721eb8f1c8fd7c886bdfd4dcc Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Tue, 17 Mar 2026 15:36:17 -0700 Subject: [PATCH] feat: skills modal redesign + runtime config editing for installed skills MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Redesigns the create/edit/view skill modal into a two-column spec manifest layout (Identity/Manifest/Deployment | Skill Content) matching the Agent Skills spec structure. Installed (readonly) skills can now have their runtime config (model, temperature, token limits, enabled) edited independently of the locked spec/content fields. - Two-column spec layout with section headings (Identity, Manifest, Deployment, Skill Content); content textarea uses monospace font and fills the column - h3 section headings for screen-reader nav; h3 UA stylesheet reset in CSS - Runtime Config collapsible uses 3-column grid; license field is now a select of SPDX identifiers (MIT, Apache-2.0, GPL-3.0, AGPL-3.0, etc.) - Origin badge (cyan) shows source URL for installed skills in view mode - server.py: _SKILL_RUNTIME_CONFIG_FIELDS frozenset; readonly skills filter updates to config-only fields (spec fields silently dropped); audit action distinguishes skill.update.config from skill.update; license field capped at 128 chars in both create and update paths - governance.js: spec fields disabled for readonly; config fields always editable; Save button shown for all skills (labeled "Save Config" when readonly); collapsible state reset between modal opens prevents state leak; esk-allowed-tools disabled state driven by auto_approve not readonly - Tests: spec-only body on readonly skill → 400; config-only → 200 with spec fields unchanged; mixed body → config fields applied, spec dropped --- tests/test_skills.py | 64 +++++- turnstone/console/server.py | 34 ++- turnstone/console/static/governance.js | 40 +++- turnstone/console/static/index.html | 273 +++++++++++++++---------- turnstone/console/static/style.css | 132 ++++++++++++ 5 files changed, 427 insertions(+), 116 deletions(-) diff --git a/tests/test_skills.py b/tests/test_skills.py index 8060601d..b7eaf327 100644 --- a/tests/test_skills.py +++ b/tests/test_skills.py @@ -802,8 +802,8 @@ class TestSkillAPI: ) assert resp.status_code == 404 - def test_update_skill_readonly_rejected(self, api_client, api_storage): - """Updating a readonly (MCP-sourced) skill returns 403.""" + def test_update_skill_readonly_spec_fields_rejected(self, api_client, api_storage): + """Updating spec fields on a readonly skill returns 400 (filtered to nothing).""" _create_template( api_storage, "s1", @@ -815,9 +815,65 @@ class TestSkillAPI: ) resp = api_client.put( "/v1/api/admin/skills/s1", - json={"description": "hacked"}, + json={"description": "hacked", "content": "evil"}, ) - assert resp.status_code == 403 + assert resp.status_code == 400 + assert "runtime config" in resp.json()["error"].lower() + + def test_update_skill_readonly_runtime_config_allowed(self, api_client, api_storage): + """Runtime config fields can be updated on a readonly (installed) skill.""" + _create_template( + api_storage, + "s1", + "installed-skill", + "external content", + origin="source", + readonly=True, + ) + resp = api_client.put( + "/v1/api/admin/skills/s1", + json={"model": "gpt-5", "temperature": 0.5, "enabled": False}, + ) + assert resp.status_code == 200 + data = resp.json() + assert data["model"] == "gpt-5" + assert data["temperature"] == 0.5 + assert data["enabled"] is False + # Spec fields must remain unchanged + assert data["content"] == "external content" + + def test_update_skill_readonly_mixed_body_filters_spec(self, api_client, api_storage): + """When JS sends all fields for a readonly skill, spec fields are silently dropped.""" + _create_template( + api_storage, + "s1", + "installed", + "original content", + origin="source", + readonly=True, + ) + resp = api_client.put( + "/v1/api/admin/skills/s1", + # Simulate what the browser form submits: every field present + json={ + "name": "hacked", + "content": "evil content", + "description": "tampered", + "model": "gpt-5", + "enabled": False, + "token_budget": 50000, + }, + ) + assert resp.status_code == 200 + data = resp.json() + # Config fields updated + assert data["model"] == "gpt-5" + assert data["enabled"] is False + assert data["token_budget"] == 50000 + # Spec fields unchanged + assert data["name"] == "installed" + assert data["content"] == "original content" + assert data["description"] == "" def test_update_skill_recomputes_token_estimate(self, api_client, api_storage): """Updating content recomputes token_estimate.""" diff --git a/turnstone/console/server.py b/turnstone/console/server.py index 921ed05e..a0fa74bf 100644 --- a/turnstone/console/server.py +++ b/turnstone/console/server.py @@ -2134,6 +2134,24 @@ async def admin_delete_policy(request: Request) -> JSONResponse: _VALID_ACTIVATIONS = {"named", "default", "search"} +# Fields that may be updated on installed (readonly) skills. +# These are local runtime configuration — not part of the SKILL.md spec — +# so they don't compromise the fidelity of an externally-sourced skill. +_SKILL_RUNTIME_CONFIG_FIELDS = frozenset( + { + "model", + "temperature", + "reasoning_effort", + "max_tokens", + "token_budget", + "agent_max_turns", + "auto_approve", + "allowed_tools", + "enabled", + "notify_on_complete", + } +) + def _parse_skill_session_config(body: dict[str, Any]) -> tuple[dict[str, Any], JSONResponse | None]: """Parse and validate session config fields from a skill request body. @@ -2372,7 +2390,7 @@ async def admin_create_skill(request: Request) -> JSONResponse: org_id = str(body.get("org_id", "")).strip()[:64] author = str(body.get("author", "")).strip()[:256] version = str(body.get("version", "1.0.0")).strip()[:64] - license_val = str(body.get("license", "")).strip() + license_val = str(body.get("license", "")).strip()[:128] compatibility = str(body.get("compatibility", "")).strip()[:500] raw_tags = body.get("tags", []) @@ -2462,8 +2480,7 @@ async def admin_update_skill(request: Request) -> JSONResponse: existing = storage.get_prompt_template(skill_id) if existing is None: return JSONResponse({"error": "Skill not found"}, status_code=404) - if existing.get("readonly"): - return JSONResponse({"error": "MCP-sourced skills are read-only"}, status_code=403) + is_readonly = bool(existing.get("readonly")) body = await read_json_or_400(request) if isinstance(body, JSONResponse): @@ -2504,7 +2521,7 @@ async def admin_update_skill(request: Request) -> JSONResponse: if "version" in body: updates["version"] = str(body["version"]).strip()[:64] if "license" in body: - updates["license"] = str(body["license"]).strip() + updates["license"] = str(body["license"]).strip()[:128] if "compatibility" in body: updates["compatibility"] = str(body["compatibility"]).strip()[:500] if "tags" in body: @@ -2519,6 +2536,13 @@ async def admin_update_skill(request: Request) -> JSONResponse: tag_str = "[]" updates["tags"] = tag_str + # Installed (readonly) skills: restrict updates to runtime config only. + # Spec/content fields are locked to preserve external-source fidelity. + if is_readonly: + updates = {k: v for k, v in updates.items() if k in _SKILL_RUNTIME_CONFIG_FIELDS} + if not updates: + return JSONResponse({"error": "No runtime config fields to update"}, status_code=400) + # Snapshot current state for version history before applying update existing_versions = storage.list_skill_versions(skill_id) version_int = len(existing_versions) + 1 @@ -2536,7 +2560,7 @@ async def admin_update_skill(request: Request) -> JSONResponse: record_audit( storage, audit_uid, - "skill.update", + "skill.update.config" if is_readonly else "skill.update", "skill", skill_id, updates, diff --git a/turnstone/console/static/governance.js b/turnstone/console/static/governance.js index 19eb4530..0069ad9b 100644 --- a/turnstone/console/static/governance.js +++ b/turnstone/console/static/governance.js @@ -1187,13 +1187,37 @@ function showEditTemplateModal(tmplId) { }); }; } + // Reset collapsible state before applying readonly rules (prevents state leak + // when switching between readonly and editable skills in the same session) + var allDetails = document.querySelectorAll( + "#edit-template-box .admin-details", + ); + for (var d = 0; d < allDetails.length; d++) allDetails[d].open = false; + // --- Readonly mode for imported skills --- var isReadonly = tmpl.readonly || false; var editTitle = document.getElementById("edit-template-title"); if (editTitle) editTitle.textContent = isReadonly ? "View Skill" : "Edit Skill"; + // Origin badge — show provenance for installed skills + var originBadge = document.getElementById("etm-origin-badge"); + if (originBadge) { + if (isReadonly && tmpl.source_url) { + originBadge.textContent = "Installed from \u00a0" + tmpl.source_url; + originBadge.style.display = "inline-flex"; + } else if (isReadonly && tmpl.origin && tmpl.origin !== "manual") { + originBadge.textContent = "Installed skill"; + originBadge.style.display = "inline-flex"; + } else { + originBadge.style.display = "none"; + } + } var submitBtn = document.getElementById("etm-submit"); - if (submitBtn) submitBtn.style.display = isReadonly ? "none" : ""; + if (submitBtn) { + submitBtn.style.display = ""; + submitBtn.textContent = isReadonly ? "Save Config" : "Save"; + } + // Spec/content fields: locked for installed skills (preserve source fidelity) [ "etm-name", "etm-category", @@ -1206,6 +1230,12 @@ function showEditTemplateModal(tmplId) { "etm-activation", "etm-content", "etm-default", + ].forEach(function (id) { + var el = document.getElementById(id); + if (el) el.disabled = isReadonly; + }); + // Runtime config fields: always editable (local settings, not part of SKILL.md spec) + [ "esk-model", "esk-temperature", "esk-reasoning-effort", @@ -1213,15 +1243,17 @@ function showEditTemplateModal(tmplId) { "esk-token-budget", "esk-agent-max-turns", "esk-auto-approve", - "esk-allowed-tools", "esk-enabled", ].forEach(function (id) { var el = document.getElementById(id); - if (el) el.disabled = isReadonly; + if (el) el.disabled = false; }); + // esk-allowed-tools follows auto_approve state, not readonly state + var allowedToolsEl = document.getElementById("esk-allowed-tools"); + if (allowedToolsEl) allowedToolsEl.disabled = tmpl.auto_approve || false; var cancelBtn = document.querySelector("#edit-template-box .modal-cancel"); if (cancelBtn) cancelBtn.textContent = isReadonly ? "Close" : "Cancel"; - // Auto-expand collapsibles in view mode + // Auto-expand Runtime Config collapsible for installed skills so config is visible if (isReadonly) { var details = document.querySelectorAll( "#edit-template-box .admin-details", diff --git a/turnstone/console/static/index.html b/turnstone/console/static/index.html index 06b58271..c2bd71c9 100644 --- a/turnstone/console/static/index.html +++ b/turnstone/console/static/index.html @@ -848,67 +848,100 @@ window.TURNSTONE_KB_SHORTCUTS = [