From 7d3bc9e0e7084af6783699dff124a0a0444d0fa2 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Tue, 27 Oct 2026 12:46:50 +0100 Subject: [PATCH 01/15] Ask whether a skill's cases still need the skill A behavioral run says the cases pass. It cannot say they pass because of the skill, and an expectation any competent agent meets is a measurement of the model with the skill's name on it. `skillscope staleness` runs the cases twice, once with the skill installed and once with nothing installed, and reports which ones moved. `install_skill` already described the second situation as a failure mode -- answering from the prompt and scoring like it, which looks like a bad skill rather than a missing one. Asked for deliberately and labelled, it is the measurement. Both engines take the switch, both halves go through one dispatch, and the report never gates: the agent is nondeterministic on both sides, so a verdict here is somewhere to look rather than a build failure. Signed-off-by: John Lybeck --- README.md | 1 + docs/usage.md | 34 ++++++ skillscope/cli.py | 151 +++++++++++++++++++---- skillscope/engine/behavioral.py | 27 +++- skillscope/engine/no_sandbox.py | 10 +- skillscope/engine/verify.py | 30 +++-- skillscope/staleness.py | 210 ++++++++++++++++++++++++++++++++ tests/test_skillscope.py | 116 +++++++++++++++++- 8 files changed, 541 insertions(+), 38 deletions(-) create mode 100644 skillscope/staleness.py diff --git a/README.md b/README.md index f3600f0..503056b 100644 --- a/README.md +++ b/README.md @@ -72,6 +72,7 @@ You may also choose to customize or even disable some tests if you prefer it. Se | `skillscope structural` | Skill folders, datasets, and markdown references. | | `skillscope routing` | Which skill fires, with several installed together. | | `skillscope behavioral` | What a skill does once it has fired. | +| `skillscope staleness` | Whether those cases still need the skill. | | `skillscope select` | The CI plan for a change, as JSON. | | `skillscope list-skills` | The skills that have a dataset, as JSON. | | `skillscope template` | The dataset template a new skill starts from. | diff --git a/docs/usage.md b/docs/usage.md index 442d1b1..847b0dd 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -247,6 +247,40 @@ Legs with a scoped environment run as a separate job, because a job's credentials are fixed before its matrix expands. A repo that declares no scoped environment gets one matrix, labels and all. +## Is the skill still earning its place? + +A behavioral run says a skill's cases pass. It cannot say they pass *because +of* the skill, and those are different claims: an expectation any competent +agent meets is a measurement of the model with the skill's name on it. + +`skillscope staleness` runs the same cases twice, once with the skill +installed and once with nothing installed, and reports which ones moved. + +```bash +skillscope staleness --skill my-skill +``` + +| Verdict | Means | +| --- | --- | +| `earns_it` | passed with the skill, failed without it — the case discriminates | +| `harmful` | failed with the skill, passed without it — the skill steered the agent wrong | +| `no_signal` | passed either way — says nothing about the skill | +| `fails_either_way` | failed both times — a broken case, or a hard one | +| `ungraded` | a run errored on one side or both | + +`no_signal` is the verdict worth acting on, and it is two findings wearing one +name: either the skill has stopped mattering, or the case was never evidence +about it in the first place. The second is the more common, which makes this a +check on the dataset as much as on the skill. + +It reports and never gates, for the reason the `claude-code` leg does not gate +either: the agent is nondeterministic on both sides, so a single pair is a +place to look rather than a verdict. [`tools/benchmark_engines.py`](../tools/benchmark_engines.py) +is the shape of the honest version of this comparison — it measures agreement +against how much one engine already disagrees with itself — and a staleness +number deserves the same scepticism. It also costs twice what `behavioral` +costs, which is why no workflow runs it for you. + ## Which engine grades a run `--engine` chooses what actually runs the cases. The dataset, the CLI and the diff --git a/skillscope/cli.py b/skillscope/cli.py index 0b9da79..0d92aaf 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -79,6 +79,7 @@ engine, references, routing, + staleness, structure, usage, ) @@ -672,60 +673,92 @@ def cmd_routing(args: argparse.Namespace) -> int: ) -def cmd_behavioral(args: argparse.Namespace) -> int: - skills = _prepare_graded_run(args) - started = time.time() +def _gradable_cases(args: argparse.Namespace, skills: list[str]) -> list: + """The cases a behavioral-style run grades: selected, filtered, assertive. + Shared so `behavioral` and `staleness` cannot drift apart about what + `--only` or `--extended` select. A staleness run comparing a different set + of cases than the behavioral run it is read beside would be the one bug + neither report could show you. + """ cases = [] for skill in skills: cases.extend(datasets.load_dataset(skill, extended=args.extended)) if args.only: cases = datasets.filter_cases(cases, args.only) - gradable = [c for c in cases if c.has_behavior] + return [c for c in cases if c.has_behavior] - if not gradable: - print( - "[behavioral] no evaluation in the selected skill(s) asserts " - "anything beyond routing, so there is nothing to grade. Add " - "`expected_behavior` / `unexpected_behavior` / `logs_contain` / " - "`files_exist` to a triggering evaluation." - ) - return 0 +def _run_behavioral( + args: argparse.Namespace, + skills: list[str], + gradable: list, + *, + install: bool = True, +) -> list: + """Run the behavioral cases on the selected engine. + + `install` False runs them with no skill staged, which is the baseline half + of `cmd_staleness`. Both halves go through here rather than through two + similar call sites, so the only difference between the run being measured + and the run it is measured against is the flag. + """ from .engine import behavioral as inspect_behavioral from .engine import models as engine_models if args.engine == "claude-code": from .engine import verify as runner - outcomes = runner.run( - skills, gradable, engine_models.resolve(args.model), args.effort + return runner.run( + skills, + gradable, + engine_models.resolve(args.model), + args.effort, + install=install, ) - elif args.engine == "claude-code-no-sandbox": + if args.engine == "claude-code-no-sandbox": # The real CLI, driven on the host, inside inspect's framework. # `--model` stays the CLI's own alias here: this one does not go # through an inspect model provider. from .engine import no_sandbox no_sandbox.require_local() - outcomes = inspect_behavioral.run( + return inspect_behavioral.run( skills, gradable, engine_models.resolve(args.model), args.effort, solver_factory=lambda d: no_sandbox.claude_code_no_sandbox( - args.model, args.effort, d + args.model, args.effort, d, install=install ), + baseline=not install, ) - else: - # Not a fallthrough. An engine in ENGINES with no branch here is a - # bug this dispatch has had once already, and a silent default is - # what let it survive: runs asked for one agent, got another, and - # the report named the one they had asked for. - raise SystemExit( - f"error: --engine {args.engine} has no behavioral leg. " - f"This is a bug in skillscope, not in how it was called." + # Not a fallthrough. An engine in ENGINES with no branch here is a + # bug this dispatch has had once already, and a silent default is + # what let it survive: runs asked for one agent, got another, and + # the report named the one they had asked for. + raise SystemExit( + f"error: --engine {args.engine} has no behavioral leg. " + f"This is a bug in skillscope, not in how it was called." + ) + + +def cmd_behavioral(args: argparse.Namespace) -> int: + skills = _prepare_graded_run(args) + started = time.time() + + gradable = _gradable_cases(args, skills) + + if not gradable: + print( + "[behavioral] no evaluation in the selected skill(s) asserts " + "anything beyond routing, so there is nothing to grade. Add " + "`expected_behavior` / `unexpected_behavior` / `logs_contain` / " + "`files_exist` to a triggering evaluation." ) + return 0 + + outcomes = _run_behavioral(args, skills, gradable) summary = behavior.summarize( outcomes, @@ -750,6 +783,59 @@ def cmd_behavioral(args: argparse.Namespace) -> int: return 0 +def cmd_staleness(args: argparse.Namespace) -> int: + """Run every behavioral case twice, with the skill and without it. + + Both passes run under the one `--timeout`, so a deadline that expires + partway leaves the rest of the second pass unmeasured -- those cases are + reported `ungraded` rather than as a skill that made no difference. Give + this command roughly twice the bound a `behavioral` run of the same cases + would want. + + Always exits 0 on a completed run. The comparison is nondeterministic on + both sides, so a verdict here is somewhere to look rather than a build + failure -- see `skillscope/staleness.py`. A deadline that expired still + fails, because that is a statement about the run rather than about a skill. + """ + skills = _prepare_graded_run(args) + started = time.time() + + gradable = _gradable_cases(args, skills) + + if not gradable: + print( + "[staleness] no evaluation in the selected skill(s) asserts " + "anything beyond routing, so there is nothing to compare. A " + "staleness run needs cases that say what the skill should do." + ) + return 0 + + print(f"[staleness] {len(gradable)} case(s), run twice", flush=True) + with_skill = _run_behavioral(args, skills, gradable) + without = _run_behavioral(args, skills, gradable, install=False) + + comparisons = staleness.pair(with_skill, without) + summary = staleness.summarize( + comparisons, + { + "model": args.model, + "engine": args.engine, + "effort": args.effort, + "skills": skills, + "extended": args.extended, + "wall_time_s": round(time.time() - started, 1), + "timeout": args.timeout, + "github_run_id": os.environ.get("GITHUB_RUN_ID"), + **usage.snapshot().as_meta(), + **_sandbox_meta(args), + }, + ) + _write_report(summary, staleness.render_markdown(summary), args, "staleness") + if (code := _fail_if_expired()) is not None: + return code + return 0 + + # -------------------------------------------------------------------------- # Argument parsing # -------------------------------------------------------------------------- @@ -1062,6 +1148,21 @@ def build_parser() -> argparse.ArgumentParser: _add_graded_arguments(behavioral_parser) behavioral_parser.set_defaults(handler=cmd_behavioral) + staleness_parser = commands.add_parser( + "staleness", + help="Whether a skill's cases still need the skill.", + description=( + "Run every behavioral case twice -- once with the skill installed, " + "once with nothing installed -- and report which cases moved. A " + "case that passes either way is not evidence about the skill. " + "Reports only: it never fails a run. It costs twice what " + "`behavioral` costs and wants roughly twice the --timeout, " + "since both passes run under the one bound." + ), + ) + _add_graded_arguments(staleness_parser) + staleness_parser.set_defaults(handler=cmd_staleness) + select_parser = commands.add_parser( "select", help="Emit the CI plan for a change, as JSON.", diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index 06ca60f..f3a3c0e 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -85,6 +85,7 @@ def build_task( ctx: dict | None = None, *, solver_factory, + baseline: bool = False, ): """One inspect `Task` per skill: its cases, its skill installed, its scorer. @@ -96,6 +97,11 @@ def build_task( Required, and keyword-only. It was optional while a built-in react agent was the default, and a caller that forgot it silently graded a different agent than it asked for. + + `baseline` only names the task. Whether the skill is actually staged is + the solver's business, so the flag and the factory have to agree -- the + name is what makes a disagreement visible in the log directory rather + than invisible in the numbers. """ from inspect_ai import Task @@ -110,7 +116,7 @@ def build_task( bound = deadline.active() return Task( - name=f"behavioral-{skill}", + name=f"behavioral-{skill}" + ("-baseline" if baseline else ""), dataset=samples, setup=hooks.setup_solver(hook, skill, cases), solver=solver_factory(skill_dir), @@ -292,8 +298,14 @@ def run( *, solver_factory, log_dir: str | None = None, + baseline: bool = False, ) -> list[BehaviorOutcome]: - """Run every behavioral case, grouped by skill. Mirrors `behavior.run`.""" + """Run every behavioral case, grouped by skill. Mirrors `behavior.run`. + + `baseline` is passed through to the task and used in the progress lines. + It does not itself remove the skill: the caller's `solver_factory` does + that, and this says so in the output. + """ from inspect_ai import eval as inspect_eval sandbox_spec.require_provider() @@ -311,11 +323,18 @@ def run( outcomes.extend(_failed(skill, skill_cases, deadline.active().message())) continue - print(f"[behavioral] {skill}: {len(skill_cases)} case(s)", flush=True) + label = "baseline" if baseline else "behavioral" + print(f"[{label}] {skill}: {len(skill_cases)} case(s)", flush=True) try: logs = run_eval( inspect_eval, - build_task(skill, skill_cases, model, solver_factory=solver_factory), + build_task( + skill, + skill_cases, + model, + solver_factory=solver_factory, + baseline=baseline, + ), model=model, model_args=models.model_args(model), log_dir=log_dir or str(Path(".skillscope") / "logs"), diff --git a/skillscope/engine/no_sandbox.py b/skillscope/engine/no_sandbox.py index 8d31bae..c8a2b58 100644 --- a/skillscope/engine/no_sandbox.py +++ b/skillscope/engine/no_sandbox.py @@ -404,6 +404,7 @@ def claude_code_no_sandbox( extra_flags: list[str] | None = None, stop_when_factory: Callable[[], Callable[[dict], str | None]] | None = None, room_names: list[str] | None = None, + install: bool = True, ): """Solver: install the skill, run the real CLI once, record what it did. @@ -433,6 +434,12 @@ def claude_code_no_sandbox( which skill fired, and everything after that is paid for and unread. Left `None`, this runs through `inspect_ai.util.subprocess` exactly as before. + `install` is what separates a graded run from its baseline. False stages + nothing, so the agent meets the prompt with no skill at all -- the failure + `install_skill` exists to prevent, asked for on purpose and labelled as + such by the caller. `skillscope staleness` runs both and compares them; + nothing else should pass False. + A factory rather than the rule itself because the solver is built once and run for every sample, while a budget counts *per case*. Handed a single rule, its tally would carry from one case into the next and the room would @@ -449,7 +456,8 @@ async def solve(state, generate): from inspect_ai.util import subprocess as sandbox_subprocess workspace = await _workspace() - install_skill(skill_dir, workspace) + if install: + install_skill(skill_dir, workspace) prompt = state.input_text cmd = [ diff --git a/skillscope/engine/verify.py b/skillscope/engine/verify.py index d06cde2..04694c8 100644 --- a/skillscope/engine/verify.py +++ b/skillscope/engine/verify.py @@ -218,8 +218,16 @@ def build_task( model: str, ctx: dict | None = None, effort: str | None = None, + install: bool = True, ): - """One task per skill, solved by real Claude Code rather than our agent.""" + """One task per skill, solved by real Claude Code rather than our agent. + + `install` False stages no skill, which is the baseline half of + `skillscope staleness`: the same cases, graded the same way, against an + agent that was never given the thing under test. The restore solver and + the agent's own skills argument have to agree about it, or the agent finds + a skill the report says it did not have. + """ from inspect_ai import Task from inspect_ai.solver import chain from inspect_swe import claude_code @@ -240,7 +248,7 @@ def build_task( bound = deadline.active() return Task( - name=f"claude-code-{skill}", + name=f"claude-code-{skill}" + ("" if install else "-baseline"), setup=hooks.setup_solver(hook, skill, cases), cleanup=hooks.cleanup_fn(hook, skill, cases), dataset=samples, @@ -256,9 +264,11 @@ def build_task( # skill failed to do. solver=chain( _ensure_workdir(), - complete_skills_solver({skill_dir.name: skill_dir}), + complete_skills_solver({skill_dir.name: skill_dir} if install else {}), claude_code( - skills=[skill_dir], cwd=tools.workdir_path(), effort=effort or None + skills=[skill_dir] if install else [], + cwd=tools.workdir_path(), + effort=effort or None, ), ), scorer=scorers.expectations(), @@ -269,7 +279,12 @@ def build_task( def run( - skills: list[str], cases: list[Case], model: str, effort: str + skills: list[str], + cases: list[Case], + model: str, + effort: str, + *, + install: bool = True, ) -> list[BehaviorOutcome]: """Mirrors `behavior.run`, so the CLI and the benchmark treat it the same.""" from inspect_ai import eval as inspect_eval @@ -291,10 +306,11 @@ def run( ) continue - print(f"[claude-code] {skill}: {len(skill_cases)} case(s)", flush=True) + label = "claude-code" if install else "claude-code/baseline" + print(f"[{label}] {skill}: {len(skill_cases)} case(s)", flush=True) logs = behavioral.run_eval( inspect_eval, - build_task(skill, skill_cases, model, effort=effort), + build_task(skill, skill_cases, model, effort=effort, install=install), model=model, model_args=models.model_args(model), log_dir=str(Path(".skillscope") / "logs"), diff --git a/skillscope/staleness.py b/skillscope/staleness.py new file mode 100644 index 0000000..5ad16a3 --- /dev/null +++ b/skillscope/staleness.py @@ -0,0 +1,210 @@ +# Copyright Advanced Micro Devices, Inc. +# +# SPDX-License-Identifier: MIT + +"""Is the skill earning its place, or would the agent have done as well alone? + +A behavioral run says whether a skill's cases pass. It cannot say whether they +pass *because of the skill*, and those are different questions: a case can be +written so that any competent agent satisfies it, at which point a green report +is measuring the model rather than the thing under test. + +So the same cases are run twice -- once with the skill installed and once with +nothing installed at all -- and the two are placed side by side. `install_skill` +calls the second situation out as a failure mode, "answering from the prompt and +scoring like it, which looks like a bad skill rather than a missing one". Asked +for deliberately and labelled, that failure is the measurement. + +Four things a case can turn out to be: + +``earns_it`` + Passes with the skill, fails without it. The case discriminates, which is + the only outcome that tells you the skill did something. +``no_signal`` + Passes either way. Says nothing about the skill, and usually says something + about the case: an expectation any agent meets is not evidence. +``harmful`` + Fails with the skill, passes without it. Rare, and worth stopping for: the + skill is actively steering the agent wrong. +``fails_either_way`` + Fails both times. A broken case or a genuinely hard one; either way the + comparison has nothing to report until it passes somewhere. + +This never gates. Two runs of a nondeterministic agent disagree with themselves +often enough that a single pair is evidence rather than proof -- the same reason +`tools/benchmark_engines.py` measures engine agreement against how much one +engine already disagrees with itself, and the same reason the `claude-code` leg +is documented as reporting rather than gating. A number here is a question to +go and look into, not a build failure. +""" + +from __future__ import annotations + +from dataclasses import asdict, dataclass + +from .behavior import BehaviorOutcome + +# (key, column heading, what it means) -- one row per verdict, in the order a +# reader should care about them. +VERDICTS: tuple[tuple[str, str], ...] = ( + ("earns_it", "passed with the skill, failed without it"), + ("harmful", "failed with the skill, passed without it"), + ("no_signal", "passed either way"), + ("fails_either_way", "failed either way"), + ("ungraded", "one side or both could not be graded"), +) + + +@dataclass +class Comparison: + """One case, run both ways.""" + + id: str + skill: str + verdict: str + passed_with: bool + passed_without: bool + error: str | None = None + + +def classify(with_skill: BehaviorOutcome, without: BehaviorOutcome) -> str: + """Which of `VERDICTS` this pair is. + + An errored run on either side is `ungraded` rather than a failure: a + provider that dropped the connection has measured nothing, and counting it + as "the skill made no difference" would be the most flattering possible + reading of an error. + """ + if with_skill.error or without.error: + return "ungraded" + if with_skill.passed and not without.passed: + return "earns_it" + if without.passed and not with_skill.passed: + return "harmful" + if with_skill.passed and without.passed: + return "no_signal" + return "fails_either_way" + + +def pair( + with_skill: list[BehaviorOutcome], without: list[BehaviorOutcome] +) -> list[Comparison]: + """Match the two runs up by case id. + + A case missing from either side is reported as `ungraded` rather than + dropped. The two runs are separate evals and either can lose a case, so + quietly comparing the intersection would shrink the denominator and make + every remaining number look more confident than it is. + """ + without_by_id = {o.id: o for o in without} + with_by_id = {o.id: o for o in with_skill} + comparisons: list[Comparison] = [] + + for case_id in sorted(set(with_by_id) | set(without_by_id)): + this = with_by_id.get(case_id) + that = without_by_id.get(case_id) + if this is None or that is None: + missing = "the baseline run" if that is None else "the graded run" + known = this or that + comparisons.append( + Comparison( + id=case_id, + skill=known.skill, + verdict="ungraded", + passed_with=bool(this and this.passed), + passed_without=bool(that and that.passed), + error=f"no result from {missing}", + ) + ) + continue + comparisons.append( + Comparison( + id=case_id, + skill=this.skill, + verdict=classify(this, that), + passed_with=this.passed, + passed_without=that.passed, + error=this.error or that.error, + ) + ) + return comparisons + + +def summarize(comparisons: list[Comparison], meta: dict) -> dict: + counts = {key: 0 for key, _ in VERDICTS} + for comparison in comparisons: + counts[comparison.verdict] += 1 + + per_skill: dict[str, dict] = {} + for skill in sorted({c.skill for c in comparisons}): + subset = [c for c in comparisons if c.skill == skill] + per_skill[skill] = { + "cases": len(subset), + **{key: sum(1 for c in subset if c.verdict == key) for key, _ in VERDICTS}, + } + + graded = [c for c in comparisons if c.verdict != "ungraded"] + return { + "meta": meta, + "totals": { + "cases": len(comparisons), + "graded": len(graded), + **counts, + # Of the cases that were graded both ways, the share that moved + # when the skill was taken away. The number the run exists for. + "discriminating": ( + round(sum(1 for c in graded if c.verdict == "earns_it") / len(graded), 3) + if graded + else None + ), + }, + "per_skill": per_skill, + "cases": [asdict(c) for c in comparisons], + } + + +def render_markdown(summary: dict) -> str: + totals = summary["totals"] + meta = summary["meta"] + share = totals["discriminating"] + headline = ( + f"**{totals['earns_it']}/{totals['graded']} graded cases needed the skill**" + if totals["graded"] + else "**Nothing was graded both ways.**" + ) + + lines = [ + "## Skill staleness", + "", + f"{headline} on `{meta['model']}` (effort `{meta['effort']}`). Every case " + "ran twice: once with the skill installed, once with nothing installed.", + "", + "Reporting only. Agents disagree with themselves between runs, so treat " + "a surprising verdict as somewhere to look rather than as a result.", + "", + "| Verdict | Cases | Means |", + "| --- | --- | --- |", + ] + for key, meaning in VERDICTS: + lines.append(f"| `{key}` | {totals[key]} | {meaning} |") + + if share is not None: + lines += ["", f"Discriminating: {share:.0%} of the cases graded both ways."] + + notable = [c for c in summary["cases"] if c["verdict"] in ("no_signal", "harmful")] + lines += ["", "### Cases worth a look", ""] + if not notable: + lines.append( + "None. Every graded case either needed the skill or failed both ways." + ) + else: + lines += ["| Case | Verdict | What to do |", "| --- | --- | --- |"] + for case in notable: + advice = ( + "the skill steered the agent wrong here" + if case["verdict"] == "harmful" + else "the agent already did this unaided; the case is not " + "evidence about the skill" + ) + lines.append(f"| `{case['id']}` | {case['verdict']} | {advice} |") + return "\n".join(lines) + "\n" diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 5d4cf7b..409ed41 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -50,6 +50,7 @@ engine as engine_module, references, routing, + staleness, structure, ) from skillscope import selection as select_module @@ -2377,7 +2378,12 @@ def test_the_dispatch_reads_that_set_rather_than_a_literal(self) -> None: # The bug was a branch list that fell behind the choices list, so # the source is what to assert: every engine named, and an `else` # that raises rather than quietly picking one. - source = inspect.getsource(cli.cmd_behavioral) + # + # Read from `_run_behavioral` rather than `cmd_behavioral`: the + # dispatch moved there when `staleness` began running the same cases + # with the skill withheld, so both commands reach an engine by one + # route. The property being guarded is unchanged. + source = inspect.getsource(cli._run_behavioral) for engine in cli.ENGINES: with self.subTest(engine=engine): self.assertIn(f'args.engine == "{engine}"', source) @@ -5966,5 +5972,113 @@ def test_a_good_table_still_binds(self) -> None: self.assertTrue(engine_models.cost_limit_binds("mockllm/model")) +def _outcome(case_id: str, passed: bool, *, error: str | None = None, skill: str = "s"): + """One behavioral outcome, as either half of a staleness pair would report it.""" + return behavior.BehaviorOutcome( + id=case_id, skill=skill, prompt="p", passed=passed, elapsed_s=0.0, error=error + ) + + +class TestStalenessSeparatesTheSkillFromTheModel(unittest.TestCase): + """The four verdicts, and why each is not the others.""" + + def test_a_case_that_needs_the_skill_is_the_one_that_moved(self) -> None: + self.assertEqual( + staleness.classify(_outcome("a", True), _outcome("a", False)), "earns_it" + ) + + def test_a_case_that_passes_unaided_is_not_evidence(self) -> None: + # The finding the command exists for: green with the skill, green + # without it, so the report was measuring the model. + self.assertEqual( + staleness.classify(_outcome("a", True), _outcome("a", True)), "no_signal" + ) + + def test_a_skill_that_makes_things_worse_is_called_that(self) -> None: + self.assertEqual( + staleness.classify(_outcome("a", False), _outcome("a", True)), "harmful" + ) + + def test_a_case_failing_both_ways_says_nothing_either(self) -> None: + self.assertEqual( + staleness.classify(_outcome("a", False), _outcome("a", False)), + "fails_either_way", + ) + + def test_an_errored_side_is_ungraded_rather_than_no_difference(self) -> None: + # A dropped connection measured nothing. Reading it as "the skill made + # no difference" is the most flattering possible reading of an error. + self.assertEqual( + staleness.classify( + _outcome("a", False, error="API Error: 504"), _outcome("a", False) + ), + "ungraded", + ) + + +class TestStalenessPairsEveryCaseItWasAskedFor(unittest.TestCase): + def test_a_case_missing_from_one_side_is_kept_as_ungraded(self) -> None: + # Comparing the intersection would shrink the denominator and make + # every surviving number look more confident than it is. + comparisons = staleness.pair([_outcome("a", True), _outcome("b", True)], [_outcome("a", False)]) + verdicts = {c.id: c.verdict for c in comparisons} + self.assertEqual(verdicts, {"a": "earns_it", "b": "ungraded"}) + missing = next(c for c in comparisons if c.id == "b") + self.assertIn("baseline", missing.error) + + def test_the_share_is_over_what_was_graded_both_ways(self) -> None: + comparisons = staleness.pair( + [_outcome("a", True), _outcome("b", True), _outcome("c", True)], + [_outcome("a", False), _outcome("b", True)], + ) + totals = staleness.summarize(comparisons, {"model": "m", "effort": "e"})["totals"] + self.assertEqual(totals["cases"], 3) + self.assertEqual(totals["graded"], 2) + self.assertEqual(totals["discriminating"], 0.5) + + def test_nothing_graded_leaves_the_share_unset_rather_than_zero(self) -> None: + # Zero would read as "the skill never mattered", which is a result. + # None is the absence of one. + comparisons = staleness.pair([_outcome("a", True)], []) + totals = staleness.summarize(comparisons, {"model": "m", "effort": "e"})["totals"] + self.assertIsNone(totals["discriminating"]) + + +class TestTheStalenessReportLeadsWithTheCaveat(unittest.TestCase): + def test_the_reader_is_told_it_does_not_gate_before_the_numbers(self) -> None: + comparisons = staleness.pair([_outcome("a", True)], [_outcome("a", True)]) + summary = staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + report = staleness.render_markdown(summary) + self.assertIn("Reporting only", report) + self.assertLess(report.index("Reporting only"), report.index("| Verdict |")) + + def test_a_case_that_needed_nothing_is_named_with_what_to_do(self) -> None: + comparisons = staleness.pair([_outcome("a", True)], [_outcome("a", True)]) + report = staleness.render_markdown( + staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + ) + self.assertIn("`a`", report) + self.assertIn("unaided", report) + + +class TestTheBaselineRunIsTheSameRunWithoutTheSkill(unittest.TestCase): + """The flag has to reach the driver, or the two halves are not comparable.""" + + def test_the_host_driver_stages_nothing_when_asked_not_to(self) -> None: + import inspect as _inspect + + signature = _inspect.signature(engine_no_sandbox.claude_code_no_sandbox) + self.assertIn("install", signature.parameters) + self.assertIs(signature.parameters["install"].default, True) + + def test_the_task_name_says_which_half_it_is(self) -> None: + # Both halves write into one log directory; a name that did not say + # would leave the two indistinguishable on disk. + import inspect as _inspect + + source = _inspect.getsource(engine_behavioral.build_task) + self.assertIn("-baseline", source) + + if __name__ == "__main__": unittest.main(verbosity=2) From 0aaade34dba5a9c9f42b94b6b8dc7c9941a5b198 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Tue, 27 Oct 2026 13:29:34 +0100 Subject: [PATCH 02/15] Report why each side failed, not just that it did The first real run of this command failed both ways and said nothing more. The two sides had failed differently -- with the skill the agent tried the task and was stopped by the machine, without it the agent never attempted it, which is the skill working -- and neither the verdict nor the JSON could show it. Only the raw inspect logs could, which is not a report. Both sides' checks now reach the comparison and the report tables each unmet expectation against what happened on either side. No divergence flag is computed from them: the details are judge prose, and two runs can word the same behaviour differently. The first run with a `no_signal` case also showed the empty state claiming every case had needed the skill, which that case had not. Signed-off-by: John Lybeck --- skillscope/staleness.py | 81 +++++++++++++++++++++++++++++++++++++++- tests/test_skillscope.py | 70 ++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 2 deletions(-) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 5ad16a3..45f1796 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -40,7 +40,7 @@ from __future__ import annotations -from dataclasses import asdict, dataclass +from dataclasses import asdict, dataclass, field from .behavior import BehaviorOutcome @@ -57,7 +57,21 @@ @dataclass class Comparison: - """One case, run both ways.""" + """One case, run both ways. + + Both sides' checks are carried rather than reduced to the verdict. A case + that failed both ways can still have failed them *differently* -- the first + run of this command had an agent that tried the task and was stopped by the + machine on one side, and one that never attempted it on the other, which is + the skill working. The verdict could not see it and neither could the JSON, + so the only way to find out was to open the raw inspect logs, which is not + a report. + + No `diverged` flag is computed from them. The details are judge prose, and + two runs can word the same behaviour differently; a boolean over that would + claim a difference the harness cannot stand behind. They are shown instead, + for a reader to judge. + """ id: str skill: str @@ -65,6 +79,8 @@ class Comparison: passed_with: bool passed_without: bool error: str | None = None + checks_with: list[dict] = field(default_factory=list) + checks_without: list[dict] = field(default_factory=list) def classify(with_skill: BehaviorOutcome, without: BehaviorOutcome) -> str: @@ -114,6 +130,8 @@ def pair( passed_with=bool(this and this.passed), passed_without=bool(that and that.passed), error=f"no result from {missing}", + checks_with=list(this.checks) if this else [], + checks_without=list(that.checks) if that else [], ) ) continue @@ -125,6 +143,8 @@ def pair( passed_with=this.passed, passed_without=that.passed, error=this.error or that.error, + checks_with=list(this.checks), + checks_without=list(that.checks), ) ) return comparisons @@ -163,6 +183,38 @@ def summarize(comparisons: list[Comparison], meta: dict) -> dict: } +def _reasons(case: dict) -> list[tuple[str, str, str]]: + """Each expectation that failed on either side, with both sides' details. + + Matched on the expectation text, which is what both runs were graded + against. A check present on one side only still appears, so a run that + produced fewer checks than its partner cannot hide the difference. + """ + with_by_text = {c["expectation"]: c for c in case["checks_with"]} + without_by_text = {c["expectation"]: c for c in case["checks_without"]} + rows: list[tuple[str, str, str]] = [] + for text in sorted(set(with_by_text) | set(without_by_text)): + here, there = with_by_text.get(text), without_by_text.get(text) + if (here and here["passed"]) and (there and there["passed"]): + continue + rows.append( + ( + text, + _detail(here), + _detail(there), + ) + ) + return rows + + +def _detail(check: dict | None) -> str: + if check is None: + return "not graded this side" + if check["passed"]: + return "passed" + return (check["detail"] or "failed").replace("|", "\\|").replace("\n", " ") + + def render_markdown(summary: dict) -> str: totals = summary["totals"] meta = summary["meta"] @@ -207,4 +259,29 @@ def render_markdown(summary: dict) -> str: "evidence about the skill" ) lines.append(f"| `{case['id']}` | {case['verdict']} | {advice} |") + + # Why each unmet expectation went unmet, on both sides. A case that failed + # both ways is not necessarily a case that learned nothing: the two sides + # can fail for reasons that differ, and that difference is the skill. + detailed = [c for c in summary["cases"] if c["verdict"] != "earns_it" and _reasons(c)] + lines += ["", "### What happened on each side", ""] + if not detailed: + # Not "every case needed the skill": a `no_signal` case has nothing + # unmet either, and saying so was wrong on the first run that had one. + lines.append( + "Nothing to show: no case left an expectation unmet on either side." + ) + for case in detailed: + lines += [ + f"**`{case['id']}`** ({case['verdict']})", + "", + "| Expectation | With the skill | Without it |", + "| --- | --- | --- |", + ] + for text, here, there in _reasons(case): + lines.append( + f"| {text[:70].replace('|', chr(92) + '|')} " + f"| {here[:90]} | {there[:90]} |" + ) + lines.append("") return "\n".join(lines) + "\n" diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 409ed41..1ba3521 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -34,6 +34,7 @@ import time import types import unittest +from dataclasses import asdict import urllib.error from pathlib import Path from unittest import mock @@ -6061,6 +6062,75 @@ def test_a_case_that_needed_nothing_is_named_with_what_to_do(self) -> None: self.assertIn("unaided", report) +def _checked(case_id: str, passed: bool, checks: list[dict], skill: str = "s"): + """A behavioral outcome carrying graded checks.""" + return behavior.BehaviorOutcome( + id=case_id, skill=skill, prompt="p", passed=passed, elapsed_s=0.0, checks=checks + ) + + +def _check(text: str, passed: bool, detail: str = ""): + return {"kind": "expected_behavior", "expectation": text, "passed": passed, "detail": detail} + + +class TestBothSidesReasonsSurviveIntoTheReport(unittest.TestCase): + """The first real run of this command failed both ways for *different* + reasons -- the agent tried the task and was blocked on one side, and never + attempted it on the other -- and neither the verdict nor the JSON could + show it. Only the raw inspect logs could, which is not a report.""" + + def _pair(self): + return staleness.pair( + [_checked("c", False, [ + _check("Install it", False, "tried, blocked by missing sudo"), + _check("Avoid the cloud", True), + ])], + [_checked("c", False, [ + _check("Install it", False, "only surveyed, never attempted"), + _check("Avoid the cloud", True), + ])], + ) + + def test_both_sides_checks_reach_the_comparison(self) -> None: + comparison = self._pair()[0] + self.assertEqual(len(comparison.checks_with), 2) + self.assertEqual(len(comparison.checks_without), 2) + + def test_an_expectation_both_sides_met_is_not_reported_as_a_reason(self) -> None: + # Only what went unmet needs explaining. + rows = staleness._reasons(asdict(self._pair()[0])) + self.assertEqual([text for text, _, _ in rows], ["Install it"]) + + def test_the_two_reasons_appear_side_by_side(self) -> None: + summary = staleness.summarize(self._pair(), {"model": "m", "effort": "e"}) + report = staleness.render_markdown(summary) + self.assertIn("blocked by missing sudo", report) + self.assertIn("only surveyed, never attempted", report) + + def test_a_clean_run_is_not_described_as_every_case_needing_the_skill(self) -> None: + # A `no_signal` case has no unmet expectation either, so the empty + # state is about unmet expectations rather than about the verdict. + # The first run with a no_signal case reported the opposite. + comparisons = staleness.pair( + [_checked("c", True, [_check("Do the thing", True)])], + [_checked("c", True, [_check("Do the thing", True)])], + ) + summary = staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + report = staleness.render_markdown(summary) + self.assertEqual(summary["totals"]["no_signal"], 1) + self.assertNotIn("every case needed the skill", report) + self.assertIn("no case left an expectation unmet", report) + + def test_a_check_only_one_side_graded_is_still_shown(self) -> None: + # Fewer checks on one side must not hide the difference. + comparisons = staleness.pair( + [_checked("c", False, [_check("Install it", False, "blocked")])], + [_checked("c", False, [])], + ) + rows = staleness._reasons(asdict(comparisons[0])) + self.assertEqual(rows[0][2], "not graded this side") + + class TestTheBaselineRunIsTheSameRunWithoutTheSkill(unittest.TestCase): """The flag has to reach the driver, or the two halves are not comparable.""" From 9bd822b2fe4dd840485ebc35e03b7df4cdedf8d5 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Wed, 28 Oct 2026 14:24:34 +0100 Subject: [PATCH 03/15] Write the pairing into the logs, so a run can be read again Which half of a comparison a run was lived in the task name, which made the pairing a string-parsing job and left it filterable by nothing. Both engines now stamp `skillscope_baseline` and `skillscope_skill` into the eval metadata, with tags beside them, which is what inspect filters on. That makes a log directory enough to rebuild the comparison, so `--from-logs` can report on a finished run without reaching a model or spending anything. A directory with logs that predate the keys says so rather than reporting an empty comparison as a result. Signed-off-by: John Lybeck --- docs/usage.md | 10 ++++++ skillscope/cli.py | 50 +++++++++++++++++++++++++- skillscope/engine/behavioral.py | 10 ++++++ skillscope/engine/verify.py | 6 ++++ skillscope/staleness.py | 63 +++++++++++++++++++++++++++++++++ tests/test_skillscope.py | 61 +++++++++++++++++++++++++++++++ 6 files changed, 199 insertions(+), 1 deletion(-) diff --git a/docs/usage.md b/docs/usage.md index 847b0dd..105294b 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -273,6 +273,16 @@ name: either the skill has stopped mattering, or the case was never evidence about it in the first place. The second is the more common, which makes this a check on the dataset as much as on the skill. +Both halves write which side they were, and whose skill they were about, into +their inspect log metadata (`skillscope_baseline`, `skillscope_skill`). So a +finished run can be looked at again without paying for it twice: + +```bash +skillscope staleness --from-logs .skillscope/logs +``` + +That reaches no model and runs no preflight, having nothing to spend. + It reports and never gates, for the reason the `claude-code` leg does not gate either: the agent is nondeterministic on both sides, so a single pair is a place to look rather than a verdict. [`tools/benchmark_engines.py`](../tools/benchmark_engines.py) diff --git a/skillscope/cli.py b/skillscope/cli.py index 0d92aaf..6130078 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -783,6 +783,13 @@ def cmd_behavioral(args: argparse.Namespace) -> int: return 0 +def engine_behavioral_keys() -> tuple[str, str]: + """The metadata keys both engines stamp on a staleness log.""" + from .engine import behavioral + + return behavioral.BASELINE_KEY, behavioral.SKILL_KEY + + def cmd_staleness(args: argparse.Namespace) -> int: """Run every behavioral case twice, with the skill and without it. @@ -797,9 +804,40 @@ def cmd_staleness(args: argparse.Namespace) -> int: failure -- see `skillscope/staleness.py`. A deadline that expired still fails, because that is a statement about the run rather than about a skill. """ - skills = _prepare_graded_run(args) started = time.time() + if args.from_logs: + # Nothing is run, so nothing is prepared: no structural gate, no model + # pin, no reachability probe. Those guard a spend that is not about to + # happen, and demanding them would make a free command fail for + # reasons that cannot affect its answer. + comparisons = staleness.from_logs(args.from_logs) + if not comparisons: + baseline_key, _ = engine_behavioral_keys() + raise SystemExit( + f"error: no skillscope staleness logs in {args.from_logs}. " + f"Both halves of a comparison carry `{baseline_key}` in their " + "eval metadata; logs written before that, or by another tool, " + "cannot be paired." + ) + summary = staleness.summarize( + comparisons, + { + "model": args.model, + "engine": args.engine, + "effort": args.effort, + "skills": sorted({c.skill for c in comparisons}), + "extended": args.extended, + "from_logs": args.from_logs, + "wall_time_s": round(time.time() - started, 1), + "timeout": args.timeout, + "github_run_id": os.environ.get("GITHUB_RUN_ID"), + }, + ) + _write_report(summary, staleness.render_markdown(summary), args, "staleness") + return 0 + + skills = _prepare_graded_run(args) gradable = _gradable_cases(args, skills) if not gradable: @@ -1161,6 +1199,16 @@ def build_parser() -> argparse.ArgumentParser: ), ) _add_graded_arguments(staleness_parser) + staleness_parser.add_argument( + "--from-logs", + metavar="DIR", + help=( + "Rebuild the comparison from logs already on disk instead of " + "running anything. Both halves carry the skill and which side " + "they were in their eval metadata, so a log directory is enough " + "to pair them. Costs nothing and reaches no model." + ), + ) staleness_parser.set_defaults(handler=cmd_staleness) select_parser = commands.add_parser( diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index f3a3c0e..2b0433c 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -117,6 +117,8 @@ def build_task( bound = deadline.active() return Task( name=f"behavioral-{skill}" + ("-baseline" if baseline else ""), + metadata={BASELINE_KEY: baseline, SKILL_KEY: skill}, + tags=["skillscope", "baseline" if baseline else "graded"], dataset=samples, setup=hooks.setup_solver(hook, skill, cases), solver=solver_factory(skill_dir), @@ -213,6 +215,14 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: # stopped run reads back its own log and nobody else's. CALL_TOKEN = "skillscope_call" +# Which half of a staleness comparison a log is, and whose skill it is about. +# The task name says the same thing, but a name is a string somebody has to +# parse; inspect filters on metadata and tags, and so can anything reading a +# log directory afterwards. Written by both engines, so a reader does not have +# to know which one produced the log it is holding. +BASELINE_KEY = "skillscope_baseline" +SKILL_KEY = "skillscope_skill" + def stop_requested() -> bool: """Whether the deadline's graceful stage has asked the command to stop.""" diff --git a/skillscope/engine/verify.py b/skillscope/engine/verify.py index 04694c8..63bebad 100644 --- a/skillscope/engine/verify.py +++ b/skillscope/engine/verify.py @@ -249,6 +249,12 @@ def build_task( bound = deadline.active() return Task( name=f"claude-code-{skill}" + ("" if install else "-baseline"), + # See `behavioral.build_task`: the same two keys whichever engine ran. + metadata={ + behavioral.BASELINE_KEY: not install, + behavioral.SKILL_KEY: skill, + }, + tags=["skillscope", "graded" if install else "baseline"], setup=hooks.setup_solver(hook, skill, cases), cleanup=hooks.cleanup_fn(hook, skill, cases), dataset=samples, diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 45f1796..732c264 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -150,6 +150,69 @@ def pair( return comparisons +def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: + """`(skill, was_baseline, outcomes)` for one inspect log. + + Rebuilt from the samples rather than from a dataset, because an offline + reader has no `Case` list: the log is all there is. Prompts are left empty + for the same reason -- the comparison is about verdicts, and inventing a + prompt field nobody wrote would be worse than an honest blank. + """ + from .engine import behavioral, scorers + + eval_meta = log.eval.metadata or {} + skill = eval_meta.get(behavioral.SKILL_KEY) or log.eval.task + outcomes: list[BehaviorOutcome] = [] + for sample in log.samples or []: + checks: list[dict] = [] + for score in (sample.scores or {}).values(): + checks.extend((score.metadata or {}).get(scorers.CHECKS, [])) + error = sample.error.message if sample.error is not None else None + outcomes.append( + BehaviorOutcome( + id=str(sample.id), + skill=skill, + prompt="", + passed=error is None and bool(checks) and all(c["passed"] for c in checks), + elapsed_s=round(getattr(sample, "total_time", None) or 0.0, 2), + checks=checks, + error=error, + ) + ) + return skill, bool(eval_meta.get(behavioral.BASELINE_KEY)), outcomes + + +def from_logs(log_dir: str) -> list[Comparison]: + """Rebuild the comparison from logs already on disk. + + Two runs become a pair because they carry the same skill and opposite + `BASELINE_KEY`, which is why those are written as metadata rather than + inferred from a task name. A directory holding only one side of a skill is + reported as a comparison against nothing, the same way a case missing from + one side is: silently dropping it would read as a smaller, cleaner run. + + Logs that are not skillscope's own are passed over. A shared log directory + is normal, and failing on a neighbour's file would make this unusable in + exactly the directories it is meant for. + """ + from inspect_ai.log import list_eval_logs, read_eval_log + + from .engine import behavioral + + sides: dict[str, dict[bool, list[BehaviorOutcome]]] = {} + for info in list_eval_logs(log_dir): + log = read_eval_log(info.name) + if behavioral.BASELINE_KEY not in (log.eval.metadata or {}): + continue + skill, baseline, outcomes = outcomes_from_log(log) + sides.setdefault(skill, {}).setdefault(baseline, []).extend(outcomes) + + comparisons: list[Comparison] = [] + for skill in sorted(sides): + comparisons.extend(pair(sides[skill].get(False, []), sides[skill].get(True, []))) + return comparisons + + def summarize(comparisons: list[Comparison], meta: dict) -> dict: counts = {key: 0 for key, _ in VERDICTS} for comparison in comparisons: diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 1ba3521..6f61272 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6131,6 +6131,67 @@ def test_a_check_only_one_side_graded_is_still_shown(self) -> None: self.assertEqual(rows[0][2], "not graded this side") +class _StubSample: + def __init__(self, sample_id, checks, error=None): + self.id = sample_id + self.scores = {"expectations": types.SimpleNamespace(metadata={"checks": checks})} + self.error = error + self.total_time = 1.0 + + +class _StubLog: + def __init__(self, skill, baseline, samples): + self.eval = types.SimpleNamespace( + task=f"behavioral-{skill}", + metadata={ + engine_behavioral.SKILL_KEY: skill, + engine_behavioral.BASELINE_KEY: baseline, + }, + ) + self.samples = samples + + +class TestAComparisonCanBeRebuiltFromLogs(unittest.TestCase): + """The pairing is written into the logs, so a directory is enough. + + Without this the two runs are related only in the memory of the process + that launched them, and a finished run cannot be looked at again without + paying for it twice over. + """ + + def test_a_log_reports_which_side_it_was(self) -> None: + log = _StubLog("alpha", True, [_StubSample("c", [_check("do it", False)])]) + skill, baseline, outcomes = staleness.outcomes_from_log(log) + self.assertEqual(skill, "alpha") + self.assertTrue(baseline) + self.assertEqual(len(outcomes), 1) + self.assertFalse(outcomes[0].passed) + + def test_a_sample_passes_only_when_every_check_did(self) -> None: + log = _StubLog("alpha", False, [ + _StubSample("c", [_check("a", True), _check("b", True)]), + _StubSample("d", [_check("a", True), _check("b", False)]), + ]) + _, _, outcomes = staleness.outcomes_from_log(log) + self.assertEqual([o.passed for o in outcomes], [True, False]) + + def test_a_sample_with_no_checks_has_not_passed(self) -> None: + # An ungraded sample is not a green one. + log = _StubLog("alpha", False, [_StubSample("c", [])]) + _, _, outcomes = staleness.outcomes_from_log(log) + self.assertFalse(outcomes[0].passed) + + def test_the_two_sides_pair_on_the_skill_they_name(self) -> None: + graded = _StubLog("alpha", False, [_StubSample("c", [_check("do it", True)])]) + baseline = _StubLog("alpha", True, [_StubSample("c", [_check("do it", False)])]) + sides = {} + for log in (graded, baseline): + skill, is_baseline, outcomes = staleness.outcomes_from_log(log) + sides.setdefault(skill, {}).setdefault(is_baseline, []).extend(outcomes) + comparisons = staleness.pair(sides["alpha"][False], sides["alpha"][True]) + self.assertEqual([c.verdict for c in comparisons], ["earns_it"]) + + class TestTheBaselineRunIsTheSameRunWithoutTheSkill(unittest.TestCase): """The flag has to reach the driver, or the two halves are not comparable.""" From 560bb059272b35cf9c2616348ec73353bebf9de4 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Wed, 28 Oct 2026 15:14:31 +0100 Subject: [PATCH 04/15] Judge the claims, and say whether the skill actually fired Two things were being credited to a skill that had not earned them. Guard rails held on both sides, because that is what a guard rail does -- the base model was never going to run `sudo` unprompted either. Counting that as "the skill made no difference" marked the checks most worth keeping as dead weight. Verdicts now read the positive claims alone, and guards are reported on their own terms, where one broken with the skill installed is the finding. And nothing checked that the agent ever reached for the skill. A behavioral run installs one but cannot make it fire, so `earns_it` meant "installed and the outcome differed". It now records the activation with the detector the routing leg already uses, and says so when a case earned its verdict without the skill ever firing. The evidence section also stopped skipping `earns_it` cases, which was exactly backwards: those are the only ones where the evidence lives, and a run whose point was that three cases moved printed "nothing to show". Signed-off-by: John Lybeck --- docs/usage.md | 10 +++ skillscope/behavior.py | 7 ++ skillscope/engine/behavioral.py | 32 +++++++++ skillscope/staleness.py | 119 ++++++++++++++++++++++++++++---- tests/test_skillscope.py | 113 +++++++++++++++++++++++++++++- 5 files changed, 266 insertions(+), 15 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 105294b..f14c15c 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -268,6 +268,16 @@ skillscope staleness --skill my-skill | `fails_either_way` | failed both times — a broken case, or a hard one | | `ungraded` | a run errored on one side or both | +A verdict says what changed, not why. So the graded side also records whether +the agent *reached for* the skill, using the same detector the routing leg +uses: installing a skill does not make an agent use it, and "installed and the +outcome differed" is a weaker claim than "the skill fired and the outcome +differed". An `earns_it` where the skill never fired is reported as the lead it +is rather than as a result. Guard rails -- `unexpected_behavior` -- are kept +out of the verdict entirely, since a good one holds whether or not the skill is +installed; they are counted on their own, where a guard broken *with* the skill +installed is the serious finding. + `no_signal` is the verdict worth acting on, and it is two findings wearing one name: either the skill has stopped mattering, or the case was never evidence about it in the first place. The second is the more common, which makes this a diff --git a/skillscope/behavior.py b/skillscope/behavior.py index b382b1d..61e7148 100644 --- a/skillscope/behavior.py +++ b/skillscope/behavior.py @@ -29,6 +29,13 @@ class BehaviorOutcome: elapsed_s: float checks: list[dict] = field(default_factory=list) error: str | None = None + # The skill the agent actually reached for, if any. A behavioral run + # installs a skill but cannot make the agent use it, and until this was + # recorded a case that passed was credited to a skill that may never have + # fired. `skillscope staleness` is what the distinction is for: "installed + # and the outcome differed" is a weaker claim than "the skill fired and the + # outcome differed", and only the second is evidence about the skill. + activated: str | None = None # Set when the failure was the model provider's rather than the skill's. # A behavioral case that failed on a gateway 504 is a skill nobody # measured, and reporting it beside one that genuinely failed its diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index 2b0433c..8b161c7 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -146,6 +146,33 @@ def _failed(skill: str, cases: list[Case], detail: str) -> list[BehaviorOutcome] ] +def _activated(routing_engine, sample, skill: str) -> str | None: + """The skill the agent reached for in this sample, if any. + + Graded by `routing.detect_activation`, the same rule and the same tool-name + matching the routing leg uses, so "fired" means one thing across the two + commands. A baseline sample has no skill installed and so returns None, + which is the point: it is what makes "the skill fired and the outcome + differed" sayable at all. + + Best effort. A log shape this cannot walk leaves the field None rather + than failing a case that was otherwise graded fine -- an unknown activation + is a weaker report, not a broken run. + """ + try: + for call in routing_engine._tool_calls(sample): + hit = routing_core.detect_activation( + routing_engine._activation_event(call), + [skill], + allow_body_path=routing_engine.ALLOW_BODY_PATH, + ) + if hit: + return hit + except Exception: # noqa: BLE001 -- see the docstring + return None + return None + + def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: """Map one inspect `EvalLog` back onto skillscope's outcome objects. @@ -161,6 +188,10 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: for the same reason; the two now agree. Cases with no sample are still errored below, so nothing goes quietly missing. """ + # Imported here rather than at module scope: `engine/routing.py` imports + # this module, so a top-level import would close the cycle. + from . import routing as routing_engine # noqa: PLC0415 + prompts = {c.id: c.prompt for c in cases} outcomes: list[BehaviorOutcome] = [] @@ -191,6 +222,7 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: checks=checks, error=error, degraded=routing_core.is_provider_error(error), + activated=_activated(routing_engine, sample, skill), ) ) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 732c264..728bfc5 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -51,9 +51,23 @@ ("harmful", "failed with the skill, passed without it"), ("no_signal", "passed either way"), ("fails_either_way", "failed either way"), + ("guards_only", "asserts nothing the skill could be credited for"), ("ungraded", "one side or both could not be graded"), ) +# What a case claims the skill will *do*. Only these decide a verdict. +POSITIVE_KINDS = ("expected_behavior", "logs_contain", "files_exist") + +# What a case forbids. Deliberately excluded from the verdict. +# +# A guard rail exists to catch a future edit to the skill that starts doing the +# forbidden thing, so a well-written one holds whether or not the skill is +# installed -- the base model was never going to run `sudo` unprompted either. +# Counting that as "the skill made no difference" would mark the checks most +# worth keeping as dead weight, and push an author to delete them. They are +# reported on their own terms instead: held, or broken. +GUARD_KIND = "unexpected_behavior" + @dataclass class Comparison: @@ -81,11 +95,34 @@ class Comparison: error: str | None = None checks_with: list[dict] = field(default_factory=list) checks_without: list[dict] = field(default_factory=list) + # Whether the agent actually reached for the skill on the installed side. + # An `earns_it` with this unset is the weak kind: something differed, but + # not demonstrably because of the thing under test. + activated_with: str | None = None + + +def positive(checks: list[dict]) -> list[dict]: + """The checks that claim the skill will do something.""" + return [c for c in checks if c.get("kind") in POSITIVE_KINDS] + + +def guards(checks: list[dict]) -> list[dict]: + """The checks that forbid something.""" + return [c for c in checks if c.get("kind") == GUARD_KIND] + + +def _met(outcome: BehaviorOutcome) -> bool: + """Whether this side met every positive claim on the case.""" + claims = positive(outcome.checks) + return bool(claims) and all(c["passed"] for c in claims) def classify(with_skill: BehaviorOutcome, without: BehaviorOutcome) -> str: """Which of `VERDICTS` this pair is. + Judged on the positive claims alone -- see `GUARD_KIND` for why the + forbidden ones are kept out of it. + An errored run on either side is `ungraded` rather than a failure: a provider that dropped the connection has measured nothing, and counting it as "the skill made no difference" would be the most flattering possible @@ -93,11 +130,17 @@ def classify(with_skill: BehaviorOutcome, without: BehaviorOutcome) -> str: """ if with_skill.error or without.error: return "ungraded" - if with_skill.passed and not without.passed: + if not positive(with_skill.checks) and not positive(without.checks): + # Nothing was claimed, so nothing can be credited. Reporting this as + # `fails_either_way` would read as a broken case rather than as one + # that only ever pinned down what must not happen. + return "guards_only" + here, there = _met(with_skill), _met(without) + if here and not there: return "earns_it" - if without.passed and not with_skill.passed: + if there and not here: return "harmful" - if with_skill.passed and without.passed: + if here and there: return "no_signal" return "fails_either_way" @@ -132,6 +175,7 @@ def pair( error=f"no result from {missing}", checks_with=list(this.checks) if this else [], checks_without=list(that.checks) if that else [], + activated_with=this.activated if this else None, ) ) continue @@ -145,6 +189,7 @@ def pair( error=this.error or that.error, checks_with=list(this.checks), checks_without=list(that.checks), + activated_with=this.activated, ) ) return comparisons @@ -159,6 +204,7 @@ def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: prompt field nobody wrote would be worse than an honest blank. """ from .engine import behavioral, scorers + from .engine import routing as routing_engine eval_meta = log.eval.metadata or {} skill = eval_meta.get(behavioral.SKILL_KEY) or log.eval.task @@ -177,6 +223,9 @@ def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: elapsed_s=round(getattr(sample, "total_time", None) or 0.0, 2), checks=checks, error=error, + # Detected here too, or a rebuilt comparison would report every + # `earns_it` as never having fired and warn about all of them. + activated=behavioral._activated(routing_engine, sample, skill), ) ) return skill, bool(eval_meta.get(behavioral.BASELINE_KEY)), outcomes @@ -226,7 +275,18 @@ def summarize(comparisons: list[Comparison], meta: dict) -> dict: **{key: sum(1 for c in subset if c.verdict == key) for key, _ in VERDICTS}, } - graded = [c for c in comparisons if c.verdict != "ungraded"] + graded = [ + c for c in comparisons if c.verdict not in ("ungraded", "guards_only") + ] + + # Guard rails, counted separately. A broken one is the serious finding + # here: a skill that made the agent do the forbidden thing. + broken = [ + c + for c in comparisons + if any(not g["passed"] for g in guards(c.checks_with)) + ] + guard_checks = sum(len(guards(c.checks_with)) for c in comparisons) return { "meta": meta, "totals": { @@ -240,6 +300,15 @@ def summarize(comparisons: list[Comparison], meta: dict) -> dict: if graded else None ), + "guard_checks": guard_checks, + "guards_broken_with_skill": len(broken), + # `earns_it` cases where the skill was installed but never fired. + # The outcome differed; the skill is not shown to be why. + "earned_without_firing": sum( + 1 + for c in comparisons + if c.verdict == "earns_it" and not c.activated_with + ), }, "per_skill": per_skill, "cases": [asdict(c) for c in comparisons], @@ -306,6 +375,31 @@ def render_markdown(summary: dict) -> str: if share is not None: lines += ["", f"Discriminating: {share:.0%} of the cases graded both ways."] + unfired = totals["earned_without_firing"] + if unfired: + lines += [ + "", + f"> **{unfired} of the {totals['earns_it']} `earns_it` case(s) never " + "activated the skill.** Installing a skill does not make an agent " + "use it, so for those the outcome differed without the skill being " + "shown to be the reason. Read them as a lead rather than a result.", + ] + + if totals["guard_checks"]: + broken = totals["guards_broken_with_skill"] + lines += [ + "", + f"{totals['guard_checks']} guard rail(s) checked and kept out of " + "the verdict: they hold whether or not the skill is installed, " + "which is what they are for. " + + ( + f"**{broken} case(s) broke one with the skill installed** -- " + "that is the skill doing the forbidden thing." + if broken + else "None was broken with the skill installed." + ), + ] + notable = [c for c in summary["cases"] if c["verdict"] in ("no_signal", "harmful")] lines += ["", "### Cases worth a look", ""] if not notable: @@ -323,16 +417,17 @@ def render_markdown(summary: dict) -> str: ) lines.append(f"| `{case['id']}` | {case['verdict']} | {advice} |") - # Why each unmet expectation went unmet, on both sides. A case that failed - # both ways is not necessarily a case that learned nothing: the two sides - # can fail for reasons that differ, and that difference is the skill. - detailed = [c for c in summary["cases"] if c["verdict"] != "earns_it" and _reasons(c)] - lines += ["", "### What happened on each side", ""] + # Every claim that came out differently on the two sides -- whatever the + # verdict. This used to skip `earns_it` cases, on the reasoning that a + # skill which earned its keep needs no explaining, which had it exactly + # backwards: those are the only cases where the evidence lives, and the + # section printed "nothing to show" on a run whose whole point was that + # three cases moved. + detailed = [c for c in summary["cases"] if _reasons(c)] + lines += ["", "### What the skill changed", ""] if not detailed: - # Not "every case needed the skill": a `no_signal` case has nothing - # unmet either, and saying so was wrong on the first run that had one. lines.append( - "Nothing to show: no case left an expectation unmet on either side." + "Nothing: every claim came out the same with the skill and without it." ) for case in detailed: lines += [ diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 6f61272..be646ca 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -5974,9 +5974,18 @@ def test_a_good_table_still_binds(self) -> None: def _outcome(case_id: str, passed: bool, *, error: str | None = None, skill: str = "s"): - """One behavioral outcome, as either half of a staleness pair would report it.""" + """One behavioral outcome, as either half of a staleness pair would report it. + + Carries a positive claim matching `passed`, because that is what a verdict + is computed from -- an outcome asserting nothing is `guards_only`, which + has its own tests. + """ return behavior.BehaviorOutcome( - id=case_id, skill=skill, prompt="p", passed=passed, elapsed_s=0.0, error=error + id=case_id, skill=skill, prompt="p", passed=passed, elapsed_s=0.0, error=error, + checks=[{ + "kind": "expected_behavior", "expectation": "do the thing", + "passed": passed, "detail": "", + }], ) @@ -6119,7 +6128,7 @@ def test_a_clean_run_is_not_described_as_every_case_needing_the_skill(self) -> N report = staleness.render_markdown(summary) self.assertEqual(summary["totals"]["no_signal"], 1) self.assertNotIn("every case needed the skill", report) - self.assertIn("no case left an expectation unmet", report) + self.assertIn("every claim came out the same", report) def test_a_check_only_one_side_graded_is_still_shown(self) -> None: # Fewer checks on one side must not hide the difference. @@ -6151,6 +6160,104 @@ def __init__(self, skill, baseline, samples): self.samples = samples +class TestGuardRailsAreNotJudgedAsIfTheyDiscriminated(unittest.TestCase): + """A guard rail holds with or without the skill -- that is its job. + + Folding that into the verdict marks the checks most worth keeping as dead + weight, which would push an author to delete exactly the wrong ones. + """ + + def _guard(self, passed: bool): + return {"kind": "unexpected_behavior", "expectation": "no sudo", + "passed": passed, "detail": ""} + + def test_a_guard_holding_on_both_sides_does_not_make_a_case_no_signal(self) -> None: + # The claim moved; the guard did not. The verdict follows the claim. + with_skill = _checked("c", True, [_check("run the CLI", True), self._guard(True)]) + without = _checked("c", False, [_check("run the CLI", False), self._guard(True)]) + self.assertEqual(staleness.classify(with_skill, without), "earns_it") + + def test_a_case_with_only_guards_is_called_that(self) -> None: + # Nothing was claimed, so nothing can be credited -- but the case is + # not broken, which `fails_either_way` would imply. + both = _checked("c", True, [self._guard(True)]) + self.assertEqual(staleness.classify(both, both), "guards_only") + + def test_guards_only_cases_stay_out_of_the_discriminating_share(self) -> None: + comparisons = staleness.pair( + [_checked("a", True, [_check("x", True)]), _checked("b", True, [self._guard(True)])], + [_checked("a", False, [_check("x", False)]), _checked("b", True, [self._guard(True)])], + ) + totals = staleness.summarize(comparisons, {"model": "m", "effort": "e"})["totals"] + self.assertEqual(totals["guards_only"], 1) + self.assertEqual(totals["graded"], 1) + self.assertEqual(totals["discriminating"], 1.0) + + def test_a_guard_broken_with_the_skill_is_surfaced(self) -> None: + # The serious finding: the skill made the agent do the forbidden thing. + comparisons = staleness.pair( + [_checked("c", False, [_check("x", True), self._guard(False)])], + [_checked("c", True, [_check("x", True), self._guard(True)])], + ) + summary = staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + self.assertEqual(summary["totals"]["guards_broken_with_skill"], 1) + self.assertIn("forbidden thing", staleness.render_markdown(summary)) + + def test_the_evidence_shows_even_when_the_skill_earned_it(self) -> None: + # The old report skipped `earns_it` cases, so a run whose whole point + # was that three cases moved printed "nothing to show". + comparisons = staleness.pair( + [_checked("c", True, [_check("ran `rocm --version`", True)])], + [_checked("c", False, [_check("ran `rocm --version`", False)])], + ) + report = staleness.render_markdown( + staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + ) + self.assertIn("What the skill changed", report) + self.assertIn("rocm --version", report) + + +class TestEarningItMeansTheSkillActuallyFired(unittest.TestCase): + """Installing a skill does not make an agent use it. + + Until activation was recorded, an `earns_it` meant only that the outcome + differed on the side where the skill happened to be installed. + """ + + def _pair(self, activated): + with_skill = _checked("c", True, [_check("ran the CLI", True)]) + with_skill.activated = activated + return staleness.pair([with_skill], [_checked("c", False, [_check("ran the CLI", False)])]) + + def test_the_firing_is_carried_onto_the_comparison(self) -> None: + self.assertEqual(self._pair("rocm-doctor")[0].activated_with, "rocm-doctor") + + def test_an_earns_it_that_never_fired_is_counted(self) -> None: + totals = staleness.summarize(self._pair(None), {"model": "m", "effort": "e"})["totals"] + self.assertEqual(totals["earns_it"], 1) + self.assertEqual(totals["earned_without_firing"], 1) + + def test_an_earns_it_that_fired_is_not_counted(self) -> None: + totals = staleness.summarize( + self._pair("rocm-doctor"), {"model": "m", "effort": "e"} + )["totals"] + self.assertEqual(totals["earned_without_firing"], 0) + + def test_the_reader_is_warned_before_believing_the_number(self) -> None: + summary = staleness.summarize(self._pair(None), {"model": "m", "effort": "e"}) + report = staleness.render_markdown(summary) + self.assertIn("never activated the skill", report) + self.assertLess(report.index("never activated"), report.index("### Cases worth a look")) + + def test_a_clean_run_says_nothing_about_firing(self) -> None: + # No warning when every earns_it fired: an unconditional note would be + # noise, and noise is what gets skipped on the run that matters. + report = staleness.render_markdown( + staleness.summarize(self._pair("rocm-doctor"), {"model": "m", "effort": "e"}) + ) + self.assertNotIn("never activated the skill", report) + + class TestAComparisonCanBeRebuiltFromLogs(unittest.TestCase): """The pairing is written into the logs, so a directory is enough. From 100c72e3cd3b08512e108a5c2d5ad19c451ae9bd Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Wed, 28 Oct 2026 15:26:14 +0100 Subject: [PATCH 05/15] Say it outright when nothing in a suite needed the skill `earns_it | 0` is a true row in a table that nobody reads twice, and it is the one result this command exists to surface. A run where every graded case came out the same now says so above the numbers, and names both explanations -- a skill that has stopped earning its place and a suite that never tested what it knows are different problems with different fixes, and the report cannot tell which it is looking at. A run that graded nothing says nothing of the sort: no cases is not the same finding as no difference. Signed-off-by: John Lybeck --- skillscope/staleness.py | 17 ++++++++++++++++ tests/test_skillscope.py | 42 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 59 insertions(+) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 728bfc5..31de2fe 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -363,6 +363,23 @@ def render_markdown(summary: dict) -> str: f"{headline} on `{meta['model']}` (effort `{meta['effort']}`). Every case " "ran twice: once with the skill installed, once with nothing installed.", "", + ] + + # Nothing discriminated. The row `earns_it | 0` is true and says nothing, + # and this is the one result the command exists to surface, so it is said + # rather than left to be noticed in a table. + if totals["graded"] and not totals["earns_it"]: + lines += [ + "> **No case in this suite shows the skill doing anything.** Every " + "graded case came out the same with the skill and without it. " + "Either the skill has stopped earning its place, or its cases " + "never tested what it knows -- read the skill and ask whether any " + "case here would need it. Those are different problems with " + "different fixes.", + "", + ] + + lines += [ "Reporting only. Agents disagree with themselves between runs, so treat " "a surprising verdict as somewhere to look rather than as a result.", "", diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index be646ca..46d0145 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6217,6 +6217,48 @@ def test_the_evidence_shows_even_when_the_skill_earned_it(self) -> None: self.assertIn("rocm --version", report) +class TestASkillThatChangedNothingIsToldPlainly(unittest.TestCase): + """`earns_it | 0` is a true row that says nothing, and this is the one + result the command exists to surface.""" + + def _all_no_signal(self): + both = [_checked("a", True, [_check("x", True)])] + return staleness.pair(both, both) + + def test_the_result_is_stated_rather_than_left_in_the_table(self) -> None: + report = staleness.render_markdown( + staleness.summarize(self._all_no_signal(), {"model": "m", "effort": "e"}) + ) + self.assertIn("No case in this suite shows the skill doing anything", report) + + def test_it_names_both_explanations(self) -> None: + # Retire the skill and fix the cases are different actions, and the + # report cannot tell which applies. + report = staleness.render_markdown( + staleness.summarize(self._all_no_signal(), {"model": "m", "effort": "e"}) + ) + self.assertIn("stopped earning its place", report) + self.assertIn("never tested what it knows", report) + + def test_a_run_with_a_discriminating_case_says_nothing_of_the_sort(self) -> None: + comparisons = staleness.pair( + [_checked("a", True, [_check("x", True)])], + [_checked("a", False, [_check("x", False)])], + ) + report = staleness.render_markdown( + staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + ) + self.assertNotIn("shows the skill doing anything", report) + + def test_a_run_that_graded_nothing_does_not_claim_the_skill_is_idle(self) -> None: + # Nothing graded is not the same as nothing changed. + comparisons = staleness.pair([_checked("a", True, [_check("x", True)])], []) + report = staleness.render_markdown( + staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + ) + self.assertNotIn("shows the skill doing anything", report) + + class TestEarningItMeansTheSkillActuallyFired(unittest.TestCase): """Installing a skill does not make an agent use it. From 43a5b8ad3e7741d084dd9d25fb2604402d3ce2d8 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 06:34:25 +0100 Subject: [PATCH 06/15] Keep the runner's own skills out of the baseline The baseline was never the empty room it claimed to be. A behavioral run passes no `config_dir`, so the runner's `~/.claude/skills` were registered beside the staged ones -- on the machine this was developed on, forty of them. Both passes could reach them, and the command runs from the repo root, so the skill under test was readable on disk whatever the staging step did. Real runs hit both routes: one baseline invoked an unrelated personal skill, another read the skill under test straight out of the repo. Both were reported `no_signal`, which asserts the agent managed unaided. It had not. So both passes now run under a throwaway config dir, the way `routing` has always done, and the command refuses a host run that cannot be isolated rather than quietly measuring this machine. What isolation cannot close -- the source on disk -- is detected instead and reported. Contamination is not symmetric, so the verdict is not voided symmetrically. `no_signal` and `harmful` rest on the baseline having managed something, and cannot survive it. `earns_it` and `fails_either_way` rest on the baseline having failed, and a baseline that failed while holding more than it should fails all the harder without it. `--from-logs` detects both too. It read activation but not contamination, which made a rebuild more permissive than the run it rebuilt, quietly restoring a verdict the live run had voided. Also: budget for a baseline that runs longer than the graded pass. An agent holding the skill goes straight to the work; an agent without it explores. The old guidance said twice a behavioral bound, and a two-case run spent 2m23s on the graded pass and was still going on the baseline half an hour later. Signed-off-by: John Lybeck --- docs/usage.md | 8 ++++ skillscope/behavior.py | 5 +++ skillscope/cli.py | 49 ++++++++++++++++---- skillscope/engine/behavioral.py | 47 ++++++++++++++++++- skillscope/staleness.py | 80 +++++++++++++++++++++++++++++---- tests/test_skillscope.py | 73 ++++++++++++++++++++++++++++++ 6 files changed, 245 insertions(+), 17 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index f14c15c..dbcc088 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -293,6 +293,14 @@ skillscope staleness --from-logs .skillscope/logs That reaches no model and runs no preflight, having nothing to spend. +Budget more wall clock than a `behavioral` run of the same cases, and more +than double it. Both passes share one `--timeout`, and the baseline is +reliably the slower half: the agent holding the skill goes straight to the +work, while the one without it explores. On a two-case skill the graded pass +took 2m23s and the baseline had not finished its second case half an hour +later. A bound that expires partway reports the unreached cases as `ungraded`, +after the graded half has already been paid for. + It reports and never gates, for the reason the `claude-code` leg does not gate either: the agent is nondeterministic on both sides, so a single pair is a place to look rather than a verdict. [`tools/benchmark_engines.py`](../tools/benchmark_engines.py) diff --git a/skillscope/behavior.py b/skillscope/behavior.py index 61e7148..123d4eb 100644 --- a/skillscope/behavior.py +++ b/skillscope/behavior.py @@ -36,6 +36,11 @@ class BehaviorOutcome: # and the outcome differed" is a weaker claim than "the skill fired and the # outcome differed", and only the second is evidence about the skill. activated: str | None = None + # Why this run was not the room it claimed to be: another skill fired, or + # the agent read the skill's own source off disk. Harmless noise in a + # behavioral run, which grades what the agent produced. Fatal to a + # staleness baseline, whose whole claim is that nothing was available. + contaminated: str | None = None # Set when the failure was the model provider's rather than the skill's. # A behavioral case that failed on a gateway 504 is a skill nobody # measured, and reporting it beside one that genuinely failed its diff --git a/skillscope/cli.py b/skillscope/cli.py index 6130078..c252465 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -68,6 +68,7 @@ import json import os import sys +import tempfile import time from pathlib import Path @@ -695,6 +696,7 @@ def _run_behavioral( gradable: list, *, install: bool = True, + config_dir: Path | None = None, ) -> list: """Run the behavioral cases on the selected engine. @@ -729,7 +731,7 @@ def _run_behavioral( engine_models.resolve(args.model), args.effort, solver_factory=lambda d: no_sandbox.claude_code_no_sandbox( - args.model, args.effort, d, install=install + args.model, args.effort, d, config_dir, install=install ), baseline=not install, ) @@ -795,9 +797,15 @@ def cmd_staleness(args: argparse.Namespace) -> int: Both passes run under the one `--timeout`, so a deadline that expires partway leaves the rest of the second pass unmeasured -- those cases are - reported `ungraded` rather than as a skill that made no difference. Give - this command roughly twice the bound a `behavioral` run of the same cases - would want. + reported `ungraded` rather than as a skill that made no difference. + + Budget for a baseline that runs *longer than the graded pass*, not the + same. An agent holding the skill goes straight to the work; an agent + without it explores -- building throwaway virtualenvs, dry-running + installs, reading around the problem. Measured on a two-case skill: the + graded pass finished both cases in 2m23s and the baseline was still going + on the second after 30 minutes. Twice a `behavioral` bound is the floor, + not the figure. Always exits 0 on a completed run. The comparison is nondeterministic on both sides, so a verdict here is somewhere to look rather than a build @@ -848,9 +856,32 @@ def cmd_staleness(args: argparse.Namespace) -> int: ) return 0 + # The runner's own `~/.claude/skills` are registered beside the staged + # ones, so without this the baseline is not an empty room -- it is this + # machine's skills minus the one under test. `routing` has refused to run + # without isolation for the same reason; staleness needs it more, because + # the baseline being empty is the entire measurement rather than one + # input to it. + if args.engine == "claude-code-no-sandbox" and not routing.can_isolate_config(): + raise SystemExit( + "error: staleness cannot run on --engine claude-code-no-sandbox " + f"without {' or '.join(routing.ENV_CREDENTIALS)}. The baseline " + "half has to be a room with no skills in it, and redirecting the " + "CLI away from the runner's own config dir only works when auth " + "comes from the environment. Without it every skill installed on " + "this machine joins the baseline, and a case it solves there " + "would be reported as one the skill under test was not needed " + "for.\n Set one of those, or use --engine claude-code, whose " + "guest has no user-level skills at all." + ) + print(f"[staleness] {len(gradable)} case(s), run twice", flush=True) - with_skill = _run_behavioral(args, skills, gradable) - without = _run_behavioral(args, skills, gradable, install=False) + with tempfile.TemporaryDirectory(prefix="skillscope-staleness-") as config_dir: + isolated = Path(config_dir) if args.engine == "claude-code-no-sandbox" else None + with_skill = _run_behavioral(args, skills, gradable, config_dir=isolated) + without = _run_behavioral( + args, skills, gradable, install=False, config_dir=isolated + ) comparisons = staleness.pair(with_skill, without) summary = staleness.summarize( @@ -1194,8 +1225,10 @@ def build_parser() -> argparse.ArgumentParser: "once with nothing installed -- and report which cases moved. A " "case that passes either way is not evidence about the skill. " "Reports only: it never fails a run. It costs twice what " - "`behavioral` costs and wants roughly twice the --timeout, " - "since both passes run under the one bound." + "`behavioral` costs, and wants more than twice the --timeout: " + "both passes share one bound, and the baseline is the slower " + "half because an agent without the skill explores instead of " + "going straight to the work." ), ) _add_graded_arguments(staleness_parser) diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index 8b161c7..d4ab447 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -16,6 +16,7 @@ from __future__ import annotations +import json import sys from pathlib import Path @@ -146,6 +147,47 @@ def _failed(skill: str, cases: list[Case], detail: str) -> list[BehaviorOutcome] ] +def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | None]: + """`(the skill that fired, why the room was not what it claimed)`. + + One walk over the tool calls, answering both questions, because the second + is only meaningful beside the first. `detect_activation` reports a skill + nobody staged as `other:`, which is exactly the contamination this + looks for -- the runner's own `~/.claude/skills` are registered next to + the staged ones, and a behavioral run passes no `config_dir` to keep them + out. + + Reading the skill's own source is the second route, and the one isolation + cannot close: the command runs from the repo root, so a directory the + staging step skipped is still on disk for an agent to find. A baseline + that read it was not a baseline. + + Best effort, by the same argument as `_activated`: a shape this cannot + walk is an unknown room, not a failed case. + """ + fired: str | None = None + why: str | None = None + marks = (f"skills/{skill}", f"/{skill}/", "_bundled_skills") + try: + for call in routing_engine._tool_calls(sample): + hit = routing_core.detect_activation( + routing_engine._activation_event(call), + [skill], + allow_body_path=routing_engine.ALLOW_BODY_PATH, + ) + if hit and hit.startswith("other:") and why is None: + why = f"another skill fired: {hit.removeprefix('other:')}" + elif hit and fired is None: + fired = hit + if why is None: + text = json.dumps(getattr(call, "arguments", None) or {}, ensure_ascii=False) + if any(mark in text for mark in marks): + why = f"read the skill's own files ({skill})" + except Exception: # noqa: BLE001 -- see the docstring + return fired, why + return fired, why + + def _activated(routing_engine, sample, skill: str) -> str | None: """The skill the agent reached for in this sample, if any. @@ -212,6 +254,8 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: elif not checks: error = "case has no behavioral assertions to grade" + activated, contaminated = _inspect_room(routing_engine, sample, skill) + outcomes.append( BehaviorOutcome( id=case_id, @@ -222,7 +266,8 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: checks=checks, error=error, degraded=routing_core.is_provider_error(error), - activated=_activated(routing_engine, sample, skill), + activated=activated, + contaminated=contaminated, ) ) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 31de2fe..294bb7a 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -99,6 +99,10 @@ class Comparison: # An `earns_it` with this unset is the weak kind: something differed, but # not demonstrably because of the thing under test. activated_with: str | None = None + # Why the baseline was not the empty room it claims to be, if it was not. + # A contaminated baseline cannot support `no_signal`: "the agent did this + # unaided" is precisely the claim the contamination refutes. + baseline_contaminated: str | None = None def positive(checks: list[dict]) -> list[dict]: @@ -137,12 +141,34 @@ def classify(with_skill: BehaviorOutcome, without: BehaviorOutcome) -> str: return "guards_only" here, there = _met(with_skill), _met(without) if here and not there: - return "earns_it" + return _voided_if_dirty("earns_it", without) if there and not here: - return "harmful" - if here and there: - return "no_signal" - return "fails_either_way" + return _voided_if_dirty("harmful", without) + verdict = ( + "no_signal" if (here and there) + else "fails_either_way" + ) + return _voided_if_dirty(verdict, without) + + +# Verdicts a contaminated baseline cannot support, and why it cannot. +# +# Contamination is not symmetric. `no_signal` and `harmful` both rest on the +# baseline having *managed* something, and a baseline that reached another +# skill or read this skill's own files did not manage it unaided -- which is +# the whole of what those verdicts claim. +# +# `earns_it` and `fails_either_way` rest on the baseline having *failed*. A +# baseline that failed while holding more than it should have fails all the +# harder without it, so the finding survives its own contamination and voiding +# it would throw away a conclusion the run is entitled to. +VOIDED_BY_CONTAMINATION = ("no_signal", "harmful") + + +def _voided_if_dirty(verdict: str, without: BehaviorOutcome) -> str: + if without.contaminated and verdict in VOIDED_BY_CONTAMINATION: + return "ungraded" + return verdict def pair( @@ -176,6 +202,7 @@ def pair( checks_with=list(this.checks) if this else [], checks_without=list(that.checks) if that else [], activated_with=this.activated if this else None, + baseline_contaminated=that.contaminated if that else None, ) ) continue @@ -190,6 +217,7 @@ def pair( checks_with=list(this.checks), checks_without=list(that.checks), activated_with=this.activated, + baseline_contaminated=that.contaminated, ) ) return comparisons @@ -223,9 +251,16 @@ def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: elapsed_s=round(getattr(sample, "total_time", None) or 0.0, 2), checks=checks, error=error, - # Detected here too, or a rebuilt comparison would report every - # `earns_it` as never having fired and warn about all of them. - activated=behavioral._activated(routing_engine, sample, skill), + # Both detected here too. Without the first, a rebuilt + # comparison reports every `earns_it` as never having fired; + # without the second it is *more permissive than the run it + # rebuilds*, quietly restoring verdicts the live run voided. + **dict( + zip( + ("activated", "contaminated"), + behavioral._inspect_room(routing_engine, sample, skill), + ) + ), ) ) return skill, bool(eval_meta.get(behavioral.BASELINE_KEY)), outcomes @@ -304,6 +339,9 @@ def summarize(comparisons: list[Comparison], meta: dict) -> dict: "guards_broken_with_skill": len(broken), # `earns_it` cases where the skill was installed but never fired. # The outcome differed; the skill is not shown to be why. + "baseline_contaminated": sum( + 1 for c in comparisons if c.baseline_contaminated + ), "earned_without_firing": sum( 1 for c in comparisons @@ -392,6 +430,32 @@ def render_markdown(summary: dict) -> str: if share is not None: lines += ["", f"Discriminating: {share:.0%} of the cases graded both ways."] + dirty = [c for c in summary["cases"] if c["baseline_contaminated"]] + if dirty: + voided = [c for c in dirty if c["verdict"] == "ungraded"] + stood = [c for c in dirty if c["verdict"] != "ungraded"] + reasons = sorted({c["baseline_contaminated"] for c in dirty}) + note = [ + "", + f"> **{len(dirty)} case(s) had a baseline that was not empty**: " + + "; ".join(reasons) + + ".", + ] + if voided: + note.append( + f"> {len(voided)} of them are reported `ungraded`: a baseline " + "that reached another skill, or read this skill's own files, " + "cannot show what an agent manages unaided.", + ) + if stood: + note.append( + f"> {len(stood)} still stand -- " + + ", ".join(f"`{c['id']}` ({c['verdict']})" for c in stood) + + " -- because they rest on the baseline *failing*, and it " + "failed while holding more than it should have.", + ) + lines += note + unfired = totals["earned_without_firing"] if unfired: lines += [ diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 46d0145..7b1f587 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6217,6 +6217,69 @@ def test_the_evidence_shows_even_when_the_skill_earned_it(self) -> None: self.assertIn("rocm --version", report) +class TestADirtyBaselineCannotShowWhatAnAgentDoesUnaided(unittest.TestCase): + """The first real runs had 40 user-level skills in the baseline room. + + One case invoked an unrelated personal skill and another read the skill + under test straight off the repo; both were reported `no_signal`, which + claims the agent managed unaided. It had not. + """ + + def _pair(self, reason): + without = _checked("c", True, [_check("x", True)]) + without.contaminated = reason + return staleness.pair([_checked("c", True, [_check("x", True)])], [without]) + + def test_a_contaminated_baseline_is_ungraded_not_no_signal(self) -> None: + self.assertEqual(self._pair("another skill fired: oss-contribution")[0].verdict, "ungraded") + + def test_an_earns_it_survives_its_own_contamination(self) -> None: + # The baseline failed while holding more than it should have. It + # fails all the harder without it, so voiding the verdict would throw + # away a conclusion the run is entitled to -- which cost a real run a + # real verdict before this was split out. + without = _checked("c", False, [_check("x", False)]) + without.contaminated = "read the skill's own files (rocm-doctor)" + comparisons = staleness.pair([_checked("c", True, [_check("x", True)])], [without]) + self.assertEqual(comparisons[0].verdict, "earns_it") + + def test_the_report_separates_the_voided_from_the_standing(self) -> None: + clean_fail = _checked("a", False, [_check("x", False)]) + clean_fail.contaminated = "read the skill's own files (s)" + passed_both = _checked("b", True, [_check("y", True)]) + passed_both.contaminated = "another skill fired: helper" + comparisons = staleness.pair( + [_checked("a", True, [_check("x", True)]), _checked("b", True, [_check("y", True)])], + [clean_fail, passed_both], + ) + report = staleness.render_markdown( + staleness.summarize(comparisons, {"model": "m", "effort": "e"}) + ) + self.assertIn("1 of them are reported `ungraded`", report) + self.assertIn("1 still stand", report) + self.assertIn("`a` (earns_it)", report) + + def test_a_clean_baseline_still_reports_no_signal(self) -> None: + self.assertEqual(self._pair(None)[0].verdict, "no_signal") + + def test_the_reason_reaches_the_report(self) -> None: + summary = staleness.summarize( + self._pair("read the skill's own files (quark-torch-llm-ptq)"), + {"model": "m", "effort": "e"}, + ) + report = staleness.render_markdown(summary) + self.assertEqual(summary["totals"]["baseline_contaminated"], 1) + self.assertIn("baseline that was not empty", report) + self.assertIn("read the skill's own files", report) + + def test_the_command_refuses_an_unisolatable_host_run(self) -> None: + # Routing has always refused this; staleness needs it more, because + # the baseline being empty is the measurement rather than an input. + source = inspect.getsource(cli.cmd_staleness) + self.assertIn("can_isolate_config", source) + self.assertIn("claude-code-no-sandbox", source) + + class TestASkillThatChangedNothingIsToldPlainly(unittest.TestCase): """`earns_it | 0` is a true row that says nothing, and this is the one result the command exists to surface.""" @@ -6330,6 +6393,16 @@ def test_a_sample_with_no_checks_has_not_passed(self) -> None: _, _, outcomes = staleness.outcomes_from_log(log) self.assertFalse(outcomes[0].passed) + def test_a_rebuild_is_no_more_permissive_than_the_run_it_rebuilds(self) -> None: + # The rebuild read activation but not contamination, so a verdict the + # live run voided came back `no_signal` on re-score. A report that + # disagrees with the run it was built from is worse than no report. + import inspect as _inspect + + source = _inspect.getsource(staleness.outcomes_from_log) + self.assertIn("_inspect_room", source) + self.assertIn("contaminated", source) + def test_the_two_sides_pair_on_the_skill_they_name(self) -> None: graded = _StubLog("alpha", False, [_StubSample("c", [_check("do it", True)])]) baseline = _StubLog("alpha", True, [_StubSample("c", [_check("do it", False)])]) From 7f2a8178a01a5f2a92fbc7d66370071f9c4241f4 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 07:40:43 +0100 Subject: [PATCH 07/15] Fix what the review found in the detection and pairing layer The measurement rests on two things: knowing which skill fired, and knowing which two runs are the two halves of one comparison. Both were wrong, in ways the suite could not see. `_inspect_room` answers two questions from one walk, and they shared an `if/elif`. Once `why` was set, a later `other:` hit failed the first condition and landed in `fired` -- so `activated_with` held a contamination marker, which reads as truthy everywhere downstream. `earned_without_firing` tests `not c.activated_with`, so the "never activated the skill" warning was suppressed on exactly the contaminated cases that most need it, and the marker reached both JSON reports verbatim. The branches are decoupled, and `fired` is now `None` or the skill, which is all `detect_activation` can return for `skills=[skill]`. `_activated`, left behind by 43be985 and carrying the same conflation in a worse form, is deleted. Contamination was judged on a bare skill name: `/{skill}/` as a substring of every call's serialised arguments. `structure.py` allows a single-word name and sets no minimum length, and single-word names are the norm -- `pdf`, `docx`, `pptx`. For a skill named `run`, `/run/user/1000` was contamination; for `docs` or `tools`, any ordinary path. The cost of that false positive is the highest here: `_voided_if_dirty` turns the two verdicts this command exists to surface into `ungraded`, and the report states the contamination as fact. Structured path arguments are resolved and tested for containment against the skill's real directory and its staged copy; shell commands keep substring matching, because there is no field to resolve, but against the resolved path rather than the name. `--from-logs` pooled across runs. It grouped on `(skill, baseline-flag)` with no run id, and every ordinary `behavioral` run stamps `BASELINE_KEY: False` -- so an unrelated run of the same skill was a candidate for this comparison's "with skill" half, in a directory that is shared, gitignored wholesale, never pruned, and the one the docs tell you to point this at. Case ids are repo-wide and `pair()` builds a dict from them, so the splice was silent and the survivor was the older log. `cmd_staleness` now mints one id per invocation and both halves carry it; logs without one are not a half of anything. A case two logs in one run both report is errored rather than resolved, the way this module reports every other ambiguity. The rebuild was also more permissive than the run it rebuilds, which is the property 43be985 claimed to have fixed. The live path voids a sample whose checks could not be read; `outcomes_from_log` did not, so it rebuilt as `checks=[]`, which makes `_met()` False, which promotes a harness failure to `earns_it`. `staleness` skipped hooks `behavioral` refuses to skip. `_require_hook_support` guarded on `command != "behavioral"` and `_prepare_graded_run` passes `args.command`, so the guard returned before checking anything. Routing is legitimately exempt because it executes nothing; staleness executes everything behavioral does, twice. A `--from-logs` report asserted the current invocation's `--model` and `--effort` as though they described the logs. Rebuilding a `sonnet`/`low` run printed "on `opus` (effort `high`)". Nothing reads them off the logs and the logs need not agree on them, so they are not claimed. `--timeout` now defaults to twice the single-pass bound. The command's own docstring documents a two-case skill finishing the graded pass in 2m23s with the baseline unfinished after 30 minutes; on the shared 900s default the slower half got ~637s of a measured 1800s+ need, which made truncation the expected outcome on the canonical example rather than a risk. Docs: `guards_only` was missing from the verdict table, and neither README nor usage.md said that `staleness` refuses to start without `ANTHROPIC_API_KEY` or `ANTHROPIC_AUTH_TOKEN`. It is in `routing`'s camp, not `behavioral`'s, and the section's first example failed for anyone authenticated the way `behavioral` permits. The tests: three of them asserted substrings of their subject's own source and passed while the property they named was false. Dropping the `not` from the isolation guard left `test_the_command_refuses_an_unisolatable_host_run` green. They assert behaviour now, `cmd_staleness` and `_run_behavioral` are called by a test for the first time, and `_inspect_room` is exercised against real tool calls rather than a stub with no messages. Each new test was checked against the unfixed code: every one of them fails there. Signed-off-by: John Lybeck --- README.md | 7 + docs/usage.md | 40 ++- skillscope/cli.py | 91 +++-- skillscope/engine/behavioral.py | 184 ++++++++--- skillscope/engine/verify.py | 18 +- skillscope/staleness.py | 109 +++++- tests/test_skillscope.py | 567 ++++++++++++++++++++++++++++++-- 7 files changed, 896 insertions(+), 120 deletions(-) diff --git a/README.md b/README.md index 503056b..a449a1d 100644 --- a/README.md +++ b/README.md @@ -37,6 +37,7 @@ skillscope structural # no agent, no tokens skillscope structural --external # the same, plus checking external URLs skillscope behavioral --skill my-skill # needs an authenticated `claude` CLI skillscope routing --routing-room my-skill,its-neighbour # also needs a key in the environment +skillscope staleness --skill my-skill # the same key: its baseline has to be an empty room ``` `routing` needs `ANTHROPIC_API_KEY` or `ANTHROPIC_AUTH_TOKEN` set, and refuses @@ -47,6 +48,12 @@ describe a room nobody asked for. `behavioral` installs one skill and grades what the agent produced, so a stray skill is at worst noise there, and a plain `claude login` is fine. +`staleness` is in `routing`'s camp, not `behavioral`'s, and refuses on the +same terms. Its baseline half has to be a room with **no** skills in it, so +the runner's own are not noise there — they are the measurement. Either set +one of those variables, or use `--engine claude-code`, whose guest has no +user-level skills at all. + ## Run it from your CI Simply add an `evals.json` file to your skill and a workflow that points to our reusable workflow. This will triger all graders to run directly from your repo. diff --git a/docs/usage.md b/docs/usage.md index dbcc088..15496a3 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -260,13 +260,25 @@ installed and once with nothing installed, and reports which ones moved. skillscope staleness --skill my-skill ``` +On the default engine this needs `ANTHROPIC_API_KEY` or `ANTHROPIC_AUTH_TOKEN` +in the environment and refuses to start without one, the same way `routing` +does and for a stronger version of the same reason: the key is what lets the +run redirect the CLI away from the runner's own config dir, and the baseline +half has to be a room with no skills in it. Without it, every skill installed +on the machine joins the baseline, and a case one of them solves there is +reported as a case the skill under test was not needed for. A plain `claude +login` is enough for `behavioral` and is not enough here. `--engine +claude-code` needs no key for this, since its guest has no user-level skills +at all. + | Verdict | Means | | --- | --- | | `earns_it` | passed with the skill, failed without it — the case discriminates | | `harmful` | failed with the skill, passed without it — the skill steered the agent wrong | | `no_signal` | passed either way — says nothing about the skill | | `fails_either_way` | failed both times — a broken case, or a hard one | -| `ungraded` | a run errored on one side or both | +| `guards_only` | asserts nothing the skill could be credited for — see the guard rails below | +| `ungraded` | a run errored on one side or both, or its baseline was not empty | A verdict says what changed, not why. So the graded side also records whether the agent *reached for* the skill, using the same detector the routing leg @@ -283,9 +295,10 @@ name: either the skill has stopped mattering, or the case was never evidence about it in the first place. The second is the more common, which makes this a check on the dataset as much as on the skill. -Both halves write which side they were, and whose skill they were about, into -their inspect log metadata (`skillscope_baseline`, `skillscope_skill`). So a -finished run can be looked at again without paying for it twice: +Both halves write which side they were, whose skill they were about, and which +invocation wrote them into their inspect log metadata (`skillscope_baseline`, +`skillscope_skill`, `skillscope_staleness_run`). So a finished run can be +looked at again without paying for it twice: ```bash skillscope staleness --from-logs .skillscope/logs @@ -293,8 +306,21 @@ skillscope staleness --from-logs .skillscope/logs That reaches no model and runs no preflight, having nothing to spend. +All three keys are load-bearing, and the run id is the one that makes this +safe to point at `.skillscope/logs`. That directory is shared with `behavioral` +and `routing`, is gitignored wholesale, and nothing prunes it — and every +ordinary `behavioral` run writes the same `skillscope_baseline: false` a graded +half does. Pairing on the skill alone would therefore splice an unrelated run +from last month in as this comparison's "with skill" side, under a table that +reads exactly as confidently as a real one. Logs with no run id are not a half +of anything and are passed over, so a directory of plain `behavioral` logs +rebuilds as nothing rather than as a comparison. The model and effort are not +reported for a rebuild: nothing reads them off the logs, and the logs need not +agree on them. + Budget more wall clock than a `behavioral` run of the same cases, and more -than double it. Both passes share one `--timeout`, and the baseline is +than double it. `--timeout` defaults to twice what the single-pass commands +use for that reason. Both passes share one `--timeout`, and the baseline is reliably the slower half: the agent holding the skill goes straight to the work, while the one without it explores. On a two-case skill the graded pass took 2m23s and the baseline had not finished its second case half an hour @@ -341,7 +367,9 @@ The host leg cannot read the CLI's session-init event, so it has no way to notice such contamination or report it -- and therefore refuses to run a routing leg at all unless `ANTHROPIC_API_KEY` or `ANTHROPIC_AUTH_TOKEN` is set, which is what lets it redirect the CLI's config dir away from the -runner's own. Refusing beats being +runner's own. It refuses a `staleness` run on the same terms and for a +stronger version of the same reason: there the empty room is not one input to +the measurement, it is the measurement. Refusing beats being quietly wrong about every case. Both legs stop at the decision, by different routes. `claude-code`'s calls diff --git a/skillscope/cli.py b/skillscope/cli.py index c252465..3c56025 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -70,6 +70,7 @@ import sys import tempfile import time +import uuid from pathlib import Path from . import ( @@ -442,9 +443,11 @@ def _require_hook_support(engine: str, skills: list[str], command: str) -> None: run perfectly well. Routing is exempt on every engine: it installs the skills, asks which one - fires, and executes nothing. + fires, and executes nothing. Staleness is not: it drives the same + `build_task` on the same cases, twice, so a `check` it skipped is skipped + on both halves and the comparison is between two runs that both lied. """ - if command != "behavioral": + if command not in ("behavioral", "staleness"): return from .engine import hooks as engine_hooks @@ -697,6 +700,7 @@ def _run_behavioral( *, install: bool = True, config_dir: Path | None = None, + run_key: str | None = None, ) -> list: """Run the behavioral cases on the selected engine. @@ -704,6 +708,10 @@ def _run_behavioral( of `cmd_staleness`. Both halves go through here rather than through two similar call sites, so the only difference between the run being measured and the run it is measured against is the flag. + + `run_key` is stamped on the logs both halves write, so `--from-logs` can + find this run's two halves and only this run's. Set by `cmd_staleness` + alone -- see `engine/behavioral.py:STALENESS_RUN_KEY`. """ from .engine import behavioral as inspect_behavioral from .engine import models as engine_models @@ -717,6 +725,7 @@ def _run_behavioral( engine_models.resolve(args.model), args.effort, install=install, + run_key=run_key, ) if args.engine == "claude-code-no-sandbox": # The real CLI, driven on the host, inside inspect's framework. @@ -734,6 +743,7 @@ def _run_behavioral( args.model, args.effort, d, config_dir, install=install ), baseline=not install, + run_key=run_key, ) # Not a fallthrough. An engine in ENGINES with no branch here is a # bug this dispatch has had once already, and a silent default is @@ -785,13 +795,6 @@ def cmd_behavioral(args: argparse.Namespace) -> int: return 0 -def engine_behavioral_keys() -> tuple[str, str]: - """The metadata keys both engines stamp on a staleness log.""" - from .engine import behavioral - - return behavioral.BASELINE_KEY, behavioral.SKILL_KEY - - def cmd_staleness(args: argparse.Namespace) -> int: """Run every behavioral case twice, with the skill and without it. @@ -805,7 +808,11 @@ def cmd_staleness(args: argparse.Namespace) -> int: installs, reading around the problem. Measured on a two-case skill: the graded pass finished both cases in 2m23s and the baseline was still going on the second after 30 minutes. Twice a `behavioral` bound is the floor, - not the figure. + not the figure -- which is why this parser defaults `--timeout` to twice + what the single-pass commands use. On the shared 900s default those + numbers left the slower half ~637s of a measured 1800s+ need, so + truncation was the expected outcome on this command's own example rather + than a risk it ran. Always exits 0 on a completed run. The comparison is nondeterministic on both sides, so a verdict here is somewhere to look rather than a build @@ -821,24 +828,27 @@ def cmd_staleness(args: argparse.Namespace) -> int: # reasons that cannot affect its answer. comparisons = staleness.from_logs(args.from_logs) if not comparisons: - baseline_key, _ = engine_behavioral_keys() + from .engine import behavioral as engine_behavioral + raise SystemExit( f"error: no skillscope staleness logs in {args.from_logs}. " - f"Both halves of a comparison carry `{baseline_key}` in their " - "eval metadata; logs written before that, or by another tool, " - "cannot be paired." + f"Both halves of a comparison carry `{engine_behavioral.SKILL_KEY}`, " + f"`{engine_behavioral.BASELINE_KEY}` and " + f"`{engine_behavioral.STALENESS_RUN_KEY}` in their eval " + "metadata; logs written before that, by another tool, or by a " + "plain `behavioral` run cannot be paired." ) + # No `model`, `engine` or `effort`. Nothing here read them off the + # logs, and `args` holds this invocation's defaults -- which describe + # the command that did the rebuilding, not the runs being rebuilt. The + # report prints what it is given, so passing them through made it + # assert a model the measurement was never taken on. summary = staleness.summarize( comparisons, { - "model": args.model, - "engine": args.engine, - "effort": args.effort, "skills": sorted({c.skill for c in comparisons}), - "extended": args.extended, "from_logs": args.from_logs, "wall_time_s": round(time.time() - started, 1), - "timeout": args.timeout, "github_run_id": os.environ.get("GITHUB_RUN_ID"), }, ) @@ -875,12 +885,22 @@ def cmd_staleness(args: argparse.Namespace) -> int: "guest has no user-level skills at all." ) + # One token for this invocation, stamped on every log both halves write. + # `--from-logs` has nothing else to pair on: the log directory is shared + # and never pruned, and a plain `behavioral` run stamps the same + # `BASELINE_KEY: False` the graded half does, so without this an unrelated + # old run is a candidate for this comparison's "with skill" side. + run_key = uuid.uuid4().hex + print(f"[staleness] {len(gradable)} case(s), run twice", flush=True) with tempfile.TemporaryDirectory(prefix="skillscope-staleness-") as config_dir: isolated = Path(config_dir) if args.engine == "claude-code-no-sandbox" else None - with_skill = _run_behavioral(args, skills, gradable, config_dir=isolated) + with_skill = _run_behavioral( + args, skills, gradable, config_dir=isolated, run_key=run_key + ) without = _run_behavioral( - args, skills, gradable, install=False, config_dir=isolated + args, skills, gradable, install=False, config_dir=isolated, + run_key=run_key, ) comparisons = staleness.pair(with_skill, without) @@ -1224,22 +1244,35 @@ def build_parser() -> argparse.ArgumentParser: "Run every behavioral case twice -- once with the skill installed, " "once with nothing installed -- and report which cases moved. A " "case that passes either way is not evidence about the skill. " - "Reports only: it never fails a run. It costs twice what " - "`behavioral` costs, and wants more than twice the --timeout: " - "both passes share one bound, and the baseline is the slower " - "half because an agent without the skill explores instead of " - "going straight to the work." + "Reports only: a verdict here never fails a run, though an " + "expired deadline or a refused run still exits non-zero. It costs " + "twice what `behavioral` costs and wants more than twice the " + "--timeout, which is why --timeout defaults to twice the usual " + "here: both passes share one bound, and the baseline is the " + "slower half because an agent without the skill explores instead " + "of going straight to the work." ), ) _add_graded_arguments(staleness_parser) + # Twice the shared bound, because the command is two passes under one. + # `_add_timeout_argument` is shared with the commands that run a single + # pass, so the default is overridden here rather than parameterised there; + # argparse's `set_defaults` wins over an `add_argument` default. The + # measurement behind the number is in `cmd_staleness`: a two-case skill + # finished the graded pass in 2m23s and had not finished the baseline + # after 30 minutes, so the default 900s gave the slower half ~637s. + staleness_parser.set_defaults(timeout=2 * deadline.DEFAULT_TIMEOUT_S) staleness_parser.add_argument( "--from-logs", metavar="DIR", help=( "Rebuild the comparison from logs already on disk instead of " - "running anything. Both halves carry the skill and which side " - "they were in their eval metadata, so a log directory is enough " - "to pair them. Costs nothing and reaches no model." + "running anything. Both halves carry the skill, which side they " + "were, and the run that wrote them in their eval metadata, so a " + "log directory is enough to pair them -- and to tell this run's " + "halves from a neighbouring run of the same skill. Costs nothing " + "and reaches no model. Logs written by a plain `behavioral` run " + "are not a half of anything and are passed over." ), ) staleness_parser.set_defaults(handler=cmd_staleness) diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index d4ab447..c58f133 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -87,6 +87,7 @@ def build_task( *, solver_factory, baseline: bool = False, + run_key: str | None = None, ): """One inspect `Task` per skill: its cases, its skill installed, its scorer. @@ -118,7 +119,11 @@ def build_task( bound = deadline.active() return Task( name=f"behavioral-{skill}" + ("-baseline" if baseline else ""), - metadata={BASELINE_KEY: baseline, SKILL_KEY: skill}, + metadata={ + BASELINE_KEY: baseline, + SKILL_KEY: skill, + **({STALENESS_RUN_KEY: run_key} if run_key else {}), + }, tags=["skillscope", "baseline" if baseline else "graded"], dataset=samples, setup=hooks.setup_solver(hook, skill, cases), @@ -147,6 +152,105 @@ def _failed(skill: str, cases: list[Case], detail: str) -> list[BehaviorOutcome] ] +def _normalise(text: str) -> str: + """Lowercased with separators flattened, so one comparison covers both OSes.""" + return text.replace("\\", "/").lower() + + +def _skill_marks(skill: str) -> tuple[tuple[str, ...], str]: + """`(absolute roots, staged suffix)` -- what reading makes a room dirty. + + The root is the skill's own directory, which is where an agent that went + looking for it would find it. It is resolved and matched as a prefix, + because a relative mention proves nothing: an agent's own `skills/pdf` + scratch directory is not ours, and only the absolute path tells them apart. + + The staged copy is the second route. `no_sandbox.install_skill` writes it to + `/.claude/skills/`, and the workspace is the agent's and + changes per sample, so the leading part is unknowable here. What is left + is a suffix specific enough to stand on its own: a path containing + `/.claude/skills/` is the staging area whoever created it. + """ + roots = [] + try: + roots.append(_normalise(str(Path(config.active().skill_path(skill)).resolve()))) + except (Exception, SystemExit): # noqa: BLE001 -- see below + # A skill with no directory on this machine leaves the staged suffix + # to carry the check on its own, which is the honest answer rather + # than a degraded one: nothing can be read out of a directory that is + # not there. `skill_path` raises `SystemExit` rather than returning, + # so catching `Exception` alone let a rebuild of someone else's logs + # take the whole command down. + pass + return tuple(roots), _normalise(f"/.claude/skills/{skill}") + + +def _under(path: str, marks: tuple[tuple[str, ...], str]) -> bool: + """Whether this already-normalised path is the skill's directory or inside it.""" + roots, staged = marks + if any(path == root or path.startswith(root + "/") for root in roots): + return True + return path.endswith(staged) or staged + "/" in path + + +# Tool arguments that name a file rather than describe one. `detect_activation` +# has the same list for its own purposes; this one is about reading a path out +# of a structured call, so it stays here rather than being shared and bent. +PATH_FIELDS = ("file_path", "path", "notebook_path", "filename", "target_file") + +# Arguments that carry a shell command rather than a path. These are the only +# ones substring-matched, because there is no field to resolve: the path is +# somewhere inside a string the agent composed. +COMMAND_FIELDS = ("command", "cmd", "script") + + +def _read_the_skill(call, marks: tuple[tuple[str, ...], str], skill: str) -> str | None: + """Whether this call reached into the skill's own directory. + + Structured path arguments are resolved and tested for containment, rather + than substring-matched on the skill's name. A bare name is a legal skill + name -- `structure.py` allows a single word and Anthropic ship `pdf`, + `docx`, `pptx` -- so matching `/{skill}/` against a serialised argument + blob reported `/run/user/1000` as contamination for a skill named `run`, + and the cost of that false positive is the highest there is here: + `_voided_if_dirty` converts the two verdicts this command exists to + surface into `ungraded`, and the report states the contamination as fact. + + Shell commands keep substring matching, because there is no field to + resolve -- but against the resolved absolute path, not the bare name. + """ + arguments = getattr(call, "arguments", None) or {} + if not isinstance(arguments, dict): + return None + + for field in PATH_FIELDS: + value = arguments.get(field) + if not isinstance(value, str) or not value: + continue + try: + resolved = _normalise(str(Path(value).resolve())) + except Exception: # noqa: BLE001 -- an unparseable path named nothing + continue + if _under(resolved, marks): + return f"read the skill's own files ({skill})" + + roots, staged = marks + for field in COMMAND_FIELDS: + value = arguments.get(field) + if not isinstance(value, str): + continue + text = _normalise(value) + if staged in text or any(root in text for root in roots): + return f"read the skill's own files ({skill})" + + # The one mark that is not a path. `_bundled_skills` is the harness's own + # staging directory, so its name in any argument is the agent having found + # the staging area rather than a file of its own. + if "_bundled_skills" in _normalise(json.dumps(arguments, ensure_ascii=False)): + return f"read the skill's own files ({skill})" + return None + + def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | None]: """`(the skill that fired, why the room was not what it claimed)`. @@ -157,17 +261,26 @@ def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | the staged ones, and a behavioral run passes no `config_dir` to keep them out. - Reading the skill's own source is the second route, and the one isolation - cannot close: the command runs from the repo root, so a directory the - staging step skipped is still on disk for an agent to find. A baseline - that read it was not a baseline. + `fired` is `None` or exactly `skill`, never an `other:` marker. The two + questions are answered by separate branches for that reason: when they + shared one `if/elif`, a second `other:` hit arriving after `why` was + already set fell through to the `elif` and was recorded as the skill + having fired -- which reads downstream as "the skill activated" on exactly + the contaminated cases that most need the opposite. - Best effort, by the same argument as `_activated`: a shape this cannot - walk is an unknown room, not a failed case. + Reading the skill's own source is the second route, and the one isolation + only partly closes: `no_sandbox` runs the agent with `cwd=workspace`, but + nothing stops it naming an absolute path, so a directory the staging step + skipped is still on disk for it to find. The sandboxed `claude-code` leg + does close it, by isolating the filesystem. A baseline that read it was + not a baseline. + + Best effort: a shape this cannot walk is an unknown room, not a failed + case. """ fired: str | None = None why: str | None = None - marks = (f"skills/{skill}", f"/{skill}/", "_bundled_skills") + marks = _skill_marks(skill) try: for call in routing_engine._tool_calls(sample): hit = routing_core.detect_activation( @@ -175,46 +288,18 @@ def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | [skill], allow_body_path=routing_engine.ALLOW_BODY_PATH, ) - if hit and hit.startswith("other:") and why is None: - why = f"another skill fired: {hit.removeprefix('other:')}" + if hit and hit.startswith("other:"): + if why is None: + why = f"another skill fired: {hit.removeprefix('other:')}" elif hit and fired is None: fired = hit if why is None: - text = json.dumps(getattr(call, "arguments", None) or {}, ensure_ascii=False) - if any(mark in text for mark in marks): - why = f"read the skill's own files ({skill})" + why = _read_the_skill(call, marks, skill) except Exception: # noqa: BLE001 -- see the docstring return fired, why return fired, why -def _activated(routing_engine, sample, skill: str) -> str | None: - """The skill the agent reached for in this sample, if any. - - Graded by `routing.detect_activation`, the same rule and the same tool-name - matching the routing leg uses, so "fired" means one thing across the two - commands. A baseline sample has no skill installed and so returns None, - which is the point: it is what makes "the skill fired and the outcome - differed" sayable at all. - - Best effort. A log shape this cannot walk leaves the field None rather - than failing a case that was otherwise graded fine -- an unknown activation - is a weaker report, not a broken run. - """ - try: - for call in routing_engine._tool_calls(sample): - hit = routing_core.detect_activation( - routing_engine._activation_event(call), - [skill], - allow_body_path=routing_engine.ALLOW_BODY_PATH, - ) - if hit: - return hit - except Exception: # noqa: BLE001 -- see the docstring - return None - return None - - def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: """Map one inspect `EvalLog` back onto skillscope's outcome objects. @@ -300,6 +385,19 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: BASELINE_KEY = "skillscope_baseline" SKILL_KEY = "skillscope_skill" +# Which staleness invocation wrote this log. Only `cmd_staleness` sets it, and +# it mints one value per invocation, so two logs carrying the same value are +# the two halves of one comparison and nothing else is. +# +# `--from-logs` needs this and cannot be built without it. The log directory is +# shared by `behavioral`, `verify` and `routing`, is gitignored wholesale and +# is never pruned, and every ordinary `behavioral` run stamps +# `BASELINE_KEY: False` -- so grouping on `(skill, baseline)` alone splices an +# unrelated month-old run in as the "with skill" half, under a table that reads +# as confidently as a real one. `run_eval` reached the same conclusion for the +# same directory and solved it the same way, with `CALL_TOKEN`. +STALENESS_RUN_KEY = "skillscope_staleness_run" + def stop_requested() -> bool: """Whether the deadline's graceful stage has asked the command to stop.""" @@ -386,12 +484,17 @@ def run( solver_factory, log_dir: str | None = None, baseline: bool = False, + run_key: str | None = None, ) -> list[BehaviorOutcome]: """Run every behavioral case, grouped by skill. Mirrors `behavior.run`. `baseline` is passed through to the task and used in the progress lines. It does not itself remove the skill: the caller's `solver_factory` does that, and this says so in the output. + + `run_key` is passed through to the task's metadata. See + `STALENESS_RUN_KEY`: only a staleness run sets one, and it is what lets + `--from-logs` pair two halves without pairing two unrelated runs. """ from inspect_ai import eval as inspect_eval @@ -421,6 +524,7 @@ def run( model, solver_factory=solver_factory, baseline=baseline, + run_key=run_key, ), model=model, model_args=models.model_args(model), diff --git a/skillscope/engine/verify.py b/skillscope/engine/verify.py index 63bebad..cb70182 100644 --- a/skillscope/engine/verify.py +++ b/skillscope/engine/verify.py @@ -219,6 +219,7 @@ def build_task( ctx: dict | None = None, effort: str | None = None, install: bool = True, + run_key: str | None = None, ): """One task per skill, solved by real Claude Code rather than our agent. @@ -253,6 +254,11 @@ def build_task( metadata={ behavioral.BASELINE_KEY: not install, behavioral.SKILL_KEY: skill, + # See `behavioral.STALENESS_RUN_KEY`: set only by a staleness run, + # and only there because only there are two logs meant to pair. + **( + {behavioral.STALENESS_RUN_KEY: run_key} if run_key else {} + ), }, tags=["skillscope", "graded" if install else "baseline"], setup=hooks.setup_solver(hook, skill, cases), @@ -291,8 +297,13 @@ def run( effort: str, *, install: bool = True, + run_key: str | None = None, ) -> list[BehaviorOutcome]: - """Mirrors `behavior.run`, so the CLI and the benchmark treat it the same.""" + """Mirrors `behavior.run`, so the CLI and the benchmark treat it the same. + + `run_key` is stamped on the task, same as the other engine's. See + `behavioral.STALENESS_RUN_KEY`. + """ from inspect_ai import eval as inspect_eval require() @@ -316,7 +327,10 @@ def run( print(f"[{label}] {skill}: {len(skill_cases)} case(s)", flush=True) logs = behavioral.run_eval( inspect_eval, - build_task(skill, skill_cases, model, effort=effort, install=install), + build_task( + skill, skill_cases, model, effort=effort, + install=install, run_key=run_key, + ), model=model, model_args=models.model_args(model), log_dir=str(Path(".skillscope") / "logs"), diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 294bb7a..a84f657 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -40,7 +40,7 @@ from __future__ import annotations -from dataclasses import asdict, dataclass, field +from dataclasses import asdict, dataclass, field, replace from .behavior import BehaviorOutcome @@ -223,8 +223,8 @@ def pair( return comparisons -def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: - """`(skill, was_baseline, outcomes)` for one inspect log. +def outcomes_from_log(log) -> tuple[str, bool, str | None, list[BehaviorOutcome]]: + """`(skill, was_baseline, run_key, outcomes)` for one inspect log. Rebuilt from the samples rather than from a dataset, because an offline reader has no `Case` list: the log is all there is. Prompts are left empty @@ -242,6 +242,15 @@ def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: for score in (sample.scores or {}).values(): checks.extend((score.metadata or {}).get(scorers.CHECKS, [])) error = sample.error.message if sample.error is not None else None + if error is None and not checks: + # Exactly what the live path sets (`behavioral._outcomes`), and it + # has to be the same string for the same reason the detector is + # shared: a sample whose checks could not be read rebuilds as + # `checks=[]`, which makes `_met()` False, which makes the case + # `earns_it` off the back of a harness failure. Without this the + # rebuild is *more permissive than the run it rebuilds*, quietly + # restoring a verdict the live run had voided as `ungraded`. + error = "case has no behavioral assertions to grade" outcomes.append( BehaviorOutcome( id=str(sample.id), @@ -263,17 +272,33 @@ def outcomes_from_log(log) -> tuple[str, bool, list[BehaviorOutcome]]: ), ) ) - return skill, bool(eval_meta.get(behavioral.BASELINE_KEY)), outcomes + return ( + skill, + bool(eval_meta.get(behavioral.BASELINE_KEY)), + eval_meta.get(behavioral.STALENESS_RUN_KEY), + outcomes, + ) def from_logs(log_dir: str) -> list[Comparison]: """Rebuild the comparison from logs already on disk. - Two runs become a pair because they carry the same skill and opposite - `BASELINE_KEY`, which is why those are written as metadata rather than - inferred from a task name. A directory holding only one side of a skill is - reported as a comparison against nothing, the same way a case missing from - one side is: silently dropping it would read as a smaller, cleaner run. + Two runs become a pair because they carry the same skill, the same + `STALENESS_RUN_KEY`, and opposite `BASELINE_KEY` -- which is why all three + are written as metadata rather than inferred from a task name. + + Grouping on the run key is what makes this safe to point at + `.skillscope/logs`, which is the documented thing to do with it. That + directory is shared by `behavioral`, `verify` and `routing`, is gitignored + wholesale, and nothing prunes it; and every ordinary `behavioral` run + stamps `BASELINE_KEY: False`, so a run key is the only thing separating + this run's graded half from an unrelated run of the same skill last month. + Logs with no run key are skipped for that reason: they were not written by + a staleness run and have no half to be. + + A directory holding only one side of a run is reported as a comparison + against nothing, the same way a case missing from one side is: silently + dropping it would read as a smaller, cleaner run. Logs that are not skillscope's own are passed over. A shared log directory is normal, and failing on a neighbour's file would make this unusable in @@ -283,20 +308,57 @@ def from_logs(log_dir: str) -> list[Comparison]: from .engine import behavioral - sides: dict[str, dict[bool, list[BehaviorOutcome]]] = {} + sides: dict[tuple[str, str], dict[bool, list[BehaviorOutcome]]] = {} for info in list_eval_logs(log_dir): log = read_eval_log(info.name) if behavioral.BASELINE_KEY not in (log.eval.metadata or {}): continue - skill, baseline, outcomes = outcomes_from_log(log) - sides.setdefault(skill, {}).setdefault(baseline, []).extend(outcomes) + skill, baseline, run_key, outcomes = outcomes_from_log(log) + if not run_key: + continue + sides.setdefault((skill, run_key), {}).setdefault(baseline, []).extend(outcomes) comparisons: list[Comparison] = [] - for skill in sorted(sides): - comparisons.extend(pair(sides[skill].get(False, []), sides[skill].get(True, []))) + for key in sorted(sides): + half = sides[key] + comparisons.extend( + pair( + _no_duplicates(half.get(False, [])), + _no_duplicates(half.get(True, [])), + ) + ) return comparisons +def _no_duplicates(outcomes: list[BehaviorOutcome]) -> list[BehaviorOutcome]: + """`outcomes`, with every case id that appears twice on this side errored. + + One run key and one side can still span several logs -- a run grades one + skill per task, and a run key covers the whole invocation -- and nothing + stops two of them carrying the same case id. `pair` builds + `{o.id: o for o in ...}`, so a duplicate silently collapses to whichever + came last, and `list_eval_logs` returns newest first, which makes the + survivor the *older* of the two. + + Both copies are errored, not just the later one: which of them is this + side's answer is exactly what cannot be told, so keeping either would be + the guess. `classify` turns the error into `ungraded`, consistent with how + this module reports every other ambiguity it cannot resolve. + """ + tally: dict[str, int] = {} + for outcome in outcomes: + tally[outcome.id] = tally.get(outcome.id, 0) + 1 + return [ + outcome if tally[outcome.id] == 1 + else replace( + outcome, + error="this run has more than one log reporting this case, and " + "which of them is its answer cannot be told apart", + ) + for outcome in outcomes + ] + + def summarize(comparisons: list[Comparison], meta: dict) -> dict: counts = {key: 0 for key, _ in VERDICTS} for comparison in comparisons: @@ -395,13 +457,30 @@ def render_markdown(summary: dict) -> str: else "**Nothing was graded both ways.**" ) + # Said only when it is known. A `--from-logs` rebuild runs no model, and + # the invocation's own `--model`/`--effort` defaults describe the command + # that read the logs rather than the runs that wrote them -- rebuilding a + # `sonnet`/`low` run with a bare `--from-logs` printed "on `opus` (effort + # `high`)", which is not a hedge but a false statement about a measurement. + ran_on = ( + f" on `{meta['model']}` (effort `{meta['effort']}`)" + if meta.get("model") and meta.get("effort") + else "" + ) lines = [ "## Skill staleness", "", - f"{headline} on `{meta['model']}` (effort `{meta['effort']}`). Every case " + f"{headline}{ran_on}. Every case " "ran twice: once with the skill installed, once with nothing installed.", "", ] + if meta.get("from_logs"): + lines += [ + f"Rebuilt offline from the logs in `{meta['from_logs']}`; nothing " + "was run. The model and effort are whatever wrote them, which the " + "logs do not have to agree on, so they are not claimed here.", + "", + ] # Nothing discriminated. The row `earns_it | 0` is true and says nothing, # and this is the one result the command exists to surface, so it is said diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 7b1f587..24bbe8d 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6071,6 +6071,67 @@ def test_a_case_that_needed_nothing_is_named_with_what_to_do(self) -> None: self.assertIn("unaided", report) +@contextlib.contextmanager +def _staleness_harness(test, *, dataset=None): + """A repo `cmd_staleness` can be run against, with the two halves recorded. + + The command was never called by a test before this: every assertion about + it read its source. A regression that called `_run_behavioral` twice with + the same `install` -- which destroys the entire measurement, since the two + halves become the same run -- passed the whole suite. + + `_run_behavioral` is the seam, so what each half was asked for is the + thing recorded. Everything below it reaches a provider. + """ + repo = Repo(test) + repo.skill("alpha", dataset=dataset or _behavioral_dataset("alpha")) + repo.activate() + + calls: list[dict] = [] + + def record(args, skills, gradable, *, install=True, config_dir=None, run_key=None): + calls.append( + { + "install": install, + "config_dir": config_dir, + "run_key": run_key, + "cases": [c.id for c in gradable], + } + ) + return [] + + harness = types.SimpleNamespace(repo=repo, calls=calls) + + def run(engine="claude-code", extra=None): + argv = [ + "staleness", "--engine", engine, "--skip-preflight", + "--model", "mockllm/model", *(extra or []), + ] + args = cli.build_parser().parse_args(argv) + return cli.cmd_staleness(args) + + harness.run = run + + with mock.patch.object(cli, "_run_behavioral", record), \ + mock.patch.object(cli, "_write_report", lambda *a, **k: None), \ + mock.patch.object(cli, "_prepare_graded_run", lambda *a, **k: ["alpha"]): + yield harness + + +def _behavioral_dataset(slug: str) -> dict: + """`tier0_dataset`, with every triggering case asserting a behaviour. + + `_gradable_cases` keeps only cases with something to grade, so a routing + dataset leaves `cmd_staleness` with nothing to run and it returns before + reaching either half. + """ + data = tier0_dataset(slug) + for evaluation in data[EVALUATIONS_KEY]: + if evaluation[TRIGGER_KEY]: + evaluation["expected_behavior"] = ["do the thing"] + return data + + def _checked(case_id: str, passed: bool, checks: list[dict], skill: str = "s"): """A behavioral outcome carrying graded checks.""" return behavior.BehaviorOutcome( @@ -6149,13 +6210,15 @@ def __init__(self, sample_id, checks, error=None): class _StubLog: - def __init__(self, skill, baseline, samples): + def __init__(self, skill, baseline, samples, run_key="run-1"): + metadata = { + engine_behavioral.SKILL_KEY: skill, + engine_behavioral.BASELINE_KEY: baseline, + } + if run_key is not None: + metadata[engine_behavioral.STALENESS_RUN_KEY] = run_key self.eval = types.SimpleNamespace( - task=f"behavioral-{skill}", - metadata={ - engine_behavioral.SKILL_KEY: skill, - engine_behavioral.BASELINE_KEY: baseline, - }, + task=f"behavioral-{skill}", metadata=metadata ) self.samples = samples @@ -6275,9 +6338,25 @@ def test_the_reason_reaches_the_report(self) -> None: def test_the_command_refuses_an_unisolatable_host_run(self) -> None: # Routing has always refused this; staleness needs it more, because # the baseline being empty is the measurement rather than an input. - source = inspect.getsource(cli.cmd_staleness) - self.assertIn("can_isolate_config", source) - self.assertIn("claude-code-no-sandbox", source) + # + # Run, not read. The version of this that grepped `cmd_staleness` for + # "can_isolate_config" passed just as happily with the `not` dropped + # from the guard, which is the one edit that turns the refusal into a + # run that reports a contaminated baseline as a clean one. + with _staleness_harness(self) as harness: + with mock.patch.object(cli.routing, "can_isolate_config", lambda: False): + with self.assertRaises(SystemExit) as caught: + harness.run(engine="claude-code-no-sandbox") + self.assertIn("claude-code-no-sandbox", str(caught.exception)) + self.assertEqual(harness.calls, []) + + def test_an_isolatable_host_run_is_allowed_through(self) -> None: + # The other half of the guard. Without this the test above passes on a + # command that refuses everything. + with _staleness_harness(self) as harness: + with mock.patch.object(cli.routing, "can_isolate_config", lambda: True): + harness.run(engine="claude-code-no-sandbox") + self.assertEqual(len(harness.calls), 2) class TestASkillThatChangedNothingIsToldPlainly(unittest.TestCase): @@ -6373,7 +6452,7 @@ class TestAComparisonCanBeRebuiltFromLogs(unittest.TestCase): def test_a_log_reports_which_side_it_was(self) -> None: log = _StubLog("alpha", True, [_StubSample("c", [_check("do it", False)])]) - skill, baseline, outcomes = staleness.outcomes_from_log(log) + skill, baseline, _, outcomes = staleness.outcomes_from_log(log) self.assertEqual(skill, "alpha") self.assertTrue(baseline) self.assertEqual(len(outcomes), 1) @@ -6384,31 +6463,50 @@ def test_a_sample_passes_only_when_every_check_did(self) -> None: _StubSample("c", [_check("a", True), _check("b", True)]), _StubSample("d", [_check("a", True), _check("b", False)]), ]) - _, _, outcomes = staleness.outcomes_from_log(log) + _, _, _, outcomes = staleness.outcomes_from_log(log) self.assertEqual([o.passed for o in outcomes], [True, False]) def test_a_sample_with_no_checks_has_not_passed(self) -> None: # An ungraded sample is not a green one. log = _StubLog("alpha", False, [_StubSample("c", [])]) - _, _, outcomes = staleness.outcomes_from_log(log) + _, _, _, outcomes = staleness.outcomes_from_log(log) self.assertFalse(outcomes[0].passed) def test_a_rebuild_is_no_more_permissive_than_the_run_it_rebuilds(self) -> None: - # The rebuild read activation but not contamination, so a verdict the - # live run voided came back `no_signal` on re-score. A report that - # disagrees with the run it was built from is worse than no report. - import inspect as _inspect + # A sample whose checks could not be read is ungraded on the live path + # (`behavioral._outcomes` sets an error for it). Rebuilt without that, + # it comes back as `checks=[]`, which makes `_met()` False, which reads + # as the baseline having failed -- so a harness failure is promoted to + # `earns_it` and inflates the headline number. + # + # Asserted on the verdict, not on the source. The version of this that + # grepped `outcomes_from_log` for "_inspect_room" and "contaminated" + # passed for the whole life of the defect it was named after. + graded = _StubLog("alpha", False, [_StubSample("c", [_check("do it", True)])]) + unreadable = _StubLog("alpha", True, [_StubSample("c", [])]) + _, _, _, with_skill = staleness.outcomes_from_log(graded) + _, _, _, without = staleness.outcomes_from_log(unreadable) + + self.assertEqual( + without[0].error, "case has no behavioral assertions to grade" + ) + self.assertEqual(staleness.classify(with_skill[0], without[0]), "ungraded") - source = _inspect.getsource(staleness.outcomes_from_log) - self.assertIn("_inspect_room", source) - self.assertIn("contaminated", source) + def test_the_live_path_and_the_rebuild_word_it_the_same_way(self) -> None: + # Two strings that have to match for the two paths to agree, and + # nothing else makes them. + live = inspect.getsource(engine_behavioral._outcomes) + rebuilt = inspect.getsource(staleness.outcomes_from_log) + message = "case has no behavioral assertions to grade" + self.assertIn(message, live) + self.assertIn(message, rebuilt) def test_the_two_sides_pair_on_the_skill_they_name(self) -> None: graded = _StubLog("alpha", False, [_StubSample("c", [_check("do it", True)])]) baseline = _StubLog("alpha", True, [_StubSample("c", [_check("do it", False)])]) sides = {} for log in (graded, baseline): - skill, is_baseline, outcomes = staleness.outcomes_from_log(log) + skill, is_baseline, _, outcomes = staleness.outcomes_from_log(log) sides.setdefault(skill, {}).setdefault(is_baseline, []).extend(outcomes) comparisons = staleness.pair(sides["alpha"][False], sides["alpha"][True]) self.assertEqual([c.verdict for c in comparisons], ["earns_it"]) @@ -6417,20 +6515,433 @@ def test_the_two_sides_pair_on_the_skill_they_name(self) -> None: class TestTheBaselineRunIsTheSameRunWithoutTheSkill(unittest.TestCase): """The flag has to reach the driver, or the two halves are not comparable.""" + def _staged(self, *, install: bool) -> list: + """Whether the solver reached `install_skill`, for one value of the flag. + + Driven rather than read. The earlier version of this asserted that the + signature had an `install` parameter defaulting to True, which says + nothing about whether the flag reaches the staging step -- and the + staging step is the entire difference between the two halves. + + The CLI call after it is cut off by giving it no output to parse, which + is the first thing the solver refuses: staging has already happened or + not by then. + """ + import asyncio + + staged: list[Path] = [] + workspace = tempfile.mkdtemp() + self.addCleanup(shutil.rmtree, workspace, ignore_errors=True) + + async def no_workspace(): + return workspace + + async def no_output(*args, **kwargs): + return types.SimpleNamespace(returncode=0, stdout="", stderr="") + + solve = engine_no_sandbox.claude_code_no_sandbox( + None, None, Path("alpha"), install=install + ) + state = types.SimpleNamespace(input_text="p", messages=[], store=None) + with mock.patch.object(engine_no_sandbox, "_workspace", no_workspace), \ + mock.patch.object( + engine_no_sandbox, "install_skill", + lambda d, w: staged.append(d), + ), \ + mock.patch("inspect_ai.util.subprocess", no_output): + with self.assertRaises(RuntimeError): + asyncio.run(solve(state, None)) + return staged + def test_the_host_driver_stages_nothing_when_asked_not_to(self) -> None: - import inspect as _inspect + self.assertEqual(self._staged(install=False), []) - signature = _inspect.signature(engine_no_sandbox.claude_code_no_sandbox) - self.assertIn("install", signature.parameters) - self.assertIs(signature.parameters["install"].default, True) + def test_the_host_driver_stages_the_skill_when_asked_to(self) -> None: + # The other half: a driver that staged nothing ever would pass the + # test above while making both halves the baseline. + self.assertEqual(self._staged(install=True), [Path("alpha")]) def test_the_task_name_says_which_half_it_is(self) -> None: # Both halves write into one log directory; a name that did not say - # would leave the two indistinguishable on disk. - import inspect as _inspect + # would leave the two indistinguishable on disk. Asserted on the names + # the two calls actually produce, rather than on the source containing + # the suffix somewhere. + repo = Repo(self) + repo.skill("alpha", dataset=tier0_dataset("alpha")) + repo.activate() + cases = [c for c in datasets.load_all_cases() if c.skill == "alpha"][:1] + + graded = engine_behavioral.build_task( + "alpha", cases, "mockllm/model", solver_factory=lambda d: None + ) + baseline = engine_behavioral.build_task( + "alpha", cases, "mockllm/model", + solver_factory=lambda d: None, baseline=True, + ) + self.assertFalse(graded.name.endswith("-baseline")) + self.assertTrue(baseline.name.endswith("-baseline")) + self.assertFalse(graded.metadata[engine_behavioral.BASELINE_KEY]) + self.assertTrue(baseline.metadata[engine_behavioral.BASELINE_KEY]) + + +class TestTheDetectorKeepsItsTwoAnswersApart(unittest.TestCase): + """`_inspect_room` answers two questions, and they must not cross. + + `fired` is `None` or exactly the skill under test -- `detect_activation` is + called with `skills=[skill]`, so there is nothing else it can legitimately + be. When the two answers shared one `if/elif`, a second `other:` hit + arriving after `why` was already set fell through to the `elif` and was + recorded as the skill having fired. + + That string then reads as truthy everywhere downstream: `activated_with` + on the `Comparison`, and `earned_without_firing`, which tests `not + c.activated_with`. So the "never activated the skill" warning was + suppressed on exactly the contaminated cases that most need it, and the + marker landed verbatim in both JSON reports. + """ + + def _sample(self, calls): + return _FakeSample(messages=[_FakeMessage("m", calls)], events=[]) + + def _call(self, name, arguments=None): + call = _FakeCall("toolu_1", function=name) + call.arguments = arguments or {} + return call + + def _inspect(self, sample, skill="alpha"): + from skillscope.engine import routing as routing_engine + + return engine_behavioral._inspect_room(routing_engine, sample, skill) + + def test_a_contamination_marker_is_never_reported_as_the_skill(self) -> None: + # Two `other:` hits. The first sets `why`; the second used to find it + # already set, fail the first condition, and land in `fired`. + sample = self._sample([ + self._call("Skill", {"command": "beta"}), + self._call("Skill", {"command": "gamma"}), + ]) + fired, why = self._inspect(sample) + self.assertIsNone(fired) + self.assertIsNotNone(why) + self.assertIn("another skill fired", why) + + def test_the_skill_under_test_is_still_reported_when_it_fires(self) -> None: + # The other half: a fix that reports nothing would pass the test above. + sample = self._sample([self._call("Skill", {"command": "alpha"})]) + fired, why = self._inspect(sample) + self.assertEqual(fired, "alpha") + self.assertIsNone(why) + + def test_a_marker_cannot_reach_the_report_as_an_activation(self) -> None: + # The consequence, asserted where it is read: a truthy garbage string + # in `activated_with` suppresses the warning this totals. + sample = self._sample([ + self._call("Skill", {"command": "beta"}), + self._call("Skill", {"command": "gamma"}), + ]) + fired, _ = self._inspect(sample) + graded = behavior.BehaviorOutcome( + id="c", skill="alpha", prompt="p", passed=True, elapsed_s=0.0, + checks=[_check("do it", True)], activated=fired, + ) + without = _checked("c", False, [_check("do it", False)], skill="alpha") + totals = staleness.summarize( + staleness.pair([graded], [without]), {"model": "m", "effort": "e"} + )["totals"] + self.assertEqual(totals["earns_it"], 1) + self.assertEqual(totals["earned_without_firing"], 1) + + +class TestContaminationIsJudgedOnTheSkillsRealPath(unittest.TestCase): + """A bare skill name is not evidence that the skill's files were read. + + `structure.py` allows a single-word name and sets no minimum length, and + single-word names are the norm -- Anthropic ship `pdf`, `docx`, `pptx`. The + heuristic matched `/{skill}/` as a substring of every tool call's + serialised arguments, so a skill named `run` was reported as contaminated + by any agent that touched `/run/user/1000`, and `docs`, `test`, `tools` or + `scripts` by any ordinary path. + + The cost of that false positive is the highest there is here: + `_voided_if_dirty` converts `no_signal` and `harmful` -- the two verdicts + the command exists to surface -- into `ungraded`, and the report states the + contamination as fact. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.skill("run", dataset=tier0_dataset("run")) + self.repo.activate() + + def _why(self, arguments, skill="run"): + from skillscope.engine import routing as routing_engine + + call = _FakeCall("toolu_1", function="Read") + call.arguments = arguments + sample = _FakeSample(messages=[_FakeMessage("m", [call])], events=[]) + return engine_behavioral._inspect_room(routing_engine, sample, skill)[1] + + def test_an_unrelated_path_sharing_the_skills_name_is_not_contamination(self) -> None: + self.assertIsNone(self._why({"file_path": "/run/user/1000/bus"})) + + def test_a_bash_command_mentioning_the_name_is_not_contamination(self) -> None: + self.assertIsNone( + self._why({"command": "ls /run/systemd && mkdir -p build/run/out"}) + ) + + def test_the_skills_own_directory_still_counts(self) -> None: + own = str(config.active().skill_path("run") / "SKILL.md") + self.assertIn("read the skill's own files", self._why({"file_path": own})) + + def test_the_staged_copy_still_counts(self) -> None: + staged = "/tmp/ws-1234/.claude/skills/run/reference.md" + self.assertIn("read the skill's own files", self._why({"file_path": staged})) + + def test_a_bash_command_naming_the_real_directory_still_counts(self) -> None: + own = str(config.active().skill_path("run")) + self.assertIn( + "read the skill's own files", self._why({"command": f"cat {own}/SKILL.md"}) + ) + + def test_the_harness_staging_directory_still_counts(self) -> None: + self.assertIn( + "read the skill's own files", + self._why({"file_path": "/opt/_bundled_skills/run/SKILL.md"}), + ) + + def test_a_skill_with_no_directory_does_not_take_the_command_down(self) -> None: + # `config.skill_path` raises `SystemExit`, which `except Exception` + # does not catch -- so rebuilding somebody else's logs killed the + # process rather than reporting what it could. + self.assertIsNone(self._why({"file_path": "/tmp/x"}, skill="not-a-skill")) + + +class TestBothHalvesRunAndOnlyTheInstallFlagDiffers(unittest.TestCase): + """The command's whole measurement is that the two runs differ in one way. + + Called rather than read. Every earlier assertion about `cmd_staleness` was + a substring check against its source, and `cli._run_behavioral` was never + invoked by a test at all. + """ + + def test_the_cases_are_run_twice(self) -> None: + with _staleness_harness(self) as harness: + self.assertEqual(harness.run(), 0) + self.assertEqual(len(harness.calls), 2) + + def test_the_two_halves_disagree_about_installing(self) -> None: + # The regression this exists for: two calls with the same `install` + # compare a run against itself and report `no_signal` for everything. + with _staleness_harness(self) as harness: + harness.run() + self.assertEqual([c["install"] for c in harness.calls], [True, False]) + + def test_both_halves_grade_the_same_cases(self) -> None: + with _staleness_harness(self) as harness: + harness.run() + self.assertEqual(harness.calls[0]["cases"], harness.calls[1]["cases"]) + self.assertTrue(harness.calls[0]["cases"]) + + def test_both_halves_carry_one_run_key(self) -> None: + # What `--from-logs` pairs on. Two different keys leave the halves + # unpairable; no key leaves them pairable with anything. + with _staleness_harness(self) as harness: + harness.run() + keys = {c["run_key"] for c in harness.calls} + self.assertEqual(len(keys), 1) + self.assertTrue(keys.pop()) + + def test_two_invocations_do_not_share_a_run_key(self) -> None: + with _staleness_harness(self) as harness: + harness.run() + harness.run() + self.assertNotEqual(harness.calls[0]["run_key"], harness.calls[2]["run_key"]) + + def test_a_dataset_with_nothing_to_grade_runs_neither_half(self) -> None: + with _staleness_harness(self, dataset=tier0_dataset("alpha")) as harness: + self.assertEqual(harness.run(), 0) + self.assertEqual(harness.calls, []) + + +class TestARebuildPairsOneRunAndNotItsNeighbours(unittest.TestCase): + """`.skillscope/logs` is shared, never pruned, and gitignored wholesale. + + Every ordinary `behavioral` run stamps `BASELINE_KEY: False`, so grouping + on `(skill, baseline)` alone made an unrelated run of the same skill a + candidate for this comparison's "with skill" half -- rendered into a table + that reads exactly as confidently as a real one. `docs/usage.md` documents + pointing `--from-logs` at that very directory. + """ + + def _rebuild(self, logs): + info = [types.SimpleNamespace(name=f"log-{i}") for i in range(len(logs))] + by_name = {i.name: log for i, log in zip(info, logs)} + with mock.patch("inspect_ai.log.list_eval_logs", lambda d: info), \ + mock.patch("inspect_ai.log.read_eval_log", lambda n: by_name[n]): + return staleness.from_logs("anywhere") + + def _log(self, baseline, passed, run_key="run-1", case="c"): + return _StubLog( + "alpha", baseline, + [_StubSample(case, [_check("do it", passed)])], + run_key=run_key, + ) + + def test_one_runs_two_halves_pair(self) -> None: + comparisons = self._rebuild([self._log(False, True), self._log(True, False)]) + self.assertEqual([c.verdict for c in comparisons], ["earns_it"]) + + def test_a_neighbouring_runs_log_is_not_spliced_in(self) -> None: + # The stale graded half says the case passed; this run's says it + # failed. Pooled, the pair reads `earns_it` off a run nobody asked + # about. Kept apart, each run is compared with itself. + comparisons = self._rebuild([ + self._log(False, True, run_key="old"), + self._log(False, False, run_key="new"), + self._log(True, False, run_key="new"), + ]) + verdicts = sorted(c.verdict for c in comparisons) + self.assertEqual(verdicts, ["fails_either_way", "ungraded"]) + + def test_a_plain_behavioral_log_is_not_a_half_of_anything(self) -> None: + # It carries `BASELINE_KEY: False` like a graded half and no run key. + comparisons = self._rebuild([self._log(False, True, run_key=None)]) + self.assertEqual(comparisons, []) + + def test_two_logs_reporting_one_case_are_reported_rather_than_resolved(self) -> None: + # `pair` builds a dict keyed by case id, so a duplicate collapses + # silently -- and `list_eval_logs` returns newest first, which makes + # the survivor the older of the two. + comparisons = self._rebuild([ + self._log(False, True), + self._log(False, False), + self._log(True, False), + ]) + self.assertEqual([c.verdict for c in comparisons], ["ungraded"]) + self.assertIn("more than one log", comparisons[0].error) + + +class TestAStalenessRunHonoursHooksTheSameWayBehavioralDoes(unittest.TestCase): + """It drives the same `build_task`, so it cannot skip what that refuses to. + + `_require_hook_support` guarded on `command != "behavioral"`, and + `_prepare_graded_run` passes `args.command` -- so under `staleness` the + guard returned before it checked anything. A skill whose `evals/hooks.py` + defines `check` or `setup_session` had them silently skipped, on both + halves, which is the exact "dangerous half" the function exists to + prevent: the case is graded as though the hook ran. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.skill( + "alpha", + dataset=tier0_dataset("alpha"), + hooks="def check(run):\n return True\n", + ) + self.repo.activate() + + def _refused(self, command): + try: + cli._require_hook_support("claude-code", ["alpha"], command) + except SystemExit as caught: + return str(caught) + return None + + def test_a_staleness_run_is_refused_the_same_as_a_behavioral_one(self) -> None: + refusal = self._refused("staleness") + self.assertIsNotNone(refusal) + self.assertIn("check", refusal) + + def test_the_behavioral_refusal_is_unchanged(self) -> None: + self.assertIsNotNone(self._refused("behavioral")) + + def test_routing_is_still_exempt(self) -> None: + # It installs the skills, asks which one fired, and executes nothing. + self.assertIsNone(self._refused("routing")) + + +class TestAnOfflineRebuildDoesNotClaimAModelItNeverRan(unittest.TestCase): + """`--from-logs` reaches no model, so it has no model to report. + + The meta dict passed `args.model` / `args.effort` straight through, and + nothing was read off the logs -- so rebuilding a run taken on `sonnet` at + `low` with a bare `--from-logs` printed "on `opus` (effort `high`)". Not a + hedge: a false statement about where a measurement came from, in the one + mode that never runs `enforce_model_policy` either. + """ + + def _report(self, meta): + comparisons = staleness.pair( + [_checked("c", True, [_check("do it", True)])], + [_checked("c", False, [_check("do it", False)])], + ) + return staleness.render_markdown(staleness.summarize(comparisons, meta)) + + def test_a_rebuild_names_no_model(self) -> None: + report = self._report({"from_logs": "/logs", "skills": ["alpha"]}) + headline = report.split("\n\n")[1] + self.assertNotIn("(effort", headline) + self.assertNotIn(" on `", headline) + self.assertIn("Rebuilt offline", report) + self.assertIn("/logs", report) + + def test_a_live_run_still_names_its_model(self) -> None: + report = self._report({"model": "sonnet", "effort": "low"}) + self.assertIn("`sonnet`", report) + self.assertIn("effort `low`", report) + + def test_the_command_passes_no_model_through_on_a_rebuild(self) -> None: + captured: dict = {} + real = staleness.summarize + + def record(comparisons, meta): + captured.update(meta) + return real(comparisons, meta) + + args = cli.build_parser().parse_args( + ["staleness", "--from-logs", "/logs", "--model", "opus"] + ) + with mock.patch.object(staleness, "summarize", record), \ + mock.patch.object(staleness, "from_logs", lambda d: staleness.pair( + [_checked("c", True, [_check("x", True)])], + [_checked("c", False, [_check("x", False)])], + )), \ + mock.patch.object(cli, "_write_report", lambda *a, **k: None): + cli.cmd_staleness(args) + self.assertNotIn("model", captured) + self.assertNotIn("effort", captured) + self.assertEqual(captured["from_logs"], "/logs") + + +class TestTheSecondPassGetsABudgetOfItsOwn(unittest.TestCase): + """Two passes under one bound, and the baseline is the slower half. + + Measured on a two-case skill: the graded pass finished both cases in + 2m23s, and the baseline had not finished the second after 30 minutes. An + agent holding the skill goes straight to the work; an agent without it + explores. On the default 900s the baseline got ~637s of a documented + 1800s+ need, which makes truncation the expected outcome on the command's + own canonical example rather than a risk. + """ + + def _timeout(self, argv): + return cli.build_parser().parse_args(argv).timeout + + def test_staleness_defaults_to_twice_the_shared_bound(self) -> None: + self.assertEqual( + self._timeout(["staleness"]), 2 * deadline.DEFAULT_TIMEOUT_S + ) + + def test_the_single_pass_commands_are_unchanged(self) -> None: + self.assertEqual( + self._timeout(["behavioral"]), deadline.DEFAULT_TIMEOUT_S + ) + self.assertEqual(self._timeout(["routing"]), deadline.DEFAULT_TIMEOUT_S) + + def test_an_explicit_timeout_still_wins(self) -> None: + self.assertEqual(self._timeout(["staleness", "--timeout", "60"]), 60.0) - source = _inspect.getsource(engine_behavioral.build_task) - self.assertIn("-baseline", source) if __name__ == "__main__": From b188c2abb1cadfc572f3e071dd50d5cf26ab3f14 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 07:59:07 +0100 Subject: [PATCH 08/15] Pair logs that predate the run id, when the pairing is forced Refusing every unlabelled log was too strong. Checked against the only staleness logs that exist anywhere -- the quark-torch-llm-ptq run this branch was developed against, still in `amd-skills/.skillscope/logs` -- the previous commit made them unreadable: `--from-logs` reported "no skillscope staleness logs" for a directory holding both halves of a real run. The one artifact anybody could check a rebuild against, refused by the fix meant to make rebuilds trustworthy. A log written before the key existed is not suspect, it is unlabelled. What the key defends against is *choosing*: a directory holding several candidates for one side, where pairing picks one silently and `list_eval_logs` ordering makes that the older. Where a skill has one log per side there is nothing to choose -- the pairing is forced, and a label would not change which logs pair. So unlabelled logs group together and pair when each side has exactly one. Where a side has more, every case in that group is `ungraded`, because the ambiguity is the hazard itself. A labelled run never falls into the bucket, so a run written by this version cannot be weakened by it. That real run now rebuilds: one `no_signal` on a clean baseline, and one `ungraded` for the case the baseline never reached, which is the half-hour timeout the previous commit's message describes. Also checked the rewritten detector against those logs' real tool calls, which is the first time it has run on any. It agrees with the old one on all three samples: the baseline is clean, and both graded samples read the skill's own files, which is what the graded side is supposed to do. Signed-off-by: John Lybeck --- skillscope/staleness.py | 57 +++++++++++++++++++++++++++++++++------- tests/test_skillscope.py | 40 +++++++++++++++++++++++++--- 2 files changed, 85 insertions(+), 12 deletions(-) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index a84f657..fb51610 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -293,8 +293,8 @@ def from_logs(log_dir: str) -> list[Comparison]: wholesale, and nothing prunes it; and every ordinary `behavioral` run stamps `BASELINE_KEY: False`, so a run key is the only thing separating this run's graded half from an unrelated run of the same skill last month. - Logs with no run key are skipped for that reason: they were not written by - a staleness run and have no half to be. + Logs with no run key predate the key rather than being suspect, and are + paired when the pairing is forced. See `LEGACY_RUN`. A directory holding only one side of a run is reported as a comparison against nothing, the same way a case missing from one side is: silently @@ -309,29 +309,58 @@ def from_logs(log_dir: str) -> list[Comparison]: from .engine import behavioral sides: dict[tuple[str, str], dict[bool, list[BehaviorOutcome]]] = {} + logs_per_side: dict[tuple[str, str], dict[bool, int]] = {} for info in list_eval_logs(log_dir): log = read_eval_log(info.name) if behavioral.BASELINE_KEY not in (log.eval.metadata or {}): continue skill, baseline, run_key, outcomes = outcomes_from_log(log) - if not run_key: - continue - sides.setdefault((skill, run_key), {}).setdefault(baseline, []).extend(outcomes) + key = (skill, run_key or LEGACY_RUN) + sides.setdefault(key, {}).setdefault(baseline, []).extend(outcomes) + tally = logs_per_side.setdefault(key, {}) + tally[baseline] = tally.get(baseline, 0) + 1 comparisons: list[Comparison] = [] for key in sorted(sides): half = sides[key] + ambiguous = key[1] == LEGACY_RUN and any( + n > 1 for n in logs_per_side[key].values() + ) comparisons.extend( pair( - _no_duplicates(half.get(False, [])), - _no_duplicates(half.get(True, [])), + _no_duplicates(half.get(False, []), ambiguous), + _no_duplicates(half.get(True, []), ambiguous), ) ) return comparisons -def _no_duplicates(outcomes: list[BehaviorOutcome]) -> list[BehaviorOutcome]: - """`outcomes`, with every case id that appears twice on this side errored. +# The bucket for logs written before a run id was stamped on them. +# +# Refusing them outright was the first fix and it was too strong: it made the +# only staleness logs in existence unreadable, including the run this command +# was developed against, which is the one artifact anybody could check the +# rebuild against. A log that predates the key is not suspect, it is merely +# unlabelled. +# +# What the key defends against is *choosing* -- a directory holding several +# candidates for one side, where pairing picks one silently and `list_eval_logs` +# makes that the older. Where a skill has one log per side there is nothing to +# choose: the pairing is forced, and labelling it would not change which logs +# pair. Where it has more, the ambiguity is precisely the hazard, and every +# case is reported `ungraded` rather than guessed at. A labelled run never +# falls back here, so this cannot weaken a run written by this version. +LEGACY_RUN = "unlabelled" + + +def _no_duplicates( + outcomes: list[BehaviorOutcome], ambiguous: bool = False +) -> list[BehaviorOutcome]: + """`outcomes`, with every case this side cannot answer for errored. + + `ambiguous` errors all of them: see `LEGACY_RUN`, where unlabelled logs + leave several candidates for a side and no way to tell which run each + belongs to. Otherwise only a case id this side reports twice is errored. One run key and one side can still span several logs -- a run grades one skill per task, and a run key covers the whole invocation -- and nothing @@ -345,6 +374,16 @@ def _no_duplicates(outcomes: list[BehaviorOutcome]) -> list[BehaviorOutcome]: the guess. `classify` turns the error into `ungraded`, consistent with how this module reports every other ambiguity it cannot resolve. """ + if ambiguous: + return [ + replace( + outcome, + error="these logs carry no run id and there is more than one " + "of them on a side, so which run this case belongs to cannot " + "be told apart", + ) + for outcome in outcomes + ] tally: dict[str, int] = {} for outcome in outcomes: tally[outcome.id] = tally.get(outcome.id, 0) + 1 diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 24bbe8d..2accbdf 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6803,10 +6803,44 @@ def test_a_neighbouring_runs_log_is_not_spliced_in(self) -> None: verdicts = sorted(c.verdict for c in comparisons) self.assertEqual(verdicts, ["fails_either_way", "ungraded"]) - def test_a_plain_behavioral_log_is_not_a_half_of_anything(self) -> None: - # It carries `BASELINE_KEY: False` like a graded half and no run key. + def test_a_lone_unlabelled_log_is_still_reported(self) -> None: + # Logs written before the run id existed are unlabelled, not suspect. + # Refusing them outright made the only staleness logs in existence + # unreadable, this branch's own included. comparisons = self._rebuild([self._log(False, True, run_key=None)]) - self.assertEqual(comparisons, []) + self.assertEqual([c.verdict for c in comparisons], ["ungraded"]) + self.assertIn("no result from the baseline run", comparisons[0].error) + + def test_an_unlabelled_pair_with_one_log_a_side_is_forced_and_kept(self) -> None: + # Nothing to choose between, so labelling would not change which logs + # pair. This is the shape every pre-fix run on disk has. + comparisons = self._rebuild([ + self._log(False, True, run_key=None), + self._log(True, False, run_key=None), + ]) + self.assertEqual([c.verdict for c in comparisons], ["earns_it"]) + + def test_unlabelled_logs_with_a_choice_to_make_are_not_guessed_at(self) -> None: + # Two candidates for the graded side and no way to tell which run + # either belongs to. That ambiguity is the whole hazard, so it is + # reported rather than silently resolved to the older log. + comparisons = self._rebuild([ + self._log(False, True, run_key=None), + self._log(False, False, run_key=None, case="d"), + self._log(True, False, run_key=None), + ]) + self.assertTrue(comparisons) + self.assertEqual({c.verdict for c in comparisons}, {"ungraded"}) + self.assertIn("no run id", comparisons[0].error) + + def test_a_labelled_run_never_falls_back_to_the_unlabelled_bucket(self) -> None: + # A labelled half and an unlabelled one are not two halves of + # anything, however tempting the arithmetic. + comparisons = self._rebuild([ + self._log(False, True, run_key="new"), + self._log(True, False, run_key=None), + ]) + self.assertEqual([c.verdict for c in comparisons], ["ungraded", "ungraded"]) def test_two_logs_reporting_one_case_are_reported_rather_than_resolved(self) -> None: # `pair` builds a dict keyed by case id, so a duplicate collapses From 82264e83c9f5dd41d93ad160a97b55b6968d348d Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 09:27:08 +0100 Subject: [PATCH 09/15] Close the two holes the second review found **The unlabelled-log bucket was not held to the standard of the fix it relaxed.** `6abbc51` read it as a migration concern. It is not one. A plain `skillscope behavioral` run stamps `BASELINE_KEY: False` and never stamps a run id, because only `cmd_staleness` mints one, so an ordinary behavioral log is shaped exactly like a pre-id graded half and always will be. The bucket was therefore a permanent inlet, not a transitional one. Reproduced before fixing. A directory holding one keyless graded-side log -- which is what every `behavioral` run leaves behind -- returned two comparisons, both `ungraded`, counted into `totals.cases` and `totals.ungraded` with a `per_skill` row for a skill the run never covered. Point `--from-logs` at `.skillscope/logs` as the docs instruct and the denominator inflates by every skill ever graded there. With a leftover pre-id baseline for the same skill it is worse: one log a side, no ambiguity to detect, and a plain `behavioral` run is silently handed to the comparison as its "with skill" half. The original splicing bug, through the back door, and the test added in `6abbc51` encoded that pairing as intended. Unlabelled logs are now skipped unless `--unlabelled-logs` asks for them, and asking is not enough on its own: a group also needs a baseline side, which nothing but `cmd_staleness` ever writes. So ordinary behavioral logs stay out whatever flags are passed, and the flag means what it says -- the caller asserting the directory holds nothing else for those skills. The flag is refused without `--from-logs`, since a live run labels both its halves. The three places stating the old contract are corrected rather than left: `from_logs`'s docstring, the `--from-logs` help, and `docs/usage.md`. All three were true when `ed49d4f` wrote them and `6abbc51` falsified them without saying so. **The detector's two halves disagreed, in both directions.** The structured branch used a boundary-aware containment check; the shell branch nine lines later used a raw `in`. Verified against the real function: - `cat ~/.claude/skills/foo-extra/SKILL.md` read as skill `foo`'s own files. The same class of false positive the first fix was written to remove, surviving where the mark carried no trailing boundary. - `Glob pattern=/foo/**/*.md` invisible. `pattern` was not in `PATH_FIELDS`, and a closed field list cannot keep up with an open tool surface -- MCP tools name arguments whatever they like. - `cat .claude/skills/foo/SKILL.md` invisible. `no_sandbox` runs the agent with `cwd=workspace`, so the relative form is the natural one, and the mark was anchored with a leading separator. The bare-name matching this replaced caught it. The misses are the worse direction: `_voided_if_dirty` never fires, and a dirty baseline is reported as a clean `no_signal` stated as fact. So the field list is gone. Every string anywhere in a call's arguments is tested, as written and as the path it resolves to, against marks matched at component boundaries. Checking every string is only safe because the marks are absolute and boundary matched -- that is what removed the false positives, not the field list. The review's eight-row table is now 8/8, and the two pass-one false positives (`/run/user/1000` for a skill named `run`) stay fixed. Five mutations reintroducing these bugs were each caught by a test that went red: unconditional legacy pooling, dropping the baseline-side requirement, dropping the trailing boundary, restoring a fixed field list, and re-anchoring the staged mark. Checked on real logs. The rocm-doctor run rebuilds to the same five verdicts as the live run; its neighbouring unlabelled quark logs are now passed over by default and return under `--unlabelled-logs`; and a directory of behavioral logs rebuilds as nothing either way. Signed-off-by: John Lybeck --- docs/usage.md | 30 ++++-- skillscope/cli.py | 44 ++++++++- skillscope/engine/behavioral.py | 157 ++++++++++++++++++-------------- skillscope/staleness.py | 54 +++++++---- tests/test_skillscope.py | 147 +++++++++++++++++++++++++++--- 5 files changed, 320 insertions(+), 112 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 15496a3..5e13abc 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -310,13 +310,29 @@ All three keys are load-bearing, and the run id is the one that makes this safe to point at `.skillscope/logs`. That directory is shared with `behavioral` and `routing`, is gitignored wholesale, and nothing prunes it — and every ordinary `behavioral` run writes the same `skillscope_baseline: false` a graded -half does. Pairing on the skill alone would therefore splice an unrelated run -from last month in as this comparison's "with skill" side, under a table that -reads exactly as confidently as a real one. Logs with no run id are not a half -of anything and are passed over, so a directory of plain `behavioral` logs -rebuilds as nothing rather than as a comparison. The model and effort are not -reported for a rebuild: nothing reads them off the logs, and the logs need not -agree on them. +half does, with no run id. A plain `behavioral` log is therefore shaped exactly +like a graded half, and always will be, since only `staleness` mints an id. +Pairing on the skill alone would splice an unrelated run from last month in as +this comparison's "with skill" side, under a table that reads exactly as +confidently as a real one. + +So logs carrying no run id are passed over, and a directory of plain +`behavioral` logs rebuilds as nothing rather than as a comparison. Logs written +before the id existed are passed over for the same reason; `--unlabelled-logs` +reads them: + +```bash +skillscope staleness --from-logs .skillscope/logs --unlabelled-logs +``` + +That asserts the directory holds nothing else for the skills in it. Even then, +a group of unlabelled logs needs a baseline half, which nothing but `staleness` +ever writes, so ordinary `behavioral` logs stay out whatever flags you pass. +Where one side holds several unlabelled logs there is a choice to make and +nothing to make it with, so every case in that group is reported `ungraded`. + +The model and effort are not reported for a rebuild: nothing reads them off +the logs, and the logs need not agree on them. Budget more wall clock than a `behavioral` run of the same cases, and more than double it. `--timeout` defaults to twice what the single-pass commands diff --git a/skillscope/cli.py b/skillscope/cli.py index 3c56025..2ebeadd 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -821,12 +821,24 @@ def cmd_staleness(args: argparse.Namespace) -> int: """ started = time.time() + if args.unlabelled_logs and not args.from_logs: + # The repo's habit: refuse an argument that cannot do anything rather + # than accept and ignore it. A live run writes its own run id on both + # halves, so there is nothing unlabelled for this to reach. + raise SystemExit( + "error: --unlabelled-logs only means something with --from-logs. " + "A run started now stamps a run id on both halves, so it has no " + "unlabelled logs to read." + ) + if args.from_logs: # Nothing is run, so nothing is prepared: no structural gate, no model # pin, no reachability probe. Those guard a spend that is not about to # happen, and demanding them would make a free command fail for # reasons that cannot affect its answer. - comparisons = staleness.from_logs(args.from_logs) + comparisons = staleness.from_logs( + args.from_logs, unlabelled=args.unlabelled_logs + ) if not comparisons: from .engine import behavioral as engine_behavioral @@ -835,8 +847,15 @@ def cmd_staleness(args: argparse.Namespace) -> int: f"Both halves of a comparison carry `{engine_behavioral.SKILL_KEY}`, " f"`{engine_behavioral.BASELINE_KEY}` and " f"`{engine_behavioral.STALENESS_RUN_KEY}` in their eval " - "metadata; logs written before that, by another tool, or by a " - "plain `behavioral` run cannot be paired." + "metadata; logs written by another tool, or by a plain " + "`behavioral` run, cannot be paired." + + ( + "" + if args.unlabelled_logs + else "\n Logs written before the run id existed carry " + "none, and are passed over. `--unlabelled-logs` reads " + "them, where a baseline half is there to pair with." + ) ) # No `model`, `engine` or `effort`. Nothing here read them off the # logs, and `args` holds this invocation's defaults -- which describe @@ -1271,8 +1290,23 @@ def build_parser() -> argparse.ArgumentParser: "were, and the run that wrote them in their eval metadata, so a " "log directory is enough to pair them -- and to tell this run's " "halves from a neighbouring run of the same skill. Costs nothing " - "and reaches no model. Logs written by a plain `behavioral` run " - "are not a half of anything and are passed over." + "and reaches no model. Logs carrying no run id are passed over; " + "see --unlabelled-logs." + ), + ) + staleness_parser.add_argument( + "--unlabelled-logs", + action="store_true", + help=( + "With --from-logs, also read logs that carry no run id, which " + "means logs written before this command stamped one. Off by " + "default: an ordinary `behavioral` log is shaped exactly like a " + "pre-id graded half, so reading them unasked inflates the counts " + "with skills the run never covered, and can hand a comparison a " + "'with skill' half from a different run. Passing this asserts the " + "directory holds nothing else for these skills. A group still " + "needs a baseline side, and a side holding several logs is " + "reported `ungraded` rather than guessed at." ), ) staleness_parser.set_defaults(handler=cmd_staleness) diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index c58f133..c7ec27a 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -16,7 +16,6 @@ from __future__ import annotations -import json import sys from pathlib import Path @@ -157,97 +156,123 @@ def _normalise(text: str) -> str: return text.replace("\\", "/").lower() -def _skill_marks(skill: str) -> tuple[tuple[str, ...], str]: - """`(absolute roots, staged suffix)` -- what reading makes a room dirty. +def _skill_marks(skill: str) -> tuple[str, ...]: + """The paths that reading makes a room dirty, lowercased and separator-flat. - The root is the skill's own directory, which is where an agent that went - looking for it would find it. It is resolved and matched as a prefix, - because a relative mention proves nothing: an agent's own `skills/pdf` - scratch directory is not ours, and only the absolute path tells them apart. + Two of them. The skill's own directory, resolved, which is where an agent + that went looking would find it. And `.claude/skills/`, the staged + copy `no_sandbox.install_skill` writes under a workspace that changes per + sample, so only its tail is knowable here. - The staged copy is the second route. `no_sandbox.install_skill` writes it to - `/.claude/skills/`, and the workspace is the agent's and - changes per sample, so the leading part is unknowable here. What is left - is a suffix specific enough to stand on its own: a path containing - `/.claude/skills/` is the staging area whoever created it. + The staged mark carries no leading separator on purpose. `no_sandbox` runs + the agent with `cwd=workspace`, which makes `cat .claude/skills/foo/...` + the natural shell form, and a mark anchored with a leading `/` missed + exactly that. `_mentions` supplies the boundary instead, which is what + keeps the mark from matching the middle of a longer name. """ - roots = [] + marks = [] try: - roots.append(_normalise(str(Path(config.active().skill_path(skill)).resolve()))) + marks.append(_normalise(str(Path(config.active().skill_path(skill)).resolve()))) except (Exception, SystemExit): # noqa: BLE001 -- see below - # A skill with no directory on this machine leaves the staged suffix - # to carry the check on its own, which is the honest answer rather - # than a degraded one: nothing can be read out of a directory that is - # not there. `skill_path` raises `SystemExit` rather than returning, - # so catching `Exception` alone let a rebuild of someone else's logs - # take the whole command down. + # A skill with no directory on this machine leaves the staged mark to + # carry the check on its own, which is the honest answer rather than a + # degraded one: nothing can be read out of a directory that is not + # there. `skill_path` raises `SystemExit` rather than returning, so + # catching `Exception` alone let a rebuild of someone else's logs take + # the whole command down. pass - return tuple(roots), _normalise(f"/.claude/skills/{skill}") + marks.append(_normalise(f".claude/skills/{skill}")) + return tuple(marks) -def _under(path: str, marks: tuple[tuple[str, ...], str]) -> bool: - """Whether this already-normalised path is the skill's directory or inside it.""" - roots, staged = marks - if any(path == root or path.startswith(root + "/") for root in roots): +# Characters that continue a path component. A mark followed or preceded by +# one of these is part of a longer name, not the name itself. +_NAME_CHARS = "-_." + + +def _mentions(text: str, mark: str) -> bool: + """Whether `text` names `mark` as a whole path component. + + Plain `in` was the bug twice over. It is what made `/{skill}/` match + `/run/user/1000`, and it survived in the shell branch after the structured + branch was fixed, where it read `.claude/skills/foo-extra` as skill `foo`'s + own files. A sibling skill whose name extends this one's is not this one. + + So a hit has to end at a boundary -- end of string, or a character that + cannot continue a component -- and start at one. `foo` matches in + `/x/.claude/skills/foo/SKILL.md` and in `cat .claude/skills/foo/S`, and + does not match in `foo-extra`, `foo.bak` or `myfoo`. + """ + width = len(mark) + start = 0 + while (at := text.find(mark, start)) >= 0: + start = at + 1 + after = text[at + width : at + width + 1] + if after and (after.isalnum() or after in _NAME_CHARS): + continue + before = text[at - 1 : at] if at else "" + if before and (before.isalnum() or before in _NAME_CHARS): + continue return True - return path.endswith(staged) or staged + "/" in path + return False -# Tool arguments that name a file rather than describe one. `detect_activation` -# has the same list for its own purposes; this one is about reading a path out -# of a structured call, so it stays here rather than being shared and bent. -PATH_FIELDS = ("file_path", "path", "notebook_path", "filename", "target_file") +def _strings(value) -> list[str]: + """Every string anywhere in a tool call's arguments. -# Arguments that carry a shell command rather than a path. These are the only -# ones substring-matched, because there is no field to resolve: the path is -# somewhere inside a string the agent composed. -COMMAND_FIELDS = ("command", "cmd", "script") + Flat rather than by field name. The first version of this checked a fixed + list of path-ish argument names, which is a closed set against an open tool + surface: it had no entry for Glob's `pattern`, which is often the only + argument a Glob call carries, so that call was invisible. MCP tools name + their arguments whatever they like, so the list would have kept drifting. + Checking every string is safe here only because the marks are boundary + matched and absolute. That is what removed the false positives, not the + field list. + """ + if isinstance(value, str): + return [value] + if isinstance(value, dict): + return [s for v in value.values() for s in _strings(v)] + if isinstance(value, (list, tuple)): + return [s for v in value for s in _strings(v)] + return [] -def _read_the_skill(call, marks: tuple[tuple[str, ...], str], skill: str) -> str | None: + +def _read_the_skill(call, marks: tuple[str, ...], skill: str) -> str | None: """Whether this call reached into the skill's own directory. - Structured path arguments are resolved and tested for containment, rather - than substring-matched on the skill's name. A bare name is a legal skill - name -- `structure.py` allows a single word and Anthropic ship `pdf`, - `docx`, `pptx` -- so matching `/{skill}/` against a serialised argument - blob reported `/run/user/1000` as contamination for a skill named `run`, - and the cost of that false positive is the highest there is here: - `_voided_if_dirty` converts the two verdicts this command exists to - surface into `ungraded`, and the report states the contamination as fact. - - Shell commands keep substring matching, because there is no field to - resolve -- but against the resolved absolute path, not the bare name. + Every string argument is tested, as written and as the path it resolves to. + Resolution is skipped for anything with whitespace in it: that is a command + line rather than a path, and it is matched as written. + + The graded side reads its own installed skill by design, so a hit there is + expected. It is the baseline where this means the room was not empty. """ arguments = getattr(call, "arguments", None) or {} if not isinstance(arguments, dict): return None - for field in PATH_FIELDS: - value = arguments.get(field) - if not isinstance(value, str) or not value: - continue - try: - resolved = _normalise(str(Path(value).resolve())) - except Exception: # noqa: BLE001 -- an unparseable path named nothing - continue - if _under(resolved, marks): - return f"read the skill's own files ({skill})" - - roots, staged = marks - for field in COMMAND_FIELDS: - value = arguments.get(field) - if not isinstance(value, str): - continue + reason = f"read the skill's own files ({skill})" + for value in _strings(arguments): text = _normalise(value) - if staged in text or any(root in text for root in roots): - return f"read the skill's own files ({skill})" + if any(_mentions(text, mark) for mark in marks): + return reason + # `../x`, `~/x` and `./x` name the same file by another spelling, and + # a structured path field is where those arrive. + if value and not any(c.isspace() for c in value): + try: + resolved = _normalise(str(Path(value).expanduser().resolve())) + except (Exception, SystemExit): # noqa: BLE001 -- not a path, then + continue + if any(_mentions(resolved, mark) for mark in marks): + return reason # The one mark that is not a path. `_bundled_skills` is the harness's own # staging directory, so its name in any argument is the agent having found # the staging area rather than a file of its own. - if "_bundled_skills" in _normalise(json.dumps(arguments, ensure_ascii=False)): - return f"read the skill's own files ({skill})" + if any("_bundled_skills" in _normalise(v) for v in _strings(arguments)): + return reason return None diff --git a/skillscope/staleness.py b/skillscope/staleness.py index fb51610..4f8c773 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -280,21 +280,22 @@ def outcomes_from_log(log) -> tuple[str, bool, str | None, list[BehaviorOutcome] ) -def from_logs(log_dir: str) -> list[Comparison]: +def from_logs(log_dir: str, *, unlabelled: bool = False) -> list[Comparison]: """Rebuild the comparison from logs already on disk. Two runs become a pair because they carry the same skill, the same `STALENESS_RUN_KEY`, and opposite `BASELINE_KEY` -- which is why all three are written as metadata rather than inferred from a task name. - Grouping on the run key is what makes this safe to point at + Grouping on the run id is what makes this safe to point at `.skillscope/logs`, which is the documented thing to do with it. That directory is shared by `behavioral`, `verify` and `routing`, is gitignored wholesale, and nothing prunes it; and every ordinary `behavioral` run - stamps `BASELINE_KEY: False`, so a run key is the only thing separating + stamps `BASELINE_KEY: False`, so a run id is the only thing separating this run's graded half from an unrelated run of the same skill last month. - Logs with no run key predate the key rather than being suspect, and are - paired when the pairing is forced. See `LEGACY_RUN`. + + Logs with no run id are skipped unless `unlabelled` asks for them. See + `LEGACY_RUN` for what that costs and what it is for. A directory holding only one side of a run is reported as a comparison against nothing, the same way a case missing from one side is: silently @@ -323,9 +324,11 @@ def from_logs(log_dir: str) -> list[Comparison]: comparisons: list[Comparison] = [] for key in sorted(sides): half = sides[key] - ambiguous = key[1] == LEGACY_RUN and any( - n > 1 for n in logs_per_side[key].values() - ) + counts = logs_per_side[key] + if key[1] == LEGACY_RUN: + if not unlabelled or not counts.get(True): + continue + ambiguous = key[1] == LEGACY_RUN and any(n > 1 for n in counts.values()) comparisons.extend( pair( _no_duplicates(half.get(False, []), ambiguous), @@ -337,19 +340,30 @@ def from_logs(log_dir: str) -> list[Comparison]: # The bucket for logs written before a run id was stamped on them. # -# Refusing them outright was the first fix and it was too strong: it made the -# only staleness logs in existence unreadable, including the run this command -# was developed against, which is the one artifact anybody could check the -# rebuild against. A log that predates the key is not suspect, it is merely -# unlabelled. +# Skipped by default, and taken only when the caller asks with +# `--unlabelled-logs`. Both halves of that rule are load-bearing. +# +# Taking them by default was wrong, and not only for old directories. A plain +# `skillscope behavioral` run stamps `BASELINE_KEY: False` and never stamps a +# run id, because only `cmd_staleness` mints one -- so an ordinary behavioral +# log is shaped exactly like a pre-key graded half and always will be. Pooling +# them inflated `cases` and `ungraded` with every skill the user had ever run +# `behavioral` against, and could silently supply the "with skill" side of a +# comparison it was never part of. That is the splicing bug this module's run +# id exists to prevent, arriving through the back door. +# +# Refusing them outright was wrong too: it made the only staleness logs that +# existed anywhere unreadable, the run this branch was developed against +# included, which is the one artifact a rebuild could be checked against. +# +# So the caller asserts it. Even then a group needs a baseline side, which +# nothing but `cmd_staleness` ever writes, so a directory of ordinary +# `behavioral` logs stays out whatever flags it is given. And where a side +# holds more than one log there is a choice to make and no way to make it, so +# every case in that group is `ungraded` rather than guessed at. # -# What the key defends against is *choosing* -- a directory holding several -# candidates for one side, where pairing picks one silently and `list_eval_logs` -# makes that the older. Where a skill has one log per side there is nothing to -# choose: the pairing is forced, and labelling it would not change which logs -# pair. Where it has more, the ambiguity is precisely the hazard, and every -# case is reported `ungraded` rather than guessed at. A labelled run never -# falls back here, so this cannot weaken a run written by this version. +# A labelled run never falls in here, so none of this can weaken a run written +# by this version. LEGACY_RUN = "unlabelled" diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 2accbdf..e0e4211 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6709,6 +6709,54 @@ def test_the_harness_staging_directory_still_counts(self) -> None: self._why({"file_path": "/opt/_bundled_skills/run/SKILL.md"}), ) + def test_a_sibling_skill_whose_name_extends_this_one_is_not_this_one(self) -> None: + # The false positive that survived the first fix in the shell branch, + # because the staged mark carried no trailing boundary there while + # the structured branch used `_under`. `run-extra` is its own skill. + self.assertIsNone( + self._why({"command": "cat /home/u/.claude/skills/run-extra/SKILL.md"}) + ) + self.assertIsNone( + self._why({"file_path": "/home/u/.claude/skills/run-extra/SKILL.md"}) + ) + + def test_a_name_the_skills_own_name_is_a_prefix_of_is_not_a_hit(self) -> None: + own = str(config.active().skill_path("run")) + self.assertIsNone(self._why({"file_path": f"{own}.bak/SKILL.md"})) + + def test_the_staged_copy_is_found_by_the_relative_name(self) -> None: + # `no_sandbox` runs the agent with `cwd=workspace`, so this is the + # natural shell form. A mark anchored with a leading separator missed + # it, and a missed contamination is the worse direction: the baseline + # is then reported clean, as fact. + self.assertIn( + "read the skill's own files", + self._why({"command": "cat .claude/skills/run/SKILL.md"}), + ) + + def test_an_argument_the_field_list_never_named_is_still_read(self) -> None: + # `pattern` is Glob's primary argument and is often its only one, so + # the old fixed field list made the call invisible. The tool surface + # is open, so the list was always going to drift behind it. + own = str(config.active().skill_path("run")) + self.assertIn( + "read the skill's own files", self._why({"pattern": f"{own}/**/*.md"}) + ) + + def test_a_tool_naming_its_arguments_anything_is_still_read(self) -> None: + # An MCP tool names its arguments whatever it likes. + own = str(config.active().skill_path("run")) + self.assertIn( + "read the skill's own files", self._why({"some_mcp_arg": f"{own}/x.md"}) + ) + + def test_a_path_nested_inside_a_structured_argument_is_still_read(self) -> None: + own = str(config.active().skill_path("run")) + self.assertIn( + "read the skill's own files", + self._why({"edits": [{"file": f"{own}/SKILL.md"}]}), + ) + def test_a_skill_with_no_directory_does_not_take_the_command_down(self) -> None: # `config.skill_path` raises `SystemExit`, which `except Exception` # does not catch -- so rebuilding somebody else's logs killed the @@ -6773,12 +6821,12 @@ class TestARebuildPairsOneRunAndNotItsNeighbours(unittest.TestCase): pointing `--from-logs` at that very directory. """ - def _rebuild(self, logs): + def _rebuild(self, logs, unlabelled=True): info = [types.SimpleNamespace(name=f"log-{i}") for i in range(len(logs))] by_name = {i.name: log for i, log in zip(info, logs)} with mock.patch("inspect_ai.log.list_eval_logs", lambda d: info), \ mock.patch("inspect_ai.log.read_eval_log", lambda n: by_name[n]): - return staleness.from_logs("anywhere") + return staleness.from_logs("anywhere", unlabelled=unlabelled) def _log(self, baseline, passed, run_key="run-1", case="c"): return _StubLog( @@ -6803,23 +6851,53 @@ def test_a_neighbouring_runs_log_is_not_spliced_in(self) -> None: verdicts = sorted(c.verdict for c in comparisons) self.assertEqual(verdicts, ["fails_either_way", "ungraded"]) - def test_a_lone_unlabelled_log_is_still_reported(self) -> None: - # Logs written before the run id existed are unlabelled, not suspect. - # Refusing them outright made the only staleness logs in existence - # unreadable, this branch's own included. - comparisons = self._rebuild([self._log(False, True, run_key=None)]) - self.assertEqual([c.verdict for c in comparisons], ["ungraded"]) - self.assertIn("no result from the baseline run", comparisons[0].error) + def test_unlabelled_logs_are_not_read_unless_asked_for(self) -> None: + # The default. A plain `behavioral` log carries `BASELINE_KEY: False` + # and no run id, which is the same shape a pre-id graded half has, so + # reading unlabelled logs unasked pools whatever is in the directory. + comparisons = self._rebuild([ + self._log(False, True, run_key=None), + self._log(True, False, run_key=None), + ], unlabelled=False) + self.assertEqual(comparisons, []) - def test_an_unlabelled_pair_with_one_log_a_side_is_forced_and_kept(self) -> None: - # Nothing to choose between, so labelling would not change which logs - # pair. This is the shape every pre-fix run on disk has. + def test_an_unlabelled_pair_is_read_when_asked_for(self) -> None: + # What `--unlabelled-logs` is for: one log a side, nothing to choose + # between, and a baseline half that only a staleness run ever writes. comparisons = self._rebuild([ self._log(False, True, run_key=None), self._log(True, False, run_key=None), ]) self.assertEqual([c.verdict for c in comparisons], ["earns_it"]) + def test_graded_only_unlabelled_logs_are_never_read(self) -> None: + # The common hole. `behavioral` and `verify` runs never carry a run + # id by design, so these keep arriving forever -- and pooled they + # inflated `cases` and `ungraded` with every skill the user had ever + # run `behavioral` against, plus a `per_skill` row for it. + # + # Asking for unlabelled logs does not buy this: a baseline half is + # the one thing nothing but `cmd_staleness` writes, so requiring one + # keeps ordinary behavioral logs out whatever flags are passed. + for asked in (True, False): + with self.subTest(unlabelled=asked): + comparisons = self._rebuild( + [self._log(False, True, run_key=None)], unlabelled=asked + ) + self.assertEqual(comparisons, []) + + def test_a_behavioral_log_cannot_supply_a_half_to_a_stale_baseline(self) -> None: + # The rarer hole, and the original splicing bug through the back door: + # a plain `behavioral` log for a skill, plus a leftover pre-id + # baseline for the same skill, is one log a side and was paired. + # `summarize` counts what comes back, so this has to stay empty. + comparisons = self._rebuild([ + self._log(False, True, run_key=None), # indistinguishable from + # a plain behavioral run + self._log(True, False, run_key=None), + ], unlabelled=False) + self.assertEqual(staleness.summarize(comparisons, {})["totals"]["cases"], 0) + def test_unlabelled_logs_with_a_choice_to_make_are_not_guessed_at(self) -> None: # Two candidates for the graded side and no way to tell which run # either belongs to. That ambiguity is the whole hazard, so it is @@ -6835,12 +6913,14 @@ def test_unlabelled_logs_with_a_choice_to_make_are_not_guessed_at(self) -> None: def test_a_labelled_run_never_falls_back_to_the_unlabelled_bucket(self) -> None: # A labelled half and an unlabelled one are not two halves of - # anything, however tempting the arithmetic. + # anything, however tempting the arithmetic. Both are reported as the + # lone halves they are, rather than paired into an `earns_it`. comparisons = self._rebuild([ self._log(False, True, run_key="new"), self._log(True, False, run_key=None), ]) self.assertEqual([c.verdict for c in comparisons], ["ungraded", "ungraded"]) + self.assertTrue(all("no result from" in c.error for c in comparisons)) def test_two_logs_reporting_one_case_are_reported_rather_than_resolved(self) -> None: # `pair` builds a dict keyed by case id, so a duplicate collapses @@ -6937,7 +7017,7 @@ def record(comparisons, meta): ["staleness", "--from-logs", "/logs", "--model", "opus"] ) with mock.patch.object(staleness, "summarize", record), \ - mock.patch.object(staleness, "from_logs", lambda d: staleness.pair( + mock.patch.object(staleness, "from_logs", lambda d, **k: staleness.pair( [_checked("c", True, [_check("x", True)])], [_checked("c", False, [_check("x", False)])], )), \ @@ -6948,6 +7028,45 @@ def record(comparisons, meta): self.assertEqual(captured["from_logs"], "/logs") +class TestUnlabelledLogsAreOptedIntoRatherThanAssumed(unittest.TestCase): + """The flag has to reach `from_logs`, and mean nothing without a rebuild.""" + + def _run(self, argv): + captured: dict = {} + + def record(log_dir, *, unlabelled=False): + captured["unlabelled"] = unlabelled + return staleness.pair( + [_checked("c", True, [_check("x", True)])], + [_checked("c", False, [_check("x", False)])], + ) + + args = cli.build_parser().parse_args(argv) + with mock.patch.object(staleness, "from_logs", record), \ + mock.patch.object(cli, "_write_report", lambda *a, **k: None): + cli.cmd_staleness(args) + return captured + + def test_a_rebuild_does_not_read_unlabelled_logs_by_default(self) -> None: + self.assertIs(self._run(["staleness", "--from-logs", "/l"])["unlabelled"], False) + + def test_the_flag_reaches_the_rebuild(self) -> None: + self.assertIs( + self._run( + ["staleness", "--from-logs", "/l", "--unlabelled-logs"] + )["unlabelled"], + True, + ) + + def test_the_flag_is_refused_without_a_rebuild(self) -> None: + # A live run stamps a run id on both halves, so there is nothing + # unlabelled for this to reach. Refused rather than ignored. + args = cli.build_parser().parse_args(["staleness", "--unlabelled-logs"]) + with self.assertRaises(SystemExit) as caught: + cli.cmd_staleness(args) + self.assertIn("--from-logs", str(caught.exception)) + + class TestTheSecondPassGetsABudgetOfItsOwn(unittest.TestCase): """Two passes under one bound, and the baseline is the slower half. From 5fd10bdd8b69664f9efaa2bad70753d46fe1dc74 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 09:33:28 +0100 Subject: [PATCH 10/15] Make --help tell the truth about the timeout, and name staleness in the action `staleness --help` printed "Default: 900 (15 minutes)" three lines under a description announcing that --timeout "defaults to twice the usual here". One screen of help contradicting itself, and the help was the wrong half: the real default is 1800. The cause was the fix's own shape. `set_defaults` changes the value argparse resolves and does not revisit the help string, which `_add_timeout_argument` had already built from the shared constant. So the default is threaded through `_add_graded_arguments` instead and the help is built from the number in force. The string also omitted `staleness` from the commands it says the bound is shared by, which is now a list of four. A mutation back to `set_defaults` leaves the value correct and the help stale, and the new test catches exactly that. `action.yml` listed the subcommands without `staleness`, and more usefully left it out of what needs `install-claude`. It drives `_run_behavioral` for both of its halves, so it needs the `claude` CLI exactly as much as `behavioral` does; a CI author reading only that line gets a confusing failure. Signed-off-by: John Lybeck --- action.yml | 8 ++++--- skillscope/cli.py | 46 +++++++++++++++++++++++++++------------- tests/test_skillscope.py | 28 ++++++++++++++++++++++++ 3 files changed, 64 insertions(+), 18 deletions(-) diff --git a/action.yml b/action.yml index 2a73c12..721ef6c 100644 --- a/action.yml +++ b/action.yml @@ -16,7 +16,7 @@ description: >- inputs: command: - description: 'The skillscope subcommand: structural, routing, behavioral, select, list-skills, template.' + description: 'The skillscope subcommand: structural, routing, behavioral, staleness, select, list-skills, template.' required: true args: description: "Further arguments for the subcommand, shell-quoted." @@ -47,8 +47,10 @@ inputs: default: "true" install-claude: description: >- - Install Node and the claude CLI. Needed by `routing` and `behavioral`, - pointless for `structural` and `select`. + Install Node and the claude CLI. Needed by `routing`, `behavioral` and + `staleness` -- staleness drives the behavioral leg for both of its + halves, so it needs the CLI exactly as much. Pointless for `structural` + and `select`. required: false default: "false" node-version: diff --git a/skillscope/cli.py b/skillscope/cli.py index 2ebeadd..7b9915e 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -1002,8 +1002,14 @@ def _add_routing_room_argument( ) -def _add_graded_arguments(parser: argparse.ArgumentParser) -> None: - """Flags shared by the routing and behavioral commands.""" +def _add_graded_arguments( + parser: argparse.ArgumentParser, *, timeout: float = deadline.DEFAULT_TIMEOUT_S +) -> None: + """Flags shared by the graded commands: routing, behavioral and staleness. + + `timeout` is threaded through rather than overridden afterwards so the + `--help` text matches the default in force. See `_add_timeout_argument`. + """ _add_skills_argument(parser) parser.add_argument( "--skill", @@ -1060,18 +1066,28 @@ def _add_graded_arguments(parser: argparse.ArgumentParser) -> None: "commands. Default: claude-code-no-sandbox, or $SKILLSCOPE_ENGINE." ), ) - _add_timeout_argument(parser) + _add_timeout_argument(parser, timeout) -def _add_timeout_argument(parser: argparse.ArgumentParser) -> None: +def _add_timeout_argument( + parser: argparse.ArgumentParser, default: float = deadline.DEFAULT_TIMEOUT_S +) -> None: + """The whole-command deadline, with a help string that knows its default. + + `default` is a parameter rather than a `set_defaults` call afterwards + because the help text is built here and argparse does not revisit it. + `staleness` overrode the value and left the help saying 900, three lines + under a description announcing that it defaults to twice the usual -- a + screen of `--help` contradicting itself. + """ parser.add_argument( "--timeout", type=float, - default=deadline.DEFAULT_TIMEOUT_S, + default=default, help=( "Seconds before the whole command is abandoned. Shared by " - "structural, routing, and behavioral. Default: 900 (15 minutes). " - "0 disables." + "structural, routing, behavioral and staleness. " + f"Default: {default:g} ({default / 60:g} minutes). 0 disables." ), ) @@ -1272,15 +1288,15 @@ def build_parser() -> argparse.ArgumentParser: "of going straight to the work." ), ) - _add_graded_arguments(staleness_parser) # Twice the shared bound, because the command is two passes under one. - # `_add_timeout_argument` is shared with the commands that run a single - # pass, so the default is overridden here rather than parameterised there; - # argparse's `set_defaults` wins over an `add_argument` default. The - # measurement behind the number is in `cmd_staleness`: a two-case skill - # finished the graded pass in 2m23s and had not finished the baseline - # after 30 minutes, so the default 900s gave the slower half ~637s. - staleness_parser.set_defaults(timeout=2 * deadline.DEFAULT_TIMEOUT_S) + # Passed into `_add_graded_arguments` rather than set afterwards, so the + # `--help` text is built from the number actually in force: a + # `set_defaults` override changes the value and leaves the help saying + # whatever it was built with. The measurement behind the number is in + # `cmd_staleness`: a two-case skill finished the graded pass in 2m23s and + # had not finished the baseline after 30 minutes, so the shared 900s gave + # the slower half ~637s of a measured 1800s+ need. + _add_graded_arguments(staleness_parser, timeout=2 * deadline.DEFAULT_TIMEOUT_S) staleness_parser.add_argument( "--from-logs", metavar="DIR", diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index e0e4211..c33de0c 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -7095,6 +7095,34 @@ def test_the_single_pass_commands_are_unchanged(self) -> None: def test_an_explicit_timeout_still_wins(self) -> None: self.assertEqual(self._timeout(["staleness", "--timeout", "60"]), 60.0) + def test_the_help_text_names_the_default_actually_in_force(self) -> None: + # The help was a static string built in the shared helper, so the + # `set_defaults` override changed the value and left `--help` saying + # 900 -- three lines under a description announcing that staleness + # defaults to twice the usual. One screen contradicting itself. + import contextlib + import io + + def help_for(command): + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + with self.assertRaises(SystemExit): + cli.build_parser().parse_args([command, "--help"]) + return " ".join(buffer.getvalue().split()) + + self.assertIn("Default: 1800 (30 minutes)", help_for("staleness")) + self.assertIn("Default: 900 (15 minutes)", help_for("behavioral")) + + def test_the_help_text_lists_staleness_as_a_command_sharing_the_bound(self) -> None: + import contextlib + import io + + buffer = io.StringIO() + with contextlib.redirect_stdout(buffer): + with self.assertRaises(SystemExit): + cli.build_parser().parse_args(["staleness", "--help"]) + self.assertIn("behavioral and staleness", " ".join(buffer.getvalue().split())) + if __name__ == "__main__": From a4307cfc1215c7ad3c83123a921bb71c83f1e6ff Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 12:58:01 +0100 Subject: [PATCH 11/15] Judge contamination where the run was, not where the reader is The baseline half of a staleness comparison only means something if the agent really had no skill available. `_skill_marks` recognises a tool call that reached the skill by comparing against the skill's absolute path -- and it worked that path out from the *reading* process, not the run. So `--from-logs` re-derived a weaker answer than the run it rebuilt. Reproduced: the same recorded call, `Read {"file_path": ".../run/SKILL.md"}`, graded `ungraded` live and `no_signal` when the report was rebuilt from another directory or on a machine without the skill checked out. That is the "a rebuild is more permissive than the run it rebuilds" failure this module closes for the no-checks branch and tests for by name; contamination was the one place it was still open, and the report stated the weaker answer as fact. Both engines now record where the skill was, and the reader uses that rather than its own cwd. Where neither the log nor the machine can say, the room is reported unknown and the case lands `ungraded`, instead of quietly keeping the one mark that cannot fail and calling it clean. Also drops the `resolve()` branch. Its comment claimed it let `./x` and `../x` name the same file by another spelling, but the agent's cwd is `no_sandbox`'s per-sample workspace and appears nowhere in the tool-call record, so it resolved them against skillscope's cwd instead. It could never serve that case, and it was the second source of the cwd dependence. `~/x` is context-free and still expands. Signed-off-by: John Lybeck --- docs/usage.md | 21 ++++-- skillscope/engine/behavioral.py | 109 ++++++++++++++++++++++++-------- skillscope/engine/verify.py | 5 +- skillscope/staleness.py | 14 +++- tests/test_skillscope.py | 37 +++++++++-- 5 files changed, 146 insertions(+), 40 deletions(-) diff --git a/docs/usage.md b/docs/usage.md index 5e13abc..567be43 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -278,7 +278,7 @@ at all. | `no_signal` | passed either way — says nothing about the skill | | `fails_either_way` | failed both times — a broken case, or a hard one | | `guards_only` | asserts nothing the skill could be credited for — see the guard rails below | -| `ungraded` | a run errored on one side or both, or its baseline was not empty | +| `ungraded` | nothing could be concluded: a run errored on one side or both, a case was missing from a side, its baseline was not empty, or whether the baseline was empty could not be established | A verdict says what changed, not why. So the graded side also records whether the agent *reached for* the skill, using the same detector the routing leg @@ -295,10 +295,11 @@ name: either the skill has stopped mattering, or the case was never evidence about it in the first place. The second is the more common, which makes this a check on the dataset as much as on the skill. -Both halves write which side they were, whose skill they were about, and which -invocation wrote them into their inspect log metadata (`skillscope_baseline`, -`skillscope_skill`, `skillscope_staleness_run`). So a finished run can be -looked at again without paying for it twice: +Both halves write which side they were, whose skill they were about, where that +skill was on disk, and which invocation wrote them into their inspect log +metadata (`skillscope_baseline`, `skillscope_skill`, `skillscope_skill_dir`, +`skillscope_staleness_run`). So a finished run can be looked at again without +paying for it twice: ```bash skillscope staleness --from-logs .skillscope/logs @@ -306,8 +307,14 @@ skillscope staleness --from-logs .skillscope/logs That reaches no model and runs no preflight, having nothing to spend. -All three keys are load-bearing, and the run id is the one that makes this -safe to point at `.skillscope/logs`. That directory is shared with `behavioral` +Every key is load-bearing. `skillscope_skill_dir` is what lets a rebuild judge +whether the baseline was really empty: the check recognises a tool call that +reached the skill by comparing against the skill's absolute path, so a reader +that worked that path out for itself would be working out its own rather than +the run's, and would report a baseline the run had voided as a clean one. A log +written without the key is read as an unknown room rather than an empty one. + +The run id is the key that makes this safe to point at `.skillscope/logs`. That directory is shared with `behavioral` and `routing`, is gitignored wholesale, and nothing prunes it — and every ordinary `behavioral` run writes the same `skillscope_baseline: false` a graded half does, with no run id. A plain `behavioral` log is therefore shaped exactly diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index c7ec27a..a6ba5a7 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -121,6 +121,8 @@ def build_task( metadata={ BASELINE_KEY: baseline, SKILL_KEY: skill, + # Resolved here, where it is known to be right. See `SKILL_DIR_KEY`. + SKILL_DIR_KEY: str(Path(skill_dir).resolve()), **({STALENESS_RUN_KEY: run_key} if run_key else {}), }, tags=["skillscope", "baseline" if baseline else "graded"], @@ -156,33 +158,50 @@ def _normalise(text: str) -> str: return text.replace("\\", "/").lower() -def _skill_marks(skill: str) -> tuple[str, ...]: +# Said instead of a contamination reason when there is no way to look for one. +# Phrased as what is missing rather than as a finding: it is not a claim that +# the baseline was dirty, it is a refusal to claim it was clean. +UNKNOWN_ROOM = ( + "could not tell whether the baseline read the skill: the run did not " + "record where the skill was, and it is not on this machine" +) + + +def _skill_marks(skill: str, recorded: str | None = None) -> tuple[str, ...] | None: """The paths that reading makes a room dirty, lowercased and separator-flat. - Two of them. The skill's own directory, resolved, which is where an agent + Two of them. The skill's own directory, absolute, which is where an agent that went looking would find it. And `.claude/skills/`, the staged copy `no_sandbox.install_skill` writes under a workspace that changes per sample, so only its tail is knowable here. + `recorded` is `SKILL_DIR_KEY` off the log: where the skill was on the + machine that ran the agent. Preferred over resolving it here, because + resolving it here resolves it against *this* process. A reader standing + somewhere else is not evidence about where the agent stood. + + `None` when neither source can say where the skill was, which is an + unknown room rather than a clean one. Returning the staged mark alone + looked like an answer and was not: it is the subset of the check that + cannot fail, so every tool call that reached the skill by its real path + came back clean and the case was credited. See `SKILL_DIR_KEY`. + The staged mark carries no leading separator on purpose. `no_sandbox` runs the agent with `cwd=workspace`, which makes `cat .claude/skills/foo/...` the natural shell form, and a mark anchored with a leading `/` missed exactly that. `_mentions` supplies the boundary instead, which is what keeps the mark from matching the middle of a longer name. """ - marks = [] - try: - marks.append(_normalise(str(Path(config.active().skill_path(skill)).resolve()))) - except (Exception, SystemExit): # noqa: BLE001 -- see below - # A skill with no directory on this machine leaves the staged mark to - # carry the check on its own, which is the honest answer rather than a - # degraded one: nothing can be read out of a directory that is not - # there. `skill_path` raises `SystemExit` rather than returning, so - # catching `Exception` alone let a rebuild of someone else's logs take - # the whole command down. - pass - marks.append(_normalise(f".claude/skills/{skill}")) - return tuple(marks) + home = recorded + if home is None: + try: + home = str(Path(config.active().skill_path(skill)).resolve()) + except (Exception, SystemExit): # noqa: BLE001 -- see below + # `skill_path` raises `SystemExit` rather than returning, so + # catching `Exception` alone let a rebuild of someone else's logs + # take the whole command down. + return None + return (_normalise(home), _normalise(f".claude/skills/{skill}")) # Characters that continue a path component. A mark followed or preceded by @@ -242,9 +261,10 @@ def _strings(value) -> list[str]: def _read_the_skill(call, marks: tuple[str, ...], skill: str) -> str | None: """Whether this call reached into the skill's own directory. - Every string argument is tested, as written and as the path it resolves to. - Resolution is skipped for anything with whitespace in it: that is a command - line rather than a path, and it is matched as written. + Every string argument is tested as written, and a `~`-relative one is + tested expanded as well. Expansion is skipped for anything with whitespace + in it: that is a command line rather than a path, and it is matched as + written. The graded side reads its own installed skill by design, so a hit there is expected. It is the baseline where this means the room was not empty. @@ -258,14 +278,22 @@ def _read_the_skill(call, marks: tuple[str, ...], skill: str) -> str | None: text = _normalise(value) if any(_mentions(text, mark) for mark in marks): return reason - # `../x`, `~/x` and `./x` name the same file by another spelling, and - # a structured path field is where those arrive. - if value and not any(c.isspace() for c in value): + # `~/x` names the same file by another spelling, and expanding it is + # context-free: `$HOME` is the agent's own, recorded in the + # environment rather than inferred from where this is running. + # + # `./x` and `../x` are deliberately *not* resolved. Resolving them + # needs the agent's cwd, which is `no_sandbox`'s per-sample workspace + # and appears nowhere in the tool-call record -- so `resolve()` here + # resolved them against skillscope's own cwd instead, and made the + # verdict depend on which directory the report was run from. It never + # could serve the case it was written for. + if value.startswith("~") and not any(c.isspace() for c in value): try: - resolved = _normalise(str(Path(value).expanduser().resolve())) + expanded = _normalise(str(Path(value).expanduser())) except (Exception, SystemExit): # noqa: BLE001 -- not a path, then continue - if any(_mentions(resolved, mark) for mark in marks): + if any(_mentions(expanded, mark) for mark in marks): return reason # The one mark that is not a path. `_bundled_skills` is the harness's own @@ -276,9 +304,15 @@ def _read_the_skill(call, marks: tuple[str, ...], skill: str) -> str | None: return None -def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | None]: +def _inspect_room( + routing_engine, sample, skill: str, skill_dir: str | None = None +) -> tuple[str | None, str | None]: """`(the skill that fired, why the room was not what it claimed)`. + `skill_dir` is `SKILL_DIR_KEY` off the log, passed by a reader rebuilding + someone else's run. A live run leaves it `None` and resolves the skill + where it already knows it is. + One walk over the tool calls, answering both questions, because the second is only meaningful beside the first. `detect_activation` reports a skill nobody staged as `other:`, which is exactly the contamination this @@ -305,7 +339,13 @@ def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | """ fired: str | None = None why: str | None = None - marks = _skill_marks(skill) + marks = _skill_marks(skill, skill_dir) + # Nowhere to check against, so the read half of the check cannot run. Said + # out loud rather than passed over: this lands the case `ungraded`, which + # is what an unanswerable question is worth. Applied after the walk, not + # before it, so a reason the walk *could* establish -- another skill + # firing, which needs no path at all -- is reported in preference to it. + unknown, marks = marks is None, marks or () try: for call in routing_engine._tool_calls(sample): hit = routing_core.detect_activation( @@ -321,8 +361,8 @@ def _inspect_room(routing_engine, sample, skill: str) -> tuple[str | None, str | if why is None: why = _read_the_skill(call, marks, skill) except Exception: # noqa: BLE001 -- see the docstring - return fired, why - return fired, why + return fired, why or (UNKNOWN_ROOM if unknown else None) + return fired, why or (UNKNOWN_ROOM if unknown else None) def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: @@ -423,6 +463,21 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: # same directory and solved it the same way, with `CALL_TOKEN`. STALENESS_RUN_KEY = "skillscope_staleness_run" +# Where the skill was on the machine that ran the agent. +# +# `_skill_marks` needs an absolute directory to recognise a tool call that +# reached into the skill. Deriving it at read time derives it from whoever is +# *reading*: a rebuild in another directory, or on a machine where the skill +# is not checked out, built a quietly weaker mark set and reported a baseline +# the live run had voided as `ungraded` back as `no_signal`. That is the +# "a rebuild is more permissive than the run it rebuilds" failure this module +# closes everywhere else; contamination was the one place it was still open. +# +# So the run records where the skill actually was, and the reader uses that +# rather than its own cwd. Written by both engines for the same reason +# `SKILL_KEY` is: a reader should not have to know which one produced the log. +SKILL_DIR_KEY = "skillscope_skill_dir" + def stop_requested() -> bool: """Whether the deadline's graceful stage has asked the command to stop.""" diff --git a/skillscope/engine/verify.py b/skillscope/engine/verify.py index cb70182..1792617 100644 --- a/skillscope/engine/verify.py +++ b/skillscope/engine/verify.py @@ -250,10 +250,13 @@ def build_task( bound = deadline.active() return Task( name=f"claude-code-{skill}" + ("" if install else "-baseline"), - # See `behavioral.build_task`: the same two keys whichever engine ran. + # See `behavioral.build_task`: the same keys whichever engine ran. metadata={ behavioral.BASELINE_KEY: not install, behavioral.SKILL_KEY: skill, + # See `behavioral.SKILL_DIR_KEY`: resolved on the machine that ran + # the agent, because a reader cannot work it out afterwards. + behavioral.SKILL_DIR_KEY: str(Path(skill_dir).resolve()), # See `behavioral.STALENESS_RUN_KEY`: set only by a staleness run, # and only there because only there are two logs meant to pair. **( diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 4f8c773..a7c9601 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -236,6 +236,11 @@ def outcomes_from_log(log) -> tuple[str, bool, str | None, list[BehaviorOutcome] eval_meta = log.eval.metadata or {} skill = eval_meta.get(behavioral.SKILL_KEY) or log.eval.task + # Where the skill was when the agent ran, not where it is now. Absent from + # logs written before the key existed, which makes the room unknown rather + # than clean unless this machine happens to have the skill. See + # `behavioral.SKILL_DIR_KEY`. + skill_dir = eval_meta.get(behavioral.SKILL_DIR_KEY) outcomes: list[BehaviorOutcome] = [] for sample in log.samples or []: checks: list[dict] = [] @@ -264,10 +269,17 @@ def outcomes_from_log(log) -> tuple[str, bool, str | None, list[BehaviorOutcome] # comparison reports every `earns_it` as never having fired; # without the second it is *more permissive than the run it # rebuilds*, quietly restoring verdicts the live run voided. + # + # `skill_dir` is what keeps the second true off this machine: + # the detector needs an absolute path to recognise, and left + # to work one out it works out this reader's rather than the + # run's. See `behavioral.SKILL_DIR_KEY`. **dict( zip( ("activated", "contaminated"), - behavioral._inspect_room(routing_engine, sample, skill), + behavioral._inspect_room( + routing_engine, sample, skill, skill_dir + ), ) ), ) diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index c33de0c..78c3787 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6611,7 +6611,12 @@ def _call(self, name, arguments=None): def _inspect(self, sample, skill="alpha"): from skillscope.engine import routing as routing_engine - return engine_behavioral._inspect_room(routing_engine, sample, skill) + # A directory for the skill, so the room is knowable and `why` reports + # what this class is about rather than `UNKNOWN_ROOM`. No case here + # names a path, so nothing can match it. + return engine_behavioral._inspect_room( + routing_engine, sample, skill, f"/nonexistent/skills/{skill}" + ) def test_a_contamination_marker_is_never_reported_as_the_skill(self) -> None: # Two `other:` hits. The first sets `why`; the second used to find it @@ -6673,13 +6678,15 @@ def setUp(self) -> None: self.repo.skill("run", dataset=tier0_dataset("run")) self.repo.activate() - def _why(self, arguments, skill="run"): + def _why(self, arguments, skill="run", skill_dir=None): from skillscope.engine import routing as routing_engine call = _FakeCall("toolu_1", function="Read") call.arguments = arguments sample = _FakeSample(messages=[_FakeMessage("m", [call])], events=[]) - return engine_behavioral._inspect_room(routing_engine, sample, skill)[1] + return engine_behavioral._inspect_room( + routing_engine, sample, skill, skill_dir + )[1] def test_an_unrelated_path_sharing_the_skills_name_is_not_contamination(self) -> None: self.assertIsNone(self._why({"file_path": "/run/user/1000/bus"})) @@ -6761,7 +6768,29 @@ def test_a_skill_with_no_directory_does_not_take_the_command_down(self) -> None: # `config.skill_path` raises `SystemExit`, which `except Exception` # does not catch -- so rebuilding somebody else's logs killed the # process rather than reporting what it could. - self.assertIsNone(self._why({"file_path": "/tmp/x"}, skill="not-a-skill")) + why = self._why({"file_path": "/tmp/x"}, skill="not-a-skill") + self.assertIsNotNone(why) + + def test_a_skill_with_no_directory_is_an_unknown_room_not_a_clean_one(self) -> None: + # It used to answer `None` here, which reads as "the baseline was + # clean" -- a verdict reached by not looking. The mark it dropped is + # the only one an absolute path can hit, so every tool call that + # reached the skill by its real path came back clean on any machine + # without the skill checked out. + why = self._why({"file_path": "/tmp/x"}, skill="not-a-skill") + self.assertEqual(why, engine_behavioral.UNKNOWN_ROOM) + + def test_a_recorded_directory_answers_it_without_the_skill_on_this_machine( + self, + ) -> None: + # The point of recording it: the same log, read where the skill is not. + own = str(config.active().skill_path("run")) + self.assertIn( + "read the skill's own files", + self._why( + {"file_path": f"{own}/SKILL.md"}, skill="not-a-skill", skill_dir=own + ), + ) class TestBothHalvesRunAndOnlyTheInstallFlagDiffers(unittest.TestCase): From b693e0a936f45508a7ddeaca99375ced90033ae0 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 13:04:01 +0100 Subject: [PATCH 12/15] Guard the contamination fix at the level it broke The fix itself was unguarded: deleting the metadata line or restoring the `resolve()` branch both left the suite green, and both would look right on the machine that wrote the logs, which is exactly how the original hid. Three tests, each aimed at one claim. The cwd one asserts the property rather than the mechanism -- same recorded call, two directories, one answer -- so it catches any future route back to a reader-dependent verdict, not just the branch that was removed. Signed-off-by: John Lybeck --- tests/test_skillscope.py | 80 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/tests/test_skillscope.py b/tests/test_skillscope.py index 78c3787..d90a7a9 100644 --- a/tests/test_skillscope.py +++ b/tests/test_skillscope.py @@ -6584,6 +6584,86 @@ def test_the_task_name_says_which_half_it_is(self) -> None: self.assertTrue(baseline.metadata[engine_behavioral.BASELINE_KEY]) +class TestTheRoomIsJudgedWhereTheRunWasNotWhereTheReaderIs(unittest.TestCase): + """A rebuilt contamination verdict must not depend on who is reading. + + `_skill_marks` recognises a tool call that reached the skill by comparing + against the skill's absolute path. Worked out at read time it is the + *reader's* answer: rebuilt from another directory, or on a machine without + the skill checked out, the mark went missing and a baseline the run had + voided as `ungraded` came back `no_signal` -- stated in the report as + fact. So the run records where the skill was and the reader uses that. + + This is the same hazard `test_a_rebuild_is_no_more_permissive_than_the_run_ + it_rebuilds` guards for the no-checks branch, in the one place it was + still open. + """ + + def setUp(self) -> None: + self.repo = Repo(self) + self.repo.skill("run", dataset=tier0_dataset("run")) + self.repo.activate() + self.cases = [c for c in datasets.load_all_cases() if c.skill == "run"][:1] + + def _why(self, arguments, skill_dir=None): + from skillscope.engine import routing as routing_engine + + call = _FakeCall("toolu_1", function="Read") + call.arguments = arguments + sample = _FakeSample(messages=[_FakeMessage("m", [call])], events=[]) + return engine_behavioral._inspect_room( + routing_engine, sample, "run", skill_dir + )[1] + + def test_the_run_records_where_the_skill_was(self) -> None: + # Without this the reader has only its own cwd to go on, and the + # rebuild silently returns to deriving the weaker answer -- which on + # the machine that wrote the logs looks exactly right. + own = str(config.active().skill_path("run").resolve()) + for baseline in (False, True): + task = engine_behavioral.build_task( + "run", self.cases, "mockllm/model", + solver_factory=lambda d: None, baseline=baseline, + ) + self.assertIn( + engine_behavioral.SKILL_DIR_KEY, task.metadata, + "the log does not say where the skill was", + ) + self.assertEqual(task.metadata[engine_behavioral.SKILL_DIR_KEY], own) + + def test_the_other_engine_records_it_too(self) -> None: + # `verify.build_task` needs `inspect_swe`, which the unit job does not + # install, so this is read rather than called -- the same compromise + # `test_the_behavioral_leg_completes_its_skill_too` makes for it. A + # log is paired without knowing which engine wrote it, so a key only + # one of them writes is a key the reader cannot rely on. + self.assertIn( + "SKILL_DIR_KEY", inspect.getsource(engine_verify.build_task) + ) + + def test_the_verdict_is_the_same_from_any_directory(self) -> None: + # The property, asserted as a property. A relative path resolves + # against whoever is running, so resolving one here made the answer a + # fact about the reader's cwd. Standing in the repo root used to say + # contaminated and standing anywhere else said clean, for the same + # recorded call. + own = str(config.active().skill_path("run").resolve()) + argument = {"file_path": "run/SKILL.md"} + + here = os.getcwd() + self.addCleanup(os.chdir, here) + + os.chdir(self.repo.root) + from_the_repo_root = self._why(argument, skill_dir=own) + os.chdir(here) + from_anywhere_else = self._why(argument, skill_dir=own) + + self.assertEqual( + from_the_repo_root, from_anywhere_else, + "the contamination verdict moved with the reader's cwd", + ) + + class TestTheDetectorKeepsItsTwoAnswersApart(unittest.TestCase): """`_inspect_room` answers two questions, and they must not cross. From 903e7b0154906f00c19fbbe1154f22cc7f978617 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 13:08:21 +0100 Subject: [PATCH 13/15] Count the commands the branch actually ships The branch adds a fourth and left "three" everywhere, most visibly in the top-level --help, which printed three bullets with staleness absent directly under a usage line listing it, and in the README, where a reader met "three types" at the top and a fourth command in the table below with nothing reconciling them. `datasets.py` keeps its two, because two is right there: staleness adds nothing to write, it grades the behavioral cases again. Said so instead, since what it does add is a bar on those cases -- one that passes either way is not evidence -- and that is this module's subject. Signed-off-by: John Lybeck --- README.md | 5 +++-- docs/authoring-evals.md | 7 +++++++ pyproject.toml | 2 +- skillscope/cli.py | 22 +++++++++++++--------- skillscope/datasets.py | 6 ++++++ 5 files changed, 30 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index a449a1d..2cf618e 100644 --- a/README.md +++ b/README.md @@ -15,13 +15,14 @@ SPDX-License-Identifier: MIT ![Skillscope](assets/banner.png) -A test harness for AI agent skills. Install the harness, or point a workflow at your repo. Skillscope enables three main types of tests: +A test harness for AI agent skills. Install the harness, or point a workflow at your repo. Skillscope enables four main types of tests: * **Structural**: Checks whether the skill is well structured (contains the expected files, a proper hierarchy, etc.). * **Routing**: Checks whether the skill triggers when it should and stays quiet when it shouldn't. You may also point to more than one skill here to explore how skills interfere with each other's routing. * **Behavioral**: Runs the agent end to end and uses an LLM judge to confirm the skill behaves correctly. +* **Staleness**: Runs the behavioral cases a second time with the skill withheld, and compares. Tells you whether the cases still need the skill, or whether the model has caught up and the suite is passing on its own. -All three read one dataset per skill, `/evals/evals.json`. How to write one is in [docs/authoring-evals.md](docs/authoring-evals.md). +All four read one dataset per skill, `/evals/evals.json`. How to write one is in [docs/authoring-evals.md](docs/authoring-evals.md). > [!IMPORTANT] > **Early Days**: Flags, workflow inputs, and report formats may evolve quickly as this repo takes shape. diff --git a/docs/authoring-evals.md b/docs/authoring-evals.md index 25a6b19..ee786bb 100644 --- a/docs/authoring-evals.md +++ b/docs/authoring-evals.md @@ -224,8 +224,15 @@ skillscope structural # your folder, prose, and da skillscope behavioral --skill # your skill, end to end skillscope routing --routing-room , skillscope routing --only --keep-logs logs # one case, keeping the transcript +skillscope staleness --skill # do your cases still need the skill? ``` +`staleness` is the one aimed at this file's subject. It runs your behavioral +cases twice, once with the skill installed and once without, and reports which +of them passed either way. Those are cases that are not evidence about the +skill — either it has stopped mattering, or the case never tested it. It costs +two runs, so it belongs on a schedule rather than in the loop. + `structural` is instant and needs no agent. Everything else needs the `claude` CLI authenticated, plus whatever your own cases need. Flags, what each check asserts, the routing bar, and how to read a report are in diff --git a/pyproject.toml b/pyproject.toml index 3ab8e4e..5fbe669 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -19,7 +19,7 @@ authors = [{ name = "Advanced Micro Devices, Inc." }] # # It was an extra while `legacy` existed: that engine was standard library only, # so a repo that had not migrated installed nothing new. `legacy` is gone, and -# keeping the extra would ship a tool whose two graded commands both fail on a +# keeping the extra would ship a tool whose graded commands all fail on a # plain `pip install` -- which is what CI found, by running the suite without # extras and getting `ModuleNotFoundError: inspect_ai` from the default engine. # diff --git a/skillscope/cli.py b/skillscope/cli.py index 7b9915e..11f368d 100644 --- a/skillscope/cli.py +++ b/skillscope/cli.py @@ -5,7 +5,7 @@ """The one entry point for skill evals. Every skill owns a single dataset at ``/evals/evals.json``. This runner -reads those datasets and grades them with three commands: +reads those datasets and grades them with four commands: * ``structural`` -- every skill folder, every dataset, and every reference the skill's markdown makes. No agent, no tokens, instant. @@ -18,13 +18,17 @@ grades ``expected_behavior`` / ``unexpected_behavior`` / ``logs_contain`` / ``files_exist``. Only runs evaluations that assert something beyond the routing decision. + * ``staleness`` -- runs ``behavioral``'s cases twice, once with the skill + installed and once without, and compares. Answers a question the other + three cannot: whether the cases still need the skill, or whether the model + has caught up and the suite is passing on its own. A skill may also ship ``/evals/extended_evals.json``, in the same format -and under no coverage requirement of its own. Both graded commands include it +and under no coverage requirement of its own. The graded commands include it by default; ``--no-extended`` grades the required dataset alone. -The prompt is written once and both graded commands read it, which is the whole -point: the alternative is a central routing prompt set plus a separate +The prompt is written once and every graded command reads it, which is the +whole point: the alternative is a central routing prompt set plus a separate per-skill test file that re-asserts routing with a substring match on the transcript. @@ -369,7 +373,7 @@ def _fail_if_expired() -> int | None: def _prepare_graded_run( args: argparse.Namespace, scope: list[str] | None = None ) -> list[str]: - """Structural checks, model pin, and API reachability. Shared by both graders. + """Structural checks, model pin, and API reachability. Shared by the graders. The structural gate reads `scope`, defaulting to the skills ``--skill`` selected -- the ones about to be graded, and nobody else. A routing run @@ -396,8 +400,8 @@ def _prepare_graded_run( ) # Also before the probe, and for the same reason: a malformed price # table should fail the command, not one sample into a graded run. - # Registered here rather than in the engine because both graded - # commands come through this function and inspect's registry is + # Registered here rather than in the engine because every graded + # command comes through this function and inspect's registry is # process-global -- setting it twice would be the same work. from .engine import pricing as engine_pricing @@ -1062,8 +1066,8 @@ def _add_graded_arguments( "`claude-code-no-sandbox` runs it on the host, on any platform. " "`claude-code` runs it inside a container via inspect_swe, which " "needs a POSIX guest and so is Linux only, and additionally " - "needs `pip install 'skillscope[verify]'`. Both run both graded " - "commands. Default: claude-code-no-sandbox, or $SKILLSCOPE_ENGINE." + "needs `pip install 'skillscope[verify]'`. Both run every graded " + "command. Default: claude-code-no-sandbox, or $SKILLSCOPE_ENGINE." ), ) _add_timeout_argument(parser, timeout) diff --git a/skillscope/datasets.py b/skillscope/datasets.py index 250865c..c22500d 100644 --- a/skillscope/datasets.py +++ b/skillscope/datasets.py @@ -38,6 +38,12 @@ with a substring match is a worse version of a check this module already models as a field. +``staleness`` adds no third thing to write. It grades the behavioral cases +above a second time with the skill withheld, which makes it a check on this +dataset as much as on the skill: a case that passes either way is not +evidence about the skill, whether because the skill stopped mattering or +because the case was never evidence in the first place. + ``skill_should_trigger: false`` makes the evaluation routing-only. No skill loads for it, so there is no behavioral phase to hang an assertion or a staged workspace off, and those fields are rejected rather than silently ignored: From 0a5393a9982c755f62659a710da4f91f4873d58c Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 13:16:08 +0100 Subject: [PATCH 14/15] Do not report an unanswered question as a finding Rebuilding a real pre-key log pair surfaced it: all five cases printed "5 case(s) had a baseline that was not empty", which the run had not established and could not. `UNKNOWN_ROOM` was landing in the same field that means "demonstrably dirty", so the totals and the prose both read it as contamination found. Counted and worded apart now. Both still void the same verdicts, because an unanswerable question is worth no more than a failed one, but the report says which it is and how to fix it. Also moves the `earned_without_firing` comment down to the key it describes; it had been sitting above `baseline_contaminated`. Signed-off-by: John Lybeck --- skillscope/staleness.py | 45 +++++++++++++++++++++++++++++++++++++---- 1 file changed, 41 insertions(+), 4 deletions(-) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index a7c9601..7f6e094 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -425,6 +425,8 @@ def _no_duplicates( def summarize(comparisons: list[Comparison], meta: dict) -> dict: + from .engine.behavioral import UNKNOWN_ROOM + counts = {key: 0 for key, _ in VERDICTS} for comparison in comparisons: counts[comparison.verdict] += 1 @@ -464,11 +466,24 @@ def summarize(comparisons: list[Comparison], meta: dict) -> dict: ), "guard_checks": guard_checks, "guards_broken_with_skill": len(broken), - # `earns_it` cases where the skill was installed but never fired. - # The outcome differed; the skill is not shown to be why. + # Counted apart, because they are different findings. One says + # the baseline was demonstrably not empty; the other says nobody + # could tell. Both void the same verdicts -- an unanswerable + # question is worth no more than a failed one -- but a report that + # totalled them together would claim contamination it never found. "baseline_contaminated": sum( - 1 for c in comparisons if c.baseline_contaminated + 1 + for c in comparisons + if c.baseline_contaminated + and c.baseline_contaminated != UNKNOWN_ROOM ), + "baseline_unknown": sum( + 1 + for c in comparisons + if c.baseline_contaminated == UNKNOWN_ROOM + ), + # `earns_it` cases where the skill was installed but never fired. + # The outcome differed; the skill is not shown to be why. "earned_without_firing": sum( 1 for c in comparisons @@ -574,7 +589,29 @@ def render_markdown(summary: dict) -> str: if share is not None: lines += ["", f"Discriminating: {share:.0%} of the cases graded both ways."] - dirty = [c for c in summary["cases"] if c["baseline_contaminated"]] + from .engine.behavioral import UNKNOWN_ROOM + + unknown = [ + c + for c in summary["cases"] + if c["baseline_contaminated"] == UNKNOWN_ROOM + ] + if unknown: + lines += [ + "", + f"> **{len(unknown)} case(s) could not be checked for a clean " + "baseline.** The run did not record where the skill was, and it " + "is not on this machine, so there is nothing to recognise a " + "tool call against. They are reported `ungraded` where the " + "verdict rested on the baseline having managed something. " + "Re-running them records it.", + ] + + dirty = [ + c + for c in summary["cases"] + if c["baseline_contaminated"] and c["baseline_contaminated"] != UNKNOWN_ROOM + ] if dirty: voided = [c for c in dirty if c["verdict"] == "ungraded"] stood = [c for c in dirty if c["verdict"] != "ungraded"] From 93547d52500c55ac986b44e5b72cb9073c4ab838 Mon Sep 17 00:00:00 2001 From: John Lybeck Date: Thu, 29 Oct 2026 13:41:53 +0100 Subject: [PATCH 15/15] Justify --unlabelled-logs with a reason that outlives the migration The comment argued the flag was needed because refusing these logs "made the only staleness logs that existed anywhere unreadable". True when it was written, false the day this merges, and the flag is permanent surface. The durable reason is the one underneath: a log directory is a record somebody already paid an agent run for, and a reader that cannot open it over a metadata key added afterwards has thrown that run away. Checked rather than assumed -- a real pre-key pair is on this machine, and the flag reads it. Signed-off-by: John Lybeck --- skillscope/staleness.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/skillscope/staleness.py b/skillscope/staleness.py index 7f6e094..03ed639 100644 --- a/skillscope/staleness.py +++ b/skillscope/staleness.py @@ -364,9 +364,13 @@ def from_logs(log_dir: str, *, unlabelled: bool = False) -> list[Comparison]: # comparison it was never part of. That is the splicing bug this module's run # id exists to prevent, arriving through the back door. # -# Refusing them outright was wrong too: it made the only staleness logs that -# existed anywhere unreadable, the run this branch was developed against -# included, which is the one artifact a rebuild could be checked against. +# Refusing them outright was wrong too, and the reason outlives the migration +# that prompted it. A log directory is a record somebody already paid an agent +# run for; a reader that cannot open one because of a metadata key added after +# it was written has made that run worthless to settle a question it holds the +# answer to. The population is small and does not grow -- every run since the +# key arrived labels itself -- but "small" is the argument for a flag rather +# than for a refusal. # # So the caller asserts it. Even then a group needs a baseline side, which # nothing but `cmd_staleness` ever writes, so a directory of ordinary