diff --git a/README.md b/README.md index f3600f0..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. @@ -37,6 +38,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 +49,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. @@ -72,6 +80,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/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/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/docs/usage.md b/docs/usage.md index 442d1b1..567be43 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -247,6 +247,117 @@ 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 +``` + +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 | +| `guards_only` | asserts nothing the skill could be credited for — see the guard rails below | +| `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 +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 +check on the dataset as much as on the skill. + +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 +``` + +That reaches no model and runs no preflight, having nothing to spend. + +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 +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 +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 +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) +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 @@ -279,7 +390,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/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/behavior.py b/skillscope/behavior.py index b382b1d..123d4eb 100644 --- a/skillscope/behavior.py +++ b/skillscope/behavior.py @@ -29,6 +29,18 @@ 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 + # 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 0b9da79..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. @@ -68,7 +72,9 @@ import json import os import sys +import tempfile import time +import uuid from pathlib import Path from . import ( @@ -79,6 +85,7 @@ engine, references, routing, + staleness, structure, usage, ) @@ -366,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 @@ -393,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 @@ -440,9 +447,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 @@ -672,60 +681,100 @@ 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, + config_dir: Path | None = None, + run_key: str | None = None, +) -> 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. + + `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 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, + run_key=run_key, ) - 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, config_dir, install=install ), + baseline=not install, + run_key=run_key, ) - 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 +799,155 @@ 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. + + 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 -- 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 + failure -- see `skillscope/staleness.py`. A deadline that expired still + fails, because that is a statement about the run rather than about a skill. + """ + 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, unlabelled=args.unlabelled_logs + ) + if not comparisons: + 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 `{engine_behavioral.SKILL_KEY}`, " + f"`{engine_behavioral.BASELINE_KEY}` and " + f"`{engine_behavioral.STALENESS_RUN_KEY}` in their eval " + "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 + # 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, + { + "skills": sorted({c.skill for c in comparisons}), + "from_logs": args.from_logs, + "wall_time_s": round(time.time() - started, 1), + "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: + 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 + + # 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." + ) + + # 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, run_key=run_key + ) + without = _run_behavioral( + args, skills, gradable, install=False, config_dir=isolated, + run_key=run_key, + ) + + 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 # -------------------------------------------------------------------------- @@ -808,8 +1006,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", @@ -862,22 +1066,32 @@ def _add_graded_arguments(parser: argparse.ArgumentParser) -> None: "`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) + _add_timeout_argument(parser, timeout) + +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. -def _add_timeout_argument(parser: argparse.ArgumentParser) -> None: + `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." ), ) @@ -1062,6 +1276,61 @@ 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: 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." + ), + ) + # Twice the shared bound, because the command is two passes under one. + # 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", + help=( + "Rebuild the comparison from logs already on disk instead of " + "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 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) + select_parser = commands.add_parser( "select", help="Emit the CI plan for a change, as JSON.", 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: diff --git a/skillscope/engine/behavioral.py b/skillscope/engine/behavioral.py index 06ca60f..a6ba5a7 100644 --- a/skillscope/engine/behavioral.py +++ b/skillscope/engine/behavioral.py @@ -85,6 +85,8 @@ def build_task( ctx: dict | None = None, *, solver_factory, + baseline: bool = False, + run_key: str | None = None, ): """One inspect `Task` per skill: its cases, its skill installed, its scorer. @@ -96,6 +98,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 +117,15 @@ def build_task( bound = deadline.active() return Task( - name=f"behavioral-{skill}", + name=f"behavioral-{skill}" + ("-baseline" if baseline else ""), + 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"], dataset=samples, setup=hooks.setup_solver(hook, skill, cases), solver=solver_factory(skill_dir), @@ -138,6 +153,218 @@ 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() + + +# 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, 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. + """ + 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 +# 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 False + + +def _strings(value) -> list[str]: + """Every string anywhere in a tool call's arguments. + + 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[str, ...], skill: str) -> str | None: + """Whether this call reached into the skill's own directory. + + 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. + """ + arguments = getattr(call, "arguments", None) or {} + if not isinstance(arguments, dict): + return None + + reason = f"read the skill's own files ({skill})" + for value in _strings(arguments): + text = _normalise(value) + if any(_mentions(text, mark) for mark in marks): + return reason + # `~/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: + expanded = _normalise(str(Path(value).expanduser())) + except (Exception, SystemExit): # noqa: BLE001 -- not a path, then + continue + 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 + # staging directory, so its name in any argument is the agent having found + # the staging area rather than a file of its own. + if any("_bundled_skills" in _normalise(v) for v in _strings(arguments)): + return reason + return 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 + 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. + + `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. + + 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 = _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( + routing_engine._activation_event(call), + [skill], + allow_body_path=routing_engine.ALLOW_BODY_PATH, + ) + 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: + why = _read_the_skill(call, marks, skill) + except Exception: # noqa: BLE001 -- see the docstring + 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]: """Map one inspect `EvalLog` back onto skillscope's outcome objects. @@ -153,6 +380,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] = [] @@ -173,6 +404,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, @@ -183,6 +416,8 @@ def _outcomes(log, skill: str, cases: list[Case]) -> list[BehaviorOutcome]: checks=checks, error=error, degraded=routing_core.is_provider_error(error), + activated=activated, + contaminated=contaminated, ) ) @@ -207,6 +442,42 @@ 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" + +# 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" + +# 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.""" @@ -292,8 +563,19 @@ 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`.""" + """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 sandbox_spec.require_provider() @@ -311,11 +593,19 @@ 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, + run_key=run_key, + ), 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..1792617 100644 --- a/skillscope/engine/verify.py +++ b/skillscope/engine/verify.py @@ -218,8 +218,17 @@ def build_task( model: str, 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.""" + """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 +249,21 @@ def build_task( bound = deadline.active() return Task( - name=f"claude-code-{skill}", + name=f"claude-code-{skill}" + ("" if install else "-baseline"), + # 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. + **( + {behavioral.STALENESS_RUN_KEY: run_key} if run_key else {} + ), + }, + tags=["skillscope", "graded" if install else "baseline"], setup=hooks.setup_solver(hook, skill, cases), cleanup=hooks.cleanup_fn(hook, skill, cases), dataset=samples, @@ -256,9 +279,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,9 +294,19 @@ 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, + 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() @@ -291,10 +326,14 @@ 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, 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 new file mode 100644 index 0000000..03ed639 --- /dev/null +++ b/skillscope/staleness.py @@ -0,0 +1,711 @@ +# 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, field, replace + +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"), + ("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: + """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 + verdict: str + 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) + # 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 + # 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]: + """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 + reading of an error. + """ + if with_skill.error or without.error: + return "ungraded" + 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 _voided_if_dirty("earns_it", without) + if there and not here: + 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( + 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}", + 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 + 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, + checks_with=list(this.checks), + checks_without=list(that.checks), + activated_with=this.activated, + baseline_contaminated=that.contaminated, + ) + ) + return comparisons + + +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 + 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 + from .engine import routing as routing_engine + + 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] = [] + 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), + 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, + # 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. + # + # `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, skill_dir + ), + ) + ), + ) + ) + return ( + skill, + bool(eval_meta.get(behavioral.BASELINE_KEY)), + eval_meta.get(behavioral.STALENESS_RUN_KEY), + outcomes, + ) + + +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 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 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 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 + 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[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) + 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] + 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), + _no_duplicates(half.get(True, []), ambiguous), + ) + ) + return comparisons + + +# The bucket for logs written before a run id was stamped on them. +# +# 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, 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 +# `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. +# +# A labelled run never falls in here, so none of this can 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 + 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. + """ + 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 + 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: + from .engine.behavioral import UNKNOWN_ROOM + + 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 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": { + "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 + ), + "guard_checks": guard_checks, + "guards_broken_with_skill": len(broken), + # 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 + 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 + if c.verdict == "earns_it" and not c.activated_with + ), + }, + "per_skill": per_skill, + "cases": [asdict(c) for c in comparisons], + } + + +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"] + share = totals["discriminating"] + headline = ( + f"**{totals['earns_it']}/{totals['graded']} graded cases needed the skill**" + if totals["graded"] + 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}{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 + # 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.", + "", + "| 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."] + + 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"] + 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 += [ + "", + 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: + 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} |") + + # 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: + lines.append( + "Nothing: every claim came out the same with the skill and without it." + ) + 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 5d4cf7b..d90a7a9 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 @@ -50,6 +51,7 @@ engine as engine_module, references, routing, + staleness, structure, ) from skillscope import selection as select_module @@ -2377,7 +2379,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 +5973,1266 @@ 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. + + 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, + checks=[{ + "kind": "expected_behavior", "expectation": "do the thing", + "passed": passed, "detail": "", + }], + ) + + +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) + + +@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( + 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("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. + 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 _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, 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=metadata + ) + 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 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. + # + # 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): + """`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. + + 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. + + 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_a_rebuild_is_no_more_permissive_than_the_run_it_rebuilds(self) -> None: + # 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") + + 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) + 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.""" + + 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: + self.assertEqual(self._staged(install=False), []) + + 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. 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 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. + + `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 + + # 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 + # 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", 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, 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"})) + + 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_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 + # process rather than reporting what it could. + 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): + """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, 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", unlabelled=unlabelled) + + 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_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_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 + # 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. 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 + # 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, **k: 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 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. + + 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) + + 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__": unittest.main(verbosity=2)