mirror of
https://github.com/turnstonelabs/turnstone.git
synced 2026-08-12 23:12:23 -06:00
refactor(skills): unify single-row lookup + allow coord-side load
Two changes that share the same kind-scoping touch point.
Lookup unification (closes the bypass Copilot flagged on _exec_skills_load):
- New ChatSession._lookup_visible_skill(name) — single source of truth for
"find me a skill by name, if it's visible to this session". Combines
storage.get_prompt_template_by_name with the kind filter in one call;
returns None for both the missing-row and out-of-kind cases so callers
don't have to branch on the reason.
- _exec_skills_get refactored from inline two-step to one helper call.
- _exec_skills_load refactored from the unscoped memory.get_skill_by_name
to the new helper — the kind-scoping bypass it had (interactive could
load a kind=coordinator skill by name) goes away by construction
because the unscoped path no longer exists on the model-tool surface.
- memory.get_skill_by_name stays available for admin / sub-agent /
rehydrate paths that need full-catalog visibility — those are
deliberate cross-kind callers, not bypass surfaces. Storage exceptions
now propagate from _lookup_visible_skill by design (distinct from the
legacy swallow-and-return-None) so the operator gets a clear signal on
DB outage rather than a misleading "not found".
Coord-side load support:
- _prepare_skills_load no longer rejects coordinator sessions. Parity
with the admin / HTTP create path that already accepts a `skill` body
field on kind=coordinator workstreams — what the operator can do at
create time, the model can now do on its own session. Visibility is
still kind-scoped via _lookup_visible_skill at exec (a coord can only
load {coordinator, any}-tagged skills; interactive can only load
{interactive, any}), matching what `find` / `get` enforce.
The kind-scoping itself is queued for a separate cleanup PR: the marker
turned out to be a discoverability hint that never gated runtime
capability, and the combinatorial complexity (every new model-tool /
HTTP path needs kind awareness) isn't worth the squeeze at this team
size. Follow-up issue to land.
Test coverage:
- TestLookupVisibleSkill — 5 cases: visible / cross-kind / missing /
storage-unavailable / kind=any-on-both-surfaces.
- TestExecSkillsLoadKindScoping — kind-rejection from both directions
(interactive→coord-only, coord→interactive-only), disabled-skill
caller-side gate, and the two new positive coord-load cases (coord
loads kind=coordinator and kind=any).
- Removed test_load_on_coord_session_errors (the rejection it pinned
is gone).
Plus the /review-suggested doc fixes that came with the unification:
- Comment in _exec_skills_load now correctly attributes the disabled
collapse to the caller's enabled check rather than implying the
helper handles it.
- _lookup_visible_skill docstring documents the deliberate
exception-propagation behavior.
This commit is contained in:
Patrick Buckley
+177
-5
@@ -147,13 +147,19 @@ class TestPrepareSkillsLoad:
|
||||
assert item["needs_approval"] is True
|
||||
assert item["approval_label"] == "skills__load__x"
|
||||
|
||||
def test_load_on_coord_session_errors(self) -> None:
|
||||
def test_load_works_on_coord_session(self) -> None:
|
||||
"""Coord sessions can ``load`` for themselves — parity with the
|
||||
admin / HTTP create path that already accepts ``skill`` in the
|
||||
coord-create body. Visibility is still kind-scoped at exec
|
||||
(a coord can only load ``{coordinator, any}``-tagged skills via
|
||||
``_lookup_visible_skill``); the rejection at prepare-time that
|
||||
used to point at ``spawn_workstream`` is gone."""
|
||||
session = _make_session(kind="coordinator")
|
||||
item = session._prepare_skills("c", {"action": "load", "name": "x"})
|
||||
assert "load: not available on coordinator sessions" in item.get("error", "")
|
||||
# Hint guides the model to the correct delegation pattern.
|
||||
assert "<system-reminder>" in item.get("error", "")
|
||||
assert "spawn_workstream(skill=" in item.get("error", "")
|
||||
# Prepare succeeds — no error item, approval-gated like interactive.
|
||||
assert "error" not in item, item
|
||||
assert item["needs_approval"] is True
|
||||
assert item["approval_label"] == "skills__load__x"
|
||||
|
||||
def test_load_missing_name(self) -> None:
|
||||
session = _make_session()
|
||||
@@ -467,6 +473,172 @@ class TestExecSkillsGet:
|
||||
assert "not visible to this session kind" not in output
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests — _lookup_visible_skill (the unified single-row kind-scoped helper)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
|
||||
class TestLookupVisibleSkill:
|
||||
"""The kind-scoped single-row lookup that both ``_exec_skills_get`` and
|
||||
``_exec_skills_load`` consume. Closes the prior bypass where ``load``
|
||||
used the unscoped ``get_skill_by_name`` while ``find`` / ``get``
|
||||
enforced kind filtering — same shape across all single-row lookups
|
||||
means a future caller can't reintroduce the bypass by picking the
|
||||
wrong helper."""
|
||||
|
||||
def test_returns_row_when_kind_matches(self) -> None:
|
||||
session = _make_session(kind="interactive")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "ic-skill",
|
||||
"kind": "interactive",
|
||||
}
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
row = session._lookup_visible_skill("ic-skill")
|
||||
assert row is not None
|
||||
assert row["name"] == "ic-skill"
|
||||
|
||||
def test_returns_row_for_any_kind_in_both_session_kinds(self) -> None:
|
||||
"""``kind='any'`` rows are visible to both surfaces."""
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "universal",
|
||||
"kind": "any",
|
||||
}
|
||||
for kind in ("interactive", "coordinator"):
|
||||
session = _make_session(kind=kind)
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
row = session._lookup_visible_skill("universal")
|
||||
assert row is not None, f"kind={kind!r} couldn't see kind='any' row"
|
||||
|
||||
def test_returns_none_when_row_missing(self) -> None:
|
||||
session = _make_session()
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = None
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
assert session._lookup_visible_skill("ghost") is None
|
||||
|
||||
def test_returns_none_when_kind_does_not_match(self) -> None:
|
||||
"""Out-of-kind rows get the same ``None`` response as missing rows —
|
||||
collapses the enumeration sidechannel."""
|
||||
session = _make_session(kind="interactive")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "coord-only",
|
||||
"kind": "coordinator",
|
||||
}
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
assert session._lookup_visible_skill("coord-only") is None
|
||||
|
||||
def test_returns_none_when_storage_unavailable(self) -> None:
|
||||
session = _make_session()
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=None):
|
||||
assert session._lookup_visible_skill("anything") is None
|
||||
|
||||
|
||||
class TestExecSkillsLoadKindScoping:
|
||||
"""Regression lock: ``_exec_skills_load`` respects kind scoping via
|
||||
``_lookup_visible_skill`` regardless of session kind. Pre-unification
|
||||
the load path used unscoped ``get_skill_by_name`` and would happily
|
||||
activate any skill by name, bypassing the kind contract that
|
||||
``find`` / ``get`` enforced. Now both interactive AND coordinator
|
||||
sessions can ``load`` for themselves, but each is still bounded to
|
||||
the skills it can see via ``find`` / ``get``."""
|
||||
|
||||
def test_load_rejects_cross_kind_skill_interactive(self) -> None:
|
||||
"""Interactive session trying to load a coord-only skill —
|
||||
same "not found" shape as a true miss. No enumeration signal."""
|
||||
session = _make_session(kind="interactive")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "coord-only",
|
||||
"kind": "coordinator",
|
||||
"enabled": True,
|
||||
"content": "should not be reachable",
|
||||
}
|
||||
item = session._prepare_skills("c", {"action": "load", "name": "coord-only"})
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
_, output = session._exec_skills(item)
|
||||
assert "not found or disabled" in output
|
||||
assert session._skill_name is None # never activated
|
||||
|
||||
def test_load_rejects_cross_kind_skill_coordinator(self) -> None:
|
||||
"""Symmetric case: coord session trying to load an interactive-only
|
||||
skill. Same response shape — the kind filter applies on both sides."""
|
||||
session = _make_session(kind="coordinator")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "ic-only",
|
||||
"kind": "interactive",
|
||||
"enabled": True,
|
||||
"content": "should not be reachable",
|
||||
}
|
||||
item = session._prepare_skills("c", {"action": "load", "name": "ic-only"})
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
_, output = session._exec_skills(item)
|
||||
assert "not found or disabled" in output
|
||||
assert session._skill_name is None
|
||||
|
||||
def test_load_rejects_disabled_skill(self) -> None:
|
||||
"""The caller-side ``enabled`` gate at the top of ``_exec_skills_load``
|
||||
is distinct from the helper's missing/cross-kind branch — both
|
||||
collapse into the same "not found or disabled" hint by design,
|
||||
but the disabled case has its own code path that needs a
|
||||
regression test (caught by /review as a coverage gap)."""
|
||||
session = _make_session(kind="interactive")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "quarantined",
|
||||
"kind": "interactive",
|
||||
"enabled": False, # admin disabled this skill
|
||||
"content": "do not load",
|
||||
}
|
||||
item = session._prepare_skills("c", {"action": "load", "name": "quarantined"})
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
_, output = session._exec_skills(item)
|
||||
assert "not found or disabled" in output
|
||||
assert session._skill_name is None # never activated
|
||||
|
||||
def test_load_works_for_coord_on_visible_skill(self) -> None:
|
||||
"""Coord session loads a coord-visible skill — exec succeeds and
|
||||
``set_skill`` fires. This is the new capability the PR adds:
|
||||
coord-side parity with interactive's load semantics."""
|
||||
session = _make_session(kind="coordinator")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "coord-persona",
|
||||
"kind": "coordinator",
|
||||
"enabled": True,
|
||||
"content": "You are a coordinator.",
|
||||
"description": "Coord orchestrator persona.",
|
||||
"risk_level": "low",
|
||||
}
|
||||
item = session._prepare_skills("c", {"action": "load", "name": "coord-persona"})
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
_, output = session._exec_skills(item)
|
||||
assert "Loaded skill 'coord-persona'" in output
|
||||
assert session._set_skill_called == ["coord-persona"]
|
||||
|
||||
def test_load_works_for_coord_on_any_kind_skill(self) -> None:
|
||||
"""A ``kind=any`` skill is loadable from a coord session too —
|
||||
``any`` is visible on both surfaces by design."""
|
||||
session = _make_session(kind="coordinator")
|
||||
storage = MagicMock()
|
||||
storage.get_prompt_template_by_name.return_value = {
|
||||
"name": "universal",
|
||||
"kind": "any",
|
||||
"enabled": True,
|
||||
"content": "...",
|
||||
"description": "Universal skill.",
|
||||
"risk_level": "low",
|
||||
}
|
||||
item = session._prepare_skills("c", {"action": "load", "name": "universal"})
|
||||
with patch("turnstone.core.storage._registry.get_storage", return_value=storage):
|
||||
_, output = session._exec_skills(item)
|
||||
assert "Loaded skill 'universal'" in output
|
||||
assert session._set_skill_called == ["universal"]
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
# Tests — Exec (write paths)
|
||||
# ---------------------------------------------------------------------------
|
||||
|
||||
+64
-40
@@ -7940,6 +7940,45 @@ class ChatSession:
|
||||
return ["coordinator", "any"]
|
||||
return ["interactive", "any"]
|
||||
|
||||
def _lookup_visible_skill(self, name: str) -> dict[str, Any] | None:
|
||||
"""Single source of truth for "find me a skill by name, if it's
|
||||
visible to this session". Returns the storage row when the named
|
||||
skill exists AND its ``kind`` is in :meth:`_skills_kinds`; returns
|
||||
``None`` for both the missing-row and out-of-kind cases so callers
|
||||
don't have to branch on the reason.
|
||||
|
||||
Collapsing missing-vs-cross-kind into a single ``None`` response
|
||||
is also what closes the enumeration sidechannel — an interactive
|
||||
session can't tell whether a name corresponds to a coord-only
|
||||
skill it can't see, vs a name that doesn't exist at all.
|
||||
|
||||
Every model-tool single-row lookup goes through this helper. The
|
||||
unscoped ``get_skill_by_name`` / ``get_prompt_template_by_name``
|
||||
helpers stay available for admin / sub-agent paths that need
|
||||
full-catalog visibility — those are deliberate cross-kind callers,
|
||||
not bypass surfaces.
|
||||
|
||||
Storage exceptions propagate by design — distinct from the
|
||||
legacy ``memory.get_skill_by_name`` path which swallowed them
|
||||
and returned ``None``. A transient DB hiccup now surfaces as
|
||||
an explicit tool-call error rather than a misleading "skill
|
||||
not found", matching the fail-fast preference for tool-layer
|
||||
errors (model recovery is the same shape either way; the
|
||||
operator gets a clearer signal).
|
||||
"""
|
||||
from turnstone.core.storage._registry import get_storage
|
||||
|
||||
storage = get_storage()
|
||||
if storage is None:
|
||||
return None
|
||||
row = storage.get_prompt_template_by_name(name)
|
||||
if row is None:
|
||||
return None
|
||||
kind = row.get("kind") or "any"
|
||||
if kind not in self._skills_kinds():
|
||||
return None
|
||||
return row
|
||||
|
||||
def _prepare_skills(self, call_id: str, args: dict[str, Any]) -> dict[str, Any]:
|
||||
"""Dispatch on ``action``. Reads auto-approve; writes require both
|
||||
operator approval and the ``model.skills.write`` permission."""
|
||||
@@ -8169,34 +8208,14 @@ class ChatSession:
|
||||
}
|
||||
|
||||
def _exec_skills_get(self, item: dict[str, Any]) -> tuple[str, str]:
|
||||
from turnstone.core.storage._registry import get_storage
|
||||
|
||||
call_id = item["call_id"]
|
||||
name = item["name"]
|
||||
storage = get_storage()
|
||||
if storage is None:
|
||||
msg = "Error: storage unavailable"
|
||||
self._report_tool_result(call_id, "skills", msg, is_error=True)
|
||||
return call_id, msg
|
||||
row = storage.get_prompt_template_by_name(name)
|
||||
row = self._lookup_visible_skill(name)
|
||||
if row is None:
|
||||
msg = self._skill_hint(
|
||||
f"skill '{name}' not found",
|
||||
system_reminder=(
|
||||
"Use skills(action='find', query='...') to discover "
|
||||
"available skill names. Names are exact-match and "
|
||||
"case-sensitive."
|
||||
),
|
||||
)
|
||||
self._report_tool_result(call_id, "skills", msg, is_error=True)
|
||||
return call_id, msg
|
||||
kind = row.get("kind") or "any"
|
||||
if kind not in self._skills_kinds():
|
||||
# Collapse cross-kind access into "not found" so the model
|
||||
# cannot enumerate the other surface's skill names by
|
||||
# name-probing. Audit still distinguishes the case via
|
||||
# the storage-side row presence for forensics, but the
|
||||
# tool response is response-shape-identical to a true miss.
|
||||
# ``_lookup_visible_skill`` returns ``None`` for both the
|
||||
# missing-row and out-of-kind cases — same response shape
|
||||
# for both so the model can't enumerate the other surface's
|
||||
# skill names by name-probing.
|
||||
msg = self._skill_hint(
|
||||
f"skill '{name}' not found",
|
||||
system_reminder=(
|
||||
@@ -8218,20 +8237,18 @@ class ChatSession:
|
||||
# -- load (interactive only) ----------------------------------------------
|
||||
|
||||
def _prepare_skills_load(self, call_id: str, args: dict[str, Any]) -> dict[str, Any]:
|
||||
if self._kind == WorkstreamKind.COORDINATOR:
|
||||
return self._coord_tool_error(
|
||||
call_id,
|
||||
"skills",
|
||||
self._skill_hint(
|
||||
"load: not available on coordinator sessions",
|
||||
system_reminder=(
|
||||
"Coordinators don't activate skills for themselves. "
|
||||
"To assign a skill to a child workstream, use "
|
||||
"spawn_workstream(skill='<name>', ...) - the skill "
|
||||
"is applied when the child session is constructed."
|
||||
),
|
||||
),
|
||||
)
|
||||
# Both kinds can ``load`` — interactive sessions replace their own
|
||||
# persona, coordinator sessions do the same for their orchestrator.
|
||||
# Parity with the admin / HTTP create path that already accepts a
|
||||
# ``skill`` body field on coord workstreams: anything an operator
|
||||
# can do at create time, the model can do at runtime through this
|
||||
# same action. Visibility is still kind-scoped via
|
||||
# ``_lookup_visible_skill`` at exec — a coord can only load
|
||||
# ``{coordinator, any}``-tagged skills, interactive can only load
|
||||
# ``{interactive, any}``, matching the discoverability filter that
|
||||
# ``find`` / ``get`` enforce. Use ``spawn_workstream(skill=...)``
|
||||
# for assigning to children; ``load`` activates on the *current*
|
||||
# session regardless of kind.
|
||||
name = self._coord_str_arg(args, "name").strip()
|
||||
if not name:
|
||||
return self._coord_tool_error(call_id, "skills", "load: 'name' is required")
|
||||
@@ -8250,7 +8267,14 @@ class ChatSession:
|
||||
def _exec_skills_load(self, item: dict[str, Any]) -> tuple[str, str]:
|
||||
call_id = item["call_id"]
|
||||
name = item["name"]
|
||||
skill_data = get_skill_by_name(name)
|
||||
# Single source of truth for "find me a visible skill" — closes
|
||||
# the prior kind-scoping bypass that let ``_exec_skills_load`` use
|
||||
# an unscoped ``get_skill_by_name`` while ``find`` / ``get``
|
||||
# enforced kind filtering. The helper returns ``None`` for
|
||||
# missing-row AND cross-kind cases; the ``enabled`` check below
|
||||
# collapses the disabled case into the same hint so the model
|
||||
# gets one consistent recovery path.
|
||||
skill_data = self._lookup_visible_skill(name)
|
||||
if not skill_data or not skill_data.get("enabled", True):
|
||||
msg = self._skill_hint(
|
||||
f"skill '{name}' not found or disabled",
|
||||
|
||||
@@ -1,6 +1,6 @@
|
||||
{
|
||||
"name": "skills",
|
||||
"description": "Manage the skill catalog and (on interactive sessions) load skills into the current session. Replaces the legacy `skill` + `list_skills` tools. Each action has its own approval policy: read actions (`find`, `get`) auto-approve; write actions (`create`, `update`, `enable`, `disable`) require approval AND the `model.skills.write` permission on the session user — default-ungranted, operators opt themselves in via the Roles tab. Session-mutating `load` is interactive-only; coordinator sessions get an explicit error (they delegate skill assignment via `spawn_workstream(skill=...)`).\n\nActions:\n- `find`: list skills filtered by `category`, `tag`, `risk_level`, `enabled_only`, `limit`, with optional `query` for BM25 ranking over the filtered set. Coord sessions see `kind IN ('coordinator', 'any')`; interactive sessions see `kind IN ('interactive', 'any')`. Returns a hint when filters yield 0 rows showing what an unfiltered query would have matched.\n- `get`: fetch a single skill by `name`, returning the full row including `content`. Use to inspect before `update`.\n- `load` (interactive only, requires approval): activate a skill by `name` in the current session (replaces current skill, applies its session config).\n- `create` (requires approval + `model.skills.write`): create a new skill with `name`, `content`, `description`, plus optional `category`, `tags`, `kind`, and session-config fields. Storage authoritatively re-computes `risk_level` from the scanner — caller cannot lie.\n- `update` (requires approval + `model.skills.write`): patch an existing skill identified by `name`. Any subset of writeable fields. Approval card shows the projected risk-tier shift if `content` or `allowed_tools` changes.\n- `enable` / `disable` (requires approval + `model.skills.write`): flip the `enabled` flag on a skill identified by `name`. Disabled skills stay in storage but are hidden from `find` (unless `enabled_only=false`) and rejected by `load` / `spawn_workstream(skill=...)`.\n\nSoft-delete only — there is no `delete` action in this tool. Hard-delete remains admin-UI exclusive to avoid model-proposed mistakes against in-use skills.",
|
||||
"description": "Manage the skill catalog and load skills into the current session. Replaces the legacy `skill` + `list_skills` tools. Each action has its own approval policy: read actions (`find`, `get`) auto-approve; write actions (`create`, `update`, `enable`, `disable`) require approval AND the `model.skills.write` permission on the session user — default-ungranted, operators opt themselves in via the Roles tab.\n\nActions:\n- `find`: list skills filtered by `category`, `tag`, `risk_level`, `enabled_only`, `limit`, with optional `query` for BM25 ranking over the filtered set. Coord sessions see `kind IN ('coordinator', 'any')`; interactive sessions see `kind IN ('interactive', 'any')`. Returns a hint when filters yield 0 rows showing what an unfiltered query would have matched.\n- `get`: fetch a single skill by `name`, returning the full row including `content`. Use to inspect before `update`.\n- `load` (requires approval): activate a skill by `name` on the current session — replaces the current skill, applies its session config. Works on both interactive and coordinator sessions; visibility is kind-scoped the same way as `find` / `get` (a session can only load what it can see). For assigning a skill to a *child* workstream instead of the current session, use `spawn_workstream(skill=...)`.\n- `create` (requires approval + `model.skills.write`): create a new skill with `name`, `content`, `description`, plus optional `category`, `tags`, `kind`, and session-config fields. Storage authoritatively re-computes `risk_level` from the scanner — caller cannot lie.\n- `update` (requires approval + `model.skills.write`): patch an existing skill identified by `name`. Any subset of writeable fields. Approval card shows the projected risk-tier shift if `content` or `allowed_tools` changes.\n- `enable` / `disable` (requires approval + `model.skills.write`): flip the `enabled` flag on a skill identified by `name`. Disabled skills stay in storage but are hidden from `find` (unless `enabled_only=false`) and rejected by `load` / `spawn_workstream(skill=...)`.\n\nSoft-delete only — there is no `delete` action in this tool. Hard-delete remains admin-UI exclusive to avoid model-proposed mistakes against in-use skills.",
|
||||
"parameters": {
|
||||
"type": "object",
|
||||
"properties": {
|
||||
|
||||
Reference in New Issue
Block a user