fix(eval): address Copilot review — checkout-agnostic docs + skill validation

- Docstrings/help said the treatment skill 'composes into the system
  message'. This harness runs on both checkouts (system on main, a context
  turn on the placement-refactor branch), so the wording now describes the
  natural set_skill composition path without asserting a placement.
- Validate each skill-bearing case's 'skill' shape up front (driver +
  CLI) so a malformed dataset fails with a clear error, not a mid-run
  KeyError. Pinned by test_rejects_malformed_skill.
This commit is contained in:
Patrick Buckley
2026-07-03 17:29:11 -07:00
parent 845df69031
commit 45010f5890
3 changed files with 56 additions and 7 deletions
+26
View File
@@ -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.
+10 -2
View File
@@ -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(
+20 -5
View File
@@ -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=<case skill>``);
* **treatment** — the skill is applied via the real ``set_skill``
composition path (``skill_mode=True, skill=<case 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