From 7085c2453095cd015ee5d416651c65dabc6ebec2 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Fri, 22 May 2026 16:16:59 -0700 Subject: [PATCH] refactor(skills): unify single-row lookup + allow coord-side load MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- tests/test_skills_tool.py | 182 +++++++++++++++++++++++++++++++++++- turnstone/core/session.py | 104 +++++++++++++-------- turnstone/tools/skills.json | 2 +- 3 files changed, 242 insertions(+), 46 deletions(-) diff --git a/tests/test_skills_tool.py b/tests/test_skills_tool.py index 4aa0732f..3d61a6f9 100644 --- a/tests/test_skills_tool.py +++ b/tests/test_skills_tool.py @@ -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 "" 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) # --------------------------------------------------------------------------- diff --git a/turnstone/core/session.py b/turnstone/core/session.py index 4f1d1805..c71887fc 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -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='', ...) - 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", diff --git a/turnstone/tools/skills.json b/turnstone/tools/skills.json index c12f5c13..b64e0f26 100644 --- a/turnstone/tools/skills.json +++ b/turnstone/tools/skills.json @@ -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": {