mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
fix(coord): omit empty allowed_tools in list_skills + clarify semantics
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.
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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}
|
||||
|
||||
# ------------------------------------------------------------------
|
||||
|
||||
@@ -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": {
|
||||
|
||||
Reference in New Issue
Block a user