From 99a309ed70af41d80db31dcab29ede096a93bc58 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Mon, 18 May 2026 18:39:04 -0700 Subject: [PATCH] fix(coord): omit empty allowed_tools in list_skills + clarify semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A skill with `allowed_tools=[]` in the coordinator's `list_skills` response read as "no tools are usable by this skill" to a model that didn't know the semantics — but the actual meaning is "no tools are pre-approved for auto-approval (auto-approve exemption list)". Real misdiagnosis incident: a code-review child appeared to have been spawned with zero tool access when in fact the skill simply hadn't declared an auto-approve allowlist. Two-part fix: - `coordinator_client.list_skills` omits the `allowed_tools` key from the per-skill dict when empty. Absence now carries the unambiguous meaning "no tool is pre-approved for this skill"; presence (with a non-empty list) keeps the standard Claude Code skill-spec shape. - `turnstone/tools/list_skills.json` description rewrites the field doc so the LLM sees: "tool names exempt from the operator approval gate ... the field is OMITTED when empty: a skill without `allowed_tools` still has access to every tool in its session's toolset; absence of the field means no tool is pre-approved for this skill, not that the skill has no tools." Field name stays `allowed_tools` — matches the upstream Claude Code skill frontmatter (`allowed-tools` hyphenated, stored as `allowed_tools` internally per `skill_parser.py:241-242`). Parser, storage column, admin UI, and SDK unchanged. --- tests/test_coordinator_client.py | 43 +++++++++++++++++++++++++ turnstone/console/coordinator_client.py | 38 +++++++++++++--------- turnstone/tools/list_skills.json | 2 +- 3 files changed, 67 insertions(+), 16 deletions(-) diff --git a/tests/test_coordinator_client.py b/tests/test_coordinator_client.py index 7c96ace2..1104f6aa 100644 --- a/tests/test_coordinator_client.py +++ b/tests/test_coordinator_client.py @@ -1228,6 +1228,49 @@ def test_list_skills_hides_interactive_only_skills(tmp_path): assert skill["kind"] in {"coordinator", "any"} +def test_list_skills_omits_allowed_tools_when_empty(tmp_path): + """``allowed_tools`` is the auto-approve allowlist (tools exempt + from the operator approval gate), NOT the set of tools the skill + can use. An empty list reads as "no tool access" to a model + that doesn't know the semantics — real misdiagnosis source: a + code-review skill with no auto-approve allowlist looked like it + had been spawned with zero tools. Dropping the key when empty + removes the ambiguity at the source; absence of the field carries + the unambiguous meaning "no tool is pre-approved for this skill" + while a tool list reads as "these specific tools bypass the prompt". + """ + st = SQLiteBackend(str(tmp_path / "skills_empty.db")) + st.create_prompt_template( + template_id="s-empty", + name="empty-skill", + category="ops", + content="", + variables="[]", + is_default=False, + org_id="", + created_by="test", + tags="[]", + allowed_tools="[]", + ) + st.create_prompt_template( + template_id="s-nonempty", + name="nonempty-skill", + category="ops", + content="", + variables="[]", + is_default=False, + org_id="", + created_by="test", + tags="[]", + allowed_tools='["read_file"]', + ) + client = _make_read_client(st) + result = client.list_skills() + by_name = {s["name"]: s for s in result["skills"]} + assert "allowed_tools" not in by_name["empty-skill"] + assert by_name["nonempty-skill"]["allowed_tools"] == ["read_file"] + + def test_list_skills_projects_allowed_tools_capped_with_sentinel(tmp_path): """Each row carries the skill's allowed_tools (capped at the projection cap with a +N more sentinel) so coordinators can pick a skill without diff --git a/turnstone/console/coordinator_client.py b/turnstone/console/coordinator_client.py index 288dc933..0d4db9d9 100644 --- a/turnstone/console/coordinator_client.py +++ b/turnstone/console/coordinator_client.py @@ -1278,21 +1278,29 @@ class CoordinatorClient: allowed_tools: list[str] = [str(t) for t in allowed_full[:_SKILL_TOOLS_PROJECTION_CAP]] if len(allowed_full) > _SKILL_TOOLS_PROJECTION_CAP: allowed_tools.append(f"+{len(allowed_full) - _SKILL_TOOLS_PROJECTION_CAP} more") - skills.append( - { - "name": r.get("name") or "", - "category": r.get("category") or "", - "tags": tags, - "version": r.get("version") or "", - "description": r.get("description") or "", - "model": r.get("model") or "", - "enabled": bool(r.get("enabled")), - "risk_level": r.get("risk_level") or "", - "activation": r.get("activation") or "", - "kind": r["kind"], - "allowed_tools": allowed_tools, - } - ) + skill_row: dict[str, Any] = { + "name": r.get("name") or "", + "category": r.get("category") or "", + "tags": tags, + "version": r.get("version") or "", + "description": r.get("description") or "", + "model": r.get("model") or "", + "enabled": bool(r.get("enabled")), + "risk_level": r.get("risk_level") or "", + "activation": r.get("activation") or "", + "kind": r["kind"], + } + # Omit ``allowed_tools`` when empty: an empty list reads as + # "no tools are usable by this skill" to a model that doesn't + # know the semantics, but the actual meaning is "no tools are + # pre-approved (auto-approve exemption list)". Real + # misdiagnosis happened in testing when a code-review skill + # with no auto-approve allowlist looked like it had been + # spawned with zero tool access. Dropping the key altogether + # when empty removes the ambiguity at the source. + if allowed_tools: + skill_row["allowed_tools"] = allowed_tools + skills.append(skill_row) return {"skills": skills, "truncated": truncated} # ------------------------------------------------------------------ diff --git a/turnstone/tools/list_skills.json b/turnstone/tools/list_skills.json index 3af34850..2e2caf6d 100644 --- a/turnstone/tools/list_skills.json +++ b/turnstone/tools/list_skills.json @@ -1,6 +1,6 @@ { "name": "list_skills", - "description": "List skills (worker profiles) available to coordinators. Use to discover skill names for `spawn_workstream`. Filters: `category` (e.g. 'engineering', 'ops'), `tag` (single tag), `risk_level` (`safe`/`low`/`medium`/`high`/`critical`; omit to include unscanned rows). Results are pre-filtered to coordinator-applicable skills (`kind='coordinator'` or `'any'`); interactive-only skills are hidden. Each row returns `name`, `category`, `tags`, `version`, `description`, model preference, `enabled`, `risk_level`, `activation`, `kind`, and `allowed_tools` (capped at 20 names with a `+N more` sentinel when truncated) — enough for an informed pick.", + "description": "List skills (worker profiles) available to coordinators. Use to discover skill names for `spawn_workstream`. Filters: `category` (e.g. 'engineering', 'ops'), `tag` (single tag), `risk_level` (`safe`/`low`/`medium`/`high`/`critical`; omit to include unscanned rows). Results are pre-filtered to coordinator-applicable skills (`kind='coordinator'` or `'any'`); interactive-only skills are hidden. Each row returns `name`, `category`, `tags`, `version`, `description`, model preference, `enabled`, `risk_level`, `activation`, and `kind`. `allowed_tools` is included ONLY when the skill declares an auto-approve allowlist — listing the tool names exempt from the operator approval gate (capped at 20 names with a `+N more` sentinel when truncated). The field is OMITTED when empty: a skill without `allowed_tools` still has access to every tool in its session's toolset; absence of the field means no tool is pre-approved for this skill, not that the skill has no tools.", "parameters": { "type": "object", "properties": {