diff --git a/tests/test_skill_adherence.py b/tests/test_skill_adherence.py index 5af53ffb..6b8fadb7 100644 --- a/tests/test_skill_adherence.py +++ b/tests/test_skill_adherence.py @@ -147,6 +147,32 @@ class TestAdherenceLift: assert row["n_runs"] == 3 assert result["mean_lift"] == pytest.approx(2.0 / 3.0) + def test_rejects_malformed_skill(self) -> None: + # A skill missing 'content' (or 'name') fails fast with a clear error, + # not a KeyError mid-run (Copilot review). Validation raises before any + # arm runs, so no _run_iteration stub is needed. + cases = [ + { + "id": "bad-skill", + "skill": {"name": "x"}, # missing 'content' + "user_prompt": "do x", + "expected_actions": [{"tool": "search"}], + } + ] + with pytest.raises(ValueError, match="non-empty 'name' and 'content'"): + run_skill_adherence( + client=None, + base_url="http://localhost:9/v1", + api_key="dummy", + model="test-model", + cases=cases, + n_runs=1, + temperature=0.7, + max_tokens=1024, + reasoning_effort="medium", + context_window=8192, + ) + def test_skipped_when_no_skill(self, monkeypatch: pytest.MonkeyPatch) -> None: # A case with no skill is not measurable — it must be skipped, not # crash, and must not contribute to the mean. diff --git a/turnstone/eval/cli.py b/turnstone/eval/cli.py index 9cf820b1..560cdd71 100644 --- a/turnstone/eval/cli.py +++ b/turnstone/eval/cli.py @@ -48,6 +48,14 @@ def _run_skill_adherence_cli( raise SystemExit(f"Test case {i} missing required 'id' field") if "user_prompt" not in case: raise SystemExit(f"Test case '{case.get('id', i)}' missing 'user_prompt'") + skill = case.get("skill") + if skill is not None and ( + not isinstance(skill, dict) or not skill.get("name") or not skill.get("content") + ): + raise SystemExit( + f"Test case '{case['id']}' has a malformed 'skill' — it must be an " + "object with non-empty 'name' and 'content'" + ) if not any(c.get("skill") for c in cases): raise SystemExit("No cases carry a 'skill' — nothing to measure for adherence") @@ -179,8 +187,8 @@ def main() -> None: action="store_true", help=( "Measure skill adherence: for each case carrying a 'skill', run a " - "treatment arm (skill composed into the system message) vs a control " - "arm (no skill) and report the pass-rate lift" + "treatment arm (skill applied via the real set_skill composition " + "path) vs a control arm (no skill) and report the pass-rate lift" ), ) parser.add_argument( diff --git a/turnstone/eval/core.py b/turnstone/eval/core.py index cb92ebd5..85f194d2 100644 --- a/turnstone/eval/core.py +++ b/turnstone/eval/core.py @@ -452,8 +452,11 @@ def _run_single_test( When ``skill_mode`` is True the session is built WITHOUT a system-prompt override so the model runs under turnstone's natural prompt composition (the base identity under test). If ``skill`` is given it is seeded into - the temp DB and activated via the real ``set_skill`` path, so the skill - body composes into the system message exactly as it would in production; + the temp DB and activated via the real ``set_skill`` path — the skill + flows through turnstone's natural composition exactly as in production, + landing wherever THAT checkout places a named skill (the system message, + or a separate context turn). This harness measures adherence regardless + of placement, which is the whole point of comparing across checkouts. ``skill`` None is the control arm (natural default, no skill). When ``skill_mode`` is False behaviour is unchanged — the system prompt is overridden as before. @@ -519,7 +522,8 @@ def _run_single_test( client=run_client, model=model, # skill_mode uses turnstone's natural composition (no override) - # so the skill can fold into the system message under test. + # so the skill folds in wherever the checkout under test places + # a named skill (system message, or a separate context turn). system_prompt_override=None if skill_mode else system_prompt, instructions=None, temperature=temperature, @@ -1254,8 +1258,9 @@ def run_skill_adherence( For every case that carries a ``skill`` this runs two arms ``n_runs`` times each, scoring both against the case's ``expected_actions``: - * **treatment** — the skill is composed into the system message via the - real ``set_skill`` path (``skill_mode=True, skill=``); + * **treatment** — the skill is applied via the real ``set_skill`` + composition path (``skill_mode=True, skill=``); where its + body lands (system message or a context turn) depends on the checkout; * **control** — the same base identity with no skill (``skill_mode=True, skill=None``). @@ -1269,6 +1274,16 @@ def run_skill_adherence( case_results: list[dict[str, Any]] = [] skill_cases = [c for c in cases if c.get("skill")] + # Validate skill shape up front so a malformed dataset fails with a clear + # message instead of a KeyError mid-run (after arms have already started). + for case in skill_cases: + s = case["skill"] + if not isinstance(s, dict) or not s.get("name") or not s.get("content"): + raise ValueError( + f"case {case.get('id', '?')!r}: 'skill' must be an object with " + "non-empty 'name' and 'content'" + ) + for ci, case in enumerate(skill_cases): skill = case["skill"] # Drop the skill key from the case handed to the runner — it is